net/sched: latent livelock in dev_deactivate_many() due to yield() usage

net/sched: latent livelock in dev_deactivate_many() due to yield() usage

[Mike Galbraith]

Greetings network wizards,

Quoting kernel/sched/core.c:
/**
 * yield - yield the current processor to other threads.
 *
 * Do not ever use this function, there's a 99% chance you're doing it wrong.
 *
 * The scheduler is at all times free to pick the calling task as the most
 * eligible task to run, if removing the yield() call from your code breaks
 * it, its already broken.
 *
 * Typical broken usage is:
 *
 * while (!event)
 *	yield();
 *
 * where one assumes that yield() will let 'the other' process run that will
 * make event true. If the current task is a SCHED_FIFO task that will never
 * happen. Never use yield() as a progress guarantee!!
 *
 * If you want to use yield() to wait for something, use wait_event().
 * If you want to use yield() to be 'nice' for others, use cond_resched().
 * If you still want to use yield(), do not!
 */

Livelock can be triggered by setting kworkers to SCHED_FIFO, then
suspend/resume.. you come back from sleepy-land with a spinning
kworker.  For whatever reason, I can only do that with an enterprise
like config, my standard config refuses to play, but no matter, it's
"Typical broken usage".

(yield() should be rendered dead)

	-Mike

Re: net/sched: latent livelock in dev_deactivate_many() due to yield() usage

[Cong Wang]

On Sat, Apr 1, 2017 at 9:28 PM, Mike Galbraith <efault@gmx.de> wrote:
> Greetings network wizards,
>
> Quoting kernel/sched/core.c:
> /**
>  * yield - yield the current processor to other threads.
>  *
>  * Do not ever use this function, there's a 99% chance you're doing it wrong.
>  *
>  * The scheduler is at all times free to pick the calling task as the most
>  * eligible task to run, if removing the yield() call from your code breaks
>  * it, its already broken.
>  *
>  * Typical broken usage is:
>  *
>  * while (!event)
>  *      yield();
>  *
>  * where one assumes that yield() will let 'the other' process run that will
>  * make event true. If the current task is a SCHED_FIFO task that will never
>  * happen. Never use yield() as a progress guarantee!!
>  *
>  * If you want to use yield() to wait for something, use wait_event().
>  * If you want to use yield() to be 'nice' for others, use cond_resched().
>  * If you still want to use yield(), do not!
>  */
>
> Livelock can be triggered by setting kworkers to SCHED_FIFO, then
> suspend/resume.. you come back from sleepy-land with a spinning
> kworker.  For whatever reason, I can only do that with an enterprise
> like config, my standard config refuses to play, but no matter, it's
> "Typical broken usage".
>
> (yield() should be rendered dead)


Thanks for the report! Looks like a quick solution here is to replace
this yield() with cond_resched(), it is harder to really wait for
all qdisc's to transmit all packets.

Re: net/sched: latent livelock in dev_deactivate_many() due to yield() usage

[Mike Galbraith]

On Tue, 2017-04-04 at 15:39 -0700, Cong Wang wrote:

> Thanks for the report! Looks like a quick solution here is to replace
> this yield() with cond_resched(), it is harder to really wait for
> all qdisc's to transmit all packets.

No, cond_resched() won't help.  What I did is below, but I suspect net
wizards will do something better.

---
 net/sched/sch_generic.c |    5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

--- a/net/sched/sch_generic.c
+++ b/net/sched/sch_generic.c
@@ -16,6 +16,7 @@
 #include <linux/types.h>
 #include <linux/kernel.h>
 #include <linux/sched.h>
+#include <linux/swait.h>
 #include <linux/string.h>
 #include <linux/errno.h>
 #include <linux/netdevice.h>
@@ -901,6 +902,7 @@ static bool some_qdisc_is_busy(struct ne
  */
 void dev_deactivate_many(struct list_head *head)
 {
+	DECLARE_SWAIT_QUEUE_HEAD_ONSTACK(swait);
 	struct net_device *dev;
 	bool sync_needed = false;
 
@@ -924,8 +926,7 @@ void dev_deactivate_many(struct list_hea
 
 	/* Wait for outstanding qdisc_run calls. */
 	list_for_each_entry(dev, head, close_list)
-		while (some_qdisc_is_busy(dev))
-			yield();
+		swait_event_timeout(swait, !some_qdisc_is_busy(dev), 1);
 }
 
 void dev_deactivate(struct net_device *dev)

Re: net/sched: latent livelock in dev_deactivate_many() due to yield() usage

[Cong Wang]

On Tue, Apr 4, 2017 at 8:20 PM, Mike Galbraith <efault@gmx.de> wrote:
> -               while (some_qdisc_is_busy(dev))
> -                       yield();
> +               swait_event_timeout(swait, !some_qdisc_is_busy(dev), 1);
>  }

I don't see why this is an improvement even if I don't care about the
hardcoded timeout for now... Why the scheduler can make a better
decision with swait_event_timeout() than with cond_resched()?

Re: net/sched: latent livelock in dev_deactivate_many() due to yield() usage

[Mike Galbraith]

On Tue, 2017-04-04 at 22:25 -0700, Cong Wang wrote:
> On Tue, Apr 4, 2017 at 8:20 PM, Mike Galbraith <efault@gmx.de> wrote:
> > -               while (some_qdisc_is_busy(dev))
> > -                       yield();
> > +               swait_event_timeout(swait,
> > !some_qdisc_is_busy(dev), 1);
> >  }
> 
> I don't see why this is an improvement even if I don't care about the
> hardcoded timeout for now... Why the scheduler can make a better
> decision with swait_event_timeout() than with cond_resched()?

