Re: [Cake] [PATCH 3/4] sch_api: Allow reducing queue backlog by a negative value
On 01/07/2019 11:47 AM, Toke Høiland-Jørgensen wrote: > From: Toke Høiland-Jørgensen > > With GSO splitting in sch_cake, we can decrease as well as increase the > qlen. To make it possible to signal this up the qdisc tree, change > qdisc_tree_reduce_backlog() to accept signed integer values as the number > of packets and bytes to reduce the backlog by. Not sure why this patch is needed, given we use u32 integers for q.qlen & qstats.backlog 0x for n is really -1 once folded... sch->q.qlen -= n; sch->qstats.backlog -= len; ___ Cake mailing list Cake@lists.bufferbloat.net https://lists.bufferbloat.net/listinfo/cake
Re: [Cake] [PATCH 2/4] sched: Fix detection of empty queues in child qdiscs
awesome. I'd (rarely) seen this bug in drr and qfq and never solved it, thus going for the all-in-one fq_codel On Mon, Jan 7, 2019 at 11:48 AM Toke Høiland-Jørgensen wrote: > > From: Toke Høiland-Jørgensen > > Several qdiscs check on enqueue whether the packet was enqueued to a class > with an empty queue, in which case the class is activated. This is done by > checking if the qlen is exactly 1 after enqueue. However, if GSO splitting > is enabled in the child qdisc, a single packet can result in a qlen longer > than 1. This means the activation check fails, leading to a stalled queue. > > Fix this by checking if the queue is empty *before* enqueue, and running > the activation logic if this was the case. > > Reported-by: Pete Heist > Signed-off-by: Toke Høiland-Jørgensen > --- > net/sched/sch_drr.c | 4 +++- > net/sched/sch_hfsc.c | 4 +++- > net/sched/sch_qfq.c | 4 +++- > 3 files changed, 9 insertions(+), 3 deletions(-) > > diff --git a/net/sched/sch_drr.c b/net/sched/sch_drr.c > index feaf47178653..09b800991065 100644 > --- a/net/sched/sch_drr.c > +++ b/net/sched/sch_drr.c > @@ -354,6 +354,7 @@ static int drr_enqueue(struct sk_buff *skb, struct Qdisc > *sch, > struct drr_sched *q = qdisc_priv(sch); > struct drr_class *cl; > int err = 0; > + bool first; > > cl = drr_classify(skb, sch, ); > if (cl == NULL) { > @@ -363,6 +364,7 @@ static int drr_enqueue(struct sk_buff *skb, struct Qdisc > *sch, > return err; > } > > + first = !cl->qdisc->q.qlen; > err = qdisc_enqueue(skb, cl->qdisc, to_free); > if (unlikely(err != NET_XMIT_SUCCESS)) { > if (net_xmit_drop_count(err)) { > @@ -372,7 +374,7 @@ static int drr_enqueue(struct sk_buff *skb, struct Qdisc > *sch, > return err; > } > > - if (cl->qdisc->q.qlen == 1) { > + if (first) { > list_add_tail(>alist, >active); > cl->deficit = cl->quantum; > } > diff --git a/net/sched/sch_hfsc.c b/net/sched/sch_hfsc.c > index 6bb8f73a8473..24cc220a3218 100644 > --- a/net/sched/sch_hfsc.c > +++ b/net/sched/sch_hfsc.c > @@ -1542,6 +1542,7 @@ hfsc_enqueue(struct sk_buff *skb, struct Qdisc *sch, > struct sk_buff **to_free) > unsigned int len = qdisc_pkt_len(skb); > struct hfsc_class *cl; > int uninitialized_var(err); > + bool first; > > cl = hfsc_classify(skb, sch, ); > if (cl == NULL) { > @@ -1551,6 +1552,7 @@ hfsc_enqueue(struct sk_buff *skb, struct Qdisc *sch, > struct sk_buff **to_free) > return err; > } > > + first = !cl->qdisc->q.qlen; > err = qdisc_enqueue(skb, cl->qdisc, to_free); > if (unlikely(err != NET_XMIT_SUCCESS)) { > if (net_xmit_drop_count(err)) { > @@ -1560,7 +1562,7 @@ hfsc_enqueue(struct sk_buff *skb, struct Qdisc *sch, > struct sk_buff **to_free) > return err; > } > > - if (cl->qdisc->q.qlen == 1) { > + if (first) { > if (cl->cl_flags & HFSC_RSC) > init_ed(cl, len); > if (cl->cl_flags & HFSC_FSC) > diff --git a/net/sched/sch_qfq.c b/net/sched/sch_qfq.c > index 8d5e55d5bed2..29f5c4a24688 100644 > --- a/net/sched/sch_qfq.c > +++ b/net/sched/sch_qfq.c > @@ -1215,6 +1215,7 @@ static int qfq_enqueue(struct sk_buff *skb, struct > Qdisc *sch, > struct qfq_class *cl; > struct qfq_aggregate *agg; > int err = 0; > + bool first; > > cl = qfq_classify(skb, sch, ); > if (cl == NULL) { > @@ -1236,6 +1237,7 @@ static int qfq_enqueue(struct sk_buff *skb, struct > Qdisc *sch, > } > > gso_segs = skb_is_gso(skb) ? skb_shinfo(skb)->gso_segs : 1; > + first = !cl->qdisc->q.qlen; > err = qdisc_enqueue(skb, cl->qdisc, to_free); > if (unlikely(err != NET_XMIT_SUCCESS)) { > pr_debug("qfq_enqueue: enqueue failed %d\n", err); > @@ -1253,7 +1255,7 @@ static int qfq_enqueue(struct sk_buff *skb, struct > Qdisc *sch, > > agg = cl->agg; > /* if the queue was not empty, then done here */ > - if (cl->qdisc->q.qlen != 1) { > + if (!first) { > if (unlikely(skb == cl->qdisc->ops->peek(cl->qdisc)) && > list_first_entry(>active, struct qfq_class, alist) > == cl && cl->deficit < len) > -- > 2.20.1 > > ___ > Cake mailing list > Cake@lists.bufferbloat.net > https://lists.bufferbloat.net/listinfo/cake -- Dave Täht CTO, TekLibre, LLC http://www.teklibre.com Tel: 1-831-205-9740 ___ Cake mailing list Cake@lists.bufferbloat.net https://lists.bufferbloat.net/listinfo/cake
[Cake] [PATCH 4/4] sch_cake: Correctly update parent qlen when splitting GSO packets
From: Toke Høiland-Jørgensen To ensure parent qdiscs have the same notion of the number of enqueued packets even after splitting a GSO packet, update the qdisc tree with the number of packets that was added due to the split. Signed-off-by: Toke Høiland-Jørgensen --- net/sched/sch_cake.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/net/sched/sch_cake.c b/net/sched/sch_cake.c index b910cd5c56f7..73940293700d 100644 --- a/net/sched/sch_cake.c +++ b/net/sched/sch_cake.c @@ -1667,7 +1667,7 @@ static s32 cake_enqueue(struct sk_buff *skb, struct Qdisc *sch, if (skb_is_gso(skb) && q->rate_flags & CAKE_FLAG_SPLIT_GSO) { struct sk_buff *segs, *nskb; netdev_features_t features = netif_skb_features(skb); - unsigned int slen = 0; + unsigned int slen = 0, numsegs = 0; segs = skb_gso_segment(skb, features & ~NETIF_F_GSO_MASK); if (IS_ERR_OR_NULL(segs)) @@ -1683,6 +1683,7 @@ static s32 cake_enqueue(struct sk_buff *skb, struct Qdisc *sch, flow_queue_add(flow, segs); sch->q.qlen++; + numsegs++; slen += segs->len; q->buffer_used += segs->truesize; b->packets++; @@ -1696,7 +1697,7 @@ static s32 cake_enqueue(struct sk_buff *skb, struct Qdisc *sch, sch->qstats.backlog += slen; q->avg_window_bytes += slen; - qdisc_tree_reduce_backlog(sch, 1, len); + qdisc_tree_reduce_backlog(sch, 1-numsegs, len-slen); consume_skb(skb); } else { /* not splitting */ -- 2.20.1 ___ Cake mailing list Cake@lists.bufferbloat.net https://lists.bufferbloat.net/listinfo/cake
[Cake] [PATCH 0/4] sched: Fix qdisc interactions exposed by using sch_cake as a leaf qdisc
This series fixes a couple of issues exposed by running sch_cake as a leaf qdisc in an HFSC tree, which were discovered and reported by Pete Heist. The interaction between CAKE's GSO splitting and the parent qdisc's notion of its own queue length could cause queue stalls. While investigating the report, I also noticed that several qdiscs would dereference the skb pointer after dequeue, which is potentially problematic since the GSO splitting code also frees the original skb. See the individual patches in the series for details. Toke Høiland-Jørgensen (4): sched: Avoid dereferencing skb pointer after child enqueue sched: Fix detection of empty queues in child qdiscs sch_api: Allow reducing queue backlog by a negative value sch_cake: Correctly update parent qlen when splitting GSO packets include/net/sch_generic.h | 3 +-- net/sched/sch_api.c | 3 +-- net/sched/sch_cake.c | 5 +++-- net/sched/sch_cbs.c | 3 ++- net/sched/sch_drr.c | 7 +-- net/sched/sch_dsmark.c| 3 ++- net/sched/sch_hfsc.c | 9 + net/sched/sch_htb.c | 3 ++- net/sched/sch_prio.c | 3 ++- net/sched/sch_qfq.c | 20 net/sched/sch_tbf.c | 3 ++- 11 files changed, 37 insertions(+), 25 deletions(-) -- 2.20.1 ___ Cake mailing list Cake@lists.bufferbloat.net https://lists.bufferbloat.net/listinfo/cake
[Cake] [PATCH 1/4] sched: Avoid dereferencing skb pointer after child enqueue
From: Toke Høiland-Jørgensen Parent qdiscs may dereference the pointer to the enqueued skb after enqueue. However, both CAKE and TBF call consume_skb() on the original skb when splitting GSO packets, leading to a potential use-after-free in the parent. Fix this by avoiding dereferencing the skb pointer after enqueueing to the child. Signed-off-by: Toke Høiland-Jørgensen --- net/sched/sch_cbs.c| 3 ++- net/sched/sch_drr.c| 3 ++- net/sched/sch_dsmark.c | 3 ++- net/sched/sch_hfsc.c | 5 ++--- net/sched/sch_htb.c| 3 ++- net/sched/sch_prio.c | 3 ++- net/sched/sch_qfq.c| 16 +--- net/sched/sch_tbf.c| 3 ++- 8 files changed, 23 insertions(+), 16 deletions(-) diff --git a/net/sched/sch_cbs.c b/net/sched/sch_cbs.c index e689e11b6d0f..c6a502933fe7 100644 --- a/net/sched/sch_cbs.c +++ b/net/sched/sch_cbs.c @@ -88,13 +88,14 @@ static int cbs_child_enqueue(struct sk_buff *skb, struct Qdisc *sch, struct Qdisc *child, struct sk_buff **to_free) { + unsigned int len = qdisc_pkt_len(skb); int err; err = child->ops->enqueue(skb, child, to_free); if (err != NET_XMIT_SUCCESS) return err; - qdisc_qstats_backlog_inc(sch, skb); + sch->qstats.backlog += len; sch->q.qlen++; return NET_XMIT_SUCCESS; diff --git a/net/sched/sch_drr.c b/net/sched/sch_drr.c index cdebaed0f8cf..feaf47178653 100644 --- a/net/sched/sch_drr.c +++ b/net/sched/sch_drr.c @@ -350,6 +350,7 @@ static struct drr_class *drr_classify(struct sk_buff *skb, struct Qdisc *sch, static int drr_enqueue(struct sk_buff *skb, struct Qdisc *sch, struct sk_buff **to_free) { + unsigned int len = qdisc_pkt_len(skb); struct drr_sched *q = qdisc_priv(sch); struct drr_class *cl; int err = 0; @@ -376,7 +377,7 @@ static int drr_enqueue(struct sk_buff *skb, struct Qdisc *sch, cl->deficit = cl->quantum; } - qdisc_qstats_backlog_inc(sch, skb); + sch->qstats.backlog += len; sch->q.qlen++; return err; } diff --git a/net/sched/sch_dsmark.c b/net/sched/sch_dsmark.c index f6f480784bc6..42471464ded3 100644 --- a/net/sched/sch_dsmark.c +++ b/net/sched/sch_dsmark.c @@ -199,6 +199,7 @@ static struct tcf_block *dsmark_tcf_block(struct Qdisc *sch, unsigned long cl, static int dsmark_enqueue(struct sk_buff *skb, struct Qdisc *sch, struct sk_buff **to_free) { + unsigned int len = qdisc_pkt_len(skb); struct dsmark_qdisc_data *p = qdisc_priv(sch); int err; @@ -271,7 +272,7 @@ static int dsmark_enqueue(struct sk_buff *skb, struct Qdisc *sch, return err; } - qdisc_qstats_backlog_inc(sch, skb); + sch->qstats.backlog += len; sch->q.qlen++; return NET_XMIT_SUCCESS; diff --git a/net/sched/sch_hfsc.c b/net/sched/sch_hfsc.c index b18ec1f6de60..6bb8f73a8473 100644 --- a/net/sched/sch_hfsc.c +++ b/net/sched/sch_hfsc.c @@ -1539,6 +1539,7 @@ hfsc_dump_qdisc(struct Qdisc *sch, struct sk_buff *skb) static int hfsc_enqueue(struct sk_buff *skb, struct Qdisc *sch, struct sk_buff **to_free) { + unsigned int len = qdisc_pkt_len(skb); struct hfsc_class *cl; int uninitialized_var(err); @@ -1560,8 +1561,6 @@ hfsc_enqueue(struct sk_buff *skb, struct Qdisc *sch, struct sk_buff **to_free) } if (cl->qdisc->q.qlen == 1) { - unsigned int len = qdisc_pkt_len(skb); - if (cl->cl_flags & HFSC_RSC) init_ed(cl, len); if (cl->cl_flags & HFSC_FSC) @@ -1576,7 +1575,7 @@ hfsc_enqueue(struct sk_buff *skb, struct Qdisc *sch, struct sk_buff **to_free) } - qdisc_qstats_backlog_inc(sch, skb); + sch->qstats.backlog += len; sch->q.qlen++; return NET_XMIT_SUCCESS; diff --git a/net/sched/sch_htb.c b/net/sched/sch_htb.c index 58b449490757..30f9da7e1076 100644 --- a/net/sched/sch_htb.c +++ b/net/sched/sch_htb.c @@ -581,6 +581,7 @@ static int htb_enqueue(struct sk_buff *skb, struct Qdisc *sch, struct sk_buff **to_free) { int uninitialized_var(ret); + unsigned int len = qdisc_pkt_len(skb); struct htb_sched *q = qdisc_priv(sch); struct htb_class *cl = htb_classify(skb, sch, ); @@ -610,7 +611,7 @@ static int htb_enqueue(struct sk_buff *skb, struct Qdisc *sch, htb_activate(q, cl); } - qdisc_qstats_backlog_inc(sch, skb); + sch->qstats.backlog += len; sch->q.qlen++; return NET_XMIT_SUCCESS; } diff --git a/net/sched/sch_prio.c b/net/sched/sch_prio.c index cdf68706e40f..847141cd900f 100644 --- a/net/sched/sch_prio.c +++ b/net/sched/sch_prio.c @@ -72,6 +72,7 @@ prio_classify(struct sk_buff *skb, struct Qdisc *sch, int *qerr) static int prio_enqueue(struct sk_buff *skb, struct Qdisc
[Cake] [PATCH 3/4] sch_api: Allow reducing queue backlog by a negative value
From: Toke Høiland-Jørgensen With GSO splitting in sch_cake, we can decrease as well as increase the qlen. To make it possible to signal this up the qdisc tree, change qdisc_tree_reduce_backlog() to accept signed integer values as the number of packets and bytes to reduce the backlog by. Signed-off-by: Toke Høiland-Jørgensen --- include/net/sch_generic.h | 3 +-- net/sched/sch_api.c | 3 +-- 2 files changed, 2 insertions(+), 4 deletions(-) diff --git a/include/net/sch_generic.h b/include/net/sch_generic.h index 9481f2c142e2..7a4957599874 100644 --- a/include/net/sch_generic.h +++ b/include/net/sch_generic.h @@ -580,8 +580,7 @@ struct Qdisc *dev_graft_qdisc(struct netdev_queue *dev_queue, void qdisc_reset(struct Qdisc *qdisc); void qdisc_put(struct Qdisc *qdisc); void qdisc_put_unlocked(struct Qdisc *qdisc); -void qdisc_tree_reduce_backlog(struct Qdisc *qdisc, unsigned int n, - unsigned int len); +void qdisc_tree_reduce_backlog(struct Qdisc *qdisc, int n, int len); #ifdef CONFIG_NET_SCHED int qdisc_offload_dump_helper(struct Qdisc *q, enum tc_setup_type type, void *type_data); diff --git a/net/sched/sch_api.c b/net/sched/sch_api.c index 7e4d1ccf4c87..03e26e8d0ec9 100644 --- a/net/sched/sch_api.c +++ b/net/sched/sch_api.c @@ -758,8 +758,7 @@ static u32 qdisc_alloc_handle(struct net_device *dev) return 0; } -void qdisc_tree_reduce_backlog(struct Qdisc *sch, unsigned int n, - unsigned int len) +void qdisc_tree_reduce_backlog(struct Qdisc *sch, int n, int len) { bool qdisc_is_offloaded = sch->flags & TCQ_F_OFFLOADED; const struct Qdisc_class_ops *cops; -- 2.20.1 ___ Cake mailing list Cake@lists.bufferbloat.net https://lists.bufferbloat.net/listinfo/cake
Re: [Cake] host hashes and NAT neglected in src/dst-host mode
Yes, of course! I was counting the flow-modes wrong. Sorry for the confusion. On Mon, Jan 7, 2019 at 3:20 AM Jonathan Morton wrote: > > > On 7 Jan, 2019, at 6:00 am, > > wrote: > > > > 633 if ((flow_override || !(flow_mode & CAKE_FLOW_FLOWS)) && > > 634 (host_override || !(flow_mode & CAKE_FLOW_HOSTS))) > > 635 goto skip_hash; > > These lines require careful reading. > > First, the "override" flags indicate whether an external filter has changed > the flow or host hashes, meaning we should not then update them ourselves. > > Secondly, the logic is "if we *don't* need the flow hash *and* we *don't* > need the host hash, then skip the complicated hash code". > > In the dual and triple modes, both the flow and host hashes are required, and > bit-level examination of the codes used to identify them should reflect that. > In "flows" and "host" modes, only one or the other are needed, but they will > still disable the above check (unless an external filter was used). > > In short, only "flowblind" mode or the use of external filters are capable of > skipping that block. > > - Jonathan Morton > ___ Cake mailing list Cake@lists.bufferbloat.net https://lists.bufferbloat.net/listinfo/cake
Re: [Cake] cake infinite loop(?) with hfsc on one-armed router
Sorry, that’s without the patch, will give that a try when I have a chance and post the results, probably tomorrow... > On Jan 7, 2019, at 12:30 PM, Toke Høiland-Jørgensen wrote: > > Pete Heist writes: > >>> On Jan 6, 2019, at 9:56 PM, Toke Høiland-Jørgensen wrote: >>> >>> Pete Heist writes: >>> Lastly, is using cake as a leaf to htb risky until a fix is made? I’ve been doing that for a while without any apparent issues, though I’m hesitating now to try that in a production environment. >>> >>> Hmm, that's a good question. I would expect so; but I would also expect >>> the issue to show up pretty much straight away, so if you haven't hit it >>> yet, I may be wrong. I'll do some more digging... Should probably also >>> try to replicate all this stuff on my own machine :) >> >> >> Ok, after what I’m seeing on my APU1 tests on 3.16.7, I’m definitely >> not putting split GSO into production. I just turned it on and off >> three times and here’s what I got: >> >> Split GSO on: >> >> https://www.heistp.net/downloads/htb_split_gso/htb_cake_split_gso.svg >> https://www.heistp.net/downloads/htb_split_gso/htb_cake_split_gso2.svg >> https://www.heistp.net/downloads/htb_split_gso/htb_cake_split_gso3.svg >> >> Split GSO off: >> >> https://www.heistp.net/downloads/htb_split_gso/htb_cake_no_split_gso.svg >> https://www.heistp.net/downloads/htb_split_gso/htb_cake_no_split_gso2.svg >> https://www.heistp.net/downloads/htb_split_gso/htb_cake_no_split_gso3.svg >> >> I’ve seen these square waves before with htb and wondered where they >> came from, and I think we may finally have an answer! What manner of >> thing causes this I don’t know, but there’s a chance you may end up >> finding out… :) > > Is this without the patch to CAKE that adjusts the qlen? And have you > tried running with that patch (with HTB)? > > -Toke ___ Cake mailing list Cake@lists.bufferbloat.net https://lists.bufferbloat.net/listinfo/cake
Re: [Cake] cake infinite loop(?) with hfsc on one-armed router
Pete Heist writes: >> On Jan 6, 2019, at 9:56 PM, Toke Høiland-Jørgensen wrote: >> >> Pete Heist writes: >> >>> Lastly, is using cake as a leaf to htb risky until a fix is made? I’ve >>> been doing that for a while without any apparent issues, though I’m >>> hesitating now to try that in a production environment. >> >> Hmm, that's a good question. I would expect so; but I would also expect >> the issue to show up pretty much straight away, so if you haven't hit it >> yet, I may be wrong. I'll do some more digging... Should probably also >> try to replicate all this stuff on my own machine :) > > > Ok, after what I’m seeing on my APU1 tests on 3.16.7, I’m definitely > not putting split GSO into production. I just turned it on and off > three times and here’s what I got: > > Split GSO on: > > https://www.heistp.net/downloads/htb_split_gso/htb_cake_split_gso.svg > https://www.heistp.net/downloads/htb_split_gso/htb_cake_split_gso2.svg > https://www.heistp.net/downloads/htb_split_gso/htb_cake_split_gso3.svg > > Split GSO off: > > https://www.heistp.net/downloads/htb_split_gso/htb_cake_no_split_gso.svg > https://www.heistp.net/downloads/htb_split_gso/htb_cake_no_split_gso2.svg > https://www.heistp.net/downloads/htb_split_gso/htb_cake_no_split_gso3.svg > > I’ve seen these square waves before with htb and wondered where they > came from, and I think we may finally have an answer! What manner of > thing causes this I don’t know, but there’s a chance you may end up > finding out… :) Is this without the patch to CAKE that adjusts the qlen? And have you tried running with that patch (with HTB)? -Toke ___ Cake mailing list Cake@lists.bufferbloat.net https://lists.bufferbloat.net/listinfo/cake
Re: [Cake] host hashes and NAT neglected in src/dst-host mode
> On 7 Jan, 2019, at 6:00 am, wrote: > > 633 if ((flow_override || !(flow_mode & CAKE_FLOW_FLOWS)) && > 634 (host_override || !(flow_mode & CAKE_FLOW_HOSTS))) > 635 goto skip_hash; These lines require careful reading. First, the "override" flags indicate whether an external filter has changed the flow or host hashes, meaning we should not then update them ourselves. Secondly, the logic is "if we *don't* need the flow hash *and* we *don't* need the host hash, then skip the complicated hash code". In the dual and triple modes, both the flow and host hashes are required, and bit-level examination of the codes used to identify them should reflect that. In "flows" and "host" modes, only one or the other are needed, but they will still disable the above check (unless an external filter was used). In short, only "flowblind" mode or the use of external filters are capable of skipping that block. - Jonathan Morton ___ Cake mailing list Cake@lists.bufferbloat.net https://lists.bufferbloat.net/listinfo/cake