| Message ID | 1360663929-1023-11-git-send-email-jiri@resnulli.us |
|---|---|
| State | Changes Requested, archived |
| Delegated to: | David Miller |
| Headers | show |
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
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
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
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
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
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); }
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(-)