| Message ID | 20231212094725.22184-7-jgross@suse.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | xen/spinlock: make recursive spinlocks a dedicated type | expand |
Hi, On 12/12/2023 09:47, Juergen Gross wrote: > Struct lock_profile contains a pointer to the spinlock it is associated > with. Prepare support of differing spinlock_t and rspinlock_t types by > adding a type indicator of the pointer. Use the highest bit of the > block_cnt member for this indicator in order to not grow the struct > while hurting only the slow path with slightly less performant code. > > Signed-off-by: Juergen Gross <jgross@suse.com> > Acked-by: Alejandro Vallejo <alejandro.vallejo@cloud.com> > --- > V2: > - new patch > --- > xen/common/spinlock.c | 26 +++++++++++++++++++------- > xen/include/xen/spinlock.h | 10 ++++++++-- > 2 files changed, 27 insertions(+), 9 deletions(-) > > diff --git a/xen/common/spinlock.c b/xen/common/spinlock.c > index c1a9ba1304..7d611d3d7d 100644 > --- a/xen/common/spinlock.c > +++ b/xen/common/spinlock.c > @@ -538,19 +538,31 @@ static void spinlock_profile_iterate(lock_profile_subfunc *sub, void *par) > static void cf_check spinlock_profile_print_elem(struct lock_profile *data, > int32_t type, int32_t idx, void *par) > { > - struct spinlock *lock = data->lock; > + unsigned int cpu; > + uint32_t lockval; > + > + if ( data->is_rlock ) > + { > + cpu = data->rlock->debug.cpu; > + lockval = data->rlock->tickets.head_tail; > + } > + else > + { > + cpu = data->lock->debug.cpu; > + lockval = data->lock->tickets.head_tail; > + } > > printk("%s ", lock_profile_ancs[type].name); > if ( type != LOCKPROF_TYPE_GLOBAL ) > printk("%d ", idx); > - printk("%s: addr=%p, lockval=%08x, ", data->name, lock, > - lock->tickets.head_tail); > - if ( lock->debug.cpu == SPINLOCK_NO_CPU ) > + printk("%s: addr=%p, lockval=%08x, ", data->name, data->lock, lockval); > + if ( cpu == SPINLOCK_NO_CPU ) > printk("not locked\n"); > else > - printk("cpu=%d\n", lock->debug.cpu); > - printk(" lock:%" PRId64 "(%" PRI_stime "), block:%" PRId64 "(%" PRI_stime ")\n", > - data->lock_cnt, data->time_hold, data->block_cnt, data->time_block); > + printk("cpu=%u\n", cpu); > + printk(" lock:%" PRIu64 "(%" PRI_stime "), block:%" PRIu64 "(%" PRI_stime ")\n", > + data->lock_cnt, data->time_hold, (uint64_t)data->block_cnt, > + data->time_block); > } > > void cf_check spinlock_profile_printall(unsigned char key) > diff --git a/xen/include/xen/spinlock.h b/xen/include/xen/spinlock.h > index 05b97c1e03..ac3bef267a 100644 > --- a/xen/include/xen/spinlock.h > +++ b/xen/include/xen/spinlock.h > @@ -76,13 +76,19 @@ union lock_debug { }; > */ > > struct spinlock; > +/* Temporary hack until a dedicated struct rspinlock is existing. */ > +#define rspinlock spinlock > > struct lock_profile { > struct lock_profile *next; /* forward link */ > const char *name; /* lock name */ > - struct spinlock *lock; /* the lock itself */ > + union { > + struct spinlock *lock; /* the lock itself */ > + struct rspinlock *rlock; /* the recursive lock itself */ > + }; > uint64_t lock_cnt; /* # of complete locking ops */ > - uint64_t block_cnt; /* # of complete wait for lock */ > + uint64_t block_cnt:63; /* # of complete wait for lock */ > + uint64_t is_rlock:1; /* use rlock pointer */ This is meant to act like a bool. So I would prefer if we use: bool is_rwlock:1; And then use true/false when set. Acked-by: Julien Grall <jgrall@amazon.com> > s_time_t time_hold; /* cumulated lock time */ > s_time_t time_block; /* cumulated wait time */ > s_time_t time_locked; /* system time of last locking */ Cheers,
On 12.12.23 19:42, Julien Grall wrote: > Hi, > > On 12/12/2023 09:47, Juergen Gross wrote: >> Struct lock_profile contains a pointer to the spinlock it is associated >> with. Prepare support of differing spinlock_t and rspinlock_t types by >> adding a type indicator of the pointer. Use the highest bit of the >> block_cnt member for this indicator in order to not grow the struct >> while hurting only the slow path with slightly less performant code. >> >> Signed-off-by: Juergen Gross <jgross@suse.com> >> Acked-by: Alejandro Vallejo <alejandro.vallejo@cloud.com> >> --- >> V2: >> - new patch >> --- >> xen/common/spinlock.c | 26 +++++++++++++++++++------- >> xen/include/xen/spinlock.h | 10 ++++++++-- >> 2 files changed, 27 insertions(+), 9 deletions(-) >> >> diff --git a/xen/common/spinlock.c b/xen/common/spinlock.c >> index c1a9ba1304..7d611d3d7d 100644 >> --- a/xen/common/spinlock.c >> +++ b/xen/common/spinlock.c >> @@ -538,19 +538,31 @@ static void >> spinlock_profile_iterate(lock_profile_subfunc *sub, void *par) >> static void cf_check spinlock_profile_print_elem(struct lock_profile *data, >> int32_t type, int32_t idx, void *par) >> { >> - struct spinlock *lock = data->lock; >> + unsigned int cpu; >> + uint32_t lockval; >> + >> + if ( data->is_rlock ) >> + { >> + cpu = data->rlock->debug.cpu; >> + lockval = data->rlock->tickets.head_tail; >> + } >> + else >> + { >> + cpu = data->lock->debug.cpu; >> + lockval = data->lock->tickets.head_tail; >> + } >> printk("%s ", lock_profile_ancs[type].name); >> if ( type != LOCKPROF_TYPE_GLOBAL ) >> printk("%d ", idx); >> - printk("%s: addr=%p, lockval=%08x, ", data->name, lock, >> - lock->tickets.head_tail); >> - if ( lock->debug.cpu == SPINLOCK_NO_CPU ) >> + printk("%s: addr=%p, lockval=%08x, ", data->name, data->lock, lockval); >> + if ( cpu == SPINLOCK_NO_CPU ) >> printk("not locked\n"); >> else >> - printk("cpu=%d\n", lock->debug.cpu); >> - printk(" lock:%" PRId64 "(%" PRI_stime "), block:%" PRId64 "(%" >> PRI_stime ")\n", >> - data->lock_cnt, data->time_hold, data->block_cnt, data->time_block); >> + printk("cpu=%u\n", cpu); >> + printk(" lock:%" PRIu64 "(%" PRI_stime "), block:%" PRIu64 "(%" >> PRI_stime ")\n", >> + data->lock_cnt, data->time_hold, (uint64_t)data->block_cnt, >> + data->time_block); >> } >> void cf_check spinlock_profile_printall(unsigned char key) >> diff --git a/xen/include/xen/spinlock.h b/xen/include/xen/spinlock.h >> index 05b97c1e03..ac3bef267a 100644 >> --- a/xen/include/xen/spinlock.h >> +++ b/xen/include/xen/spinlock.h >> @@ -76,13 +76,19 @@ union lock_debug { }; >> */ >> struct spinlock; >> +/* Temporary hack until a dedicated struct rspinlock is existing. */ >> +#define rspinlock spinlock >> struct lock_profile { >> struct lock_profile *next; /* forward link */ >> const char *name; /* lock name */ >> - struct spinlock *lock; /* the lock itself */ >> + union { >> + struct spinlock *lock; /* the lock itself */ >> + struct rspinlock *rlock; /* the recursive lock itself */ >> + }; >> uint64_t lock_cnt; /* # of complete locking ops */ >> - uint64_t block_cnt; /* # of complete wait for lock */ >> + uint64_t block_cnt:63; /* # of complete wait for lock */ >> + uint64_t is_rlock:1; /* use rlock pointer */ > > This is meant to act like a bool. So I would prefer if we use: > > bool is_rwlock:1; > > And then use true/false when set. Do we want to do that? AFAIK it would depend on the compiler what the size of the struct is when mixing types in bitfields (in this case: bool and uint64_t). > Acked-by: Julien Grall <jgrall@amazon.com> Thanks, Juergen
Hi Juergen, On 13/12/2023 06:05, Juergen Gross wrote: > On 12.12.23 19:42, Julien Grall wrote: >> Hi, >> >> On 12/12/2023 09:47, Juergen Gross wrote: >>> Struct lock_profile contains a pointer to the spinlock it is associated >>> with. Prepare support of differing spinlock_t and rspinlock_t types by >>> adding a type indicator of the pointer. Use the highest bit of the >>> block_cnt member for this indicator in order to not grow the struct >>> while hurting only the slow path with slightly less performant code. >>> >>> Signed-off-by: Juergen Gross <jgross@suse.com> >>> Acked-by: Alejandro Vallejo <alejandro.vallejo@cloud.com> >>> --- >>> V2: >>> - new patch >>> --- >>> xen/common/spinlock.c | 26 +++++++++++++++++++------- >>> xen/include/xen/spinlock.h | 10 ++++++++-- >>> 2 files changed, 27 insertions(+), 9 deletions(-) >>> >>> diff --git a/xen/common/spinlock.c b/xen/common/spinlock.c >>> index c1a9ba1304..7d611d3d7d 100644 >>> --- a/xen/common/spinlock.c >>> +++ b/xen/common/spinlock.c >>> @@ -538,19 +538,31 @@ static void >>> spinlock_profile_iterate(lock_profile_subfunc *sub, void *par) >>> static void cf_check spinlock_profile_print_elem(struct >>> lock_profile *data, >>> int32_t type, int32_t idx, void *par) >>> { >>> - struct spinlock *lock = data->lock; >>> + unsigned int cpu; >>> + uint32_t lockval; >>> + >>> + if ( data->is_rlock ) >>> + { >>> + cpu = data->rlock->debug.cpu; >>> + lockval = data->rlock->tickets.head_tail; >>> + } >>> + else >>> + { >>> + cpu = data->lock->debug.cpu; >>> + lockval = data->lock->tickets.head_tail; >>> + } >>> printk("%s ", lock_profile_ancs[type].name); >>> if ( type != LOCKPROF_TYPE_GLOBAL ) >>> printk("%d ", idx); >>> - printk("%s: addr=%p, lockval=%08x, ", data->name, lock, >>> - lock->tickets.head_tail); >>> - if ( lock->debug.cpu == SPINLOCK_NO_CPU ) >>> + printk("%s: addr=%p, lockval=%08x, ", data->name, data->lock, >>> lockval); >>> + if ( cpu == SPINLOCK_NO_CPU ) >>> printk("not locked\n"); >>> else >>> - printk("cpu=%d\n", lock->debug.cpu); >>> - printk(" lock:%" PRId64 "(%" PRI_stime "), block:%" PRId64 "(%" >>> PRI_stime ")\n", >>> - data->lock_cnt, data->time_hold, data->block_cnt, >>> data->time_block); >>> + printk("cpu=%u\n", cpu); >>> + printk(" lock:%" PRIu64 "(%" PRI_stime "), block:%" PRIu64 "(%" >>> PRI_stime ")\n", >>> + data->lock_cnt, data->time_hold, (uint64_t)data->block_cnt, >>> + data->time_block); >>> } >>> void cf_check spinlock_profile_printall(unsigned char key) >>> diff --git a/xen/include/xen/spinlock.h b/xen/include/xen/spinlock.h >>> index 05b97c1e03..ac3bef267a 100644 >>> --- a/xen/include/xen/spinlock.h >>> +++ b/xen/include/xen/spinlock.h >>> @@ -76,13 +76,19 @@ union lock_debug { }; >>> */ >>> struct spinlock; >>> +/* Temporary hack until a dedicated struct rspinlock is existing. */ >>> +#define rspinlock spinlock >>> struct lock_profile { >>> struct lock_profile *next; /* forward link */ >>> const char *name; /* lock name */ >>> - struct spinlock *lock; /* the lock itself */ >>> + union { >>> + struct spinlock *lock; /* the lock itself */ >>> + struct rspinlock *rlock; /* the recursive lock itself */ >>> + }; >>> uint64_t lock_cnt; /* # of complete locking ops */ >>> - uint64_t block_cnt; /* # of complete wait for lock */ >>> + uint64_t block_cnt:63; /* # of complete wait for lock */ >>> + uint64_t is_rlock:1; /* use rlock pointer */ >> >> This is meant to act like a bool. So I would prefer if we use: >> >> bool is_rwlock:1; >> >> And then use true/false when set. > > Do we want to do that? AFAIK it would depend on the compiler what the > size of > the struct is when mixing types in bitfields (in this case: bool and > uint64_t). I looked through our codebase and I could already find some use. Such as: typedef union { struct { uint8_t vector; uint8_t type:3; bool ev:1; uint32_t resvd1:19; bool v:1; uint32_t ec; }; uint64_t raw; } intinfo_t; So I would assume that a mix is possible or we would have problem in other places. Anyway, this is not a critical comment. It is just a preference of using true/false when a field can only be 0 or 1. Cheers,
On 13.12.2023 07:05, Juergen Gross wrote: > On 12.12.23 19:42, Julien Grall wrote: >> On 12/12/2023 09:47, Juergen Gross wrote: >>> --- a/xen/include/xen/spinlock.h >>> +++ b/xen/include/xen/spinlock.h >>> @@ -76,13 +76,19 @@ union lock_debug { }; >>> */ >>> struct spinlock; >>> +/* Temporary hack until a dedicated struct rspinlock is existing. */ >>> +#define rspinlock spinlock >>> struct lock_profile { >>> struct lock_profile *next; /* forward link */ >>> const char *name; /* lock name */ >>> - struct spinlock *lock; /* the lock itself */ >>> + union { >>> + struct spinlock *lock; /* the lock itself */ >>> + struct rspinlock *rlock; /* the recursive lock itself */ >>> + }; >>> uint64_t lock_cnt; /* # of complete locking ops */ >>> - uint64_t block_cnt; /* # of complete wait for lock */ >>> + uint64_t block_cnt:63; /* # of complete wait for lock */ >>> + uint64_t is_rlock:1; /* use rlock pointer */ >> >> This is meant to act like a bool. So I would prefer if we use: >> >> bool is_rwlock:1; >> >> And then use true/false when set. > > Do we want to do that? AFAIK it would depend on the compiler what the size of > the struct is when mixing types in bitfields (in this case: bool and uint64_t). I thought in a similar way as you did when Andrew introduced similar patterns (see Julien's reply for an example), and was then convinced that the compiler really is supposed to be doing what we want here. So yes, I second Julien's desire to have bool used when boolean is meant. Jan
On 13.12.23 09:36, Jan Beulich wrote: > On 13.12.2023 07:05, Juergen Gross wrote: >> On 12.12.23 19:42, Julien Grall wrote: >>> On 12/12/2023 09:47, Juergen Gross wrote: >>>> --- a/xen/include/xen/spinlock.h >>>> +++ b/xen/include/xen/spinlock.h >>>> @@ -76,13 +76,19 @@ union lock_debug { }; >>>> */ >>>> struct spinlock; >>>> +/* Temporary hack until a dedicated struct rspinlock is existing. */ >>>> +#define rspinlock spinlock >>>> struct lock_profile { >>>> struct lock_profile *next; /* forward link */ >>>> const char *name; /* lock name */ >>>> - struct spinlock *lock; /* the lock itself */ >>>> + union { >>>> + struct spinlock *lock; /* the lock itself */ >>>> + struct rspinlock *rlock; /* the recursive lock itself */ >>>> + }; >>>> uint64_t lock_cnt; /* # of complete locking ops */ >>>> - uint64_t block_cnt; /* # of complete wait for lock */ >>>> + uint64_t block_cnt:63; /* # of complete wait for lock */ >>>> + uint64_t is_rlock:1; /* use rlock pointer */ >>> >>> This is meant to act like a bool. So I would prefer if we use: >>> >>> bool is_rwlock:1; >>> >>> And then use true/false when set. >> >> Do we want to do that? AFAIK it would depend on the compiler what the size of >> the struct is when mixing types in bitfields (in this case: bool and uint64_t). > > I thought in a similar way as you did when Andrew introduced similar > patterns (see Julien's reply for an example), and was then convinced > that the compiler really is supposed to be doing what we want here. > So yes, I second Julien's desire to have bool used when boolean is > meant. Okay, fine with me. Juergen
On 12.12.2023 10:47, Juergen Gross wrote: > --- a/xen/common/spinlock.c > +++ b/xen/common/spinlock.c > @@ -538,19 +538,31 @@ static void spinlock_profile_iterate(lock_profile_subfunc *sub, void *par) > static void cf_check spinlock_profile_print_elem(struct lock_profile *data, > int32_t type, int32_t idx, void *par) > { > - struct spinlock *lock = data->lock; > + unsigned int cpu; > + uint32_t lockval; Any reason for this not being unsigned int as well? The more that ... > + if ( data->is_rlock ) > + { > + cpu = data->rlock->debug.cpu; > + lockval = data->rlock->tickets.head_tail; > + } > + else > + { > + cpu = data->lock->debug.cpu; > + lockval = data->lock->tickets.head_tail; > + } > > printk("%s ", lock_profile_ancs[type].name); > if ( type != LOCKPROF_TYPE_GLOBAL ) > printk("%d ", idx); > - printk("%s: addr=%p, lockval=%08x, ", data->name, lock, > - lock->tickets.head_tail); > - if ( lock->debug.cpu == SPINLOCK_NO_CPU ) > + printk("%s: addr=%p, lockval=%08x, ", data->name, data->lock, lockval); ... it's then printed with plain x as the format char. > + if ( cpu == SPINLOCK_NO_CPU ) > printk("not locked\n"); > else > - printk("cpu=%d\n", lock->debug.cpu); > - printk(" lock:%" PRId64 "(%" PRI_stime "), block:%" PRId64 "(%" PRI_stime ")\n", > - data->lock_cnt, data->time_hold, data->block_cnt, data->time_block); > + printk("cpu=%u\n", cpu); > + printk(" lock:%" PRIu64 "(%" PRI_stime "), block:%" PRIu64 "(%" PRI_stime ")\n", > + data->lock_cnt, data->time_hold, (uint64_t)data->block_cnt, I think I know why the cast is suddenly / unexpectedly needed, but imo such wants stating in the description, when generally we aim at avoiding casts where possible. > --- a/xen/include/xen/spinlock.h > +++ b/xen/include/xen/spinlock.h > @@ -76,13 +76,19 @@ union lock_debug { }; > */ > > struct spinlock; > +/* Temporary hack until a dedicated struct rspinlock is existing. */ > +#define rspinlock spinlock > > struct lock_profile { > struct lock_profile *next; /* forward link */ > const char *name; /* lock name */ > - struct spinlock *lock; /* the lock itself */ > + union { > + struct spinlock *lock; /* the lock itself */ > + struct rspinlock *rlock; /* the recursive lock itself */ > + }; _LOCK_PROFILE() wants to initialize this field, unconditionally using .lock. While I expect that problem to be taken care of in one of the later patches, use of the macro won't work anymore with this union in use with very old gcc that formally we still support. While a road to generally raising the baseline requirements is still pretty unclear to me, an option might be to require (and document) that to enable DEBUG_LOCK_PROFILE somewhat newer gcc needs using. > uint64_t lock_cnt; /* # of complete locking ops */ > - uint64_t block_cnt; /* # of complete wait for lock */ > + uint64_t block_cnt:63; /* # of complete wait for lock */ > + uint64_t is_rlock:1; /* use rlock pointer */ bool? Jan
On 28.02.24 16:19, Jan Beulich wrote: > On 12.12.2023 10:47, Juergen Gross wrote: >> --- a/xen/common/spinlock.c >> +++ b/xen/common/spinlock.c >> @@ -538,19 +538,31 @@ static void spinlock_profile_iterate(lock_profile_subfunc *sub, void *par) >> static void cf_check spinlock_profile_print_elem(struct lock_profile *data, >> int32_t type, int32_t idx, void *par) >> { >> - struct spinlock *lock = data->lock; >> + unsigned int cpu; >> + uint32_t lockval; > > Any reason for this not being unsigned int as well? The more that ... > >> + if ( data->is_rlock ) >> + { >> + cpu = data->rlock->debug.cpu; >> + lockval = data->rlock->tickets.head_tail; >> + } >> + else >> + { >> + cpu = data->lock->debug.cpu; >> + lockval = data->lock->tickets.head_tail; >> + } I've used the same type as tickets.head_tail. >> >> printk("%s ", lock_profile_ancs[type].name); >> if ( type != LOCKPROF_TYPE_GLOBAL ) >> printk("%d ", idx); >> - printk("%s: addr=%p, lockval=%08x, ", data->name, lock, >> - lock->tickets.head_tail); >> - if ( lock->debug.cpu == SPINLOCK_NO_CPU ) >> + printk("%s: addr=%p, lockval=%08x, ", data->name, data->lock, lockval); > > ... it's then printed with plain x as the format char. Which hasn't been changed by the patch. I can change it to PRIx32 if you want. > >> + if ( cpu == SPINLOCK_NO_CPU ) >> printk("not locked\n"); >> else >> - printk("cpu=%d\n", lock->debug.cpu); >> - printk(" lock:%" PRId64 "(%" PRI_stime "), block:%" PRId64 "(%" PRI_stime ")\n", >> - data->lock_cnt, data->time_hold, data->block_cnt, data->time_block); >> + printk("cpu=%u\n", cpu); >> + printk(" lock:%" PRIu64 "(%" PRI_stime "), block:%" PRIu64 "(%" PRI_stime ")\n", >> + data->lock_cnt, data->time_hold, (uint64_t)data->block_cnt, > > I think I know why the cast is suddenly / unexpectedly needed, but imo > such wants stating in the description, when generally we aim at avoiding > casts where possible. Okay, will add a sentence. > >> --- a/xen/include/xen/spinlock.h >> +++ b/xen/include/xen/spinlock.h >> @@ -76,13 +76,19 @@ union lock_debug { }; >> */ >> >> struct spinlock; >> +/* Temporary hack until a dedicated struct rspinlock is existing. */ >> +#define rspinlock spinlock >> >> struct lock_profile { >> struct lock_profile *next; /* forward link */ >> const char *name; /* lock name */ >> - struct spinlock *lock; /* the lock itself */ >> + union { >> + struct spinlock *lock; /* the lock itself */ >> + struct rspinlock *rlock; /* the recursive lock itself */ >> + }; > > _LOCK_PROFILE() wants to initialize this field, unconditionally using > .lock. While I expect that problem to be taken care of in one of the > later patches, use of the macro won't work anymore with this union in > use with very old gcc that formally we still support. While a road to > generally raising the baseline requirements is still pretty unclear to > me, an option might be to require (and document) that to enable > DEBUG_LOCK_PROFILE somewhat newer gcc needs using. Patch 8 is using either .lock or .rlock depending on the lock type. What is the problem with the old gcc version? Static initializers of anonymous union members? > >> uint64_t lock_cnt; /* # of complete locking ops */ >> - uint64_t block_cnt; /* # of complete wait for lock */ >> + uint64_t block_cnt:63; /* # of complete wait for lock */ >> + uint64_t is_rlock:1; /* use rlock pointer */ > > bool? Yes. Juergen
On 28.02.2024 16:43, Jürgen Groß wrote: > On 28.02.24 16:19, Jan Beulich wrote: >> On 12.12.2023 10:47, Juergen Gross wrote: >>> --- a/xen/common/spinlock.c >>> +++ b/xen/common/spinlock.c >>> @@ -538,19 +538,31 @@ static void spinlock_profile_iterate(lock_profile_subfunc *sub, void *par) >>> static void cf_check spinlock_profile_print_elem(struct lock_profile *data, >>> int32_t type, int32_t idx, void *par) >>> { >>> - struct spinlock *lock = data->lock; >>> + unsigned int cpu; >>> + uint32_t lockval; >> >> Any reason for this not being unsigned int as well? The more that ... >> >>> + if ( data->is_rlock ) >>> + { >>> + cpu = data->rlock->debug.cpu; >>> + lockval = data->rlock->tickets.head_tail; >>> + } >>> + else >>> + { >>> + cpu = data->lock->debug.cpu; >>> + lockval = data->lock->tickets.head_tail; >>> + } > > I've used the same type as tickets.head_tail. > >>> >>> printk("%s ", lock_profile_ancs[type].name); >>> if ( type != LOCKPROF_TYPE_GLOBAL ) >>> printk("%d ", idx); >>> - printk("%s: addr=%p, lockval=%08x, ", data->name, lock, >>> - lock->tickets.head_tail); >>> - if ( lock->debug.cpu == SPINLOCK_NO_CPU ) >>> + printk("%s: addr=%p, lockval=%08x, ", data->name, data->lock, lockval); >> >> ... it's then printed with plain x as the format char. > > Which hasn't been changed by the patch. I can change it to PRIx32 if you want. As per ./CODING_STYLE unsigned int is preferred. >>> --- a/xen/include/xen/spinlock.h >>> +++ b/xen/include/xen/spinlock.h >>> @@ -76,13 +76,19 @@ union lock_debug { }; >>> */ >>> >>> struct spinlock; >>> +/* Temporary hack until a dedicated struct rspinlock is existing. */ >>> +#define rspinlock spinlock >>> >>> struct lock_profile { >>> struct lock_profile *next; /* forward link */ >>> const char *name; /* lock name */ >>> - struct spinlock *lock; /* the lock itself */ >>> + union { >>> + struct spinlock *lock; /* the lock itself */ >>> + struct rspinlock *rlock; /* the recursive lock itself */ >>> + }; >> >> _LOCK_PROFILE() wants to initialize this field, unconditionally using >> .lock. While I expect that problem to be taken care of in one of the >> later patches, use of the macro won't work anymore with this union in >> use with very old gcc that formally we still support. While a road to >> generally raising the baseline requirements is still pretty unclear to >> me, an option might be to require (and document) that to enable >> DEBUG_LOCK_PROFILE somewhat newer gcc needs using. > > Patch 8 is using either .lock or .rlock depending on the lock type. > > What is the problem with the old gcc version? Static initializers of > anonymous union members? Yes. Jan
On 28.02.24 17:02, Jan Beulich wrote: > On 28.02.2024 16:43, Jürgen Groß wrote: >> On 28.02.24 16:19, Jan Beulich wrote: >>> On 12.12.2023 10:47, Juergen Gross wrote: >>>> --- a/xen/common/spinlock.c >>>> +++ b/xen/common/spinlock.c >>>> @@ -538,19 +538,31 @@ static void spinlock_profile_iterate(lock_profile_subfunc *sub, void *par) >>>> static void cf_check spinlock_profile_print_elem(struct lock_profile *data, >>>> int32_t type, int32_t idx, void *par) >>>> { >>>> - struct spinlock *lock = data->lock; >>>> + unsigned int cpu; >>>> + uint32_t lockval; >>> >>> Any reason for this not being unsigned int as well? The more that ... >>> >>>> + if ( data->is_rlock ) >>>> + { >>>> + cpu = data->rlock->debug.cpu; >>>> + lockval = data->rlock->tickets.head_tail; >>>> + } >>>> + else >>>> + { >>>> + cpu = data->lock->debug.cpu; >>>> + lockval = data->lock->tickets.head_tail; >>>> + } >> >> I've used the same type as tickets.head_tail. >> >>>> >>>> printk("%s ", lock_profile_ancs[type].name); >>>> if ( type != LOCKPROF_TYPE_GLOBAL ) >>>> printk("%d ", idx); >>>> - printk("%s: addr=%p, lockval=%08x, ", data->name, lock, >>>> - lock->tickets.head_tail); >>>> - if ( lock->debug.cpu == SPINLOCK_NO_CPU ) >>>> + printk("%s: addr=%p, lockval=%08x, ", data->name, data->lock, lockval); >>> >>> ... it's then printed with plain x as the format char. >> >> Which hasn't been changed by the patch. I can change it to PRIx32 if you want. > > As per ./CODING_STYLE unsigned int is preferred. Okay, I'll switch to unsigned int then. > >>>> --- a/xen/include/xen/spinlock.h >>>> +++ b/xen/include/xen/spinlock.h >>>> @@ -76,13 +76,19 @@ union lock_debug { }; >>>> */ >>>> >>>> struct spinlock; >>>> +/* Temporary hack until a dedicated struct rspinlock is existing. */ >>>> +#define rspinlock spinlock >>>> >>>> struct lock_profile { >>>> struct lock_profile *next; /* forward link */ >>>> const char *name; /* lock name */ >>>> - struct spinlock *lock; /* the lock itself */ >>>> + union { >>>> + struct spinlock *lock; /* the lock itself */ >>>> + struct rspinlock *rlock; /* the recursive lock itself */ >>>> + }; >>> >>> _LOCK_PROFILE() wants to initialize this field, unconditionally using >>> .lock. While I expect that problem to be taken care of in one of the >>> later patches, use of the macro won't work anymore with this union in >>> use with very old gcc that formally we still support. While a road to >>> generally raising the baseline requirements is still pretty unclear to >>> me, an option might be to require (and document) that to enable >>> DEBUG_LOCK_PROFILE somewhat newer gcc needs using. >> >> Patch 8 is using either .lock or .rlock depending on the lock type. >> >> What is the problem with the old gcc version? Static initializers of >> anonymous union members? > > Yes. The easiest solution might then be to give the union a name. Juergen
diff --git a/xen/common/spinlock.c b/xen/common/spinlock.c index c1a9ba1304..7d611d3d7d 100644 --- a/xen/common/spinlock.c +++ b/xen/common/spinlock.c @@ -538,19 +538,31 @@ static void spinlock_profile_iterate(lock_profile_subfunc *sub, void *par) static void cf_check spinlock_profile_print_elem(struct lock_profile *data, int32_t type, int32_t idx, void *par) { - struct spinlock *lock = data->lock; + unsigned int cpu; + uint32_t lockval; + + if ( data->is_rlock ) + { + cpu = data->rlock->debug.cpu; + lockval = data->rlock->tickets.head_tail; + } + else + { + cpu = data->lock->debug.cpu; + lockval = data->lock->tickets.head_tail; + } printk("%s ", lock_profile_ancs[type].name); if ( type != LOCKPROF_TYPE_GLOBAL ) printk("%d ", idx); - printk("%s: addr=%p, lockval=%08x, ", data->name, lock, - lock->tickets.head_tail); - if ( lock->debug.cpu == SPINLOCK_NO_CPU ) + printk("%s: addr=%p, lockval=%08x, ", data->name, data->lock, lockval); + if ( cpu == SPINLOCK_NO_CPU ) printk("not locked\n"); else - printk("cpu=%d\n", lock->debug.cpu); - printk(" lock:%" PRId64 "(%" PRI_stime "), block:%" PRId64 "(%" PRI_stime ")\n", - data->lock_cnt, data->time_hold, data->block_cnt, data->time_block); + printk("cpu=%u\n", cpu); + printk(" lock:%" PRIu64 "(%" PRI_stime "), block:%" PRIu64 "(%" PRI_stime ")\n", + data->lock_cnt, data->time_hold, (uint64_t)data->block_cnt, + data->time_block); } void cf_check spinlock_profile_printall(unsigned char key) diff --git a/xen/include/xen/spinlock.h b/xen/include/xen/spinlock.h index 05b97c1e03..ac3bef267a 100644 --- a/xen/include/xen/spinlock.h +++ b/xen/include/xen/spinlock.h @@ -76,13 +76,19 @@ union lock_debug { }; */ struct spinlock; +/* Temporary hack until a dedicated struct rspinlock is existing. */ +#define rspinlock spinlock struct lock_profile { struct lock_profile *next; /* forward link */ const char *name; /* lock name */ - struct spinlock *lock; /* the lock itself */ + union { + struct spinlock *lock; /* the lock itself */ + struct rspinlock *rlock; /* the recursive lock itself */ + }; uint64_t lock_cnt; /* # of complete locking ops */ - uint64_t block_cnt; /* # of complete wait for lock */ + uint64_t block_cnt:63; /* # of complete wait for lock */ + uint64_t is_rlock:1; /* use rlock pointer */ s_time_t time_hold; /* cumulated lock time */ s_time_t time_block; /* cumulated wait time */ s_time_t time_locked; /* system time of last locking */