Re: [RFC] Sleep waiting for an rwsem to be unlocked

[Waiman Long <longman@redhat.com>]

On 1/9/24 16:42, Matthew Wilcox wrote:
> 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.

OK, I focused on the commit log and it didn't mention that. I also 
looked at __wait_read_common() and hadn't paid much attention to the 
other wrappers.

BTW, how did the a lockless reader notices an inconsistent state? Will 
something like a write sequence count help? Though that will require 
increasing the size of the rwsem.

>
>> 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.
Some minor change is needed in case the RWSEM_WAITING_FOR_RELEASE waiter 
is the first one in the queue to be woken up.
>
>> 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?

I am talking about the case that the new waiter is the only one to be 
waking up and the rwsem has no reader or writer owner.

I also think that __wait_read_common() should be merged with 
rwsem_down_read_slowpath() in some way to minimize duplicated code.

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

That will probably work.

Cheers,
Longman