1. Patchew
  2. Xen
  3. View series

[XEN PATCH] xen/spinlock: mechanically rename parameter name 'debug'

Nicola Vetrini posted 1 patch 9 months, 1 week ago
Patches applied successfully (tree, apply log)
git fetch https://gitlab.com/xen-project/patchew/xen tags/patchew/78255b6c5caaaa02dad638c7d4102ea793a09b88.1689953306.git.nicola.vetrini@bugseng.com
There is a newer version of this series
xen/common/spinlock.c      | 38 +++++++++++++++++++-------------------
xen/include/xen/spinlock.h |  6 +++---
2 files changed, 22 insertions(+), 22 deletions(-)
[XEN PATCH] xen/spinlock: mechanically rename parameter name 'debug'
Posted by Nicola Vetrini 9 months, 1 week ago 
Rule 5.3 has the following headline:
"An identifier declared in an inner scope shall not hide an
identifier declared in an outer scope"

To avoid any confusion resulting from the parameter 'debug'
hiding the homonymous function declared at
'xen/arch/x86/include/asm/processor.h:428'
the rename of parameters s/debug/dbg is performed.

Signed-off-by: Nicola Vetrini <nicola.vetrini@bugseng.com>
---
x86 maintainers CC'ed because the violation is caused by a declaration
in an x86 file, but I reckon it would harm understandability if
a function was renamed to 'dbg'
---
 xen/common/spinlock.c      | 38 +++++++++++++++++++-------------------
 xen/include/xen/spinlock.h |  6 +++---
 2 files changed, 22 insertions(+), 22 deletions(-)

diff --git a/xen/common/spinlock.c b/xen/common/spinlock.c
index 7f453234a9..d8d2e6ad1a 100644
--- a/xen/common/spinlock.c
+++ b/xen/common/spinlock.c
@@ -78,7 +78,7 @@ static int __init cf_check lockdebug_init(void)
 }
 presmp_initcall(lockdebug_init);
 
