Message-ID: <20231018210456.lgdicnuekvmvcgvm@moria.home.lan>
Date:   Wed, 18 Oct 2023 17:04:56 -0400
From:   Kent Overstreet <kent.overstreet@...ux.dev>
To:     Ingo Molnar <mingo@...nel.org>
Cc:     Waiman Long <longman@...hat.com>, linux-bcachefs@...r.kernel.org,
        linux-fsdevel@...r.kernel.org, linux-kernel@...r.kernel.org,
        Peter Zijlstra <peterz@...radead.org>,
        Ingo Molnar <mingo@...hat.com>,
        Boqun Feng <boqun.feng@...il.com>,
        Linus Torvalds <torvalds@...ux-foundation.org>,
        Thomas Gleixner <tglx@...utronix.de>,
        Stephen Rothwell <sfr@...b.auug.org.au>
Subject: Re: [NAK] Re: [PATCH 11/20] locking/osq: Export osq_(lock|unlock)

On Tue, Oct 10, 2023 at 10:09:38AM +0200, Ingo Molnar wrote:
> > 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 ...

Well, there was already a long off-list discussion about adding six lock
features to rwsem.

Basically, it looks to me like a total redesign of rwsem in order to do
it correctly, and I don't think that would fly. The rwsem code has
separate entrypoints for every lock state, and adding a third lock state
would at a minimum add a lot of new - nearly duplicate - code.

There's also features and optimizations in six locks that rwsem doesn't
have, and it's not clear to me that it would be appropriate to add them
to rwsem - each of them would need real discussion. The big ones are:

 - percpu reader mode, used for locks for interior nodes and subvolume
   keys in bcachefs
 - exposing of waitlist entries (and this requires nontrivial guarantees
   to do correctly!), so that bcachefs can do cycle detection deadlock
   avoidance on top.

In short, this would _not_ be a small project, and I think the saner
approach if we really did want to condense down to a single locking
implementation would be to replace rwsem with six locks. But before even
contemplating that we'd want to see six locks getting wider usage and
testing first.

Hence why we're at leaving six locks in fs/bcachefs/ for now.

> > 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):

Perhaps we could get some justification for why you want osq locks to be
private?

My initial pull request had six locks in kernel/locking/, specifically
to keep osq locks private, as requested by locking people (some years
back). But since Linus shot that down, I need an alternative.

If you're really dead set against exporting osq locks (and again, why?),
my only alternative will be to either take optimistic spinning out of
six locks, or implement optimistic spinning another way (which is
something I was already looking at before; the way lock handoff works in
six locks now makes that an attractive idea anyways, but of course the
devil is in the details with locking code).

Powered by blists - more mailing lists