Re: [PATCH 2/6] pkt_sched: sch_htb: Consider used jiffies in htb_dequeue()

[Patrick McHardy <kaber@trash.net>]

Jarek Poplawski wrote:
> To David Miller: David don't apply yet - this patch needs change.
> 
> Patrick, read below:
> 
> On Tue, Dec 09, 2008 at 02:20:00PM +0100, Patrick McHardy wrote:
>>>>
>>>> The focus on jiffies is wrong IMO, the reason why we get high
>>>> load is because the CPU can't keep up, delaying things even
>>>> longer is not going to help get the work done. The only reason to
>>>> look at jiffies is because its a cheap indication that it has
>>>> ran for too long and we should give other tasks a change to run
>>>> as well, but it should continue immediately after it did that.
>>>> So all it should do is make sure that the watchdog is scheduled
>>>> with a very small positive delay.
 >>>>
>>> This needs additional psched_get_time(), and as I've written before
>>> there is no apparent advantage in problematic cases, but this would
>>> add more overhead for common cases.
 >>>
>> htb_do_events() exceeding two jiffies is fortunately not a common
>> case. You (incorrectly) made the calculation somewhat of a common
>> case by also adding to the delay if the inner classes simply throttled
>> and already returned the exact delay they want.
> 
> I see! You are right and this needs fixing.
> 
>> Much better (again considering what we want to achieve here) would
>> be to not use the hrtimer watchdog at all. We want to give lower
>> priority tasks a chance to run, so ideally we'd use a low priority
>> task for wakeup.
> 
> I'm not sure I get ot right: for precise scheduling hrtimers look
> useful. Do you really mean "at all"?

I meant "at all" for the wakeup after we've decided HTB has too much
work to do at once. A work queue seems better suited since that makes
sure we allow other processes to run, but don't wait unnecessarily
long when there is no other work.

>>>> As for the implementation: the increase in delay (the snippet
>>>> above) is also done in the case that no packets were available
>>>> for other reasons (throttling), in which case we might needlessly
>>>> delay for an extra jiffie if jiffies wrapped while it tried to
>>>> dequeue.
 >>>>
>>> But in another similar cases there could be no change in jiffies, but
>>> almost a jiffie used for counting, so wrong schedule time as well.
>> Its not "wrong". We don't want to delay. Its a courtesy to the
>> remaining system.
> 
> In this case it's probably self-courtesy too: this ksoftirqd takes
> most of the time and it's useless.

Well, it calls back to HTB, which continues to do real work. But
leaving HTB, scheduling a timer just to be called immediately again
is useless overhead, I agree.

>>> Approximatly this all should be fine, and it still can be tuned later.
>>> IMHO, this all should not affect "common" cases, which are expected to
>>> use less then jiffie here.
 >>>
>> Jiffies might wrap even if it only took only a few nanoseconds.
>> And its not fine, in the case of throttled classes there's no
>> reason to add extra delay *at all*.
> 
> Yes, you are right with this. I can try too fix this tomorrow, unless
> you prefer to send your version of this patch.

I don't have a version of my own, so please go ahead :)