Patchwork [net-next,v5,10/11] tbf: take into account gso skbs

login
register
mail settings
Submitter Jiri Pirko
Date Feb. 12, 2013, 10:12 a.m.
Message ID <1360663929-1023-11-git-send-email-jiri@resnulli.us>
Download mbox | patch
Permalink /patch/219768/
State Changes Requested
Delegated to: David Miller
Headers show

Comments

Jiri Pirko - Feb. 12, 2013, 10:12 a.m.
Ignore max_size check for gso skbs. This check made bigger packets
incorrectly dropped. Remove this limitation for gso skbs.

Also for peaks, ignore mtu for gso skbs.

Signed-off-by: Jiri Pirko <jiri@resnulli.us>
---
 net/sched/sch_tbf.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)
Eric Dumazet - Feb. 12, 2013, 4:39 p.m.
On Tue, 2013-02-12 at 11:12 +0100, Jiri Pirko wrote:
> Ignore max_size check for gso skbs. This check made bigger packets
> incorrectly dropped. Remove this limitation for gso skbs.
> 
> Also for peaks, ignore mtu for gso skbs.
> 
> Signed-off-by: Jiri Pirko <jiri@resnulli.us>
> ---
>  net/sched/sch_tbf.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/net/sched/sch_tbf.c b/net/sched/sch_tbf.c
> index c8388f3..8973e93 100644
> --- a/net/sched/sch_tbf.c
> +++ b/net/sched/sch_tbf.c
> @@ -121,7 +121,7 @@ static int tbf_enqueue(struct sk_buff *skb, struct Qdisc *sch)
>  	struct tbf_sched_data *q = qdisc_priv(sch);
>  	int ret;
>  
> -	if (qdisc_pkt_len(skb) > q->max_size)
> +	if (qdisc_pkt_len(skb) > q->max_size && !skb_is_gso(skb))
>  		return qdisc_reshape_fail(skb, sch);
>  
>  	ret = qdisc_enqueue(skb, q->qdisc);
> @@ -165,7 +165,7 @@ static struct sk_buff *tbf_dequeue(struct Qdisc *sch)
>  
>  		if (q->peak_present) {
>  			ptoks = toks + q->ptokens;
> -			if (ptoks > q->mtu)
> +			if (ptoks > q->mtu && !skb_is_gso(skb))
>  				ptoks = q->mtu;
>  			ptoks -= (s64) psched_l2t_ns(&q->peak, len);
>  		}


I guess this part is wrong.

If we dont cap ptoks to q->mtu we allow bigger bursts.

Ideally we could re-segment the skb if psched_l2t_ns(&q->peak, len) is
bigger than q->mtu






--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Jiri Pirko - Feb. 12, 2013, 5:31 p.m.
Tue, Feb 12, 2013 at 05:39:42PM CET, eric.dumazet@gmail.com wrote:
>On Tue, 2013-02-12 at 11:12 +0100, Jiri Pirko wrote:
>> Ignore max_size check for gso skbs. This check made bigger packets
>> incorrectly dropped. Remove this limitation for gso skbs.
>> 
>> Also for peaks, ignore mtu for gso skbs.
>> 
>> Signed-off-by: Jiri Pirko <jiri@resnulli.us>
>> ---
>>  net/sched/sch_tbf.c | 4 ++--
>>  1 file changed, 2 insertions(+), 2 deletions(-)
>> 
>> diff --git a/net/sched/sch_tbf.c b/net/sched/sch_tbf.c
>> index c8388f3..8973e93 100644
>> --- a/net/sched/sch_tbf.c
>> +++ b/net/sched/sch_tbf.c
>> @@ -121,7 +121,7 @@ static int tbf_enqueue(struct sk_buff *skb, struct Qdisc *sch)
>>  	struct tbf_sched_data *q = qdisc_priv(sch);
>>  	int ret;
>>  
>> -	if (qdisc_pkt_len(skb) > q->max_size)
>> +	if (qdisc_pkt_len(skb) > q->max_size && !skb_is_gso(skb))
>>  		return qdisc_reshape_fail(skb, sch);
>>  
>>  	ret = qdisc_enqueue(skb, q->qdisc);
>> @@ -165,7 +165,7 @@ static struct sk_buff *tbf_dequeue(struct Qdisc *sch)
>>  
>>  		if (q->peak_present) {
>>  			ptoks = toks + q->ptokens;
>> -			if (ptoks > q->mtu)
>> +			if (ptoks > q->mtu && !skb_is_gso(skb))
>>  				ptoks = q->mtu;
>>  			ptoks -= (s64) psched_l2t_ns(&q->peak, len);
>>  		}
>
>
>I guess this part is wrong.
>
>If we dont cap ptoks to q->mtu we allow bigger bursts.
>
>Ideally we could re-segment the skb if psched_l2t_ns(&q->peak, len) is
>bigger than q->mtu

Okay - that sounds reasonable. Can you give me some hint how would you
imagine to do this?

Thanks.

Jiri

>
>
>
>
>
>
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Eric Dumazet - Feb. 12, 2013, 5:54 p.m.
On Tue, 2013-02-12 at 18:31 +0100, Jiri Pirko wrote:
> Tue, Feb 12, 2013 at 05:39:42PM CET, eric.dumazet@gmail.com wrote:

