Re: [RESEND PATCH] fs/pipe: Introduce a check to skip sleeping processes during pipe read/write

[Oleg Nesterov <oleg@redhat.com>]

I mostly agree, see my reply to this patch, but...

On 12/26, Linus Torvalds wrote:
>
> If the optimization is correct, there is no point to having a config option.
>
> If the optimization is incorrect, there is no point to having the code.
>
> Either way, there's no way we'd ever have a config option for this.

Agreed,

> > +       if (was_full && pipe_check_wq_has_sleeper(&pipe->wr_wait))
> >                 wake_up_interruptible_sync_poll(&pipe->wr_wait, EPOLLOUT | EPOLLWRNORM);
>
> End result: you need to explain why the race cannot exist.

Agreed,

> And I think your patch is simply buggy

Agreed again, but probably my reasoning was wrong,

> But now waiters use "wait_event_interruptible_exclusive()" explicitly
> outside the pipe mutex, so the waiters and wakers aren't actually
> serialized.

This is what I don't understand... Could you spell ?

I _think_ that

	wait_event_whatever(WQ, CONDITION);

vs

	CONDITION = 1;
	if (wq_has_sleeper(WQ))
		wake_up_xxx(WQ, ...);
	
is fine.

Both wq_has_sleeper() and wait_event_whatever()->prepare_to_wait_event()
have the necessary barriers to serialize the waiters and wakers.

Damn I am sure I missed something ;)

> And no, you are unlikely to see the races in practice (particularly
> the memory ordering ones). So you have to *think* about them, not test
> them.

Yes! agreed again.

Oleg.