Re: [PATCH] net: sched: dev_deactivate_many(): use msleep(1) instead of yield() to wait for outstanding qdisc_run calls

[David Lang]

On Sun, 9 Mar 2014, Ben Hutchings wrote:

> On Sun, 2014-03-09 at 18:53 -0400, David Miller wrote:
> > From: Ben Hutchings <ben@xxxxxxxxxxxxxxx>
> > Date: Sun, 09 Mar 2014 19:09:20 +0000
> >
> > > On Thu, 2014-03-06 at 16:06 -0500, David Miller wrote:
> > > > From: Marc Kleine-Budde <mkl@xxxxxxxxxxxxxx>
> > > > Date: Wed,  5 Mar 2014 00:49:47 +0100
> > > >
> > > > > @@ -839,7 +839,7 @@ void dev_deactivate_many(struct list_head *head)
> > > > >       /* Wait for outstanding qdisc_run calls. */
> > > > >       list_for_each_entry(dev, head, unreg_list)
> > > > >               while (some_qdisc_is_busy(dev))
> > > > > -                     yield();
> > > > > +                     msleep(1)
> > > > >  }
> > > >
> > > > I don't understand this.
> > > >
> > > > yield() should really _mean_ yield.
> > > >
> > > > The intent of a yield() call, like this one here, is unambiguously
> > > > that the current thread cannot do anything until some other thread
> > > > gets onto the cpu and makes forward progress.
> > > >
> > > > Therefore it should allow lower priority threads to run, not just
> > > > equal or higher priority ones.
> > >
> > > Until when?
> > >
> > > yield() is not a sensible operation in a preemptive multitasking system,
> > > regardless of RT.
> >
> > To me it means "I've got nothing to do if other tasks want to run right
> > now"  Yes, I even see it having this meaning when an RT task executes
> > it.
> >
> > How else can you interpret the intent above?
>
> The problem is that 'I've got nothing to do ... now' is information
> about a *point* in time, which gives the scheduler no clue as to when
> this task might be ready again.  So far as task *state* goes, it never
> ceases to be ready.
>
> > If you change it to msleep(1), you're assigning an extra completely
> > arbitrary time limit to the yield.  The code doesn't want to sleep
> > for 1ms, that's not what it's asking for.
> [...]
>
> I think you want to give up a 'time slice' to any task that's available.
> But that's not a meaningful concept for all schedulers.  If your task is
> highest priority and is ready, it must run.  You could drop priority
> temporarily, but then you need it to somehow be bumped up again at some
> time in the future.  Well, sleeping effectively does that.
>
> I do understand that unconditionally sleeping also isn't ideal - the
> task that unblocks this one might already be running on another CPU and
> able to finish sooner than the sleep timeout.
>
> Maybe the answer is a yield_for(time) which sleeps for the given time or
> until there's an idle CPU, whichever is sooner.  But I don't know how
> hard that would be to implement or how widely useful it would be.

what is msleep(0) defined to do? would it be any better?

David Lang
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/