Re: Warning from swake_up_all in 4.14.15-rt13 non-RT

[Sebastian Andrzej Siewior]

On 2018-03-09 23:26:43 [+0100], Peter Zijlstra wrote:
> On Fri, Mar 09, 2018 at 09:25:50PM +0100, Sebastian Andrzej Siewior wrote:
> > Is it just about the irqsave() usage or something else? I doubt it is
> > the list walk. It is still unbound if not called from irq-off region.
> 
> The current list walk is preemptible. You put the entire iteration (of
> unbound length) inside a single critical section which destroy RT.

I considered that list walk as cheap. We don't do any wake ups with the
list walk - just mark the task for a later wake up. But if it is not I
could add an upper limit of 20 iterations or so.

> > But it is now possible, I agree. The wake_q usage should be cheaper
> > compared to IRQ off+on in each loop. And we wanted to do the wake ups
> > with enabled interrupts - there is still the list_splice() from that
> > attempt. Now it can be.
> 
> Unbound is still unbound, inf/n := inf. A 'cheaper' unbound doesn't RT
> make.

What I meant is that wake_q() is invoked with interrupts enabled and we
don't need the IRQ on/off on each iteration. But as I said in the upper
paragraph, I can add an upper limit for the list walk. And wake up
itself is with enabled interrupts.

> > > Yes, wake_up_all() is crap, it is also fundamentally incompatible with
> > > in-*irq usage. Nothing to be done about that.
> > I still have (or need) completions which are swait based and do
> > complete_all(). 
> 
> That's fine, as long as they're done from preemptible context. Back when
> we introduced swait this was an explicit design goal/limitation. And
> there were no in-irq users of this.

Yes at that time in !RT. wake_up() is using sleeping locks on RT and
swait is the only thing that can be used there. So if I don't get rid if
that !preemptible part I try to switch to swait.

> > There are complete_all() caller which wake more than one
> > waiter (that is PM and crypto from the reports I got once I added the
> > WARN_ON())).
> > The in-IRQ usage is !RT only and was there before.
> 
> Then that's broken and needs to be undone. Also, why did you need the
> WARN, lockdep should've equally triggered on this, no?

I added WARN_ON() and I didn't even think about lockdep. I wanted to
see a warning even with lockdep off.
After adding this for testing:

        {
                 raw_spin_lock_irq(&lock1);
                 raw_spin_unlock_irq(&lock1);
                 raw_spin_lock_irq(&lock2);
                 raw_spin_unlock_irq(&lock2);
 
                 raw_spin_lock_irq(&lock1);
                 raw_spin_lock_irq(&lock2);
                 raw_spin_unlock_irq(&lock2);
                 raw_spin_unlock_irq(&lock1);

                 raw_spin_lock_irq(&lock2);
                 raw_spin_lock_irq(&lock1);
                 raw_spin_unlock_irq(&lock1);
                 raw_spin_unlock_irq(&lock2);
         }

I see only one complaint about the lock order in the last block. With
that one gone there is no complain about the second block. So no,
lockdep does not report such things (this was just tested on RT and
TIP).

> > > So NAK on this.
> > So I need completions to be swait based and do complete_all() from IRQ
> > (on !RT, not RT). I have this one call which breaks the usage on !RT and
> > has wake_up_all() in it in vanilla which needs an swait equivalent since
> > it calls its callback from an rcu-sched section.
> 
> Why isn't this a problem on RT?
So we remain in the preempt_disable() section due to RCU-sched so we
have this, yes. But the "disabled interrupts" part is due to
spin_lock_irqsave() which is a non-issue on RT. So if we managed to get
rid of the rcu-sched then the swait can go and we can stick with the
wake_up_all() on RT, too.

Sebastian