[NAK] Re: [PATCH 11/20] locking/osq: Export osq_(lock|unlock)

[Ingo Molnar]

* Kent Overstreet <kent.overstreet@xxxxxxxxx> wrote:

> On Wed, Aug 02, 2023 at 05:09:13PM -0400, Waiman Long wrote:
> > On 8/2/23 16:44, Kent Overstreet wrote:
> > > On Wed, Aug 02, 2023 at 04:16:12PM -0400, Waiman Long wrote:
> > > > On 7/12/23 17:11, Kent Overstreet wrote:
> > > > > These are used by bcachefs's six locks.
> > > > > 
> > > > > Signed-off-by: Kent Overstreet <kent.overstreet@xxxxxxxxx>
> > > > > Cc: Peter Zijlstra <peterz@xxxxxxxxxxxxx>
> > > > > Cc: Ingo Molnar <mingo@xxxxxxxxxx>
> > > > > Cc: Waiman Long <longman@xxxxxxxxxx>
> > > > > Cc: Boqun Feng <boqun.feng@xxxxxxxxx>
> > > > > ---
> > > > >    kernel/locking/osq_lock.c | 2 ++
> > > > >    1 file changed, 2 insertions(+)
> > > > > 
> > > > > diff --git a/kernel/locking/osq_lock.c b/kernel/locking/osq_lock.c
> > > > > index d5610ad52b..b752ec5cc6 100644
> > > > > --- a/kernel/locking/osq_lock.c
> > > > > +++ b/kernel/locking/osq_lock.c
> > > > > @@ -203,6 +203,7 @@ bool osq_lock(struct optimistic_spin_queue *lock)
> > > > >    	return false;
> > > > >    }
> > > > > +EXPORT_SYMBOL_GPL(osq_lock);
> > > > >    void osq_unlock(struct optimistic_spin_queue *lock)
> > > > >    {
> > > > > @@ -230,3 +231,4 @@ void osq_unlock(struct optimistic_spin_queue *lock)
> > > > >    	if (next)
> > > > >    		WRITE_ONCE(next->locked, 1);
> > > > >    }
> > > > > +EXPORT_SYMBOL_GPL(osq_unlock);
> > > > Have you considered extending the current rw_semaphore to support a SIX lock
> > > > semantics? There are a number of instances in the kernel that a up_read() is
> > > > followed by a down_write(). Basically, the code try to upgrade the lock from
> > > > read to write. I have been thinking about adding a upgrade_read() API to do
> > > > that. However, the concern that I had was that another writer may come in
> > > > and make modification before the reader can be upgraded to have exclusive
> > > > write access and will make the task to repeat what has been done in the read
> > > > lock part. By adding a read with intent to upgrade to write, we can have
> > > > that guarantee.
> > > It's been discussed, Linus had the same thought.
> > > 
> > > But it'd be a massive change to the rw semaphore code; this "read with
> > > intent" really is a third lock state which needs all the same
> > > lock/trylock/unlock paths, and with the way rw semaphore has separate
> > > entry points for read and write it'd be a _ton_ of new code. It really
> > > touches everything - waitlist handling included.
> > 
> > Yes, it is a major change, but I had done that before and it is certainly
> > doable. There are spare bits in the low byte of rwsem->count that can be
> > used as an intent bit. We also need to add a new rwsem_wake_type for that
> > for waitlist handling.
> > 
> > 
> > > 
> > > And six locks have several other features that bcachefs needs, and other
> > > users may also end up wanting, that rw semaphores don't have; the two
> > > main features being a percpu read lock mode and support for an external
> > > cycle detector (which requires exposing lock waitlists, with some
> > > guarantees about how those waitlists are used).
> > 
> > Can you provide more information about those features?
> > 
> > > 
> > > > With that said, I would prefer to keep osq_{lock/unlock} for internal use by
> > > > some higher level locking primitives - mutex, rwsem and rt_mutex.
> > > Yeah, I'm aware, but it seems like exposing osq_(lock|unlock) is the
> > > most palatable solution for now. Long term, I'd like to get six locks
> > > promoted to kernel/locking.
> > 
> > Your SIX overlaps with rwsem in term of features. So we will have to somehow
> > merge them instead of having 2 APIs with somewhat similar functionality.
> 
> Waiman, if you think you can add all the features of six locks to rwsem,
> knock yourself out - but right now this is a vaporware idea for you, not
> something I can seriously entertain. I'm looking to merge bcachefs next
> cycle, not sit around and bikeshed for the next six months.

That's an entirely inappropriate response to valid review feedback.

Not having two overlapping locking facilities is not 'bikeshedding' at all ...

> If you start making a serious effort on adding those features to rwsem
> I'll start walking you through everything six locks has, but right now
> this is a major digression on a patch that just exports two symbols.

In Linux the burden of work is on people submitting new code, not on 
reviewers. The rule is that you should not reinvent the wheel in new
features - extend existing locking facilities please.

Waiman gave you some pointers as to how to extend rwsems.

Meanwhile, NAK on the export of osq_(lock|unlock):

    NAKed-by: Ingo Molnar <mingo@xxxxxxxxxx>

Ie. NAK on this commit in linux-next:

    97da2065b7cb ("locking/osq: Export osq_(lock|unlock)")

This is an internal function of Linux locking subsystem we would not like to
expose or export.

This commit was applied without an Ack or Reviewed-by by a locking subsystem
maintainer or reviewer (which Waiman Long is).

Thanks,

	Ingo