-void check_lock(union lock_debug *debug, bool try)
+void check_lock(union lock_debug *dbg, bool try)
 {
     bool irq_safe = !local_irq_is_enabled();
     unsigned int cpu = smp_processor_id();
@@ -118,12 +118,12 @@ void check_lock(union lock_debug *debug, bool try)
     if ( try && irq_safe )
         return;
 
-    if ( unlikely(debug->irq_safe != irq_safe) )
+    if ( unlikely(dbg->irq_safe != irq_safe) )
     {
         union lock_debug seen, new = { 0 };
 
         new.irq_safe = irq_safe;
-        seen.val = cmpxchg(&debug->val, LOCK_DEBUG_INITVAL, new.val);
+        seen.val = cmpxchg(&dbg->val, LOCK_DEBUG_INITVAL, new.val);
 
         if ( !seen.unseen && seen.irq_safe == !irq_safe )
         {
@@ -137,14 +137,14 @@ void check_lock(union lock_debug *debug, bool try)
         return;
 
     for ( i = 0; i < nr_taken; i++ )
-        if ( taken[i] == debug )
+        if ( taken[i] == dbg )
         {
-            printk("CHECKLOCK FAILURE: lock at %p taken recursively\n", debug);
+            printk("CHECKLOCK FAILURE: lock at %p taken recursively\n", dbg);
             BUG();
         }
 }
 
-static void check_barrier(union lock_debug *debug)
+static void check_barrier(union lock_debug *dbg)
 {
     if ( unlikely(atomic_read(&spin_debug) <= 0) )
         return;
@@ -160,10 +160,10 @@ static void check_barrier(union lock_debug *debug)
      * However, if we spin on an IRQ-unsafe lock with IRQs disabled then that
      * is clearly wrong, for the same reason outlined in check_lock() above.
      */
-    BUG_ON(!local_irq_is_enabled() && !debug->irq_safe);
+    BUG_ON(!local_irq_is_enabled() && !dbg->irq_safe);
 }
 
-void lock_enter(const union lock_debug *debug)
+void lock_enter(const union lock_debug *dbg)
 {
     unsigned int cpu = smp_processor_id();
     const union lock_debug **taken = per_cpu(locks_taken, cpu);
@@ -176,7 +176,7 @@ void lock_enter(const union lock_debug *debug)
     local_irq_save(flags);
 
     if ( *nr_taken < lock_depth_size )
-        taken[(*nr_taken)++] = debug;
+        taken[(*nr_taken)++] = dbg;
     else if ( !max_depth_reached )
     {
         max_depth_reached = true;
@@ -187,7 +187,7 @@ void lock_enter(const union lock_debug *debug)
     local_irq_restore(flags);
 }
 
-void lock_exit(const union lock_debug *debug)
+void lock_exit(const union lock_debug *dbg)
 {
     unsigned int cpu = smp_processor_id();
     const union lock_debug **taken = per_cpu(locks_taken, cpu);
@@ -202,7 +202,7 @@ void lock_exit(const union lock_debug *debug)
 
     for ( i = *nr_taken; i > 0; i-- )
     {
-        if ( taken[i - 1] == debug )
+        if ( taken[i - 1] == dbg )
         {
             memmove(taken + i - 1, taken + i,
                     (*nr_taken - i) * sizeof(*taken));
@@ -217,28 +217,28 @@ void lock_exit(const union lock_debug *debug)
 
     if ( !max_depth_reached )
     {
-        printk("CHECKLOCK released lock at %p not recorded!\n", debug);
+        printk("CHECKLOCK released lock at %p not recorded!\n", dbg);
         WARN();
     }
 
     local_irq_restore(flags);
 }
 
-static void got_lock(union lock_debug *debug)
+static void got_lock(union lock_debug *dbg)
 {
-    debug->cpu = smp_processor_id();
+    dbg->cpu = smp_processor_id();
 
-    lock_enter(debug);
+    lock_enter(dbg);
 }
 
-static void rel_lock(union lock_debug *debug)
+static void rel_lock(union lock_debug *dbg)
 {
     if ( atomic_read(&spin_debug) > 0 )
-        BUG_ON(debug->cpu != smp_processor_id());
+        BUG_ON(dbg->cpu != smp_processor_id());
 
-    lock_exit(debug);
+    lock_exit(dbg);
 
-    debug->cpu = SPINLOCK_NO_CPU;
+    dbg->cpu = SPINLOCK_NO_CPU;
 }
 
 void spin_debug_enable(void)
diff --git a/xen/include/xen/spinlock.h b/xen/include/xen/spinlock.h
index 0a02a527dc..d303c56f8a 100644
--- a/xen/include/xen/spinlock.h
+++ b/xen/include/xen/spinlock.h
@@ -22,9 +22,9 @@ union lock_debug {
     };
 };
 #define _LOCK_DEBUG { LOCK_DEBUG_INITVAL }
-void check_lock(union lock_debug *debug, bool try);
-void lock_enter(const union lock_debug *debug);
-void lock_exit(const union lock_debug *debug);
+void check_lock(union lock_debug *dbg, bool try);
+void lock_enter(const union lock_debug *dbg);
+void lock_exit(const union lock_debug *dbg);
 void spin_debug_enable(void);
 void spin_debug_disable(void);
 #else
-- 
2.34.1
Re: [XEN PATCH] xen/spinlock: mechanically rename parameter name 'debug'
Posted by Jan Beulich 9 months, 1 week ago 
On 21.07.2023 17:29, Nicola Vetrini wrote:
> --- a/xen/common/spinlock.c
> +++ b/xen/common/spinlock.c
> @@ -78,7 +78,7 @@ static int __init cf_check lockdebug_init(void)
>  }
>  presmp_initcall(lockdebug_init);
>  
> -void check_lock(union lock_debug *debug, bool try)
> +void check_lock(union lock_debug *dbg, bool try)

I consider it reasonably likely that something named "dbg" might also
appear, when we already have something with as generic a named as
"debug". How about naming these parameters "ld", "ldbg", "lkdbg", or
yet something along these lines, but (at least slightly) less generic?

Another alternative would be to rename debug() to entry_DB(), along
the lines of entry_CP(). Andrew, didn't you once propose renaming all
the exception entry points like this?

Jan
Re: [XEN PATCH] xen/spinlock: mechanically rename parameter name 'debug'
Posted by Nicola Vetrini 9 months, 1 week ago 

On 24/07/23 10:14, Jan Beulich wrote:
> On 21.07.2023 17:29, Nicola Vetrini wrote:
>> --- a/xen/common/spinlock.c
>> +++ b/xen/common/spinlock.c
>> @@ -78,7 +78,7 @@ static int __init cf_check lockdebug_init(void)
>>   }
>>   presmp_initcall(lockdebug_init);
>>   
>> -void check_lock(union lock_debug *debug, bool try)
>> +void check_lock(union lock_debug *dbg, bool try)
> 
> I consider it reasonably likely that something named "dbg" might also
> appear, when we already have something with as generic a named as
> "debug". How about naming these parameters "ld", "ldbg", "lkdbg", or
> yet something along these lines, but (at least slightly) less generic?
> 

No problem. I prefer "lkdbg" among the alternatives.

-- 
Nicola Vetrini, BSc
Software Engineer, BUGSENG srl (https://bugseng.com)
Re: [XEN PATCH] xen/spinlock: mechanically rename parameter name 'debug'
Posted by Stefano Stabellini 9 months, 1 week ago 
On Fri, 21 Jul 2023, Nicola Vetrini wrote:
> Rule 5.3 has the following headline:
> "An identifier declared in an inner scope shall not hide an
> identifier declared in an outer scope"
> 
> To avoid any confusion resulting from the parameter 'debug'
> hiding the homonymous function declared at
> 'xen/arch/x86/include/asm/processor.h:428'
> the rename of parameters s/debug/dbg is performed.
> 
> Signed-off-by: Nicola Vetrini <nicola.vetrini@bugseng.com>

Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>


> ---
> x86 maintainers CC'ed because the violation is caused by a declaration
> in an x86 file, but I reckon it would harm understandability if
> a function was renamed to 'dbg'
> ---
>  xen/common/spinlock.c      | 38 +++++++++++++++++++-------------------
>  xen/include/xen/spinlock.h |  6 +++---
>  2 files changed, 22 insertions(+), 22 deletions(-)
> 
> diff --git a/xen/common/spinlock.c b/xen/common/spinlock.c
> index 7f453234a9..d8d2e6ad1a 100644
> --- a/xen/common/spinlock.c
> +++ b/xen/common/spinlock.c
> @@ -78,7 +78,7 @@ static int __init cf_check lockdebug_init(void)
>  }
>  presmp_initcall(lockdebug_init);
>  
> -void check_lock(union lock_debug *debug, bool try)
> +void check_lock(union lock_debug *dbg, bool try)
>  {
>      bool irq_safe = !local_irq_is_enabled();
>      unsigned int cpu = smp_processor_id();
> @@ -118,12 +118,12 @@ void check_lock(union lock_debug *debug, bool try)
>      if ( try && irq_safe )
>          return;
>  
> -    if ( unlikely(debug->irq_safe != irq_safe) )
> +    if ( unlikely(dbg->irq_safe != irq_safe) )
>      {
>          union lock_debug seen, new = { 0 };
>  
>          new.irq_safe = irq_safe;
> -        seen.val = cmpxchg(&debug->val, LOCK_DEBUG_INITVAL, new.val);
> +        seen.val = cmpxchg(&dbg->val, LOCK_DEBUG_INITVAL, new.val);
>  
>          if ( !seen.unseen && seen.irq_safe == !irq_safe )
>          {
> @@ -137,14 +137,14 @@ void check_lock(union lock_debug *debug, bool try)
>          return;
>  
>      for ( i = 0; i < nr_taken; i++ )
> -        if ( taken[i] == debug )
> +        if ( taken[i] == dbg )
>          {
> -            printk("CHECKLOCK FAILURE: lock at %p taken recursively\n", debug);
> +            printk("CHECKLOCK FAILURE: lock at %p taken recursively\n", dbg);
>              BUG();
>          }
>  }
>  
> -static void check_barrier(union lock_debug *debug)
> +static void check_barrier(union lock_debug *dbg)
>  {
>      if ( unlikely(atomic_read(&spin_debug) <= 0) )
>          return;
> @@ -160,10 +160,10 @@ static void check_barrier(union lock_debug *debug)
>       * However, if we spin on an IRQ-unsafe lock with IRQs disabled then that
>       * is clearly wrong, for the same reason outlined in check_lock() above.
>       */
> -    BUG_ON(!local_irq_is_enabled() && !debug->irq_safe);
> +    BUG_ON(!local_irq_is_enabled() && !dbg->irq_safe);
>  }
>  
> -void lock_enter(const union lock_debug *debug)
> +void lock_enter(const union lock_debug *dbg)
>  {
>      unsigned int cpu = smp_processor_id();
>      const union lock_debug **taken = per_cpu(locks_taken, cpu);
> @@ -176,7 +176,7 @@ void lock_enter(const union lock_debug *debug)
>      local_irq_save(flags);
>  
>      if ( *nr_taken < lock_depth_size )
> -        taken[(*nr_taken)++] = debug;
> +        taken[(*nr_taken)++] = dbg;
>      else if ( !max_depth_reached )
>      {
>          max_depth_reached = true;
> @@ -187,7 +187,7 @@ void lock_enter(const union lock_debug *debug)
>      local_irq_restore(flags);
>  }
>  
> -void lock_exit(const union lock_debug *debug)
> +void lock_exit(const union lock_debug *dbg)
>  {
>      unsigned int cpu = smp_processor_id();
>      const union lock_debug **taken = per_cpu(locks_taken, cpu);
> @@ -202,7 +202,7 @@ void lock_exit(const union lock_debug *debug)
>  
>      for ( i = *nr_taken; i > 0; i-- )
>      {
> -        if ( taken[i - 1] == debug )
> +        if ( taken[i - 1] == dbg )
>          {
>              memmove(taken + i - 1, taken + i,
>                      (*nr_taken - i) * sizeof(*taken));
> @@ -217,28 +217,28 @@ void lock_exit(const union lock_debug *debug)
>  
>      if ( !max_depth_reached )
>      {
> -        printk("CHECKLOCK released lock at %p not recorded!\n", debug);
> +        printk("CHECKLOCK released lock at %p not recorded!\n", dbg);
>          WARN();
>      }
>  
>      local_irq_restore(flags);
>  }
>  
> -static void got_lock(union lock_debug *debug)
> +static void got_lock(union lock_debug *dbg)
>  {
> -    debug->cpu = smp_processor_id();
> +    dbg->cpu = smp_processor_id();
>  
> -    lock_enter(debug);
> +    lock_enter(dbg);
>  }
>  
> -static void rel_lock(union lock_debug *debug)
> +static void rel_lock(union lock_debug *dbg)
>  {
>      if ( atomic_read(&spin_debug) > 0 )
> -        BUG_ON(debug->cpu != smp_processor_id());
> +        BUG_ON(dbg->cpu != smp_processor_id());
>  
> -    lock_exit(debug);
> +    lock_exit(dbg);
>  
> -    debug->cpu = SPINLOCK_NO_CPU;
> +    dbg->cpu = SPINLOCK_NO_CPU;
>  }
>  
>  void spin_debug_enable(void)
> diff --git a/xen/include/xen/spinlock.h b/xen/include/xen/spinlock.h
> index 0a02a527dc..d303c56f8a 100644
> --- a/xen/include/xen/spinlock.h
> +++ b/xen/include/xen/spinlock.h
> @@ -22,9 +22,9 @@ union lock_debug {
>      };
>  };
>  #define _LOCK_DEBUG { LOCK_DEBUG_INITVAL }
> -void check_lock(union lock_debug *debug, bool try);
> -void lock_enter(const union lock_debug *debug);
> -void lock_exit(const union lock_debug *debug);
> +void check_lock(union lock_debug *dbg, bool try);
> +void lock_enter(const union lock_debug *dbg);
> +void lock_exit(const union lock_debug *dbg);
>  void spin_debug_enable(void);
>  void spin_debug_disable(void);
>  #else
> -- 
> 2.34.1
>

Patchew 2.1-devel-0870f84

© 2016 - 2024 Red Hat, Inc.