Because sleeping gets you out of the way?  There is no other decision
the scheduler can make while a SCHED_FIFO task is trying to yield when
it is the one and only task at it's priority.  The scheduler is doing
exactly what it is supposed to do, problem is people calling yield()
tend to think it does something it does not do, which is why it is
decorated with "if you think you want yield(), think again"

Yes, yield semantics suck rocks, basically don't exist.  Hop in your
time machine and slap whoever you find claiming responsibility :)

	-Mike

Re: net/sched: latent livelock in dev_deactivate_many() due to yield() usage

[Cong Wang]

On Tue, Apr 4, 2017 at 11:12 PM, Mike Galbraith <efault@gmx.de> wrote:
> On Tue, 2017-04-04 at 22:25 -0700, Cong Wang wrote:
>> On Tue, Apr 4, 2017 at 8:20 PM, Mike Galbraith <efault@gmx.de> wrote:
>> > -               while (some_qdisc_is_busy(dev))
>> > -                       yield();
>> > +               swait_event_timeout(swait,
>> > !some_qdisc_is_busy(dev), 1);
>> >  }
>>
>> I don't see why this is an improvement even if I don't care about the
>> hardcoded timeout for now... Why the scheduler can make a better
>> decision with swait_event_timeout() than with cond_resched()?
>
> Because sleeping gets you out of the way?  There is no other decision
> the scheduler can make while a SCHED_FIFO task is trying to yield when
> it is the one and only task at it's priority.  The scheduler is doing
> exactly what it is supposed to do, problem is people calling yield()
> tend to think it does something it does not do, which is why it is
> decorated with "if you think you want yield(), think again"
>
> Yes, yield semantics suck rocks, basically don't exist.  Hop in your
> time machine and slap whoever you find claiming responsibility :)

I am not trying to defend for yield(), I am trying to understand when
cond_resched() is not a right solution to replace yield() and when it is.
For me, the dev_deactivate_many() case is, because I interpret
"be nice" differently.

Thanks.

Re: net/sched: latent livelock in dev_deactivate_many() due to yield() usage

[Mike Galbraith]

On Wed, 2017-04-05 at 16:55 -0700, Cong Wang wrote:
> On Tue, Apr 4, 2017 at 11:12 PM, Mike Galbraith <efault@gmx.de> wrote:
> > On Tue, 2017-04-04 at 22:25 -0700, Cong Wang wrote:
> > > On Tue, Apr 4, 2017 at 8:20 PM, Mike Galbraith <efault@gmx.de> wrote:
> > > > -               while (some_qdisc_is_busy(dev))
> > > > -                       yield();
> > > > +               swait_event_timeout(swait,
> > > > !some_qdisc_is_busy(dev), 1);
> > > >  }
> > > 
> > > I don't see why this is an improvement even if I don't care about the
> > > hardcoded timeout for now... Why the scheduler can make a better
> > > decision with swait_event_timeout() than with cond_resched()?
> > 
> > Because sleeping gets you out of the way?  There is no other decision
> > the scheduler can make while a SCHED_FIFO task is trying to yield when
> > it is the one and only task at it's priority.  The scheduler is doing
> > exactly what it is supposed to do, problem is people calling yield()
> > tend to think it does something it does not do, which is why it is
> > decorated with "if you think you want yield(), think again"
> > 
> > Yes, yield semantics suck rocks, basically don't exist.  Hop in your
> > time machine and slap whoever you find claiming responsibility :)
> 
> I am not trying to defend for yield(), I am trying to understand when
> cond_resched() is not a right solution to replace yield() and when it is.
> For me, the dev_deactivate_many() case is, because I interpret
> "be nice" differently.

Yeah, I know you weren't defending it, just as I know that the net-fu
masters don't need that comment held close to their noses in order to
be able to read it.. waving it about wasn't for their benefit ;-)

	-Mike