> >Ideally we could re-segment the skb if psched_l2t_ns(&q->peak, len) is
> >bigger than q->mtu
> 
> Okay - that sounds reasonable. Can you give me some hint how would you
> imagine to do this?
> 

This should be a generic helper, and we could use it in sch_codel /
sch_fq_codel / netem as well.

The trick in a qdisc is that we have to call qdisc_tree_decrease_qlen()
to alert parents that packet count changed.

If a GSO packet with 10 segments is segmented, we have to
qdisc_tree_decrease_qlen(sch, 1 - 10);



--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Jiri Pirko - Feb. 17, 2013, 4:18 p.m.
Tue, Feb 12, 2013 at 05:39:42PM CET, eric.dumazet@gmail.com wrote:
>On Tue, 2013-02-12 at 11:12 +0100, Jiri Pirko wrote:
>> Ignore max_size check for gso skbs. This check made bigger packets
>> incorrectly dropped. Remove this limitation for gso skbs.
>> 
>> Also for peaks, ignore mtu for gso skbs.
>> 
>> Signed-off-by: Jiri Pirko <jiri@resnulli.us>
>> ---
>>  net/sched/sch_tbf.c | 4 ++--
>>  1 file changed, 2 insertions(+), 2 deletions(-)
>> 
>> diff --git a/net/sched/sch_tbf.c b/net/sched/sch_tbf.c
>> index c8388f3..8973e93 100644
>> --- a/net/sched/sch_tbf.c
>> +++ b/net/sched/sch_tbf.c
>> @@ -121,7 +121,7 @@ static int tbf_enqueue(struct sk_buff *skb, struct Qdisc *sch)
>>  	struct tbf_sched_data *q = qdisc_priv(sch);
>>  	int ret;
>>  
>> -	if (qdisc_pkt_len(skb) > q->max_size)
>> +	if (qdisc_pkt_len(skb) > q->max_size && !skb_is_gso(skb))
>>  		return qdisc_reshape_fail(skb, sch);
>>  
>>  	ret = qdisc_enqueue(skb, q->qdisc);
>> @@ -165,7 +165,7 @@ static struct sk_buff *tbf_dequeue(struct Qdisc *sch)
>>  
>>  		if (q->peak_present) {
>>  			ptoks = toks + q->ptokens;
>> -			if (ptoks > q->mtu)
>> +			if (ptoks > q->mtu && !skb_is_gso(skb))
>>  				ptoks = q->mtu;
>>  			ptoks -= (s64) psched_l2t_ns(&q->peak, len);
>>  		}
>
>
>I guess this part is wrong.
>
>If we dont cap ptoks to q->mtu we allow bigger bursts.


I'm going through this issue back and front and on the second thought,
I think this patch might not be so wrong after all.

"Accumulating" time in ptoks would effectively cause the skb to be sent
only in case time for whole skb is available (accumulated).

The re-segmenting will only cause the skb fragments sent in each time frame.

I can't see how the bigger bursts you are reffering to can happen.

Or am I missing something?

>
>Ideally we could re-segment the skb if psched_l2t_ns(&q->peak, len) is
>bigger than q->mtu
>
>
>
>
>
>
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Eric Dumazet - Feb. 17, 2013, 5:54 p.m.
On Sun, 2013-02-17 at 17:18 +0100, Jiri Pirko wrote:

> I'm going through this issue back and front and on the second thought,
> I think this patch might not be so wrong after all.
> 
> "Accumulating" time in ptoks would effectively cause the skb to be sent
> only in case time for whole skb is available (accumulated).
> 
> The re-segmenting will only cause the skb fragments sent in each time frame.
> 
> I can't see how the bigger bursts you are reffering to can happen.
> 
> Or am I missing something?

Token Bucket Filter doesnt allow to accumulate tokens above a given
threshold. Thats the whole point of the algo.

After a one hour idle time, you don't want to allow your device sending
a burst exceeding the constraint.

This is all about avoiding packet drops in a device with a very small
queue.

Your patch was pretty close to solve the problem.


--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch

diff --git a/net/sched/sch_tbf.c b/net/sched/sch_tbf.c
index c8388f3..8973e93 100644
--- a/net/sched/sch_tbf.c
+++ b/net/sched/sch_tbf.c
@@ -121,7 +121,7 @@  static int tbf_enqueue(struct sk_buff *skb, struct Qdisc *sch)
 	struct tbf_sched_data *q = qdisc_priv(sch);
 	int ret;
 
-	if (qdisc_pkt_len(skb) > q->max_size)
+	if (qdisc_pkt_len(skb) > q->max_size && !skb_is_gso(skb))
 		return qdisc_reshape_fail(skb, sch);
 
 	ret = qdisc_enqueue(skb, q->qdisc);
@@ -165,7 +165,7 @@  static struct sk_buff *tbf_dequeue(struct Qdisc *sch)
 
 		if (q->peak_present) {
 			ptoks = toks + q->ptokens;
-			if (ptoks > q->mtu)
+			if (ptoks > q->mtu && !skb_is_gso(skb))
 				ptoks = q->mtu;
 			ptoks -= (s64) psched_l2t_ns(&q->peak, len);
 		}