Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Date:   Tue, 14 May 2019 07:13:50 -0500
From:   Corey Minyard <cminyard@mvista.com>
To:     Sebastian Andrzej Siewior <bigeasy@linutronix.de>
Cc:     Peter Zijlstra <peterz@infradead.org>, minyard@acm.org,
        linux-rt-users@vger.kernel.org, linux-kernel@vger.kernel.org,
        tglx@linutronix.de, Steven Rostedt <rostedt@goodmis.org>,
        Ingo Molnar <mingo@kernel.org>
Subject: Re: [PATCH RT v2] Fix a lockup in wait_for_completion() and friends
Message-ID: <20190514121350.GA6050@minyard.net>
Reply-To: cminyard@mvista.com
References: <20190508205728.25557-1-minyard@acm.org>
 <20190509161925.kul66w54wpjcinuc@linutronix.de>
 <20190514084356.GJ2589@hirez.programming.kicks-ass.net>
 <20190514091219.nesriqe7qplk3476@linutronix.de>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20190514091219.nesriqe7qplk3476@linutronix.de>
User-Agent: Mutt/1.9.4 (2018-02-28)
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On Tue, May 14, 2019 at 11:12:19AM +0200, Sebastian Andrzej Siewior wrote:
> On 2019-05-14 10:43:56 [+0200], Peter Zijlstra wrote:
> > Now.. that will fix it, but I think it is also wrong.
> > 
> > The problem being that it violates FIFO, something that might be more
> > important on -RT than elsewhere.
> 
> Wouldn't -RT be more about waking the task with the highest priority
> instead the one that waited the longest?
> 
> > The regular wait API seems confused/inconsistent when it uses
> > autoremove_wake_function and default_wake_function, which doesn't help,
> > but we can easily support this with swait -- the problematic thing is
> > the custom wake functions, we musn't do that.
> > 
> > (also, mingo went and renamed a whole bunch of wait_* crap and didn't do
> > the same to swait_ so now its named all different :/)
> > 
> > Something like the below perhaps.
> 
> This still violates FIFO because a task can do wait_for_completion(),
> not enqueue itself on the list because it noticed a pending wake and
> leave. The list order is preserved, we have that.
> But this a completion list. We have probably multiple worker waiting for
> something to do so all of those should be of equal priority, maybe one
> for each core or so. So it shouldn't matter which one we wake up.
> 
> Corey, would it make any change which waiter is going to be woken up?

In the application that found this, the wake order probably isn't
relevant.

For other applications, I really doubt that very many are using multiple
waiters.  If so, this bug would have been reported sooner, I think.

As you mention, for RT you would want waiter woken by priority and FIFO
within priority.  I don't think POSIX says anything about FIFO within
priority, but that's probably a good idea.  That's no longer a simple
wait queue  The way it is now is probably closer to that than what Peter
suggested, but not really that close.

This is heavily used in drivers and fs code, where it probably doesn't
matter.  I looked through a few users in mm and kernel, and they had
one waiter or were init/shutdown type things where order is not important.

So I'm not sure it's important.

-corey

> 
> Sebastian