| Message ID | a66f1084686b77b098c5ccf3d0cf5f52980fdf5a.1690317797.git.nicola.vetrini@bugseng.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [XEN,v3] xen/spinlock: mechanically rename parameter name 'debug' | expand |
On Tue, 25 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/lkdbg/ is performed. > > Signed-off-by: Nicola Vetrini <nicola.vetrini@bugseng.com> Reviewed-by: Stefano Stabellini <sstabellini@kernel.org> > --- > Changes in v2: > - s/dbg/lkdbg/ > Changes in v3: > - Added missing renames for consistency > --- > 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..d4088f910d 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 *lkdbg, 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(lkdbg->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(&lkdbg->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] == lkdbg ) > { > - printk("CHECKLOCK FAILURE: lock at %p taken recursively\n", debug); > + printk("CHECKLOCK FAILURE: lock at %p taken recursively\n", lkdbg); > BUG(); > } > } > > -static void check_barrier(union lock_debug *debug) > +static void check_barrier(union lock_debug *lkdbg) > { > 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() && !lkdbg->irq_safe); > } > > -void lock_enter(const union lock_debug *debug) > +void lock_enter(const union lock_debug *lkdbg) > { > 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)++] = lkdbg; > 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 *lkdbg) > { > 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] == lkdbg ) > { > 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", lkdbg); > WARN(); > } > > local_irq_restore(flags); > } > > -static void got_lock(union lock_debug *debug) > +static void got_lock(union lock_debug *lkdbg) > { > - debug->cpu = smp_processor_id(); > + lkdbg->cpu = smp_processor_id(); > > - lock_enter(debug); > + lock_enter(lkdbg); > } > > -static void rel_lock(union lock_debug *debug) > +static void rel_lock(union lock_debug *lkdbg) > { > if ( atomic_read(&spin_debug) > 0 ) > - BUG_ON(debug->cpu != smp_processor_id()); > + BUG_ON(lkdbg->cpu != smp_processor_id()); > > - lock_exit(debug); > + lock_exit(lkdbg); > > - debug->cpu = SPINLOCK_NO_CPU; > + lkdbg->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..464af705eb 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 *lkdbg, bool try); > +void lock_enter(const union lock_debug *lkdbg); > +void lock_exit(const union lock_debug *lkdbg); > void spin_debug_enable(void); > void spin_debug_disable(void); > #else > -- > 2.34.1 >
On 25.07.2023 22:45, 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/lkdbg/ is performed. > > Signed-off-by: Nicola Vetrini <nicola.vetrini@bugseng.com> > --- > Changes in v2: > - s/dbg/lkdbg/ > Changes in v3: > - Added missing renames for consistency Hmm, you asked whether to send v3, but then you didn't wait for an answer. So to repeat what I said there: I'd prefer if we could first settle whether to rename the conflicting x86 symbol. Jan
On 26/07/23 08:34, Jan Beulich wrote: > On 25.07.2023 22:45, 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/lkdbg/ is performed. >> >> Signed-off-by: Nicola Vetrini <nicola.vetrini@bugseng.com> >> --- >> Changes in v2: >> - s/dbg/lkdbg/ >> Changes in v3: >> - Added missing renames for consistency > > Hmm, you asked whether to send v3, but then you didn't wait for an > answer. So to repeat what I said there: I'd prefer if we could first > settle whether to rename the conflicting x86 symbol. > Stefano replied asking for a v3 [1] before I had a chance to read your message this morning. [1] https://lore.kernel.org/xen-devel/0a7742a3-7f94-de37-cab7-ef54ffcef8ac@suse.com/T/#m14b4eebd936e3b349491a1b9f46a17a5d9bb9c30
On 26.07.2023 08:42, Nicola Vetrini wrote: > On 26/07/23 08:34, Jan Beulich wrote: >> On 25.07.2023 22:45, 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/lkdbg/ is performed. >>> >>> Signed-off-by: Nicola Vetrini <nicola.vetrini@bugseng.com> >>> --- >>> Changes in v2: >>> - s/dbg/lkdbg/ >>> Changes in v3: >>> - Added missing renames for consistency >> >> Hmm, you asked whether to send v3, but then you didn't wait for an >> answer. So to repeat what I said there: I'd prefer if we could first >> settle whether to rename the conflicting x86 symbol. >> > > Stefano replied asking for a v3 [1] before I had a chance to read your > message this morning. Right, sorry, I spotted his reply only after seeing the v3. Jan
On Wed, 26 Jul 2023, Jan Beulich wrote: > On 26.07.2023 08:42, Nicola Vetrini wrote: > > On 26/07/23 08:34, Jan Beulich wrote: > >> On 25.07.2023 22:45, 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/lkdbg/ is performed. > >>> > >>> Signed-off-by: Nicola Vetrini <nicola.vetrini@bugseng.com> > >>> --- > >>> Changes in v2: > >>> - s/dbg/lkdbg/ > >>> Changes in v3: > >>> - Added missing renames for consistency > >> > >> Hmm, you asked whether to send v3, but then you didn't wait for an > >> answer. So to repeat what I said there: I'd prefer if we could first > >> settle whether to rename the conflicting x86 symbol. > >> > > > > Stefano replied asking for a v3 [1] before I had a chance to read your > > message this morning. > > Right, sorry, I spotted his reply only after seeing the v3. For what is worth I prefer the current implementation compared to renaming debug()
On 26.07.2023 23:49, Stefano Stabellini wrote: > On Wed, 26 Jul 2023, Jan Beulich wrote: >> On 26.07.2023 08:42, Nicola Vetrini wrote: >>> On 26/07/23 08:34, Jan Beulich wrote: >>>> On 25.07.2023 22:45, 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/lkdbg/ is performed. >>>>> >>>>> Signed-off-by: Nicola Vetrini <nicola.vetrini@bugseng.com> >>>>> --- >>>>> Changes in v2: >>>>> - s/dbg/lkdbg/ >>>>> Changes in v3: >>>>> - Added missing renames for consistency >>>> >>>> Hmm, you asked whether to send v3, but then you didn't wait for an >>>> answer. So to repeat what I said there: I'd prefer if we could first >>>> settle whether to rename the conflicting x86 symbol. >>>> >>> >>> Stefano replied asking for a v3 [1] before I had a chance to read your >>> message this morning. >> >> Right, sorry, I spotted his reply only after seeing the v3. > > For what is worth I prefer the current implementation compared to > renaming debug() I don't. My replacement name suggestions were only "just in case"; I don't really like them. Jan
On Thu, 27 Jul 2023, Jan Beulich wrote: > On 26.07.2023 23:49, Stefano Stabellini wrote: > > On Wed, 26 Jul 2023, Jan Beulich wrote: > >> On 26.07.2023 08:42, Nicola Vetrini wrote: > >>> On 26/07/23 08:34, Jan Beulich wrote: > >>>> On 25.07.2023 22:45, 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/lkdbg/ is performed. > >>>>> > >>>>> Signed-off-by: Nicola Vetrini <nicola.vetrini@bugseng.com> > >>>>> --- > >>>>> Changes in v2: > >>>>> - s/dbg/lkdbg/ > >>>>> Changes in v3: > >>>>> - Added missing renames for consistency > >>>> > >>>> Hmm, you asked whether to send v3, but then you didn't wait for an > >>>> answer. So to repeat what I said there: I'd prefer if we could first > >>>> settle whether to rename the conflicting x86 symbol. > >>>> > >>> > >>> Stefano replied asking for a v3 [1] before I had a chance to read your > >>> message this morning. > >> > >> Right, sorry, I spotted his reply only after seeing the v3. > > > > For what is worth I prefer the current implementation compared to > > renaming debug() > > I don't. My replacement name suggestions were only "just in case"; I > don't really like them. Understood :-) How would you like to proceed? 1. we commit this patch as is 2. we wait for a third opinion from another maintainer 3. we find a new name for the variable 4. we change debug() instead ?
Hi, On 27/07/2023 20:35, Stefano Stabellini wrote: > On Thu, 27 Jul 2023, Jan Beulich wrote: >> On 26.07.2023 23:49, Stefano Stabellini wrote: >>> On Wed, 26 Jul 2023, Jan Beulich wrote: >>>> On 26.07.2023 08:42, Nicola Vetrini wrote: >>>>> On 26/07/23 08:34, Jan Beulich wrote: >>>>>> On 25.07.2023 22:45, 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/lkdbg/ is performed. >>>>>>> >>>>>>> Signed-off-by: Nicola Vetrini <nicola.vetrini@bugseng.com> >>>>>>> --- >>>>>>> Changes in v2: >>>>>>> - s/dbg/lkdbg/ >>>>>>> Changes in v3: >>>>>>> - Added missing renames for consistency >>>>>> >>>>>> Hmm, you asked whether to send v3, but then you didn't wait for an >>>>>> answer. So to repeat what I said there: I'd prefer if we could first >>>>>> settle whether to rename the conflicting x86 symbol. >>>>>> >>>>> >>>>> Stefano replied asking for a v3 [1] before I had a chance to read your >>>>> message this morning. >>>> >>>> Right, sorry, I spotted his reply only after seeing the v3. >>> >>> For what is worth I prefer the current implementation compared to >>> renaming debug() >> >> I don't. My replacement name suggestions were only "just in case"; I >> don't really like them. > > Understood :-) > > How would you like to proceed? > > 1. we commit this patch as is > 2. we wait for a third opinion from another maintainer > 3. we find a new name for the variable > 4. we change debug() instead IMO, the name debug() is quite generic and it is not obvious that the function is a trap handler. So I think renaming debug() is the right way to go. Cheers,
On 27.07.2023 21:35, Stefano Stabellini wrote: > On Thu, 27 Jul 2023, Jan Beulich wrote: >> On 26.07.2023 23:49, Stefano Stabellini wrote: >>> On Wed, 26 Jul 2023, Jan Beulich wrote: >>>> On 26.07.2023 08:42, Nicola Vetrini wrote: >>>>> On 26/07/23 08:34, Jan Beulich wrote: >>>>>> On 25.07.2023 22:45, 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/lkdbg/ is performed. >>>>>>> >>>>>>> Signed-off-by: Nicola Vetrini <nicola.vetrini@bugseng.com> >>>>>>> --- >>>>>>> Changes in v2: >>>>>>> - s/dbg/lkdbg/ >>>>>>> Changes in v3: >>>>>>> - Added missing renames for consistency >>>>>> >>>>>> Hmm, you asked whether to send v3, but then you didn't wait for an >>>>>> answer. So to repeat what I said there: I'd prefer if we could first >>>>>> settle whether to rename the conflicting x86 symbol. >>>>>> >>>>> >>>>> Stefano replied asking for a v3 [1] before I had a chance to read your >>>>> message this morning. >>>> >>>> Right, sorry, I spotted his reply only after seeing the v3. >>> >>> For what is worth I prefer the current implementation compared to >>> renaming debug() >> >> I don't. My replacement name suggestions were only "just in case"; I >> don't really like them. > > Understood :-) > > How would you like to proceed? > > 1. we commit this patch as is > 2. we wait for a third opinion from another maintainer > 3. we find a new name for the variable > 4. we change debug() instead 4 is planned already anyway; actually a patch doing that (and quite a few more things) was posted by Andrew a while back. We "just" need to settle on the few open items there. Jan
diff --git a/xen/common/spinlock.c b/xen/common/spinlock.c index 7f453234a9..d4088f910d 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 *lkdbg, 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(lkdbg->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(&lkdbg->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] == lkdbg ) { - printk("CHECKLOCK FAILURE: lock at %p taken recursively\n", debug); + printk("CHECKLOCK FAILURE: lock at %p taken recursively\n", lkdbg); BUG(); } } -static void check_barrier(union lock_debug *debug) +static void check_barrier(union lock_debug *lkdbg) { 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() && !lkdbg->irq_safe); } -void lock_enter(const union lock_debug *debug) +void lock_enter(const union lock_debug *lkdbg) { 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)++] = lkdbg; 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 *lkdbg) { 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] == lkdbg ) { 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", lkdbg); WARN(); } local_irq_restore(flags); } -static void got_lock(union lock_debug *debug) +static void got_lock(union lock_debug *lkdbg) { - debug->cpu = smp_processor_id(); + lkdbg->cpu = smp_processor_id(); - lock_enter(debug); + lock_enter(lkdbg); } -static void rel_lock(union lock_debug *debug) +static void rel_lock(union lock_debug *lkdbg) { if ( atomic_read(&spin_debug) > 0 ) - BUG_ON(debug->cpu != smp_processor_id()); + BUG_ON(lkdbg->cpu != smp_processor_id()); - lock_exit(debug); + lock_exit(lkdbg); - debug->cpu = SPINLOCK_NO_CPU; + lkdbg->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..464af705eb 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 *lkdbg, bool try); +void lock_enter(const union lock_debug *lkdbg); +void lock_exit(const union lock_debug *lkdbg); void spin_debug_enable(void); void spin_debug_disable(void); #else
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/lkdbg/ is performed. Signed-off-by: Nicola Vetrini <nicola.vetrini@bugseng.com> --- Changes in v2: - s/dbg/lkdbg/ Changes in v3: - Added missing renames for consistency --- xen/common/spinlock.c | 38 +++++++++++++++++++------------------- xen/include/xen/spinlock.h | 6 +++--- 2 files changed, 22 insertions(+), 22 deletions(-)