Re: [PATCH v4 03/12] xen/spinlock: introduce new type for recursive spinlocks

[Julien Grall <julien@xxxxxxx>]

Hi Juergen,

On 12/12/2023 13:04, Juergen Gross wrote:
> On 12.12.23 13:57, Julien Grall wrote:
> > Hi Juergen,
> >
> > On 12/12/2023 09:47, Juergen Gross wrote:
> > > diff --git a/xen/include/xen/spinlock.h b/xen/include/xen/spinlock.h
> > > index 1cd9120eac..20d15f34dd 100644
> > > --- a/xen/include/xen/spinlock.h
> > > +++ b/xen/include/xen/spinlock.h
> > > @@ -45,7 +45,7 @@ union lock_debug { };
> > >       lock profiling on:
> > >       Global locks which should be subject to profiling must be 
> > > declared via
> > > -    DEFINE_SPINLOCK.
> > > +    DEFINE_[R]SPINLOCK.
> > >       For locks in structures further measures are necessary:
> > >       - the structure definition must include a profile_head with 
> > > exactly this
> > > @@ -56,7 +56,7 @@ union lock_debug { };
> > >       - the single locks which are subject to profiling have to be 
> > > initialized
> > >         via
> > > -      spin_lock_init_prof(ptr, lock);
> > > +      [r]spin_lock_init_prof(ptr, lock);
> > >         with ptr being the main structure pointer and lock the 
> > > spinlock field
> > > @@ -109,12 +109,16 @@ struct lock_profile_qhead {
> > >       spinlock_t l = 
> > > _SPIN_LOCK_UNLOCKED(NULL);                                 \
> > >       static struct lock_profile __lock_profile_data_##l = 
> > > _LOCK_PROFILE(l);    \
> > >       _LOCK_PROFILE_PTR(l)
> > > +#define 
> > > DEFINE_RSPINLOCK(l)                                                   \
> > > +    rspinlock_t l = 
> > > _SPIN_LOCK_UNLOCKED(NULL);                                \
> > > +    static struct lock_profile __lock_profile_data_##l = 
> > > _LOCK_PROFILE(l);    \
> > > +    _LOCK_PROFILE_PTR(l)
> > > -#define spin_lock_init_prof(s, 
> > > l)                                             \
> > > +#define __spin_lock_init_prof(s, l, 
> > > locktype)                                 \
> > If I am not mistaken the double-underscore prefix is a violation in 
> > MISRA. So can this be renamed to:
> > spin_lock_init_prof__()?
> Fine with me.
>
> Note that __lock_profile_data_##l and __lock_profile_##name seem to violate
> MISRA, too. Probably a good idea to change them as well? This should 
> probably
> be done as a prereq patch then?
It would be preferable if this is done in a separate patch. But I don't 
mind if this is not part of this series or if you don't want to do it. 
They are existing violations and not the only ones in the codebase.
But we should try to avoid adding new ones.

Cheers,

--
Julien Grall