Received-SPF: pass (google.com: domain of linux-kernel+bounces-21437-linux.lists.archive=gmail.com@vger.kernel.org designates 139.178.88.99 as permitted sender) client-ip=139.178.88.99;
Date: Tue, 9 Jan 2024 21:42:03 +0000
From: Matthew Wilcox <willy@infradead.org>
To: Waiman Long <longman@redhat.com>
Cc: Peter Zijlstra <peterz@infradead.org>, Ingo Molnar <mingo@redhat.com>,
	Will Deacon <will@kernel.org>, linux-kernel@vger.kernel.org,
	Suren Baghdasaryan <surenb@google.com>,
	"Liam R. Howlett" <liam.howlett@oracle.com>,
	"Paul E. McKenney" <paulmck@kernel.org>
Subject: Re: [RFC] Sleep waiting for an rwsem to be unlocked
Message-ID: <ZZ29q3x3I0Z3dQ8y@casper.infradead.org>
References: <ZZ1+ZicgN8dZ3zj3@casper.infradead.org>
 <04d23913-fb0e-4f47-9e6c-dae50ee28a3a@redhat.com>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <04d23913-fb0e-4f47-9e6c-dae50ee28a3a@redhat.com>

On Tue, Jan 09, 2024 at 04:04:08PM -0500, Waiman Long wrote:
> On 1/9/24 12:12, Matthew Wilcox wrote:
> > The problem we're trying to solve is a lock-free walk of
> > /proc/$pid/maps. If the process is modifying the VMAs at the same time
> > the reader is walking them, it can see garbage.  For page faults, we
> > handle this by taking the mmap_lock for read and retrying the page fault
> > (excluding any further modifications).
> > 
> > We don't want to take that approach for the maps file.  The monitoring
> > task may have a significantly lower process priority, and so taking
> > the mmap_lock for read can block it for a significant period of time.
> > The obvious answer is to do some kind of backoff+sleep.  But we already
> > have a wait queue, so why not use it?
> > 
> > I haven't done the rwbase version; this is just a demonstration of what
> > we could do.  It's also untested other than by compilation.  It might
> > well be missing something.
> 
> It is not clear what exactly is the purpose of this new API. Are you just

.. really?  I wrote it out in the part you quoted, and I wrote it out
differently in the kernel-doc for the function:

+ * rwsem_wait_killable - Wait for current write lock holder to release lock
+ * @sem: The semaphore to wait on.
+ *
+ * This is equivalent to calling down_read(); up_read() but avoids the
+ * possibility that the thread will be preempted while holding the lock
+ * causing threads that want to take the lock for writes to block.  The
+ * intended use case is for lockless readers who notice an inconsistent
+ * state and want to wait for the current writer to finish.

Something I forgot to add was that we only guarantee that _a_ writer
finished; another writer may have the lock when the function returns.

> waiting in the rwsem wait queue until it gets waken up without taking a read
> or write lock? I see two issues at the moment.
> 
> 1) The handoff processing should exclude the new RWSEM_WAITING_FOR_RELEASE
> waiter types.

Hmm.  I thought I'd done that by only incrementing 'woken' for
RWSEM_WAITING_FOR_READ types.

> 2) If the rwsem is free, it should call rwsem_wake() again to wake up the
> next waiter, like what is being done in up_write().

because the wait queue might have a waiter followed by a writer?  I
think calling rwsem_wake() again is probably a bad idea as it will
defeat the MAX_READERS_WAKEUP limit.  Probably rwsem_mark_wake()
needs to handle that case itself; maybe something like this?

+++ b/kernel/locking/rwsem.c
@@ -419,6 +419,7 @@ static void rwsem_mark_wake(struct rw_semaphore *sem,

        lockdep_assert_held(&sem->wait_lock);

+again:
        /*
         * Take a peek at the queue head waiter such that we can determine
         * the wakeup(s) to perform.
@@ -542,6 +543,12 @@ static void rwsem_mark_wake(struct rw_semaphore *sem,
                 */
                if (oldcount & RWSEM_FLAG_HANDOFF)
                        adjustment -= RWSEM_FLAG_HANDOFF;
+       } else {
+               /*
+                * Everybody we woke was a waiter, not a reader.  Wake the
+                * first writer instead.
+                */
+               goto again;
        }

        if (adjustment)