| Message ID | 1437475945.9913.7.camel@edumazet-glaptop2.roam.corp.google.com |
|---|---|
| State | RFC, archived |
| Delegated to: | David Miller |
| Headers | show |
On Tue, Jul 21, 2015 at 3:52 AM, Eric Dumazet <eric.dumazet@gmail.com> wrote: > On Tue, 2015-07-21 at 06:04 -0400, Jamal Hadi Salim wrote: > >> It is worrisome to fix the core code for this. The root cause seems to >> be codel. Dont have time but in general, reset would be something like: >> >> struct fq_codel_sched_data *q = qdisc_priv(sch); >> qdisc_reset(q) > > This only works for very simple qdisc with one queue. > >> >> or something along those lines... >> But certainly dequeue semantics dont seem right there.. > > Well, reset() is trivial to implement like this > > while (skb = local_dequeue(sch)) { > kfree_skb(skb); > } > > And I guess I copy/pasted sfq code here, because I was lazy. > > But yes, qdisc_tree_decrease_qlen() would have to be not called. Hmm, so the semantic is each qdisc resets qlen for its own and calls qdisc_reset() to reset its leaf qdisc's, that makes sense for me. > > It seems I coded fq_reset() differently. > > Alex, please try instead : > > diff --git a/net/sched/sch_fq_codel.c b/net/sched/sch_fq_codel.c > index 21ca33c9f036..3f0320ab6029 100644 > --- a/net/sched/sch_fq_codel.c > +++ b/net/sched/sch_fq_codel.c > @@ -288,10 +288,21 @@ begin: > > static void fq_codel_reset(struct Qdisc *sch) > { > - struct sk_buff *skb; > + struct fq_codel_sched_data *q = qdisc_priv(sch); > + int i; > > - while ((skb = fq_codel_dequeue(sch)) != NULL) > - kfree_skb(skb); > + INIT_LIST_HEAD(&q->new_flows); > + INIT_LIST_HEAD(&q->old_flows); > + for (i = 0; i < q->flows_cnt; i++) { > + struct fq_codel_flow *flow = q->flows + i; > + > + while (flow->head) > + kfree_skb(dequeue_head(flow)); > + > + INIT_LIST_HEAD(&flow->flowchain); You probably need to call codel_vars_init(&flow->cvars) as well. > + } > + memset(q->backlogs, 0, q->flows_cnt * sizeof(u32)); > + sch->q.qlen = 0; > } > > static const struct nla_policy fq_codel_policy[TCA_FQ_CODEL_MAX + 1] = { > > > Thanks. -- 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, 2015-07-21 at 11:12 -0700, Cong Wang wrote: > > - kfree_skb(skb); > > + INIT_LIST_HEAD(&q->new_flows); > > + INIT_LIST_HEAD(&q->old_flows); > > + for (i = 0; i < q->flows_cnt; i++) { > > + struct fq_codel_flow *flow = q->flows + i; > > + > > + while (flow->head) > > + kfree_skb(dequeue_head(flow)); > > + > > + INIT_LIST_HEAD(&flow->flowchain); > > > You probably need to call codel_vars_init(&flow->cvars) as well. It is not necessary : flow->cvars only matter in the event of a dequeue, but whole qdisc is dismantled and no packet will be dequeued. -- 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, Jul 21, 2015 at 1:57 PM, Eric Dumazet <eric.dumazet@gmail.com> wrote: > On Tue, 2015-07-21 at 11:12 -0700, Cong Wang wrote: > >> > - kfree_skb(skb); >> > + INIT_LIST_HEAD(&q->new_flows); >> > + INIT_LIST_HEAD(&q->old_flows); >> > + for (i = 0; i < q->flows_cnt; i++) { >> > + struct fq_codel_flow *flow = q->flows + i; >> > + >> > + while (flow->head) >> > + kfree_skb(dequeue_head(flow)); >> > + >> > + INIT_LIST_HEAD(&flow->flowchain); >> >> >> You probably need to call codel_vars_init(&flow->cvars) as well. > > It is not necessary : flow->cvars only matter in the event of a dequeue, > but whole qdisc is dismantled and no packet will be dequeued. > But it will affect the next dequeue _after_ reset? which is not supposed to happen as we expect a fresh start after reset? -- 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, 2015-07-21 at 19:03 -0700, Cong Wang wrote: > On Tue, Jul 21, 2015 at 1:57 PM, Eric Dumazet <eric.dumazet@gmail.com> wrote: > > On Tue, 2015-07-21 at 11:12 -0700, Cong Wang wrote: > > > >> > - kfree_skb(skb); > >> > + INIT_LIST_HEAD(&q->new_flows); > >> > + INIT_LIST_HEAD(&q->old_flows); > >> > + for (i = 0; i < q->flows_cnt; i++) { > >> > + struct fq_codel_flow *flow = q->flows + i; > >> > + > >> > + while (flow->head) > >> > + kfree_skb(dequeue_head(flow)); > >> > + > >> > + INIT_LIST_HEAD(&flow->flowchain); > >> > >> > >> You probably need to call codel_vars_init(&flow->cvars) as well. > > > > It is not necessary : flow->cvars only matter in the event of a dequeue, > > but whole qdisc is dismantled and no packet will be dequeued. > > > > But it will affect the next dequeue _after_ reset? which is not supposed > to happen as we expect a fresh start after reset? Hmm... I thought reset() was only done at queue dismantle, so no new packet should be added later, and since no packet should be left after reset, no dequeue should happen. For completeness, we still can add the codel_vars_init(), no problem. Thanks. -- 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_fq_codel.c b/net/sched/sch_fq_codel.c index 21ca33c9f036..3f0320ab6029 100644 --- a/net/sched/sch_fq_codel.c +++ b/net/sched/sch_fq_codel.c @@ -288,10 +288,21 @@ begin: static void fq_codel_reset(struct Qdisc *sch) { - struct sk_buff *skb; + struct fq_codel_sched_data *q = qdisc_priv(sch); + int i; - while ((skb = fq_codel_dequeue(sch)) != NULL) - kfree_skb(skb); + INIT_LIST_HEAD(&q->new_flows); + INIT_LIST_HEAD(&q->old_flows); + for (i = 0; i < q->flows_cnt; i++) { + struct fq_codel_flow *flow = q->flows + i; + + while (flow->head) + kfree_skb(dequeue_head(flow)); + + INIT_LIST_HEAD(&flow->flowchain); + } + memset(q->backlogs, 0, q->flows_cnt * sizeof(u32)); + sch->q.qlen = 0; } static const struct nla_policy fq_codel_policy[TCA_FQ_CODEL_MAX + 1] = {