Message-ID: <20230407163817.laev7h7mxwtz72fh@offworld>
Date:   Fri, 7 Apr 2023 09:38:17 -0700
From:   Davidlohr Bueso <dave@...olabs.net>
To:     Luis Chamberlain <mcgrof@...nel.org>
Cc:     david@...hat.com, patches@...ts.linux.dev,
        linux-modules@...r.kernel.org, linux-mm@...ck.org,
        linux-kernel@...r.kernel.org, pmladek@...e.com,
        petr.pavlu@...e.com, prarit@...hat.com,
        torvalds@...ux-foundation.org, gregkh@...uxfoundation.org,
        rafael@...nel.org, christophe.leroy@...roup.eu, tglx@...utronix.de,
        peterz@...radead.org, song@...nel.org, rppt@...nel.org,
        willy@...radead.org, vbabka@...e.cz, mhocko@...e.com,
        dave.hansen@...ux.intel.com, colin.i.king@...il.com,
        jim.cromie@...il.com, catalin.marinas@....com, jbaron@...mai.com,
        rick.p.edgecombe@...el.com
Subject: Re: [PATCH v2 1/2] Change DEFINE_SEMAPHORE() to take a number
 argument

On Wed, 05 Apr 2023, Luis Chamberlain wrote:

>From: Peter Zijlstra <peterz@...radead.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@...radead.org>
>[mcgrof: add some tribal knowledge about why some folks prefer
> binary sempahores over mutexes]
>Signed-off-by: Luis Chamberlain <mcgrof@...nel.org>

Reviewed-by: Davidlohr Bueso <dave@...olabs.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
>

Powered by blists - more mailing lists