Re: [PATCH 2/2] xen/spinlock: merge recurse_cpu and debug.cpu fields in struct spinlock

[Juergen Gross <jgross@xxxxxxxx>]

On 25.02.22 09:36, Juergen Gross wrote:
> On 24.02.22 17:11, Jan Beulich wrote:
> > On 24.02.2022 11:54, Juergen Gross wrote:
> > > --- a/xen/arch/x86/mm/mm-locks.h
> > > +++ b/xen/arch/x86/mm/mm-locks.h
> > > @@ -42,7 +42,7 @@ static inline void mm_lock_init(mm_lock_t *l)
> > >   static inline bool mm_locked_by_me(const mm_lock_t *l)
> > >   {
> > > -    return (l->lock.recurse_cpu == current->processor);
> > > +    return (l->lock.data.cpu == current->processor);
> > >   }
> > I see a fair risk with this: Behavior will now differ between debug and
> > non-debug builds. E.g. a livelock because of trying to acquire the same
> > lock again would not be noticed in a debug build if the acquire is
> > conditional upon this function's return value. I think this is the main
> > reason behind having two separate field, despite the apparent redundancy.
> You are aware that mm_locked_by_me() is used for recursive spinlocks
> only?
BTW, it might make sense to add another bool for the debug case to mark
recursive lock usage. I don't think anything good can come from using a
lock in both modes (recursive and non-recursive).


Juergen