Re: [PATCH v3 04/13] xen/spinlock: introduce new type for recursive spinlocks

[Juergen Gross <jgross@xxxxxxxx>]

On 24.11.23 19:59, Alejandro Vallejo wrote:
> On 20/11/2023 11:38, Juergen Gross wrote:
> > Introduce a new type "rspinlock_t" to be used for recursive spinlocks.
> >
> > For now it is only an alias of spinlock_t, so both types can still be
> > used for recursive spinlocks. This will be changed later, though.
> >
> > Switch all recursive spinlocks to the new type.
> >
> > Define the initializer helpers and use them where appropriate.
> >
> > Signed-off-by: Juergen Gross <jgross@xxxxxxxx>
> > ---
> > V2:
> > - carved out from V1 patch
> > ---
> >   xen/arch/x86/include/asm/mm.h |  2 +-
> >   xen/arch/x86/mm/mm-locks.h    |  2 +-
> >   xen/common/domain.c           |  4 ++--
> >   xen/common/ioreq.c            |  2 +-
> >   xen/drivers/char/console.c    |  4 ++--
> >   xen/drivers/passthrough/pci.c |  2 +-
> >   xen/include/xen/sched.h       |  6 +++---
> >   xen/include/xen/spinlock.h    | 19 +++++++++++++++----
> >   8 files changed, 26 insertions(+), 15 deletions(-)
> >
> > diff --git a/xen/arch/x86/include/asm/mm.h b/xen/arch/x86/include/asm/mm.h
> > index 05dfe35502..8a6e0c283f 100644
> > --- a/xen/arch/x86/include/asm/mm.h
> > +++ b/xen/arch/x86/include/asm/mm.h
> > @@ -596,7 +596,7 @@ unsigned long domain_get_maximum_gpfn(struct domain *d);
> >   /* Definition of an mm lock: spinlock with extra fields for debugging */
> >   typedef struct mm_lock {
> > -    spinlock_t         lock;
> > +    rspinlock_t        lock;
> Considering it's rspinlock_t rather than spinlock_recursive, do we want
> to change also mm_lock_recursive() and paging_lock_recursive() into
> mm_rlock() and paging_rlock() ?
I don't think this should be part of this series.

> > diff --git a/xen/include/xen/spinlock.h b/xen/include/xen/spinlock.h
> > index bbe1472571..19561d5e61 100644
> > --- a/xen/include/xen/spinlock.h
> > +++ b/xen/include/xen/spinlock.h
>  > [snip]
> > @@ -182,8 +191,10 @@ typedef struct spinlock {
> >   #endif
> >   } spinlock_t;
> > +typedef spinlock_t rspinlock_t;
> >   #define spin_lock_init(l) (*(l) = (spinlock_t)SPIN_LOCK_UNLOCKED)
> > +#define rspin_lock_init(l) (*(l) = (rspinlock_t)SPIN_LOCK_UNLOCKED)
> nit: Both variants of [r]spin_lock_init(l) could be inline functions
> rather than macros.
I was following the spin_lock_init() example, and I guess that is following
the Linux kernel example.

I don't mind either way, but maybe other maintainers have a preference?


Juergen

Attachment: OpenPGP_0xB0DE9DD628BF132F.asc
Description: OpenPGP public key

Attachment: OpenPGP_signature.asc
Description: OpenPGP digital signature