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

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

[David Miller]

sch_hfsc: Use ->requeue queue instead of ops.

In fact this "peek head SKB for len" sequence could be
optimized even further.

Signed-off-by: David S. Miller <davem@davemloft.net>
---
 net/sched/sch_hfsc.c |    7 +------
 1 files changed, 1 insertions(+), 6 deletions(-)

diff --git a/net/sched/sch_hfsc.c b/net/sched/sch_hfsc.c
index ea46760..bed4fd2 100644
--- a/net/sched/sch_hfsc.c
+++ b/net/sched/sch_hfsc.c
@@ -896,12 +896,7 @@ qdisc_peek_len(struct Qdisc *sch)
 		return 0;
 	}
 	len = qdisc_pkt_len(skb);
-	if (unlikely(sch->ops->requeue(skb, sch) != NET_XMIT_SUCCESS)) {
-		if (net_ratelimit())
-			printk("qdisc_peek_len: failed to requeue\n");
-		qdisc_tree_decrease_qlen(sch, 1);
-		return 0;
-	}
+	__skb_queue_head(&sch->requeue, skb);
 	return len;
 }
 
-- 
1.5.6.5.GIT

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

[Patrick McHardy]

David Miller wrote:
> sch_hfsc: Use ->requeue queue instead of ops.
> 
> In fact this "peek head SKB for len" sequence could be
> optimized even further.
> 
> Signed-off-by: David S. Miller <davem@davemloft.net>
> ---
>  net/sched/sch_hfsc.c |    7 +------
>  1 files changed, 1 insertions(+), 6 deletions(-)
> 
> diff --git a/net/sched/sch_hfsc.c b/net/sched/sch_hfsc.c
> index ea46760..bed4fd2 100644
> --- a/net/sched/sch_hfsc.c
> +++ b/net/sched/sch_hfsc.c
> @@ -896,12 +896,7 @@ qdisc_peek_len(struct Qdisc *sch)
>  		return 0;
>  	}
>  	len = qdisc_pkt_len(skb);
> -	if (unlikely(sch->ops->requeue(skb, sch) != NET_XMIT_SUCCESS)) {
> -		if (net_ratelimit())
> -			printk("qdisc_peek_len: failed to requeue\n");
> -		qdisc_tree_decrease_qlen(sch, 1);
> -		return 0;
> -	}
> +	__skb_queue_head(&sch->requeue, skb);

Sorry for the late response, I didn't follow netdev very closely the
past days. This change does not work properly I'm afraid, it breaks
priorization on two levels:

- when a new packet is enqueued to a higher prioritized class within
HFSC (or a different qdisc), the class peeked at will still be served
on dequeue regardless, increasing worst case latency by a full packet
transmission time.

- when a packet is enqueued to the same class and inner qdisc after
peeking, the inner qdisc can decide to prioritize that packet and
hand it out on the next ->dequeue, but the upper qdisc will serve
the packet peeked at regardless. This results in the same effect,
the worst case latency increases by a full packet transmission time.

I think we really need either ->requeue or a real ->peek operation.

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

[David Miller]

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.

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

[Herbert Xu]

David Miller <davem@davemloft.net> wrote:
> 
> Maybe we can simply add a "bool peek" argument or some flags to
> ->dequeue() instead.

Yeah a peek + the forced dequeue of a specific skb should be
sufficient to replace requeue.

Cheers,
-- 
Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

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

[Patrick McHardy]

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.

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

[Jarek Poplawski]

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.

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

[David Miller]

From: Jarek Poplawski <jarkao2@gmail.com>
Date: Thu, 21 Aug 2008 00:09:32 +0200

> 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.

I still have to pour over all of this feedback, so don't
worry I won't be merging anything like this any time soon :)