[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [PATCH v3 02/13] xen/spinlock: reduce lock profile ifdefs
Hi, On 20/11/2023 11:38, Juergen Gross wrote:> With some small adjustments to the LOCK_PROFILE_* macros some #ifdefs can be dropped from spinlock.c. Signed-off-by: Juergen Gross <jgross@xxxxxxxx> --- V2: - new patch V3: - add variable name to macros parameter (Jan Beulich) --- xen/common/spinlock.c | 49 +++++++++++++++++++------------------------ 1 file changed, 21 insertions(+), 28 deletions(-) diff --git a/xen/common/spinlock.c b/xen/common/spinlock.c index d7194e518c..ce18fbdd8a 100644 --- a/xen/common/spinlock.c +++ b/xen/common/spinlock.c @@ -267,25 +267,28 @@ void spin_debug_disable(void) lock->profile->time_hold += NOW() - lock->profile->time_locked; \ lock->profile->lock_cnt++; \ } -#define LOCK_PROFILE_VAR s_time_t block = 0 -#define LOCK_PROFILE_BLOCK block = block ? : NOW(); -#define LOCK_PROFILE_GOT \ +#define LOCK_PROFILE_VAR(var, val) s_time_t var = (val) +#define LOCK_PROFILE_BLOCK(var ) var = var ? : NOW()nit: spaces before the closing parenthesis And that's as far as I can complain on your changes. As for things that were already present... +#define LOCK_PROFILE_BLKACC(tst, val) \ + if ( tst ) \ + { \ + lock->profile->time_block += lock->profile->time_locked - (val); \ + lock->profile->block_cnt++; \ + }> +#define LOCK_PROFILE_GOT(val) \ if ( lock->profile ) \ { \ lock->profile->time_locked = NOW(); \ - if ( block ) \ - { \ - lock->profile->time_block += lock->profile->time_locked - block; \ - lock->profile->block_cnt++; \ - } \ + LOCK_PROFILE_BLKACC(val, val); \ }... these 2 probably want `lock` to be the first argument so they don't rely on the variable "lock" to be in context, and... #else #define LOCK_PROFILE_REL-#define LOCK_PROFILE_VAR -#define LOCK_PROFILE_BLOCK -#define LOCK_PROFILE_GOT +#define LOCK_PROFILE_VAR(var, val) +#define LOCK_PROFILE_BLOCK(var) +#define LOCK_PROFILE_BLKACC(tst, val) +#define LOCK_PROFILE_GOT(val)... I'd feel better if these where actually statements rather than fully empty. i.e: (void)0, or something like that. Then they would behave well in conditionals without braces. #endif @@ -308,7 +311,7 @@ static void always_inline spin_lock_common(spinlock_t *lock,void (*cb)(void *), void *data) { spinlock_tickets_t tickets = SPINLOCK_TICKET_INC; - LOCK_PROFILE_VAR; + LOCK_PROFILE_VAR(block, 0);check_lock(&lock->debug, false);preempt_disable(); @@ -316,14 +319,14 @@ static void always_inline spin_lock_common(spinlock_t *lock, tickets.head_tail); while ( tickets.tail != observe_head(&lock->tickets) ) { - LOCK_PROFILE_BLOCK; + LOCK_PROFILE_BLOCK(block); if ( cb ) cb(data); arch_lock_relax(); } arch_lock_acquire_barrier(); got_lock(&lock->debug); - LOCK_PROFILE_GOT; + LOCK_PROFILE_GOT(block); }void _spin_lock(spinlock_t *lock)@@ -411,19 +414,15 @@ int _spin_trylock(spinlock_t *lock) * arch_lock_acquire_barrier(). */ got_lock(&lock->debug); -#ifdef CONFIG_DEBUG_LOCK_PROFILE - if ( lock->profile ) - lock->profile->time_locked = NOW(); -#endif + LOCK_PROFILE_GOT(0); + return 1; }void _spin_barrier(spinlock_t *lock){ spinlock_tickets_t sample; -#ifdef CONFIG_DEBUG_LOCK_PROFILE - s_time_t block = NOW(); -#endif + LOCK_PROFILE_VAR(block, NOW());check_barrier(&lock->debug);smp_mb(); @@ -432,13 +431,7 @@ void _spin_barrier(spinlock_t *lock) { while ( observe_head(&lock->tickets) == sample.head ) arch_lock_relax(); -#ifdef CONFIG_DEBUG_LOCK_PROFILE - if ( lock->profile ) - { - lock->profile->time_block += NOW() - block; - lock->profile->block_cnt++; - } -#endif + LOCK_PROFILE_BLKACC(lock->profile, block); } smp_mb(); } Besides the first nit, the others were already present and the usage of the macros is very localised, so take it or leave it. It's fairly inconsequential. Otherwise LGTM. With the 1st nit sorted Reviewed-by: Alejandro Vallejo <alejandro.vallejo@xxxxxxxxx> Cheers, Alejandro
|
Lists.xenproject.org is hosted with RackSpace, monitoring our |