Re: [RFC PATCH 0/3] xen/spinlock: make recursive spinlocks a dedicated type

[Juergen Gross <jgross@xxxxxxxx>]

On 15.12.22 08:48, Jan Beulich wrote:
> On 14.12.2022 17:36, Juergen Gross wrote:
> > On 14.12.22 17:25, Daniel P. Smith wrote:
> > > On 12/14/22 10:31, Juergen Gross wrote:
> > > > On 14.12.22 16:03, Daniel P. Smith wrote:
> > > > > On 9/10/22 11:49, Juergen Gross wrote:
> > > > > > Instead of being able to use normal spinlocks as recursive ones, too,
> > > > > > make recursive spinlocks a special lock type.
> > > > > >
> > > > > > This will make the spinlock structure smaller in production builds and
> > > > > > add type-safety.
> > > > > Just a little yak shaving, IMHO a spinlock is normally not expected to be
> > > > > recursive. Thus explicitly naming a spinlock as non-recursive I find to be
> > > > > redundant along with being expensive for typing. Whereas a recursive spinlock
> > > > > is the special instance and should have a declarative distinction. Only
> > > > > codifying the recursive type would significantly cut down on the size of the
> > > > > series and still provide equal type and visual clarification.
> > > > A "normal" spinlock is non-recursive.
> > > >
> > > > A recursive spinlock in Xen can be either taken recursive, or it can be taken
> > > > non-recursive, causing further recursive attempts to spin.
> > > Yes, I understand the current situation.
> > >
> > > > So the explicit non-recursive locking applies to that special treatment of
> > > > recursive spinlocks.
> > > I understand this, but to help clarify what I am saying is that individuals
> > > coming from outside Xen would expect is the spinlock family of calls to behave
> > > as a non-recursive spinlocks and recursive spinlock family of calls would
> > > provide the recursive behavior. Currently Xen's behavior is backwards to this,
> > > which this series continues and is a valid approach. Here spinlock and recursive
> > > spinlock family of calls are recursive and must use non-recursive spinlock
> > > family to have "normal" spinlock behavior. IMHO it would greatly simplify the
> > My series doesn't change treatment of normal spinlocks. They are still used via
> > spin_{lock,locked,unlock}.
> >
> > > code and align with the "normal" understanding of spinlocks if instead
> > > spin_{lock,locked,unlock} macros were the non-recursive calls and
> > > spin_{lock,locked,unlock}_recursive macros were the recursive calls. Then there
> > > would only be two suites of calls for spinlocks and a lot less keystrokes for
> > > need for most development.
> > Only the recursive spinlock type user must specify, whether a lock is meant to
> > be handled as a recursive or a non-recursive lock attempt. This is similar to
> > a rwlock, where the user must specify whether to lock as a reader or a writer.
> While I can't come up with anything neat right away, it feels like it should be
> possible to come up with some trickery to make spin_lock() usable on both lock
> types, eliminating the need to ..._nonrecursive() variants visible at use sites
> (they may still be necessary as helpers then). At least if a spinlock_t instance
> wasn't embedded in the recursive lock struct (as I did suggest), then something
> along the lines of what tgmath.h does may be possible.
Might be, but do we really want that?

Wouldn't it make more sense to let the user explicitly say that he wants a lock
to be taken non-recursively? Allowing "spin_lock()" would add some more risk to
use it by accident e.g. because of copy/paste without noticing that it is a
recursive lock that is taken non-recursively. Same applies for patch reviews.

I'd prefer to make this easily visible.


Juergen

Attachment: OpenPGP_0xB0DE9DD628BF132F.asc
Description: OpenPGP public key

Attachment: OpenPGP_signature
Description: OpenPGP digital signature