Re: net/sched: latent livelock in dev_deactivate_many() due to yield() usage

[Peter Zijlstra]

On Tue, Apr 04, 2017 at 10:25:19PM -0700, Cong Wang wrote:
> On Tue, Apr 4, 2017 at 8:20 PM, Mike Galbraith <efault@gmx.de> wrote:
> > -               while (some_qdisc_is_busy(dev))
> > -                       yield();
> > +               swait_event_timeout(swait, !some_qdisc_is_busy(dev), 1);
> >  }
> 
> I don't see why this is an improvement even if I don't care about the
> hardcoded timeout for now... Why the scheduler can make a better
> decision with swait_event_timeout() than with cond_resched()?

cond_resched() might be a no-op.

and doing yield() will result in a priority inversion deadlock. Imagine
the task doing yield() being the top priority (fifo99) task in the
system. Then it will simply spin forever, not giving whatever task is
required to make your condition true time to run.

Re: net/sched: latent livelock in dev_deactivate_many() due to yield() usage

[Stephen Hemminger]

On Sun, 02 Apr 2017 06:28:41 +0200
Mike Galbraith <efault@gmx.de> wrote:

> Livelock can be triggered by setting kworkers to SCHED_FIFO, then
> suspend/resume.. you come back from sleepy-land with a spinning
> kworker.  For whatever reason, I can only do that with an enterprise
> like config, my standard config refuses to play, but no matter, it's
> "Typical broken usage".
> 
> (yield() should be rendered dead)

The kernel is not normally built to have kworkers run at SCHED_FIFO.
The user has do some action to alter the process priorities.

I classify this as user error. We don't support killing kworker threads
either.

Re: net/sched: latent livelock in dev_deactivate_many() due to yield() usage

[Mike Galbraith]

On Wed, 2017-04-05 at 17:31 -0700, Stephen Hemminger wrote:
> On Sun, 02 Apr 2017 06:28:41 +0200
> Mike Galbraith <efault@gmx.de> wrote:
> 
> > Livelock can be triggered by setting kworkers to SCHED_FIFO, then
> > suspend/resume.. you come back from sleepy-land with a spinning
> > kworker.  For whatever reason, I can only do that with an enterprise
> > like config, my standard config refuses to play, but no matter, it's
> > "Typical broken usage".
> > 
> > (yield() should be rendered dead)
> 
> The kernel is not normally built to have kworkers run at SCHED_FIFO.
> The user has do some action to alter the process priorities.
> 
> I classify this as user error. We don't support killing kworker threads
> either.

We'll have to agree to disagree on that.  I assert that any thread that
must run as SCHED_OTHER in order to be safe is in fact broken.

	-Mike

Re: net/sched: latent livelock in dev_deactivate_many() due to yield() usage

[Peter Zijlstra]

On Wed, Apr 05, 2017 at 05:31:05PM -0700, Stephen Hemminger wrote:
> On Sun, 02 Apr 2017 06:28:41 +0200
> Mike Galbraith <efault@gmx.de> wrote:
> 
> > Livelock can be triggered by setting kworkers to SCHED_FIFO, then
> > suspend/resume.. you come back from sleepy-land with a spinning
> > kworker.  For whatever reason, I can only do that with an enterprise
> > like config, my standard config refuses to play, but no matter, it's
> > "Typical broken usage".
> > 
> > (yield() should be rendered dead)
> 
> The kernel is not normally built to have kworkers run at SCHED_FIFO.
> The user has do some action to alter the process priorities.
> 
> I classify this as user error. We don't support killing kworker threads
> either.

PI can boost anybody to FIFO, all you need is to be holding a lock.

Re: net/sched: latent livelock in dev_deactivate_many() due to yield() usage

[Peter Zijlstra]

On Thu, Apr 06, 2017 at 12:28:44PM +0200, Peter Zijlstra wrote:
> On Wed, Apr 05, 2017 at 05:31:05PM -0700, Stephen Hemminger wrote:
> > On Sun, 02 Apr 2017 06:28:41 +0200
> > Mike Galbraith <efault@gmx.de> wrote:
> > 
> > > Livelock can be triggered by setting kworkers to SCHED_FIFO, then
> > > suspend/resume.. you come back from sleepy-land with a spinning
> > > kworker.  For whatever reason, I can only do that with an enterprise
> > > like config, my standard config refuses to play, but no matter, it's
> > > "Typical broken usage".
> > > 
> > > (yield() should be rendered dead)
> > 
> > The kernel is not normally built to have kworkers run at SCHED_FIFO.
> > The user has do some action to alter the process priorities.
> > 
> > I classify this as user error. We don't support killing kworker threads
> > either.
> 
> PI can boost anybody to FIFO, all you need is to be holding a lock.

Note that this extends to rcu_read_lock(), which can cause boosting
under some cases.