| Message ID | 20230405203505.1343562-2-mcgrof@kernel.org (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | kmod: simplify with a semaphore | expand |
On Wed, 05 Apr 2023, Luis Chamberlain wrote: >From: Peter Zijlstra <peterz@infradead.org> > >Fundamentally semaphores are a counted primitive, but >DEFINE_SEMAPHORE() does not expose this and explicitly creates a >binary semaphore. > >Change DEFINE_SEMAPHORE() to take a number argument and use that in the >few places that open-coded it using __SEMAPHORE_INITIALIZER(). No big deal considering the changelog is small, but I would have expected just for __SEMAPHORE_INITIALIZER() to be used in the next patch, instead of changing DEFINE_SEMAPHORE, which users just need because of the mutex api restrictions. > >Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org> >[mcgrof: add some tribal knowledge about why some folks prefer > binary sempahores over mutexes] >Signed-off-by: Luis Chamberlain <mcgrof@kernel.org> Reviewed-by: Davidlohr Bueso <dave@stgolabs.net> >--- > arch/mips/cavium-octeon/setup.c | 2 +- > arch/x86/kernel/cpu/intel.c | 2 +- > drivers/firmware/efi/runtime-wrappers.c | 2 +- > drivers/firmware/efi/vars.c | 2 +- > drivers/macintosh/adb.c | 2 +- > drivers/net/ethernet/broadcom/bnx2x/bnx2x_main.c | 2 +- > drivers/platform/x86/intel/ifs/sysfs.c | 2 +- > drivers/scsi/esas2r/esas2r_ioctl.c | 2 +- > .../vc04_services/interface/vchiq_arm/vchiq_arm.c | 2 +- > include/linux/semaphore.h | 11 +++++++++-- > kernel/printk/printk.c | 2 +- > net/rxrpc/call_object.c | 6 ++---- > 12 files changed, 21 insertions(+), 16 deletions(-) > >diff --git a/arch/mips/cavium-octeon/setup.c b/arch/mips/cavium-octeon/setup.c >index a71727f7a608..c5561016f577 100644 >--- a/arch/mips/cavium-octeon/setup.c >+++ b/arch/mips/cavium-octeon/setup.c >@@ -72,7 +72,7 @@ extern void pci_console_init(const char *arg); > static unsigned long long max_memory = ULLONG_MAX; > static unsigned long long reserve_low_mem; > >-DEFINE_SEMAPHORE(octeon_bootbus_sem); >+DEFINE_SEMAPHORE(octeon_bootbus_sem, 1); > EXPORT_SYMBOL(octeon_bootbus_sem); > > static struct octeon_boot_descriptor *octeon_boot_desc_ptr; >diff --git a/arch/x86/kernel/cpu/intel.c b/arch/x86/kernel/cpu/intel.c >index 291d4167fab8..12bad63822f0 100644 >--- a/arch/x86/kernel/cpu/intel.c >+++ b/arch/x86/kernel/cpu/intel.c >@@ -1177,7 +1177,7 @@ static const struct { > static struct ratelimit_state bld_ratelimit; > > static unsigned int sysctl_sld_mitigate = 1; >-static DEFINE_SEMAPHORE(buslock_sem); >+static DEFINE_SEMAPHORE(buslock_sem, 1); > > #ifdef CONFIG_PROC_SYSCTL > static struct ctl_table sld_sysctls[] = { >diff --git a/drivers/firmware/efi/runtime-wrappers.c b/drivers/firmware/efi/runtime-wrappers.c >index 1fba4e09cdcf..a400c4312c82 100644 >--- a/drivers/firmware/efi/runtime-wrappers.c >+++ b/drivers/firmware/efi/runtime-wrappers.c >@@ -158,7 +158,7 @@ void efi_call_virt_check_flags(unsigned long flags, const char *call) > * none of the remaining functions are actually ever called at runtime. > * So let's just use a single lock to serialize all Runtime Services calls. > */ >-static DEFINE_SEMAPHORE(efi_runtime_lock); >+static DEFINE_SEMAPHORE(efi_runtime_lock, 1); > > /* > * Expose the EFI runtime lock to the UV platform >diff --git a/drivers/firmware/efi/vars.c b/drivers/firmware/efi/vars.c >index bd75b87f5fc1..bfc5fa6aa47b 100644 >--- a/drivers/firmware/efi/vars.c >+++ b/drivers/firmware/efi/vars.c >@@ -21,7 +21,7 @@ > /* Private pointer to registered efivars */ > static struct efivars *__efivars; > >-static DEFINE_SEMAPHORE(efivars_lock); >+static DEFINE_SEMAPHORE(efivars_lock, 1); > > static efi_status_t check_var_size(bool nonblocking, u32 attributes, > unsigned long size) >diff --git a/drivers/macintosh/adb.c b/drivers/macintosh/adb.c >index 23bd0c77ac1a..56599515d51a 100644 >--- a/drivers/macintosh/adb.c >+++ b/drivers/macintosh/adb.c >@@ -80,7 +80,7 @@ static struct adb_driver *adb_controller; > BLOCKING_NOTIFIER_HEAD(adb_client_list); > static int adb_got_sleep; > static int adb_inited; >-static DEFINE_SEMAPHORE(adb_probe_mutex); >+static DEFINE_SEMAPHORE(adb_probe_mutex, 1); > static int sleepy_trackpad; > static int autopoll_devs; > int __adb_probe_sync; >diff --git a/drivers/net/ethernet/broadcom/bnx2x/bnx2x_main.c b/drivers/net/ethernet/broadcom/bnx2x/bnx2x_main.c >index 5d1e4fe335aa..5a105bab4387 100644 >--- a/drivers/net/ethernet/broadcom/bnx2x/bnx2x_main.c >+++ b/drivers/net/ethernet/broadcom/bnx2x/bnx2x_main.c >@@ -298,7 +298,7 @@ const u32 dmae_reg_go_c[] = { > > /* Global resources for unloading a previously loaded device */ > #define BNX2X_PREV_WAIT_NEEDED 1 >-static DEFINE_SEMAPHORE(bnx2x_prev_sem); >+static DEFINE_SEMAPHORE(bnx2x_prev_sem, 1); > static LIST_HEAD(bnx2x_prev_list); > > /* Forward declaration */ >diff --git a/drivers/platform/x86/intel/ifs/sysfs.c b/drivers/platform/x86/intel/ifs/sysfs.c >index ee636a76b083..4c3c642ee19a 100644 >--- a/drivers/platform/x86/intel/ifs/sysfs.c >+++ b/drivers/platform/x86/intel/ifs/sysfs.c >@@ -13,7 +13,7 @@ > * Protects against simultaneous tests on multiple cores, or > * reloading can file while a test is in progress > */ >-static DEFINE_SEMAPHORE(ifs_sem); >+static DEFINE_SEMAPHORE(ifs_sem, 1); > > /* > * The sysfs interface to check additional details of last test >diff --git a/drivers/scsi/esas2r/esas2r_ioctl.c b/drivers/scsi/esas2r/esas2r_ioctl.c >index e003d923acbf..055d2e87a2c8 100644 >--- a/drivers/scsi/esas2r/esas2r_ioctl.c >+++ b/drivers/scsi/esas2r/esas2r_ioctl.c >@@ -56,7 +56,7 @@ dma_addr_t esas2r_buffered_ioctl_addr; > u32 esas2r_buffered_ioctl_size; > struct pci_dev *esas2r_buffered_ioctl_pcid; > >-static DEFINE_SEMAPHORE(buffered_ioctl_semaphore); >+static DEFINE_SEMAPHORE(buffered_ioctl_semaphore, 1); > typedef int (*BUFFERED_IOCTL_CALLBACK)(struct esas2r_adapter *, > struct esas2r_request *, > struct esas2r_sg_context *, >diff --git a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c >index cddcd3c596c9..1a656fdc9445 100644 >--- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c >+++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c >@@ -149,7 +149,7 @@ static char *g_fragments_base; > static char *g_free_fragments; > static struct semaphore g_free_fragments_sema; > >-static DEFINE_SEMAPHORE(g_free_fragments_mutex); >+static DEFINE_SEMAPHORE(g_free_fragments_mutex, 1); > > static int > vchiq_blocking_bulk_transfer(struct vchiq_instance *instance, unsigned int handle, void *data, >diff --git a/include/linux/semaphore.h b/include/linux/semaphore.h >index 6694d0019a68..2d6aa3fd7861 100644 >--- a/include/linux/semaphore.h >+++ b/include/linux/semaphore.h >@@ -25,8 +25,15 @@ struct semaphore { > .wait_list = LIST_HEAD_INIT((name).wait_list), \ > } > >-#define DEFINE_SEMAPHORE(name) \ >- struct semaphore name = __SEMAPHORE_INITIALIZER(name, 1) >+/* >+ * There is a big difference between a binary semaphore and a mutex. >+ * You cannot call mutex_unlock() from IRQ context because it takes an >+ * internal mutex spin_lock in a non-IRQ-safe manner. Both try_lock() >+ * and unlock() can be called from IRQ context. A mutex must also be >+ * released in the same context that locked it. >+ */ >+#define DEFINE_SEMAPHORE(_name, _n) \ >+ struct semaphore _name = __SEMAPHORE_INITIALIZER(_name, _n) > > static inline void sema_init(struct semaphore *sem, int val) > { >diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c >index fd0c9f913940..76987aaa5a45 100644 >--- a/kernel/printk/printk.c >+++ b/kernel/printk/printk.c >@@ -89,7 +89,7 @@ static DEFINE_MUTEX(console_mutex); > * console_sem protects updates to console->seq and console_suspended, > * and also provides serialization for console printing. > */ >-static DEFINE_SEMAPHORE(console_sem); >+static DEFINE_SEMAPHORE(console_sem, 1); > HLIST_HEAD(console_list); > EXPORT_SYMBOL_GPL(console_list); > DEFINE_STATIC_SRCU(console_srcu); >diff --git a/net/rxrpc/call_object.c b/net/rxrpc/call_object.c >index e9f1f49d18c2..3e5cc70884dd 100644 >--- a/net/rxrpc/call_object.c >+++ b/net/rxrpc/call_object.c >@@ -40,10 +40,8 @@ const char *const rxrpc_call_completions[NR__RXRPC_CALL_COMPLETIONS] = { > > struct kmem_cache *rxrpc_call_jar; > >-static struct semaphore rxrpc_call_limiter = >- __SEMAPHORE_INITIALIZER(rxrpc_call_limiter, 1000); >-static struct semaphore rxrpc_kernel_call_limiter = >- __SEMAPHORE_INITIALIZER(rxrpc_kernel_call_limiter, 1000); >+static DEFINE_SEMAPHORE(rxrpc_call_limiter, 1000); >+static DEFINE_SEMAPHORE(rxrpc_kernel_call_limiter, 1000); > > void rxrpc_poke_call(struct rxrpc_call *call, enum rxrpc_call_poke_trace what) > { >-- >2.39.2 >
On Wed, Apr 05, 2023 at 01:35:04PM -0700, Luis Chamberlain wrote: > diff --git a/include/linux/semaphore.h b/include/linux/semaphore.h > index 6694d0019a68..2d6aa3fd7861 100644 > --- a/include/linux/semaphore.h > +++ b/include/linux/semaphore.h > @@ -25,8 +25,15 @@ struct semaphore { > .wait_list = LIST_HEAD_INIT((name).wait_list), \ > } > > -#define DEFINE_SEMAPHORE(name) \ > - struct semaphore name = __SEMAPHORE_INITIALIZER(name, 1) > +/* > + * There is a big difference between a binary semaphore and a mutex. > + * You cannot call mutex_unlock() from IRQ context because it takes an > + * internal mutex spin_lock in a non-IRQ-safe manner. Both try_lock() > + * and unlock() can be called from IRQ context. A mutex must also be > + * released in the same context that locked it. > + */ I think this confuses cause and effect. How about this: /* * Binary semaphores and mutexes differ in that mutexes have an owner * so they cannot be used from interrupt context and cannot be passed * from one thread to another. down_trylock() and up() can be called * from interrupt context. */ Or this: /* * Unlike mutexes, binary semaphores do not have an owner, so up() can * be called in a different thread from the one which called down(). * It is also safe to call down_trylock() and up() from interrupt * context. */ I'd like to mention completions as an alternative to semaphores, but can't figure out a nice way to fit that in.
On Fri, Apr 7, 2023 at 1:37 PM Matthew Wilcox <willy@infradead.org> wrote: > > I'd like to mention completions as an alternative to semaphores, but > can't figure out a nice way to fit that in. I'm personally a bit sorry completions ever became a thing. There's a real reason for having them, but they have been used and mis-used in so many confusing ways that I am worried every time I see a completion. We've had some nasty use of 'init_completion()' in particular. There are many obvious uses of completions, and they have nice strict semantics wrt last-use etc (so that you can put them on the stack and know that you're the last user when you return, which is not necessarily true of locking in general). But there are several less-than-obvious uses too, and any use of reinit_completion() ends up just making me go "Uhh". The serialization needed for that to actually work right often means that you might as well have used a "wait_event()" with a "smp_store_release()" variable instead and made the code more obvious. I dunno. I might have had a few bad experiences and it's just rare enough to be one of those things that I feel wasn't worth the abstraction cost. And I can't even blame anybody else. I think I'm to blame for that horror. Linus
On (23/04/05 13:35), Luis Chamberlain wrote: > +++ b/kernel/printk/printk.c > @@ -89,7 +89,7 @@ static DEFINE_MUTEX(console_mutex); > * console_sem protects updates to console->seq and console_suspended, > * and also provides serialization for console printing. > */ > -static DEFINE_SEMAPHORE(console_sem); > +static DEFINE_SEMAPHORE(console_sem, 1); FWIW, Reviewed-by: Sergey Senozhatsky <senozhatsky@chromium.org> # printk
On Fri, Apr 07, 2023 at 09:36:30PM +0100, Matthew Wilcox wrote: > Or this: > > /* > * Unlike mutexes, binary semaphores do not have an owner, so up() can > * be called in a different thread from the one which called down(). > * It is also safe to call down_trylock() and up() from interrupt > * context. > */ I went with that. Thanks for helping me paint this shed! Luis
diff --git a/arch/mips/cavium-octeon/setup.c b/arch/mips/cavium-octeon/setup.c index a71727f7a608..c5561016f577 100644 --- a/arch/mips/cavium-octeon/setup.c +++ b/arch/mips/cavium-octeon/setup.c @@ -72,7 +72,7 @@ extern void pci_console_init(const char *arg); static unsigned long long max_memory = ULLONG_MAX; static unsigned long long reserve_low_mem; -DEFINE_SEMAPHORE(octeon_bootbus_sem); +DEFINE_SEMAPHORE(octeon_bootbus_sem, 1); EXPORT_SYMBOL(octeon_bootbus_sem); static struct octeon_boot_descriptor *octeon_boot_desc_ptr; diff --git a/arch/x86/kernel/cpu/intel.c b/arch/x86/kernel/cpu/intel.c index 291d4167fab8..12bad63822f0 100644 --- a/arch/x86/kernel/cpu/intel.c +++ b/arch/x86/kernel/cpu/intel.c @@ -1177,7 +1177,7 @@ static const struct { static struct ratelimit_state bld_ratelimit; static unsigned int sysctl_sld_mitigate = 1; -static DEFINE_SEMAPHORE(buslock_sem); +static DEFINE_SEMAPHORE(buslock_sem, 1); #ifdef CONFIG_PROC_SYSCTL static struct ctl_table sld_sysctls[] = { diff --git a/drivers/firmware/efi/runtime-wrappers.c b/drivers/firmware/efi/runtime-wrappers.c index 1fba4e09cdcf..a400c4312c82 100644 --- a/drivers/firmware/efi/runtime-wrappers.c +++ b/drivers/firmware/efi/runtime-wrappers.c @@ -158,7 +158,7 @@ void efi_call_virt_check_flags(unsigned long flags, const char *call) * none of the remaining functions are actually ever called at runtime. * So let's just use a single lock to serialize all Runtime Services calls. */ -static DEFINE_SEMAPHORE(efi_runtime_lock); +static DEFINE_SEMAPHORE(efi_runtime_lock, 1); /* * Expose the EFI runtime lock to the UV platform diff --git a/drivers/firmware/efi/vars.c b/drivers/firmware/efi/vars.c index bd75b87f5fc1..bfc5fa6aa47b 100644 --- a/drivers/firmware/efi/vars.c +++ b/drivers/firmware/efi/vars.c @@ -21,7 +21,7 @@ /* Private pointer to registered efivars */ static struct efivars *__efivars; -static DEFINE_SEMAPHORE(efivars_lock); +static DEFINE_SEMAPHORE(efivars_lock, 1); static efi_status_t check_var_size(bool nonblocking, u32 attributes, unsigned long size) diff --git a/drivers/macintosh/adb.c b/drivers/macintosh/adb.c index 23bd0c77ac1a..56599515d51a 100644 --- a/drivers/macintosh/adb.c +++ b/drivers/macintosh/adb.c @@ -80,7 +80,7 @@ static struct adb_driver *adb_controller; BLOCKING_NOTIFIER_HEAD(adb_client_list); static int adb_got_sleep; static int adb_inited; -static DEFINE_SEMAPHORE(adb_probe_mutex); +static DEFINE_SEMAPHORE(adb_probe_mutex, 1); static int sleepy_trackpad; static int autopoll_devs; int __adb_probe_sync; diff --git a/drivers/net/ethernet/broadcom/bnx2x/bnx2x_main.c b/drivers/net/ethernet/broadcom/bnx2x/bnx2x_main.c index 5d1e4fe335aa..5a105bab4387 100644 --- a/drivers/net/ethernet/broadcom/bnx2x/bnx2x_main.c +++ b/drivers/net/ethernet/broadcom/bnx2x/bnx2x_main.c @@ -298,7 +298,7 @@ const u32 dmae_reg_go_c[] = { /* Global resources for unloading a previously loaded device */ #define BNX2X_PREV_WAIT_NEEDED 1 -static DEFINE_SEMAPHORE(bnx2x_prev_sem); +static DEFINE_SEMAPHORE(bnx2x_prev_sem, 1); static LIST_HEAD(bnx2x_prev_list); /* Forward declaration */ diff --git a/drivers/platform/x86/intel/ifs/sysfs.c b/drivers/platform/x86/intel/ifs/sysfs.c index ee636a76b083..4c3c642ee19a 100644 --- a/drivers/platform/x86/intel/ifs/sysfs.c +++ b/drivers/platform/x86/intel/ifs/sysfs.c @@ -13,7 +13,7 @@ * Protects against simultaneous tests on multiple cores, or * reloading can file while a test is in progress */ -static DEFINE_SEMAPHORE(ifs_sem); +static DEFINE_SEMAPHORE(ifs_sem, 1); /* * The sysfs interface to check additional details of last test diff --git a/drivers/scsi/esas2r/esas2r_ioctl.c b/drivers/scsi/esas2r/esas2r_ioctl.c index e003d923acbf..055d2e87a2c8 100644 --- a/drivers/scsi/esas2r/esas2r_ioctl.c +++ b/drivers/scsi/esas2r/esas2r_ioctl.c @@ -56,7 +56,7 @@ dma_addr_t esas2r_buffered_ioctl_addr; u32 esas2r_buffered_ioctl_size; struct pci_dev *esas2r_buffered_ioctl_pcid; -static DEFINE_SEMAPHORE(buffered_ioctl_semaphore); +static DEFINE_SEMAPHORE(buffered_ioctl_semaphore, 1); typedef int (*BUFFERED_IOCTL_CALLBACK)(struct esas2r_adapter *, struct esas2r_request *, struct esas2r_sg_context *, diff --git a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c index cddcd3c596c9..1a656fdc9445 100644 --- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c +++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c @@ -149,7 +149,7 @@ static char *g_fragments_base; static char *g_free_fragments; static struct semaphore g_free_fragments_sema; -static DEFINE_SEMAPHORE(g_free_fragments_mutex); +static DEFINE_SEMAPHORE(g_free_fragments_mutex, 1); static int vchiq_blocking_bulk_transfer(struct vchiq_instance *instance, unsigned int handle, void *data, diff --git a/include/linux/semaphore.h b/include/linux/semaphore.h index 6694d0019a68..2d6aa3fd7861 100644 --- a/include/linux/semaphore.h +++ b/include/linux/semaphore.h @@ -25,8 +25,15 @@ struct semaphore { .wait_list = LIST_HEAD_INIT((name).wait_list), \ } -#define DEFINE_SEMAPHORE(name) \ - struct semaphore name = __SEMAPHORE_INITIALIZER(name, 1) +/* + * There is a big difference between a binary semaphore and a mutex. + * You cannot call mutex_unlock() from IRQ context because it takes an + * internal mutex spin_lock in a non-IRQ-safe manner. Both try_lock() + * and unlock() can be called from IRQ context. A mutex must also be + * released in the same context that locked it. + */ +#define DEFINE_SEMAPHORE(_name, _n) \ + struct semaphore _name = __SEMAPHORE_INITIALIZER(_name, _n) static inline void sema_init(struct semaphore *sem, int val) { diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c index fd0c9f913940..76987aaa5a45 100644 --- a/kernel/printk/printk.c +++ b/kernel/printk/printk.c @@ -89,7 +89,7 @@ static DEFINE_MUTEX(console_mutex); * console_sem protects updates to console->seq and console_suspended, * and also provides serialization for console printing. */ -static DEFINE_SEMAPHORE(console_sem); +static DEFINE_SEMAPHORE(console_sem, 1); HLIST_HEAD(console_list); EXPORT_SYMBOL_GPL(console_list); DEFINE_STATIC_SRCU(console_srcu); diff --git a/net/rxrpc/call_object.c b/net/rxrpc/call_object.c index e9f1f49d18c2..3e5cc70884dd 100644 --- a/net/rxrpc/call_object.c +++ b/net/rxrpc/call_object.c @@ -40,10 +40,8 @@ const char *const rxrpc_call_completions[NR__RXRPC_CALL_COMPLETIONS] = { struct kmem_cache *rxrpc_call_jar; -static struct semaphore rxrpc_call_limiter = - __SEMAPHORE_INITIALIZER(rxrpc_call_limiter, 1000); -static struct semaphore rxrpc_kernel_call_limiter = - __SEMAPHORE_INITIALIZER(rxrpc_kernel_call_limiter, 1000); +static DEFINE_SEMAPHORE(rxrpc_call_limiter, 1000); +static DEFINE_SEMAPHORE(rxrpc_kernel_call_limiter, 1000); void rxrpc_poke_call(struct rxrpc_call *call, enum rxrpc_call_poke_trace what) {