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

[Oleg Nesterov <oleg@redhat.com>]

Hi Manfred,

Sorry, I don't understand, perhaps you misunderstood me too.

On 12/28, Manfred Spraul wrote:
>
> >Even simpler,
> >
> >	void wait(void)
> >	{
> >		DEFINE_WAIT(entry);
> >
> >		__set_current_state(XXX);
> >		add_wait_queue(WQ, entry);
> >
> >		if (!CONDITION)
> >			schedule();
> >
> >		remove_wait_queue(WQ, entry);
> >		__set_current_state(TASK_RUNNING);
> >	}
> >
> >This code is ugly but currently correct unless I am totally confused.

What I tried to say: the code above is another (simpler) example of
the currently correct (afaics) code which will be broken by your patch.

Of course, wait() assumes that

	void wake(void)
	{
		CONDITION = 1;
		wake_up(WQ);
	}

calls __wake_up_common_lock() and takes WQ->lock unconditionally, and
thus wait() doesn't need the additional barries.

> And: Your proposal is in conflict with
>
> https://git.kernel.org/pub/scm/linux/kernel/git/tglx/history.git/commit/kernel/fork.c?h=v2.6.0&id=e220fdf7a39b54a758f4102bdd9d0d5706aa32a7

I proposed nothing ;) But yes sure, this code doesn't match the comment
above waitqueue_active(), and that is why the wake() paths can't check
list_empty() to avoid __wake_up_common_lock().

> But I do not see the issue, the worst possible scenario should be something like:
>
> 	// add_wait_queue
> 		spin_lock(WQ->lock);
> 		LOAD(CONDITION);	// false!
> 		list_add(entry, head);
> 		STORE(current_state)
> 		spin_unlock(WQ->lock);

Again, wake() can happen between LOAD() and list_add()...

But sorry again, I guess I completely misunderstood you...

Oleg.