[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [PATCH v4 06/12] xen/spinlock: make struct lock_profile rspinlock_t aware
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
|
![]() |
Lists.xenproject.org is hosted with RackSpace, monitoring our |