Re: [PATCH v4 10/12] xen/spinlock: split recursive spinlocks from normal ones

From: Jan Beulich <jbeulich@suse.com>
To: Juergen Gross <jgross@suse.com>
Cc: Andrew Cooper <andrew.cooper3@citrix.com>,
	George Dunlap <george.dunlap@citrix.com>,
	Julien Grall <julien@xen.org>,
	Stefano Stabellini <sstabellini@kernel.org>, Wei Liu <wl@xen.org>,
	xen-devel@lists.xenproject.org
Subject: Re: [PATCH v4 10/12] xen/spinlock: split recursive spinlocks from normal ones
Date: Thu, 29 Feb 2024 16:32:16 +0100	[thread overview]
Message-ID: <a037facf-c4d9-491f-b39f-56ed4221abf1@suse.com> (raw)
In-Reply-To: <20231212094725.22184-11-jgross@suse.com>

On 12.12.2023 10:47, Juergen Gross wrote:
> --- a/xen/common/spinlock.c
> +++ b/xen/common/spinlock.c
> @@ -541,6 +541,55 @@ void rspin_unlock_irqrestore(rspinlock_t *lock, unsigned long flags)
>      local_irq_restore(flags);
>  }
>  
> +int nrspin_trylock(rspinlock_t *lock)
> +{
> +    check_lock(&lock->debug, true);
> +
> +    if ( unlikely(lock->recurse_cpu != SPINLOCK_NO_CPU) )
> +        return 0;
> +
> +    return spin_trylock_common(&lock->tickets, &lock->debug, LOCK_PROFILE_PAR);
> +}

I wonder if we shouldn't take the opportunity and stop having trylock
functions have "int" return type. Perhaps already spin_trylock_common()
when introduced could use "bool" instead, then here following suit.

> +void nrspin_lock(rspinlock_t *lock)
> +{
> +    spin_lock_common(&lock->tickets, &lock->debug, LOCK_PROFILE_PAR, NULL,
> +                     NULL);
> +}
> +
> +void nrspin_unlock(rspinlock_t *lock)
> +{
> +    spin_unlock_common(&lock->tickets, &lock->debug, LOCK_PROFILE_PAR);
> +}
> +
> +void nrspin_lock_irq(rspinlock_t *lock)
> +{
> +    ASSERT(local_irq_is_enabled());
> +    local_irq_disable();
> +    nrspin_lock(lock);
> +}
> +
> +void nrspin_unlock_irq(rspinlock_t *lock)
> +{
> +    nrspin_unlock(lock);
> +    local_irq_enable();
> +}
> +
> +unsigned long __nrspin_lock_irqsave(rspinlock_t *lock)
> +{
> +    unsigned long flags;
> +
> +    local_irq_save(flags);
> +    nrspin_lock(lock);
> +    return flags;

Nit: Strictly speaking we want a blank line ahead of this "return".

> @@ -166,11 +172,15 @@ struct lock_profile_qhead { };
>  struct lock_profile { };
>  
>  #define SPIN_LOCK_UNLOCKED {                                                  \
> +    .debug =_LOCK_DEBUG,                                                      \
> +}
> +#define RSPIN_LOCK_UNLOCKED {                                                 \
> +    .debug =_LOCK_DEBUG,                                                      \
>      .recurse_cpu = SPINLOCK_NO_CPU,                                           \
>      .debug =_LOCK_DEBUG,                                                      \
>  }

Initializing .debug twice?

> @@ -180,7 +190,6 @@ struct lock_profile { };
>  
>  #endif
>  
> -
>  typedef union {
>      uint32_t head_tail;
>      struct {

Looks like this might be undoing what the earlier patch isn't going to
do anymore?

> @@ -242,6 +257,19 @@ void rspin_unlock_irqrestore(rspinlock_t *lock, unsigned long flags);
>  int rspin_is_locked(const rspinlock_t *lock);
>  void rspin_barrier(rspinlock_t *lock);
>  
> +int nrspin_trylock(rspinlock_t *lock);
> +void nrspin_lock(rspinlock_t *lock);
> +void nrspin_unlock(rspinlock_t *lock);
> +void nrspin_lock_irq(rspinlock_t *lock);
> +void nrspin_unlock_irq(rspinlock_t *lock);
> +#define nrspin_lock_irqsave(l, f)                               \
> +    ({                                                          \
> +        BUILD_BUG_ON(sizeof(f) != sizeof(unsigned long));       \
> +        ((f) = __nrspin_lock_irqsave(l));                       \

I don't think the outer pair of parentheses is needed here. As to the
leading double underscores, see comments elsewhere.

Jan