When to lock pipe->rd_wait.lock?

When to lock pipe->rd_wait.lock?

[Max Kellermann]

Hi,

I'm trying to understand the code that allocates a new buffer from a
pipe's buffer ring. I'd like to write a patch that eliminates some
duplicate code, to make it less error prone.

In fs/pipe.c, pipe_write() locks pipe->rd_wait.lock while the pipe's
head gets evaluated and incremented (all while pipe->mutex is locked).
My limited understand is that holding this spinlock is important
because it protects the head/tail accesses in pipe_readable() which is
gets called by wait_event while the spinlock is held, but without
pipe->mutex.

However in fs/splice.c, splice_pipe_to_pipe() contains very similar
code; a new buffer gets allocated, head gets incremented - but without
caring for pipe->rd_wait.lock.
Please help me understand the point of locking pipe->rd_wait.lock, and
why it's necessary in pipe_write() but not in splice_pipe_to_pipe().
Is that a bug or am I missing something?

Max

Re: When to lock pipe->rd_wait.lock?

[Christian Brauner]

On Wed, Sep 20, 2023 at 02:34:51PM +0200, Max Kellermann wrote:
> Hi,
> 
> I'm trying to understand the code that allocates a new buffer from a
> pipe's buffer ring. I'd like to write a patch that eliminates some
> duplicate code, to make it less error prone.
> 
> In fs/pipe.c, pipe_write() locks pipe->rd_wait.lock while the pipe's
> head gets evaluated and incremented (all while pipe->mutex is locked).
> My limited understand is that holding this spinlock is important
> because it protects the head/tail accesses in pipe_readable() which is
> gets called by wait_event while the spinlock is held, but without
> pipe->mutex.
> 
> However in fs/splice.c, splice_pipe_to_pipe() contains very similar
> code; a new buffer gets allocated, head gets incremented - but without
> caring for pipe->rd_wait.lock.
> Please help me understand the point of locking pipe->rd_wait.lock, and
> why it's necessary in pipe_write() but not in splice_pipe_to_pipe().
> Is that a bug or am I missing something?

Afaict, the mutex is sufficient protection unless you're using
watchqueues which use post_one_notification() that cannot acquire the
pipe mutex. Since splice operations aren't supported on such kernel
notification pipes - see get_pipe_info() - it should be unproblematic.

Re: When to lock pipe->rd_wait.lock?

[Max Kellermann]

On Wed, Sep 20, 2023 at 3:30 PM Christian Brauner <brauner@kernel.org> wrote:
> Afaict, the mutex is sufficient protection unless you're using
> watchqueues which use post_one_notification() that cannot acquire the
> pipe mutex. Since splice operations aren't supported on such kernel
> notification pipes - see get_pipe_info() - it should be unproblematic.

Which means that the spinlocks can safely be removed from
pipe_write(), because they are unnecessary overhead?

Re: When to lock pipe->rd_wait.lock?

[Christian Brauner]

On Wed, Sep 20, 2023 at 05:21:03PM +0200, Max Kellermann wrote:
> On Wed, Sep 20, 2023 at 3:30 PM Christian Brauner <brauner@kernel.org> wrote:
> > Afaict, the mutex is sufficient protection unless you're using
> > watchqueues which use post_one_notification() that cannot acquire the
> > pipe mutex. Since splice operations aren't supported on such kernel
> > notification pipes - see get_pipe_info() - it should be unproblematic.
> 
> Which means that the spinlocks can safely be removed from
> pipe_write(), because they are unnecessary overhead?

I don't think so, O_NOTIFICATION/watch queue pipes allow userspace to
use pipe_read() and pipe_write() but prevent the usage of splice. The
spinlock is there for post_one_notification() which is called from
kernel context.

Re: When to lock pipe->rd_wait.lock?

[Max Kellermann]

