linux-kernel - Re: [PATCH -v2 5/7] locking/percpu-rwsem: Remove the embedded rwsem

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Tue, 4 Feb 2020 09:50:49 +0100
From:   Peter Zijlstra <peterz@...radead.org>
To:     Christoph Hellwig <hch@...radead.org>
Cc:     mingo@...nel.org, will@...nel.org, oleg@...hat.com,
        tglx@...utronix.de, linux-kernel@...r.kernel.org,
        bigeasy@...utronix.de, juri.lelli@...hat.com, williams@...hat.com,
        bristot@...hat.com, longman@...hat.com, dave@...olabs.net,
        jack@...e.com
Subject: Re: [PATCH -v2 5/7] locking/percpu-rwsem: Remove the embedded rwsem

On Mon, Feb 03, 2020 at 09:48:31AM -0800, Christoph Hellwig wrote:
> On Mon, Feb 03, 2020 at 04:09:33PM +0100, Peter Zijlstra wrote:
> > > Can you split the removal of the non-owned resem support into a separate
> > > patch?  I still think keeping this one and moving aio to that scheme is
> > > a better idea than the current ad-hoc locking scheme that has all kinds
> > > of issues.
> > 
> > That's basically 2 lines of code and a comment, surely we can ressurect
> > that if/when it's needed again?
> 
> Sure, I could.  But then you'd still need to update your commit log for
> this patch explaining why it includes unrelated changes to a different
> subsystem.  By splitting it you also document your changes much better.

I really don't see the argument; the diffstat is:

include/linux/percpu-rwsem.h  |   19 +----
include/linux/rwsem.h         |    6 -
include/linux/wait.h          |    1
kernel/locking/percpu-rwsem.c |  153 ++++++++++++++++++++++++++++++------------
kernel/locking/rwsem.c        |   11 +--
kernel/locking/rwsem.h        |   12 ---

There is no unrelated subsystem, it's all locking (well, strictly
speaking wait.h is sched, bit that one flag is actually mentioned in the
Changelog).

Also, cleaning up unused bits is quite common without mention.

Anyway, I'll go split, since you seem to care so deeply.