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