On Wed, Sep 20, 2023 at 5:50 PM Christian Brauner <brauner@kernel.org> wrote:
> I don't think so, O_NOTIFICATION/watch queue pipes allow userspace to
> use pipe_read() and pipe_write() but prevent the usage of splice. The
> spinlock is there for post_one_notification() which is called from
> kernel context.

Oohh, that was watch_queue, not wait_queue! This is how I
misunderstood your previous email.

So the spinlock is used in pipe_write() only just in case this is a
O_NOTIFICATION_PIPE. If I understand this correctly, it means the
spinlock could be omitted for the majority of pipes that are not
O_NOTIFICATION_PIPE?
Do you think it could be a worthy optimization to make the spinlock
conditional? I mean, how many pipes are there usually, and how many of
these are really O_NOTIFICATION_PIPE? This is a very rare exotic
feature, isn't it?

Re: When to lock pipe->rd_wait.lock?

[Max Kellermann]

On Wed, Sep 20, 2023 at 3:30 PM Christian Brauner <brauner@kernel.org> wrote:
> Afaict, the mutex is sufficient protection unless you're using
> watchqueues which use post_one_notification() that cannot acquire the
> pipe mutex. Since splice operations aren't supported on such kernel
> notification pipes - see get_pipe_info() - it should be unproblematic.

I had another look at this, and something's fishy with the code or
with your explanation (or I still don't get it). If there is a
watch_queue, pipe_write() fails early with EXDEV - writing to such a
pipe is simply forbidden, the code is not reachable in the presence of
a watch_queue, therefore locking just because there might be a
wait_queue does not appear to make sense?

Re: When to lock pipe->rd_wait.lock?

[Max Kellermann]

On Thu, Sep 21, 2023 at 9:28 AM Max Kellermann <max.kellermann@ionos.com> wrote:
> I had another look at this, and something's fishy with the code or
> with your explanation (or I still don't get it). If there is a
> watch_queue, pipe_write() fails early with EXDEV - writing to such a
> pipe is simply forbidden, the code is not reachable in the presence of
> a watch_queue, therefore locking just because there might be a
> wait_queue does not appear to make sense?

Meanwhile I have figured that the spinlock in pipe_write() is
obsolete. It was added by David as preparation for the notification
feature, but the notification was finally merged, it had the EXDEV,
and I believe it was not initially planned to implement it that way?
So I believe the spinlock is really not necessary (anymore) and I have
posted a patch that removes it. (David, you would have received my
submission, unfortunately I misspelled your email address....)

Max

Re: When to lock pipe->rd_wait.lock?

[Christian Brauner]

On Thu, Sep 21, 2023 at 10:05:24AM +0200, Max Kellermann wrote:
> On Thu, Sep 21, 2023 at 9:28 AM Max Kellermann <max.kellermann@ionos.com> wrote:
> > I had another look at this, and something's fishy with the code or
> > with your explanation (or I still don't get it). If there is a
> > watch_queue, pipe_write() fails early with EXDEV - writing to such a
> > pipe is simply forbidden, the code is not reachable in the presence of
> > a watch_queue, therefore locking just because there might be a
> > wait_queue does not appear to make sense?
> 
> Meanwhile I have figured that the spinlock in pipe_write() is
> obsolete. It was added by David as preparation for the notification
> feature, but the notification was finally merged, it had the EXDEV,
> and I believe it was not initially planned to implement it that way?

It was supposed to get write support most likely but never got it.
Good catch.

> So I believe the spinlock is really not necessary (anymore) and I have

For pipe_write() it isn't we still need it for pipe_read().

Re: When to lock pipe->rd_wait.lock?

[Max Kellermann]

On Thu, Sep 21, 2023 at 11:18 AM Christian Brauner <brauner@kernel.org> wrote:
> For pipe_write() it isn't we still need it for pipe_read().

Agree. My patch set removes the spinlock from pipe_write(); the next
patch makes it optional for pipe_read(), eliminating the spinlock for
the most common case.