Re: [PATCH 8/9]: sch_hfsc: Use ->requeue queue instead of ops.

[Jarek Poplawski <jarkao2@gmail.com>]

On Tue, Aug 19, 2008 at 03:07:10PM +0200, Patrick McHardy wrote:
> David Miller wrote:
>> From: Patrick McHardy <kaber@trash.net>
>> Date: Mon, 18 Aug 2008 16:11:57 +0200
>>
>>> I think we really need either ->requeue or a real ->peek operation.
>>
>> All the code duplication and complexity is what I'm trying to avoid.
>>
>> I see no value in overhauling and auditing all of these ->requeue()
>> implementations and how they return status codes when the facility
>> itself is largely superfluous.
>>
>> Maybe we can simply add a "bool peek" argument or some flags to
>> ->dequeue() instead.
>
> Yes, that should work. It might get a big ugly though since the
> ->dequeue functions have to make sure not to modify any state
> while peeking.

I'm not sure what are conclusions here wrt. this patchset, but since
David made this mistake and added me to CC, here are my doubts:

- maybe I miss something, but it seems there is something strange with
  using qdisc_dequeue(), e.g. how htb_dequeue_queue() in this call
  skb = qdisc_dequeue(cl->un.leaf.q);
  can ever get anything here?:
  struct sk_buff *skb = __skb_dequeue(&sch->requeue);
  Isn't it requeued in root qdisc? But even if it's OK, isn't there
  needed some additional code to control queue length?

- initially David wrote about simplifying this, so I thought it's
  about some simple buffer outside of qdiscs' code; now it's a bit
  more than this; sure, it's simpler but I guess, soon, after a few
  (ADSL?) fixes and optimizations there will be probably no difference.
  Peeking doesn't look to me necessarily simpler either.

So, IMHO, if it's not going to be something really simple I doubt it's
worth to bother with this. Anyway, I hope David will give some warning
yet before merging this.

Thanks,
Jarek P.