| Message ID | 20190829101846.21369-6-jgross@suse.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | enhance lock debugging | expand |
On 29.08.2019 12:18, Juergen Gross wrote: > In order to have unique names when doing lock profiling several local > locks "lock" need to be renamed. But these are all named simply "lock" for a good reason, including because they're all function scope symbols (and typically the functions are all sufficiently short). The issue stems from the dual use of "name" in #define _LOCK_PROFILE(name) { 0, #name, &name, 0, 0, 0, 0, 0 } so I'd rather suggest making this a derivation of a new #define _LOCK_PROFILE_NAME(lock, name) { 0, #name, &lock, 0, 0, 0, 0, 0 } if there's no other (transparent) way of disambiguating the names. Jan
On 03.09.19 16:53, Jan Beulich wrote: > On 29.08.2019 12:18, Juergen Gross wrote: >> In order to have unique names when doing lock profiling several local >> locks "lock" need to be renamed. > > But these are all named simply "lock" for a good reason, including > because they're all function scope symbols (and typically the > functions are all sufficiently short). The issue stems from the > dual use of "name" in > > #define _LOCK_PROFILE(name) { 0, #name, &name, 0, 0, 0, 0, 0 } > > so I'd rather suggest making this a derivation of a new > > #define _LOCK_PROFILE_NAME(lock, name) { 0, #name, &lock, 0, 0, 0, 0, 0 } > > if there's no other (transparent) way of disambiguating the names. This will require to use a different DEFINE_SPINLOCK() variant, so e.g. DEFINE_SPINLOCK_LOCAL(), which will then include the needed "static" and add "@<func>" to the lock profiling name. Is this okay? Juergen
On 03.09.2019 17:03, Juergen Gross wrote: > On 03.09.19 16:53, Jan Beulich wrote: >> On 29.08.2019 12:18, Juergen Gross wrote: >>> In order to have unique names when doing lock profiling several local >>> locks "lock" need to be renamed. >> >> But these are all named simply "lock" for a good reason, including >> because they're all function scope symbols (and typically the >> functions are all sufficiently short). The issue stems from the >> dual use of "name" in >> >> #define _LOCK_PROFILE(name) { 0, #name, &name, 0, 0, 0, 0, 0 } >> >> so I'd rather suggest making this a derivation of a new >> >> #define _LOCK_PROFILE_NAME(lock, name) { 0, #name, &lock, 0, 0, 0, 0, 0 } >> >> if there's no other (transparent) way of disambiguating the names. > > This will require to use a different DEFINE_SPINLOCK() variant, so e.g. > DEFINE_SPINLOCK_LOCAL(), which will then include the needed "static" and > add "@<func>" to the lock profiling name. Is this okay? To be frank - not really. I dislike both, and would hence prefer to stick to what there is currently, until someone invents a transparent way to disambiguate these. I'm sorry for being unhelpful here. Jan
On 03.09.19 17:09, Jan Beulich wrote: > On 03.09.2019 17:03, Juergen Gross wrote: >> On 03.09.19 16:53, Jan Beulich wrote: >>> On 29.08.2019 12:18, Juergen Gross wrote: >>>> In order to have unique names when doing lock profiling several local >>>> locks "lock" need to be renamed. >>> >>> But these are all named simply "lock" for a good reason, including >>> because they're all function scope symbols (and typically the >>> functions are all sufficiently short). The issue stems from the >>> dual use of "name" in >>> >>> #define _LOCK_PROFILE(name) { 0, #name, &name, 0, 0, 0, 0, 0 } >>> >>> so I'd rather suggest making this a derivation of a new >>> >>> #define _LOCK_PROFILE_NAME(lock, name) { 0, #name, &lock, 0, 0, 0, 0, 0 } >>> >>> if there's no other (transparent) way of disambiguating the names. >> >> This will require to use a different DEFINE_SPINLOCK() variant, so e.g. >> DEFINE_SPINLOCK_LOCAL(), which will then include the needed "static" and >> add "@<func>" to the lock profiling name. Is this okay? > > To be frank - not really. I dislike both, and would hence prefer to > stick to what there is currently, until someone invents a transparent > way to disambiguate these. I'm sorry for being unhelpful here. NP with me. It was Andrew who asked for a way to differentiate between multiple locks. I'm not depending in any way on this patch. Juergen
On 03.09.19 17:09, Jan Beulich wrote: > On 03.09.2019 17:03, Juergen Gross wrote: >> On 03.09.19 16:53, Jan Beulich wrote: >>> On 29.08.2019 12:18, Juergen Gross wrote: >>>> In order to have unique names when doing lock profiling several local >>>> locks "lock" need to be renamed. >>> >>> But these are all named simply "lock" for a good reason, including >>> because they're all function scope symbols (and typically the >>> functions are all sufficiently short). The issue stems from the >>> dual use of "name" in >>> >>> #define _LOCK_PROFILE(name) { 0, #name, &name, 0, 0, 0, 0, 0 } >>> >>> so I'd rather suggest making this a derivation of a new >>> >>> #define _LOCK_PROFILE_NAME(lock, name) { 0, #name, &lock, 0, 0, 0, 0, 0 } >>> >>> if there's no other (transparent) way of disambiguating the names. >> >> This will require to use a different DEFINE_SPINLOCK() variant, so e.g. >> DEFINE_SPINLOCK_LOCAL(), which will then include the needed "static" and >> add "@<func>" to the lock profiling name. Is this okay? > > To be frank - not really. I dislike both, and would hence prefer to > stick to what there is currently, until someone invents a transparent > way to disambiguate these. I'm sorry for being unhelpful here. I think I have found a way: I could add __FILE__ and __LINE__ data to struct lock_profile. In lock_prof_init() I could look for non-unique lock names and mark those to be printed with the __FILE__ and __LINE__ data added to the names. Would you be fine with this approach? Juergen
On 04.09.2019 10:25, Juergen Gross wrote: > On 03.09.19 17:09, Jan Beulich wrote: >> On 03.09.2019 17:03, Juergen Gross wrote: >>> On 03.09.19 16:53, Jan Beulich wrote: >>>> On 29.08.2019 12:18, Juergen Gross wrote: >>>>> In order to have unique names when doing lock profiling several local >>>>> locks "lock" need to be renamed. >>>> >>>> But these are all named simply "lock" for a good reason, including >>>> because they're all function scope symbols (and typically the >>>> functions are all sufficiently short). The issue stems from the >>>> dual use of "name" in >>>> >>>> #define _LOCK_PROFILE(name) { 0, #name, &name, 0, 0, 0, 0, 0 } >>>> >>>> so I'd rather suggest making this a derivation of a new >>>> >>>> #define _LOCK_PROFILE_NAME(lock, name) { 0, #name, &lock, 0, 0, 0, 0, 0 } >>>> >>>> if there's no other (transparent) way of disambiguating the names. >>> >>> This will require to use a different DEFINE_SPINLOCK() variant, so e.g. >>> DEFINE_SPINLOCK_LOCAL(), which will then include the needed "static" and >>> add "@<func>" to the lock profiling name. Is this okay? >> >> To be frank - not really. I dislike both, and would hence prefer to >> stick to what there is currently, until someone invents a transparent >> way to disambiguate these. I'm sorry for being unhelpful here. > > I think I have found a way: I could add __FILE__ and __LINE__ data to > struct lock_profile. In lock_prof_init() I could look for non-unique > lock names and mark those to be printed with the __FILE__ and __LINE__ > data added to the names. > > Would you be fine with this approach? I would be, but I'm afraid Andrew won't (as with any new uses of __LINE__). I wonder if __func__ or __FUNCTION__ could be used - the main question is how they behave when used outside of a function. If either would be NULL (rather than triggering a diagnostic), it might be usable here. Of course this would be fragile if just based on observed (rather than documented) behavior. Jan
On 04.09.19 10:40, Jan Beulich wrote: > On 04.09.2019 10:25, Juergen Gross wrote: >> On 03.09.19 17:09, Jan Beulich wrote: >>> On 03.09.2019 17:03, Juergen Gross wrote: >>>> On 03.09.19 16:53, Jan Beulich wrote: >>>>> On 29.08.2019 12:18, Juergen Gross wrote: >>>>>> In order to have unique names when doing lock profiling several local >>>>>> locks "lock" need to be renamed. >>>>> >>>>> But these are all named simply "lock" for a good reason, including >>>>> because they're all function scope symbols (and typically the >>>>> functions are all sufficiently short). The issue stems from the >>>>> dual use of "name" in >>>>> >>>>> #define _LOCK_PROFILE(name) { 0, #name, &name, 0, 0, 0, 0, 0 } >>>>> >>>>> so I'd rather suggest making this a derivation of a new >>>>> >>>>> #define _LOCK_PROFILE_NAME(lock, name) { 0, #name, &lock, 0, 0, 0, 0, 0 } >>>>> >>>>> if there's no other (transparent) way of disambiguating the names. >>>> >>>> This will require to use a different DEFINE_SPINLOCK() variant, so e.g. >>>> DEFINE_SPINLOCK_LOCAL(), which will then include the needed "static" and >>>> add "@<func>" to the lock profiling name. Is this okay? >>> >>> To be frank - not really. I dislike both, and would hence prefer to >>> stick to what there is currently, until someone invents a transparent >>> way to disambiguate these. I'm sorry for being unhelpful here. >> >> I think I have found a way: I could add __FILE__ and __LINE__ data to >> struct lock_profile. In lock_prof_init() I could look for non-unique >> lock names and mark those to be printed with the __FILE__ and __LINE__ >> data added to the names. >> >> Would you be fine with this approach? > > I would be, but I'm afraid Andrew won't (as with any new uses of __LINE__). > I wonder if __func__ or __FUNCTION__ could be used - the main question is > how they behave when used outside of a function. If either would be NULL > (rather than triggering a diagnostic), it might be usable here. Of course > this would be fragile if just based on observed (rather than documented) > behavior. With gcc 7.4.1 it fails: /home/gross/xen/xen/include/xen/spinlock.h:80:41: error: ‘__func__’ is not defined outside of function scope [-Werror] #define _LOCK_PROFILE(name) { 0, #name, __func__, &name, 0, 0, 0, 0, 0 } Juergen
On 04.09.2019 10:47, Juergen Gross wrote: > On 04.09.19 10:40, Jan Beulich wrote: >> On 04.09.2019 10:25, Juergen Gross wrote: >>> On 03.09.19 17:09, Jan Beulich wrote: >>>> On 03.09.2019 17:03, Juergen Gross wrote: >>>>> On 03.09.19 16:53, Jan Beulich wrote: >>>>>> On 29.08.2019 12:18, Juergen Gross wrote: >>>>>>> In order to have unique names when doing lock profiling several local >>>>>>> locks "lock" need to be renamed. >>>>>> >>>>>> But these are all named simply "lock" for a good reason, including >>>>>> because they're all function scope symbols (and typically the >>>>>> functions are all sufficiently short). The issue stems from the >>>>>> dual use of "name" in >>>>>> >>>>>> #define _LOCK_PROFILE(name) { 0, #name, &name, 0, 0, 0, 0, 0 } >>>>>> >>>>>> so I'd rather suggest making this a derivation of a new >>>>>> >>>>>> #define _LOCK_PROFILE_NAME(lock, name) { 0, #name, &lock, 0, 0, 0, 0, 0 } >>>>>> >>>>>> if there's no other (transparent) way of disambiguating the names. >>>>> >>>>> This will require to use a different DEFINE_SPINLOCK() variant, so e.g. >>>>> DEFINE_SPINLOCK_LOCAL(), which will then include the needed "static" and >>>>> add "@<func>" to the lock profiling name. Is this okay? >>>> >>>> To be frank - not really. I dislike both, and would hence prefer to >>>> stick to what there is currently, until someone invents a transparent >>>> way to disambiguate these. I'm sorry for being unhelpful here. >>> >>> I think I have found a way: I could add __FILE__ and __LINE__ data to >>> struct lock_profile. In lock_prof_init() I could look for non-unique >>> lock names and mark those to be printed with the __FILE__ and __LINE__ >>> data added to the names. >>> >>> Would you be fine with this approach? >> >> I would be, but I'm afraid Andrew won't (as with any new uses of __LINE__). >> I wonder if __func__ or __FUNCTION__ could be used - the main question is >> how they behave when used outside of a function. If either would be NULL >> (rather than triggering a diagnostic), it might be usable here. Of course >> this would be fragile if just based on observed (rather than documented) >> behavior. > > With gcc 7.4.1 it fails: > > /home/gross/xen/xen/include/xen/spinlock.h:80:41: error: ‘__func__’ is > not defined outside of function scope [-Werror] > #define _LOCK_PROFILE(name) { 0, #name, __func__, &name, 0, 0, 0, 0, 0 } Right, as I was afraid of. But __PRETTY_FUNCTION__ looks to be suitable (as per the gcc doc, and as per there being clear indications that clang also supports this). Jan
On 04/09/2019 09:40, Jan Beulich wrote: > On 04.09.2019 10:25, Juergen Gross wrote: >> On 03.09.19 17:09, Jan Beulich wrote: >>> On 03.09.2019 17:03, Juergen Gross wrote: >>>> On 03.09.19 16:53, Jan Beulich wrote: >>>>> On 29.08.2019 12:18, Juergen Gross wrote: >>>>>> In order to have unique names when doing lock profiling several local >>>>>> locks "lock" need to be renamed. >>>>> But these are all named simply "lock" for a good reason, including >>>>> because they're all function scope symbols (and typically the >>>>> functions are all sufficiently short). The issue stems from the >>>>> dual use of "name" in >>>>> >>>>> #define _LOCK_PROFILE(name) { 0, #name, &name, 0, 0, 0, 0, 0 } >>>>> >>>>> so I'd rather suggest making this a derivation of a new >>>>> >>>>> #define _LOCK_PROFILE_NAME(lock, name) { 0, #name, &lock, 0, 0, 0, 0, 0 } >>>>> >>>>> if there's no other (transparent) way of disambiguating the names. >>>> This will require to use a different DEFINE_SPINLOCK() variant, so e.g. >>>> DEFINE_SPINLOCK_LOCAL(), which will then include the needed "static" and >>>> add "@<func>" to the lock profiling name. Is this okay? >>> To be frank - not really. I dislike both, and would hence prefer to >>> stick to what there is currently, until someone invents a transparent >>> way to disambiguate these. I'm sorry for being unhelpful here. >> I think I have found a way: I could add __FILE__ and __LINE__ data to >> struct lock_profile. In lock_prof_init() I could look for non-unique >> lock names and mark those to be printed with the __FILE__ and __LINE__ >> data added to the names. >> >> Would you be fine with this approach? > I would be, but I'm afraid Andrew won't (as with any new uses of __LINE__). The ok-ness of using __LINE__ is inversely proportional to the likelihood of developing a livepatch for this particular build of Xen, and what additional patching delta it would cause through unrelated changes. We still have __LINE__ in a few cases in release builds because the utility has been deemed to outweigh the additional livepatch overhead. A specific example is x86_emulate(), where the extra patching delta is 0 given that it is a single function. For stuff which wouldn't be active in production builds, and unlikely to be on the majority of debug builds, then it should be fine to use. The one remaining problematic use of __LINE__ is in domain_crash() and it needs to disappear because it tends to have a massive unrelated-patch-delta for release builds. ~Andrew
On 04.09.19 10:58, Andrew Cooper wrote: > On 04/09/2019 09:40, Jan Beulich wrote: >> On 04.09.2019 10:25, Juergen Gross wrote: >>> On 03.09.19 17:09, Jan Beulich wrote: >>>> On 03.09.2019 17:03, Juergen Gross wrote: >>>>> On 03.09.19 16:53, Jan Beulich wrote: >>>>>> On 29.08.2019 12:18, Juergen Gross wrote: >>>>>>> In order to have unique names when doing lock profiling several local >>>>>>> locks "lock" need to be renamed. >>>>>> But these are all named simply "lock" for a good reason, including >>>>>> because they're all function scope symbols (and typically the >>>>>> functions are all sufficiently short). The issue stems from the >>>>>> dual use of "name" in >>>>>> >>>>>> #define _LOCK_PROFILE(name) { 0, #name, &name, 0, 0, 0, 0, 0 } >>>>>> >>>>>> so I'd rather suggest making this a derivation of a new >>>>>> >>>>>> #define _LOCK_PROFILE_NAME(lock, name) { 0, #name, &lock, 0, 0, 0, 0, 0 } >>>>>> >>>>>> if there's no other (transparent) way of disambiguating the names. >>>>> This will require to use a different DEFINE_SPINLOCK() variant, so e.g. >>>>> DEFINE_SPINLOCK_LOCAL(), which will then include the needed "static" and >>>>> add "@<func>" to the lock profiling name. Is this okay? >>>> To be frank - not really. I dislike both, and would hence prefer to >>>> stick to what there is currently, until someone invents a transparent >>>> way to disambiguate these. I'm sorry for being unhelpful here. >>> I think I have found a way: I could add __FILE__ and __LINE__ data to >>> struct lock_profile. In lock_prof_init() I could look for non-unique >>> lock names and mark those to be printed with the __FILE__ and __LINE__ >>> data added to the names. >>> >>> Would you be fine with this approach? >> I would be, but I'm afraid Andrew won't (as with any new uses of __LINE__). > > The ok-ness of using __LINE__ is inversely proportional to the > likelihood of developing a livepatch for this particular build of Xen, > and what additional patching delta it would cause through unrelated changes. Not related to this patch, but to __LINE__ and livepatching: have you considered to use the "#line" directive to avoid unrelated diffs? Juergen
On 04/09/2019 10:11, Juergen Gross wrote: > On 04.09.19 10:58, Andrew Cooper wrote: >> On 04/09/2019 09:40, Jan Beulich wrote: >>> On 04.09.2019 10:25, Juergen Gross wrote: >>>> On 03.09.19 17:09, Jan Beulich wrote: >>>>> On 03.09.2019 17:03, Juergen Gross wrote: >>>>>> On 03.09.19 16:53, Jan Beulich wrote: >>>>>>> On 29.08.2019 12:18, Juergen Gross wrote: >>>>>>>> In order to have unique names when doing lock profiling several >>>>>>>> local >>>>>>>> locks "lock" need to be renamed. >>>>>>> But these are all named simply "lock" for a good reason, including >>>>>>> because they're all function scope symbols (and typically the >>>>>>> functions are all sufficiently short). The issue stems from the >>>>>>> dual use of "name" in >>>>>>> >>>>>>> #define _LOCK_PROFILE(name) { 0, #name, &name, 0, 0, 0, 0, 0 } >>>>>>> >>>>>>> so I'd rather suggest making this a derivation of a new >>>>>>> >>>>>>> #define _LOCK_PROFILE_NAME(lock, name) { 0, #name, &lock, 0, 0, >>>>>>> 0, 0, 0 } >>>>>>> >>>>>>> if there's no other (transparent) way of disambiguating the names. >>>>>> This will require to use a different DEFINE_SPINLOCK() variant, >>>>>> so e.g. >>>>>> DEFINE_SPINLOCK_LOCAL(), which will then include the needed >>>>>> "static" and >>>>>> add "@<func>" to the lock profiling name. Is this okay? >>>>> To be frank - not really. I dislike both, and would hence prefer to >>>>> stick to what there is currently, until someone invents a transparent >>>>> way to disambiguate these. I'm sorry for being unhelpful here. >>>> I think I have found a way: I could add __FILE__ and __LINE__ data to >>>> struct lock_profile. In lock_prof_init() I could look for non-unique >>>> lock names and mark those to be printed with the __FILE__ and __LINE__ >>>> data added to the names. >>>> >>>> Would you be fine with this approach? >>> I would be, but I'm afraid Andrew won't (as with any new uses of >>> __LINE__). >> >> The ok-ness of using __LINE__ is inversely proportional to the >> likelihood of developing a livepatch for this particular build of Xen, >> and what additional patching delta it would cause through unrelated >> changes. > > Not related to this patch, but to __LINE__ and livepatching: have you > considered to use the "#line" directive to avoid unrelated diffs? There are ways to play with __LINE__, yes. #line was brought up in the original discussion. As a thought experiment, how would you expect this to be used to simplify a livepatch? ~Andrew
On 04.09.19 11:15, Andrew Cooper wrote: > On 04/09/2019 10:11, Juergen Gross wrote: >> On 04.09.19 10:58, Andrew Cooper wrote: >>> On 04/09/2019 09:40, Jan Beulich wrote: >>>> On 04.09.2019 10:25, Juergen Gross wrote: >>>>> On 03.09.19 17:09, Jan Beulich wrote: >>>>>> On 03.09.2019 17:03, Juergen Gross wrote: >>>>>>> On 03.09.19 16:53, Jan Beulich wrote: >>>>>>>> On 29.08.2019 12:18, Juergen Gross wrote: >>>>>>>>> In order to have unique names when doing lock profiling several >>>>>>>>> local >>>>>>>>> locks "lock" need to be renamed. >>>>>>>> But these are all named simply "lock" for a good reason, including >>>>>>>> because they're all function scope symbols (and typically the >>>>>>>> functions are all sufficiently short). The issue stems from the >>>>>>>> dual use of "name" in >>>>>>>> >>>>>>>> #define _LOCK_PROFILE(name) { 0, #name, &name, 0, 0, 0, 0, 0 } >>>>>>>> >>>>>>>> so I'd rather suggest making this a derivation of a new >>>>>>>> >>>>>>>> #define _LOCK_PROFILE_NAME(lock, name) { 0, #name, &lock, 0, 0, >>>>>>>> 0, 0, 0 } >>>>>>>> >>>>>>>> if there's no other (transparent) way of disambiguating the names. >>>>>>> This will require to use a different DEFINE_SPINLOCK() variant, >>>>>>> so e.g. >>>>>>> DEFINE_SPINLOCK_LOCAL(), which will then include the needed >>>>>>> "static" and >>>>>>> add "@<func>" to the lock profiling name. Is this okay? >>>>>> To be frank - not really. I dislike both, and would hence prefer to >>>>>> stick to what there is currently, until someone invents a transparent >>>>>> way to disambiguate these. I'm sorry for being unhelpful here. >>>>> I think I have found a way: I could add __FILE__ and __LINE__ data to >>>>> struct lock_profile. In lock_prof_init() I could look for non-unique >>>>> lock names and mark those to be printed with the __FILE__ and __LINE__ >>>>> data added to the names. >>>>> >>>>> Would you be fine with this approach? >>>> I would be, but I'm afraid Andrew won't (as with any new uses of >>>> __LINE__). >>> >>> The ok-ness of using __LINE__ is inversely proportional to the >>> likelihood of developing a livepatch for this particular build of Xen, >>> and what additional patching delta it would cause through unrelated >>> changes. >> >> Not related to this patch, but to __LINE__ and livepatching: have you >> considered to use the "#line" directive to avoid unrelated diffs? > > There are ways to play with __LINE__, yes. #line was brought up in the > original discussion. > > As a thought experiment, how would you expect this to be used to > simplify a livepatch? It should be rather strait forward to write a tool adding #line directives to a patch recovering the previous line numbers in the code following a modification which added or removed lines. Juergen
On 04.09.19 10:51, Jan Beulich wrote: > On 04.09.2019 10:47, Juergen Gross wrote: >> On 04.09.19 10:40, Jan Beulich wrote: >>> On 04.09.2019 10:25, Juergen Gross wrote: >>>> On 03.09.19 17:09, Jan Beulich wrote: >>>>> On 03.09.2019 17:03, Juergen Gross wrote: >>>>>> On 03.09.19 16:53, Jan Beulich wrote: >>>>>>> On 29.08.2019 12:18, Juergen Gross wrote: >>>>>>>> In order to have unique names when doing lock profiling several local >>>>>>>> locks "lock" need to be renamed. >>>>>>> >>>>>>> But these are all named simply "lock" for a good reason, including >>>>>>> because they're all function scope symbols (and typically the >>>>>>> functions are all sufficiently short). The issue stems from the >>>>>>> dual use of "name" in >>>>>>> >>>>>>> #define _LOCK_PROFILE(name) { 0, #name, &name, 0, 0, 0, 0, 0 } >>>>>>> >>>>>>> so I'd rather suggest making this a derivation of a new >>>>>>> >>>>>>> #define _LOCK_PROFILE_NAME(lock, name) { 0, #name, &lock, 0, 0, 0, 0, 0 } >>>>>>> >>>>>>> if there's no other (transparent) way of disambiguating the names. >>>>>> >>>>>> This will require to use a different DEFINE_SPINLOCK() variant, so e.g. >>>>>> DEFINE_SPINLOCK_LOCAL(), which will then include the needed "static" and >>>>>> add "@<func>" to the lock profiling name. Is this okay? >>>>> >>>>> To be frank - not really. I dislike both, and would hence prefer to >>>>> stick to what there is currently, until someone invents a transparent >>>>> way to disambiguate these. I'm sorry for being unhelpful here. >>>> >>>> I think I have found a way: I could add __FILE__ and __LINE__ data to >>>> struct lock_profile. In lock_prof_init() I could look for non-unique >>>> lock names and mark those to be printed with the __FILE__ and __LINE__ >>>> data added to the names. >>>> >>>> Would you be fine with this approach? >>> >>> I would be, but I'm afraid Andrew won't (as with any new uses of __LINE__). >>> I wonder if __func__ or __FUNCTION__ could be used - the main question is >>> how they behave when used outside of a function. If either would be NULL >>> (rather than triggering a diagnostic), it might be usable here. Of course >>> this would be fragile if just based on observed (rather than documented) >>> behavior. >> >> With gcc 7.4.1 it fails: >> >> /home/gross/xen/xen/include/xen/spinlock.h:80:41: error: ‘__func__’ is >> not defined outside of function scope [-Werror] >> #define _LOCK_PROFILE(name) { 0, #name, __func__, &name, 0, 0, 0, 0, 0 } > > Right, as I was afraid of. But __PRETTY_FUNCTION__ looks to be suitable > (as per the gcc doc, and as per there being clear indications that clang > also supports this). Yes, this is working. Will modify the patch. Juergen
diff --git a/xen/arch/x86/irq.c b/xen/arch/x86/irq.c index 0ee33464d2..c348f9e6c9 100644 --- a/xen/arch/x86/irq.c +++ b/xen/arch/x86/irq.c @@ -867,14 +867,14 @@ void set_direct_apic_vector( void alloc_direct_apic_vector( uint8_t *vector, void (*handler)(struct cpu_user_regs *)) { - static DEFINE_SPINLOCK(lock); + static DEFINE_SPINLOCK(apic_alloc_lock); - spin_lock(&lock); + spin_lock(&apic_alloc_lock); if (*vector == 0) { *vector = alloc_hipriority_vector(); set_direct_apic_vector(*vector, handler); } - spin_unlock(&lock); + spin_unlock(&apic_alloc_lock); } void do_IRQ(struct cpu_user_regs *regs) diff --git a/xen/arch/x86/time.c b/xen/arch/x86/time.c index d8242295ef..9e8f3e65b1 100644 --- a/xen/arch/x86/time.c +++ b/xen/arch/x86/time.c @@ -1426,9 +1426,9 @@ static void tsc_check_slave(void *unused) static void tsc_check_reliability(void) { unsigned int cpu = smp_processor_id(); - static DEFINE_SPINLOCK(lock); + static DEFINE_SPINLOCK(tsc_check_lock); - spin_lock(&lock); + spin_lock(&tsc_check_lock); tsc_check_count++; smp_call_function(tsc_check_slave, NULL, 0); @@ -1439,7 +1439,7 @@ static void tsc_check_reliability(void) while ( !cpumask_empty(&tsc_check_cpumask) ) cpu_relax(); - spin_unlock(&lock); + spin_unlock(&tsc_check_lock); } /* diff --git a/xen/common/keyhandler.c b/xen/common/keyhandler.c index c36baa4dff..6e4ff890db 100644 --- a/xen/common/keyhandler.c +++ b/xen/common/keyhandler.c @@ -371,9 +371,9 @@ static void read_clocks(unsigned char key) static u64 sumdif_stime = 0, maxdif_stime = 0; static u64 sumdif_cycles = 0, maxdif_cycles = 0; static u32 count = 0; - static DEFINE_SPINLOCK(lock); + static DEFINE_SPINLOCK(read_clocks_lock); - spin_lock(&lock); + spin_lock(&read_clocks_lock); smp_call_function(read_clocks_slave, NULL, 0); @@ -408,7 +408,7 @@ static void read_clocks(unsigned char key) min_cycles = per_cpu(read_cycles_time, min_cycles_cpu); max_cycles = per_cpu(read_cycles_time, max_cycles_cpu); - spin_unlock(&lock); + spin_unlock(&read_clocks_lock); dif_stime = max_stime - min_stime; if ( dif_stime > maxdif_stime ) diff --git a/xen/common/perfc.c b/xen/common/perfc.c index 3abe35892a..8a099fb0be 100644 --- a/xen/common/perfc.c +++ b/xen/common/perfc.c @@ -241,10 +241,10 @@ static int perfc_copy_info(XEN_GUEST_HANDLE_64(xen_sysctl_perfc_desc_t) desc, /* Dom0 control of perf counters */ int perfc_control(struct xen_sysctl_perfc_op *pc) { - static DEFINE_SPINLOCK(lock); + static DEFINE_SPINLOCK(perfc_control_lock); int rc; - spin_lock(&lock); + spin_lock(&perfc_control_lock); switch ( pc->cmd ) { @@ -262,7 +262,7 @@ int perfc_control(struct xen_sysctl_perfc_op *pc) break; } - spin_unlock(&lock); + spin_unlock(&perfc_control_lock); pc->nr_counters = NR_PERFCTRS; pc->nr_vals = perfc_nbr_vals; diff --git a/xen/common/trace.c b/xen/common/trace.c index d1ef81407b..a216bea00c 100644 --- a/xen/common/trace.c +++ b/xen/common/trace.c @@ -368,10 +368,10 @@ void __init init_trace_bufs(void) */ int tb_control(struct xen_sysctl_tbuf_op *tbc) { - static DEFINE_SPINLOCK(lock); + static DEFINE_SPINLOCK(tb_control_lock); int rc = 0; - spin_lock(&lock); + spin_lock(&tb_control_lock); switch ( tbc->cmd ) { @@ -432,7 +432,7 @@ int tb_control(struct xen_sysctl_tbuf_op *tbc) break; } - spin_unlock(&lock); + spin_unlock(&tb_control_lock); return rc; } diff --git a/xen/drivers/char/console.c b/xen/drivers/char/console.c index e133534be7..e713eeff8e 100644 --- a/xen/drivers/char/console.c +++ b/xen/drivers/char/console.c @@ -1168,7 +1168,7 @@ void panic(const char *fmt, ...) { va_list args; unsigned long flags; - static DEFINE_SPINLOCK(lock); + static DEFINE_SPINLOCK(panic_lock); static char buf[128]; spin_debug_disable(); @@ -1176,7 +1176,7 @@ void panic(const char *fmt, ...) debugtrace_dump(); /* Protects buf[] and ensure multi-line message prints atomically. */ - spin_lock_irqsave(&lock, flags); + spin_lock_irqsave(&panic_lock, flags); va_start(args, fmt); (void)vsnprintf(buf, sizeof(buf), fmt, args); @@ -1196,7 +1196,7 @@ void panic(const char *fmt, ...) printk("Reboot in five seconds...\n"); #endif - spin_unlock_irqrestore(&lock, flags); + spin_unlock_irqrestore(&panic_lock, flags); debugger_trap_immediate();
In order to have unique names when doing lock profiling several local locks "lock" need to be renamed. Signed-off-by: Juergen Gross <jgross@suse.com> --- xen/arch/x86/irq.c | 6 +++--- xen/arch/x86/time.c | 6 +++--- xen/common/keyhandler.c | 6 +++--- xen/common/perfc.c | 6 +++--- xen/common/trace.c | 6 +++--- xen/drivers/char/console.c | 6 +++--- 6 files changed, 18 insertions(+), 18 deletions(-)