Re: [PATCH v6 6/8] xen/spinlock: support higher number of cpus

[Jürgen Groß <jgross@xxxxxxxx>]

On 02.04.24 16:42, Jan Beulich wrote:
> On 27.03.2024 16:22, Juergen Gross wrote:
> > Allow 16 bits per cpu number, which is the limit imposed by
> > spinlock_tickets_t.
> >
> > This will allow up to 65535 cpus, while increasing only the size of
> > recursive spinlocks in debug builds from 8 to 12 bytes.
> >
> > The current Xen limit of 4095 cpus is imposed by SPINLOCK_CPU_BITS
> > being 12. There are machines available with more cpus than the current
> > Xen limit, so it makes sense to have the possibility to use more cpus.
> >
> > Signed-off-by: Juergen Gross <jgross@xxxxxxxx>
> Reviewed-by: Jan Beulich <jbeulich@xxxxxxxx>
> albeit I have to say that I'm not entirely convinced of ...
>
> > --- a/xen/common/spinlock.c
> > +++ b/xen/common/spinlock.c
> > @@ -485,7 +485,9 @@ bool _rspin_trylock(rspinlock_t *lock)
> >   
> >       /* Don't allow overflow of recurse_cpu field. */
> >       BUILD_BUG_ON(NR_CPUS > SPINLOCK_NO_CPU);
> > +    BUILD_BUG_ON(SPINLOCK_CPU_BITS > sizeof(lock->recurse_cpu) * 8);
> >       BUILD_BUG_ON(SPINLOCK_RECURSE_BITS < 3);
> > +    BUILD_BUG_ON(SPINLOCK_MAX_RECURSE > ((1u << SPINLOCK_RECURSE_BITS) - 1));
> >   
> >       check_lock(&lock->debug, true);
> ... the two additions here: The two checks we had verify independent
> properties, whereas the new ones basically check that struct rspinlock
> and its associated #define-s were got right. We don't check such
> elsewhere, I don't think.
I think we do.

What about:
  BUILD_BUG_ON(sizeof(hwp_req) != sizeof(hwp_req.raw))
checking that two union elements are of the same size (and both elements don't
contain any other structs).

Additionally it is not obvious at a first glance that SPINLOCK_CPU_BITS defined
in line 11 is relevant for the definition of recurse_cpu in line 217.

Regarding the second added BUILD_BUG_ON() there was a comment by Julien related
to the definition of SPINLOCK_MAX_RECURSE in V4 of this patch. We settled to use
the current form including the added BUILD_BUG_ON().


Juergen