Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Date:   Mon, 6 Apr 2020 14:13:42 +0100
From:   Will Deacon <will@kernel.org>
To:     Linus Torvalds <torvalds@linux-foundation.org>
Cc:     Waiman Long <longman@redhat.com>,
        "Eric W. Biederman" <ebiederm@xmission.com>,
        Ingo Molnar <mingo@kernel.org>,
        Bernd Edlinger <bernd.edlinger@hotmail.de>,
        Linux Kernel Mailing List <linux-kernel@vger.kernel.org>,
        Alexey Gladkov <gladkov.alexey@gmail.com>, peterz@infradead.org
Subject: Re: [GIT PULL] Please pull proc and exec work for 5.7-rc1
Message-ID: <20200406131341.GA3178@willie-the-truck>
References: <CAHk-=wg5LvjumW9PVQiF7jB8yig98K8XTk4tHo9W-sYmxzW+9g@mail.gmail.com>
 <87lfnda3w3.fsf@x220.int.ebiederm.org>
 <CAHk-=wjxyGCj9675mf31uhoJCyHn74ON_+O6SjSqBSSvqWxC1Q@mail.gmail.com>
 <328f5ad3-f8b3-09b9-f2f7-b6dae0137542@redhat.com>
 <CAHk-=wgww8LFqUenUtNV7hzYSxAemjbOVYfZMXqOxK7DGRrZaw@mail.gmail.com>
 <c7c770c9-2c5c-4878-a224-d115720068f3@redhat.com>
 <86aa9fc6-6ac9-a0c2-3e1d-a602ef16d873@redhat.com>
 <CAHk-=wgOykL7cM34NraiNGsjJC5Uq6H0ybYHWhdXDSn-wzVXDQ@mail.gmail.com>
 <5c04cc6d-ec44-b840-071d-248ac81a0f91@redhat.com>
 <CAHk-=wgkE=qT6CSUJHfXNsyhW+=j4SVgxh2jEc8nWY_n3SFZ=w@mail.gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <CAHk-=wgkE=qT6CSUJHfXNsyhW+=j4SVgxh2jEc8nWY_n3SFZ=w@mail.gmail.com>
User-Agent: Mutt/1.10.1 (2018-07-13)
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

[+Peter]

On Fri, Apr 03, 2020 at 07:28:36PM -0700, Linus Torvalds wrote:
> On Fri, Apr 3, 2020 at 7:02 PM Waiman Long <longman@redhat.com> wrote:
> >
> > So in term of priority, my current thinking is
> >
> >     upgrading unfair reader > unfair reader > reader/writer
> >
> > A higher priority locker will block other lockers from acquiring the lock.
> 
> An alternative option might be to have readers normally be 100% normal
> (ie with fairness wrt writers), and not really introduce any special
> "unfair reader" lock.
> 
> Instead, all the unfairness would come into play only when the special
> case - execve() - does it's special "lock for reading with intent to
> upgrade".
> 
> But when it enters that kind of "intent to upgrade" lock state, it
> would not only block all subsequent writers, it would also guarantee
> that all other readers can continue to go).
> 
> So then the new rwsem operations would be
> 
>  - read_with_write_intent_lock_interruptible()
> 
>    This is the beginning of "execve()", and waits for all writers to
> exit, and puts the lock into "all readers can go" mode.
> 
>    You could think of it as a "I'm queuing myself for a write lock,
> but I'm allowing readers to go ahead" state.
> 
>  - read_lock_to_write_upgrade()
> 
>    This is the "now this turns into a regular write lock". It needs to
> wait for all other readers to exit, of course.

... and at this point, subsequent readers queue behind the upgrader so we
can't run into the usual "stream of readers prevents forward progress"
issue, which was my initial worry when I started reading the thread. Makes
sense.

>  - read_with_write_intent_unlock()
> 
>    This is the "I'm unqueuing myself, I aborted and will not become a
> write lock after all" operation.
> 
> NOTE! In this model, there may be multiple threads that do that
> initial queuing thing. We only guarantee that only one of them will
> get to the actual write lock stage, and the others will abort before
> that happens.

I do worry a bit about how much of this we can enforce, but I suppose I'll
wait for the patches. For example, it would nice for
read_lock_to_write_upgrade() to return -EBUSY if there was a concurrent
(successful) upgrade rather than some pathological failure mode like
deadlock, but that feels like it might be a pain to do. It would probably
also be nice to scream if read_lock_to_write_upgrade() is called on a lock
where the upgrade *did* go ahead. Maybe some of this is food for lockdep.

That said, if this all ends up being spelled task_cred_*() then perhaps
it doesn't matter.

Will