* [Patch net 1/2] sch_pie: schedule the timer after all init succeed
@ 2014-10-24 23:55 Cong Wang
2014-10-24 23:55 ` [Patch net 2/2] net_sched: always call ->destroy when ->init() fails Cong Wang
` (2 more replies)
0 siblings, 3 replies; 19+ messages in thread
From: Cong Wang @ 2014-10-24 23:55 UTC (permalink / raw)
To: netdev; +Cc: davem, Cong Wang, Vijay Subramanian
Cc: Vijay Subramanian <vijaynsu@cisco.com>
Cc: David S. Miller <davem@davemloft.net>
Signed-off-by: Cong Wang <xiyou.wangcong@gmail.com>
---
net/sched/sch_pie.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/net/sched/sch_pie.c b/net/sched/sch_pie.c
index 33d7a98..b783a44 100644
--- a/net/sched/sch_pie.c
+++ b/net/sched/sch_pie.c
@@ -445,7 +445,6 @@ static int pie_init(struct Qdisc *sch, struct nlattr *opt)
sch->limit = q->params.limit;
setup_timer(&q->adapt_timer, pie_timer, (unsigned long)sch);
- mod_timer(&q->adapt_timer, jiffies + HZ / 2);
if (opt) {
int err = pie_change(sch, opt);
@@ -454,6 +453,7 @@ static int pie_init(struct Qdisc *sch, struct nlattr *opt)
return err;
}
+ mod_timer(&q->adapt_timer, jiffies + HZ / 2);
return 0;
}
--
1.8.3.1
^ permalink raw reply related [flat|nested] 19+ messages in thread* [Patch net 2/2] net_sched: always call ->destroy when ->init() fails 2014-10-24 23:55 [Patch net 1/2] sch_pie: schedule the timer after all init succeed Cong Wang @ 2014-10-24 23:55 ` Cong Wang 2014-10-25 0:14 ` Eric Dumazet 2014-10-29 18:29 ` David Miller 2014-10-25 0:17 ` [Patch net 1/2] sch_pie: schedule the timer after all init succeed Eric Dumazet 2014-10-29 18:28 ` David Miller 2 siblings, 2 replies; 19+ messages in thread From: Cong Wang @ 2014-10-24 23:55 UTC (permalink / raw) To: netdev Cc: davem, Cong Wang, Wang Bo, John Fastabend, Eric Dumazet, Patrick McHardy, Terry Lam In qdisc_create(), when ->init() exists and it fails, we should call ->destroy() to clean up the potentially partially initialized qdisc's. This will also make the ->init() implementation easier. qdisc_create_dflt() already does that. Most callers of qdisc_create_dflt() simply use noop_qdisc when it fails. And, most of the ->destroy() implementations are already able to clean up partially initialization, we don't need to worry. The following ->init()'s need to catch up: fq_codel_init(), hhf_init(), multiq_init(), sfq_init(), mq_init(), mqprio_init(). Reported-by: Wang Bo <wang.bo116@zte.com.cn> Cc: Wang Bo <wang.bo116@zte.com.cn> Cc: John Fastabend <john.r.fastabend@intel.com> Cc: Eric Dumazet <edumazet@google.com> Cc: David S. Miller <davem@davemloft.net> Cc: Patrick McHardy <kaber@trash.net> Cc: Terry Lam <vtlam@google.com> Signed-off-by: Cong Wang <xiyou.wangcong@gmail.com> --- net/sched/sch_api.c | 2 ++ net/sched/sch_drr.c | 1 - net/sched/sch_fq_codel.c | 6 +++--- net/sched/sch_generic.c | 1 + net/sched/sch_hhf.c | 8 ++------ net/sched/sch_mq.c | 6 +----- net/sched/sch_mqprio.c | 18 +++++------------- net/sched/sch_multiq.c | 9 ++------- net/sched/sch_qfq.c | 1 - net/sched/sch_sfq.c | 7 ++++--- 10 files changed, 20 insertions(+), 39 deletions(-) diff --git a/net/sched/sch_api.c b/net/sched/sch_api.c index 76f402e..7c308c9 100644 --- a/net/sched/sch_api.c +++ b/net/sched/sch_api.c @@ -990,6 +990,8 @@ qdisc_create(struct net_device *dev, struct netdev_queue *dev_queue, qdisc_list_add(sch); return sch; + } else { + goto err_out4; } err_out3: dev_put(dev); diff --git a/net/sched/sch_drr.c b/net/sched/sch_drr.c index 3387060..6c69b88 100644 --- a/net/sched/sch_drr.c +++ b/net/sched/sch_drr.c @@ -121,7 +121,6 @@ static int drr_change_class(struct Qdisc *sch, u32 classid, u32 parentid, qdisc_root_sleeping_lock(sch), tca[TCA_RATE]); if (err) { - qdisc_destroy(cl->qdisc); kfree(cl); return err; } diff --git a/net/sched/sch_fq_codel.c b/net/sched/sch_fq_codel.c index b9ca32e..05c725f 100644 --- a/net/sched/sch_fq_codel.c +++ b/net/sched/sch_fq_codel.c @@ -406,11 +406,11 @@ static int fq_codel_init(struct Qdisc *sch, struct nlattr *opt) sizeof(struct fq_codel_flow)); if (!q->flows) return -ENOMEM; + q->backlogs = fq_codel_zalloc(q->flows_cnt * sizeof(u32)); - if (!q->backlogs) { - fq_codel_free(q->flows); + if (!q->backlogs) return -ENOMEM; - } + for (i = 0; i < q->flows_cnt; i++) { struct fq_codel_flow *flow = q->flows + i; diff --git a/net/sched/sch_generic.c b/net/sched/sch_generic.c index 6efca30..d1e2ed6 100644 --- a/net/sched/sch_generic.c +++ b/net/sched/sch_generic.c @@ -622,6 +622,7 @@ struct Qdisc *qdisc_alloc(struct netdev_queue *dev_queue, return ERR_PTR(err); } +/* Callers don't need to clean up on failure, we do here. */ struct Qdisc *qdisc_create_dflt(struct netdev_queue *dev_queue, const struct Qdisc_ops *ops, unsigned int parentid) diff --git a/net/sched/sch_hhf.c b/net/sched/sch_hhf.c index 15d3aab..8f94bb9 100644 --- a/net/sched/sch_hhf.c +++ b/net/sched/sch_hhf.c @@ -639,10 +639,8 @@ static int hhf_init(struct Qdisc *sch, struct nlattr *opt) for (i = 0; i < HHF_ARRAYS_CNT; i++) { q->hhf_arrays[i] = hhf_zalloc(HHF_ARRAYS_LEN * sizeof(u32)); - if (!q->hhf_arrays[i]) { - hhf_destroy(sch); + if (!q->hhf_arrays[i]) return -ENOMEM; - } } q->hhf_arrays_reset_timestamp = hhf_time_stamp(); @@ -650,10 +648,8 @@ static int hhf_init(struct Qdisc *sch, struct nlattr *opt) for (i = 0; i < HHF_ARRAYS_CNT; i++) { q->hhf_valid_bits[i] = hhf_zalloc(HHF_ARRAYS_LEN / BITS_PER_BYTE); - if (!q->hhf_valid_bits[i]) { - hhf_destroy(sch); + if (!q->hhf_valid_bits[i]) return -ENOMEM; - } } /* Initialize Weighted DRR buckets. */ diff --git a/net/sched/sch_mq.c b/net/sched/sch_mq.c index f3cbaec..8f009c9 100644 --- a/net/sched/sch_mq.c +++ b/net/sched/sch_mq.c @@ -61,17 +61,13 @@ static int mq_init(struct Qdisc *sch, struct nlattr *opt) TC_H_MAKE(TC_H_MAJ(sch->handle), TC_H_MIN(ntx + 1))); if (qdisc == NULL) - goto err; + return -ENOMEM; priv->qdiscs[ntx] = qdisc; qdisc->flags |= TCQ_F_ONETXQUEUE; } sch->flags |= TCQ_F_MQROOT; return 0; - -err: - mq_destroy(sch); - return -ENOMEM; } static void mq_attach(struct Qdisc *sch) diff --git a/net/sched/sch_mqprio.c b/net/sched/sch_mqprio.c index 3811a74..d172819 100644 --- a/net/sched/sch_mqprio.c +++ b/net/sched/sch_mqprio.c @@ -117,20 +117,16 @@ static int mqprio_init(struct Qdisc *sch, struct nlattr *opt) /* pre-allocate qdisc, attachment can't fail */ priv->qdiscs = kcalloc(dev->num_tx_queues, sizeof(priv->qdiscs[0]), GFP_KERNEL); - if (priv->qdiscs == NULL) { - err = -ENOMEM; - goto err; - } + if (priv->qdiscs == NULL) + return -ENOMEM; for (i = 0; i < dev->num_tx_queues; i++) { dev_queue = netdev_get_tx_queue(dev, i); qdisc = qdisc_create_dflt(dev_queue, default_qdisc_ops, TC_H_MAKE(TC_H_MAJ(sch->handle), TC_H_MIN(i + 1))); - if (qdisc == NULL) { - err = -ENOMEM; - goto err; - } + if (qdisc == NULL) + return -ENOMEM; priv->qdiscs[i] = qdisc; qdisc->flags |= TCQ_F_ONETXQUEUE; } @@ -143,7 +139,7 @@ static int mqprio_init(struct Qdisc *sch, struct nlattr *opt) priv->hw_owned = 1; err = dev->netdev_ops->ndo_setup_tc(dev, qopt->num_tc); if (err) - goto err; + return err; } else { netdev_set_num_tc(dev, qopt->num_tc); for (i = 0; i < qopt->num_tc; i++) @@ -157,10 +153,6 @@ static int mqprio_init(struct Qdisc *sch, struct nlattr *opt) sch->flags |= TCQ_F_MQROOT; return 0; - -err: - mqprio_destroy(sch); - return err; } static void mqprio_attach(struct Qdisc *sch) diff --git a/net/sched/sch_multiq.c b/net/sched/sch_multiq.c index 42dd218..31dd2d2 100644 --- a/net/sched/sch_multiq.c +++ b/net/sched/sch_multiq.c @@ -252,7 +252,7 @@ static int multiq_tune(struct Qdisc *sch, struct nlattr *opt) static int multiq_init(struct Qdisc *sch, struct nlattr *opt) { struct multiq_sched_data *q = qdisc_priv(sch); - int i, err; + int i; q->queues = NULL; @@ -267,12 +267,7 @@ static int multiq_init(struct Qdisc *sch, struct nlattr *opt) for (i = 0; i < q->max_bands; i++) q->queues[i] = &noop_qdisc; - err = multiq_tune(sch, opt); - - if (err) - kfree(q->queues); - - return err; + return multiq_tune(sch, opt); } static int multiq_dump(struct Qdisc *sch, struct sk_buff *skb) diff --git a/net/sched/sch_qfq.c b/net/sched/sch_qfq.c index 3ec7e88..55ac6a4 100644 --- a/net/sched/sch_qfq.c +++ b/net/sched/sch_qfq.c @@ -522,7 +522,6 @@ static int qfq_change_class(struct Qdisc *sch, u32 classid, u32 parentid, return 0; destroy_class: - qdisc_destroy(cl->qdisc); kfree(cl); return err; } diff --git a/net/sched/sch_sfq.c b/net/sched/sch_sfq.c index b877140..7ad2879 100644 --- a/net/sched/sch_sfq.c +++ b/net/sched/sch_sfq.c @@ -761,11 +761,12 @@ static int sfq_init(struct Qdisc *sch, struct nlattr *opt) } q->ht = sfq_alloc(sizeof(q->ht[0]) * q->divisor); + if (!q->ht) + return -ENOMEM; q->slots = sfq_alloc(sizeof(q->slots[0]) * q->maxflows); - if (!q->ht || !q->slots) { - sfq_destroy(sch); + if (!q->slots) return -ENOMEM; - } + for (i = 0; i < q->divisor; i++) q->ht[i] = SFQ_EMPTY_SLOT; -- 1.8.3.1 ^ permalink raw reply related [flat|nested] 19+ messages in thread
* Re: [Patch net 2/2] net_sched: always call ->destroy when ->init() fails 2014-10-24 23:55 ` [Patch net 2/2] net_sched: always call ->destroy when ->init() fails Cong Wang @ 2014-10-25 0:14 ` Eric Dumazet 2014-10-25 0:36 ` Cong Wang 2014-10-25 0:36 ` Patrick McHardy 2014-10-29 18:29 ` David Miller 1 sibling, 2 replies; 19+ messages in thread From: Eric Dumazet @ 2014-10-25 0:14 UTC (permalink / raw) To: Cong Wang Cc: netdev, davem, Wang Bo, John Fastabend, Eric Dumazet, Patrick McHardy, Terry Lam On Fri, 2014-10-24 at 16:55 -0700, Cong Wang wrote: > In qdisc_create(), when ->init() exists and it fails, we should > call ->destroy() to clean up the potentially partially initialized > qdisc's. This will also make the ->init() implementation easier. > Why is this patch needed ? You are adding bugs, its not clear what bug you are fixing. I really do not like the idea of ->init() relying on a ->destroy() to cleanup a failed ->init(). This is not what most management functions do in our stack. I very much prefer that a function returning an error has no side effect, like if it hadnt be called at all. > qdisc_create_dflt() already does that. Most callers of qdisc_create_dflt() > simply use noop_qdisc when it fails. > > And, most of the ->destroy() implementations are already able to clean up > partially initialization, we don't need to worry. > > The following ->init()'s need to catch up: > fq_codel_init(), hhf_init(), multiq_init(), sfq_init(), mq_init(), > mqprio_init(). > ... > > static int multiq_dump(struct Qdisc *sch, struct sk_buff *skb) > diff --git a/net/sched/sch_qfq.c b/net/sched/sch_qfq.c > index 3ec7e88..55ac6a4 100644 > --- a/net/sched/sch_qfq.c > +++ b/net/sched/sch_qfq.c > @@ -522,7 +522,6 @@ static int qfq_change_class(struct Qdisc *sch, u32 classid, u32 parentid, > return 0; > > destroy_class: > - qdisc_destroy(cl->qdisc); > kfree(cl); > return err; > } Really ? I am calling this a leak of cl->qdisc. ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [Patch net 2/2] net_sched: always call ->destroy when ->init() fails 2014-10-25 0:14 ` Eric Dumazet @ 2014-10-25 0:36 ` Cong Wang 2014-10-25 0:40 ` Eric Dumazet 2014-10-25 0:42 ` Eric Dumazet 2014-10-25 0:36 ` Patrick McHardy 1 sibling, 2 replies; 19+ messages in thread From: Cong Wang @ 2014-10-25 0:36 UTC (permalink / raw) To: Eric Dumazet Cc: Cong Wang, netdev, David Miller, Wang Bo, John Fastabend, Eric Dumazet, Patrick McHardy, Terry Lam On Fri, Oct 24, 2014 at 5:14 PM, Eric Dumazet <eric.dumazet@gmail.com> wrote: > On Fri, 2014-10-24 at 16:55 -0700, Cong Wang wrote: >> In qdisc_create(), when ->init() exists and it fails, we should >> call ->destroy() to clean up the potentially partially initialized >> qdisc's. This will also make the ->init() implementation easier. >> > > Why is this patch needed ? > > You are adding bugs, its not clear what bug you are fixing. You missed what Wang Bo reported, we could double free mq qdisc on failure path, one in mq_init() and one in qdisc_create_dflt(). > > I really do not like the idea of ->init() relying on a ->destroy() to > cleanup a failed ->init(). > > This is not what most management functions do in our stack. > > I very much prefer that a function returning an error has no side > effect, like if it hadnt be called at all. If you would like to fix all qdisc_create_dflt() callers, yes, and good luck with even more complex error handling there. Read the diffstat, if we could fix a bug with less code, why not.... > >> qdisc_create_dflt() already does that. Most callers of qdisc_create_dflt() >> simply use noop_qdisc when it fails. >> >> And, most of the ->destroy() implementations are already able to clean up >> partially initialization, we don't need to worry. >> >> The following ->init()'s need to catch up: >> fq_codel_init(), hhf_init(), multiq_init(), sfq_init(), mq_init(), >> mqprio_init(). >> > > ... > >> >> static int multiq_dump(struct Qdisc *sch, struct sk_buff *skb) >> diff --git a/net/sched/sch_qfq.c b/net/sched/sch_qfq.c >> index 3ec7e88..55ac6a4 100644 >> --- a/net/sched/sch_qfq.c >> +++ b/net/sched/sch_qfq.c >> @@ -522,7 +522,6 @@ static int qfq_change_class(struct Qdisc *sch, u32 classid, u32 parentid, >> return 0; >> >> destroy_class: >> - qdisc_destroy(cl->qdisc); >> kfree(cl); >> return err; >> } > > > Really ? I am calling this a leak of cl->qdisc. > > How? I already added a comment on qdisc_create_dflt(), callers don't need to clean up on failure path. ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [Patch net 2/2] net_sched: always call ->destroy when ->init() fails 2014-10-25 0:36 ` Cong Wang @ 2014-10-25 0:40 ` Eric Dumazet 2014-10-25 0:42 ` Eric Dumazet 1 sibling, 0 replies; 19+ messages in thread From: Eric Dumazet @ 2014-10-25 0:40 UTC (permalink / raw) To: Cong Wang Cc: Cong Wang, netdev, David Miller, Wang Bo, John Fastabend, Eric Dumazet, Patrick McHardy, Terry Lam On Fri, 2014-10-24 at 17:36 -0700, Cong Wang wrote: > >> static int multiq_dump(struct Qdisc *sch, struct sk_buff *skb) > >> diff --git a/net/sched/sch_qfq.c b/net/sched/sch_qfq.c > >> index 3ec7e88..55ac6a4 100644 > >> --- a/net/sched/sch_qfq.c > >> +++ b/net/sched/sch_qfq.c > >> @@ -522,7 +522,6 @@ static int qfq_change_class(struct Qdisc *sch, u32 classid, u32 parentid, > >> return 0; > >> > >> destroy_class: > >> - qdisc_destroy(cl->qdisc); > >> kfree(cl); > >> return err; > >> } > > > > > > Really ? I am calling this a leak of cl->qdisc. > > > > > > How? I already added a comment on qdisc_create_dflt(), callers don't > need to clean up > on failure path. Seriously, can you read the function again ? What happens if gen_new_estimator() fails ? ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [Patch net 2/2] net_sched: always call ->destroy when ->init() fails 2014-10-25 0:36 ` Cong Wang 2014-10-25 0:40 ` Eric Dumazet @ 2014-10-25 0:42 ` Eric Dumazet 1 sibling, 0 replies; 19+ messages in thread From: Eric Dumazet @ 2014-10-25 0:42 UTC (permalink / raw) To: Cong Wang Cc: Cong Wang, netdev, David Miller, Wang Bo, John Fastabend, Eric Dumazet, Patrick McHardy, Terry Lam On Fri, 2014-10-24 at 17:36 -0700, Cong Wang wrote: > You missed what Wang Bo reported, we could double free > mq qdisc on failure path, one in mq_init() and one in qdisc_create_dflt(). So fix mq/mqprio ? I am sorry, but sending patches on Friday afternoon should be forbidden, too much beer lying around ;) ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [Patch net 2/2] net_sched: always call ->destroy when ->init() fails 2014-10-25 0:14 ` Eric Dumazet 2014-10-25 0:36 ` Cong Wang @ 2014-10-25 0:36 ` Patrick McHardy 2014-10-25 0:38 ` Cong Wang 1 sibling, 1 reply; 19+ messages in thread From: Patrick McHardy @ 2014-10-25 0:36 UTC (permalink / raw) To: Eric Dumazet Cc: Cong Wang, netdev, davem, Wang Bo, John Fastabend, Eric Dumazet, Terry Lam On Fri, Oct 24, 2014 at 05:14:13PM -0700, Eric Dumazet wrote: > On Fri, 2014-10-24 at 16:55 -0700, Cong Wang wrote: > > In qdisc_create(), when ->init() exists and it fails, we should > > call ->destroy() to clean up the potentially partially initialized > > qdisc's. This will also make the ->init() implementation easier. > > > > Why is this patch needed ? > > You are adding bugs, its not clear what bug you are fixing. > > I really do not like the idea of ->init() relying on a ->destroy() to > cleanup a failed ->init(). > > This is not what most management functions do in our stack. > > I very much prefer that a function returning an error has no side > effect, like if it hadnt be called at all. Absolutely. Again, the correct fix is to make qdisc_create_dflt() not call qdisc_destroy() but clean up the qdisc manually as done in qdisc_create(). ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [Patch net 2/2] net_sched: always call ->destroy when ->init() fails 2014-10-25 0:36 ` Patrick McHardy @ 2014-10-25 0:38 ` Cong Wang 2014-10-25 0:44 ` Patrick McHardy 0 siblings, 1 reply; 19+ messages in thread From: Cong Wang @ 2014-10-25 0:38 UTC (permalink / raw) To: Patrick McHardy Cc: Eric Dumazet, Cong Wang, netdev, David Miller, Wang Bo, John Fastabend, Eric Dumazet, Terry Lam On Fri, Oct 24, 2014 at 5:36 PM, Patrick McHardy <kaber@trash.net> wrote: > > Again, the correct fix is to make qdisc_create_dflt() not call > qdisc_destroy() but clean up the qdisc manually as done in > qdisc_create(). I kindly wish you a good luck with fixing all callers of qdisc_create_dflt(). Go ahead. :) ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [Patch net 2/2] net_sched: always call ->destroy when ->init() fails 2014-10-25 0:38 ` Cong Wang @ 2014-10-25 0:44 ` Patrick McHardy 2014-10-25 0:53 ` Cong Wang 0 siblings, 1 reply; 19+ messages in thread From: Patrick McHardy @ 2014-10-25 0:44 UTC (permalink / raw) To: Cong Wang Cc: Eric Dumazet, Cong Wang, netdev, David Miller, Wang Bo, John Fastabend, Eric Dumazet, Terry Lam On Fri, Oct 24, 2014 at 05:38:48PM -0700, Cong Wang wrote: > On Fri, Oct 24, 2014 at 5:36 PM, Patrick McHardy <kaber@trash.net> wrote: > > > > Again, the correct fix is to make qdisc_create_dflt() not call > > qdisc_destroy() but clean up the qdisc manually as done in > > qdisc_create(). > > I kindly wish you a good luck with fixing all callers of qdisc_create_dflt(). > Go ahead. :) Here you go: diff --git a/net/sched/sch_generic.c b/net/sched/sch_generic.c index fc04fe9..3a71e51 100644 --- a/net/sched/sch_generic.c +++ b/net/sched/sch_generic.c @@ -590,18 +590,21 @@ struct Qdisc *qdisc_create_dflt(struct netdev_queue *dev_queue, struct Qdisc *sch; if (!try_module_get(ops->owner)) - goto errout; + goto err1; sch = qdisc_alloc(dev_queue, ops); if (IS_ERR(sch)) - goto errout; + goto err2; sch->parent = parentid; if (!ops->init || ops->init(sch, NULL) == 0) return sch; - qdisc_destroy(sch); -errout: + dev_put(sch->dev_queue->dev); + kfree((char *)sch - sch->padded); +err2: + module_put(ops->owner); +err1: return NULL; } EXPORT_SYMBOL(qdisc_create_dflt); ^ permalink raw reply related [flat|nested] 19+ messages in thread
* Re: [Patch net 2/2] net_sched: always call ->destroy when ->init() fails 2014-10-25 0:44 ` Patrick McHardy @ 2014-10-25 0:53 ` Cong Wang 2014-10-25 1:04 ` Patrick McHardy 0 siblings, 1 reply; 19+ messages in thread From: Cong Wang @ 2014-10-25 0:53 UTC (permalink / raw) To: Patrick McHardy Cc: Eric Dumazet, Cong Wang, netdev, David Miller, Wang Bo, John Fastabend, Eric Dumazet, Terry Lam On Fri, Oct 24, 2014 at 5:44 PM, Patrick McHardy <kaber@trash.net> wrote: > On Fri, Oct 24, 2014 at 05:38:48PM -0700, Cong Wang wrote: >> On Fri, Oct 24, 2014 at 5:36 PM, Patrick McHardy <kaber@trash.net> wrote: >> > >> > Again, the correct fix is to make qdisc_create_dflt() not call >> > qdisc_destroy() but clean up the qdisc manually as done in >> > qdisc_create(). >> >> I kindly wish you a good luck with fixing all callers of qdisc_create_dflt(). >> Go ahead. :) > > Here you go: > ... Did you check all ->init() call ->destroy() on failure? Look at the sch_pie I have fixed in 1/2. Also check those xxx_init() calling xxx_change(). Really, we don't have to make all ->init() doing cleanup by itself. ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [Patch net 2/2] net_sched: always call ->destroy when ->init() fails 2014-10-25 0:53 ` Cong Wang @ 2014-10-25 1:04 ` Patrick McHardy 2014-10-28 19:58 ` David Miller 0 siblings, 1 reply; 19+ messages in thread From: Patrick McHardy @ 2014-10-25 1:04 UTC (permalink / raw) To: Cong Wang Cc: Eric Dumazet, Cong Wang, netdev, David Miller, Wang Bo, John Fastabend, Eric Dumazet, Terry Lam On Fri, Oct 24, 2014 at 05:53:33PM -0700, Cong Wang wrote: > On Fri, Oct 24, 2014 at 5:44 PM, Patrick McHardy <kaber@trash.net> wrote: > > On Fri, Oct 24, 2014 at 05:38:48PM -0700, Cong Wang wrote: > >> On Fri, Oct 24, 2014 at 5:36 PM, Patrick McHardy <kaber@trash.net> wrote: > >> > > >> > Again, the correct fix is to make qdisc_create_dflt() not call > >> > qdisc_destroy() but clean up the qdisc manually as done in > >> > qdisc_create(). > >> > >> I kindly wish you a good luck with fixing all callers of qdisc_create_dflt(). > >> Go ahead. :) > > > > Here you go: > > > > ... > > Did you check all ->init() call ->destroy() on failure? Look at the > sch_pie I have fixed in 1/2. Why should they? They need to clean up internally, how they do it is entirely up to them. > Also check those xxx_init() calling xxx_change(). Please point to conrete bugs if you have any doubts. Real ones, not things like qdisc_watchdog_init(). This is how the API to which the qdiscs have been written has always worked. And yes, I did check the qdisc error paths many times in the past. > Really, we don't have to make all ->init() doing cleanup by itself. Are you seriously suggesting that it would be better to have ->destroy() check what parts were actually initialized and what needs to be cleaned up instead of assuming a consistent state and have the only function that actually knows the current state on error (->init()) do its own cleanup? That's not even worth arguing about, its utterly and completely wrong. ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [Patch net 2/2] net_sched: always call ->destroy when ->init() fails 2014-10-25 1:04 ` Patrick McHardy @ 2014-10-28 19:58 ` David Miller 0 siblings, 0 replies; 19+ messages in thread From: David Miller @ 2014-10-28 19:58 UTC (permalink / raw) To: kaber Cc: cwang, eric.dumazet, xiyou.wangcong, netdev, wang.bo116, john.r.fastabend, edumazet, vtlam From: Patrick McHardy <kaber@trash.net> Date: Sat, 25 Oct 2014 02:04:00 +0100 >> Really, we don't have to make all ->init() doing cleanup by itself. > > Are you seriously suggesting that it would be better to have ->destroy() > check what parts were actually initialized and what needs to be cleaned > up instead of assuming a consistent state and have the only function that > actually knows the current state on error (->init()) do its own cleanup? > > That's not even worth arguing about, its utterly and completely wrong. I agree with Patrick here. ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [Patch net 2/2] net_sched: always call ->destroy when ->init() fails 2014-10-24 23:55 ` [Patch net 2/2] net_sched: always call ->destroy when ->init() fails Cong Wang 2014-10-25 0:14 ` Eric Dumazet @ 2014-10-29 18:29 ` David Miller 2014-10-30 3:09 ` John Fastabend 1 sibling, 1 reply; 19+ messages in thread From: David Miller @ 2014-10-29 18:29 UTC (permalink / raw) To: xiyou.wangcong Cc: netdev, wang.bo116, john.r.fastabend, edumazet, kaber, vtlam From: Cong Wang <xiyou.wangcong@gmail.com> Date: Fri, 24 Oct 2014 16:55:59 -0700 > In qdisc_create(), when ->init() exists and it fails, we should > call ->destroy() to clean up the potentially partially initialized > qdisc's. This will also make the ->init() implementation easier. > > qdisc_create_dflt() already does that. Most callers of qdisc_create_dflt() > simply use noop_qdisc when it fails. > > And, most of the ->destroy() implementations are already able to clean up > partially initialization, we don't need to worry. > > The following ->init()'s need to catch up: > fq_codel_init(), hhf_init(), multiq_init(), sfq_init(), mq_init(), > mqprio_init(). > > Reported-by: Wang Bo <wang.bo116@zte.com.cn> > Signed-off-by: Cong Wang <xiyou.wangcong@gmail.com> As discussed, I really want to see ->init() clean up it's own crap. There are certain kinds of initializations that cannot be tested for, such as setting up a workqueue etc. Therefore, the mere idea that we can call ->destroy() to handle this is a pure non-starter as far as I am concerned. Thanks. ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [Patch net 2/2] net_sched: always call ->destroy when ->init() fails 2014-10-29 18:29 ` David Miller @ 2014-10-30 3:09 ` John Fastabend 0 siblings, 0 replies; 19+ messages in thread From: John Fastabend @ 2014-10-30 3:09 UTC (permalink / raw) To: xiyou.wangcong, wang.bo116 Cc: David Miller, netdev, john.r.fastabend, edumazet, kaber, vtlam On 10/29/2014 11:29 AM, David Miller wrote: > From: Cong Wang <xiyou.wangcong@gmail.com> > Date: Fri, 24 Oct 2014 16:55:59 -0700 > >> In qdisc_create(), when ->init() exists and it fails, we should >> call ->destroy() to clean up the potentially partially initialized >> qdisc's. This will also make the ->init() implementation easier. >> >> qdisc_create_dflt() already does that. Most callers of qdisc_create_dflt() >> simply use noop_qdisc when it fails. >> >> And, most of the ->destroy() implementations are already able to clean up >> partially initialization, we don't need to worry. >> >> The following ->init()'s need to catch up: >> fq_codel_init(), hhf_init(), multiq_init(), sfq_init(), mq_init(), >> mqprio_init(). >> >> Reported-by: Wang Bo <wang.bo116@zte.com.cn> >> Signed-off-by: Cong Wang <xiyou.wangcong@gmail.com> > > As discussed, I really want to see ->init() clean up it's own crap. > > There are certain kinds of initializations that cannot be tested for, > such as setting up a workqueue etc. > > Therefore, the mere idea that we can call ->destroy() to handle this > is a pure non-starter as far as I am concerned. > > Thanks. > -- Cong/Wang, anyone working on this? Otherwise I'll take a stab at it tomorrow. Thanks! John -- John Fastabend Intel Corporation ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [Patch net 1/2] sch_pie: schedule the timer after all init succeed 2014-10-24 23:55 [Patch net 1/2] sch_pie: schedule the timer after all init succeed Cong Wang 2014-10-24 23:55 ` [Patch net 2/2] net_sched: always call ->destroy when ->init() fails Cong Wang @ 2014-10-25 0:17 ` Eric Dumazet 2014-10-25 0:28 ` Cong Wang 2014-10-29 18:28 ` David Miller 2 siblings, 1 reply; 19+ messages in thread From: Eric Dumazet @ 2014-10-25 0:17 UTC (permalink / raw) To: Cong Wang; +Cc: netdev, davem, Vijay Subramanian On Fri, 2014-10-24 at 16:55 -0700, Cong Wang wrote: > Cc: Vijay Subramanian <vijaynsu@cisco.com> > Cc: David S. Miller <davem@davemloft.net> > Signed-off-by: Cong Wang <xiyou.wangcong@gmail.com> > --- > net/sched/sch_pie.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) Acked-by: Eric Dumazet <edumazet@google.com> Not sure why you use "Cc: David S. Miller <davem@davemloft.net>", given that all networking patches have "Signed-off-by: David S. Miller <davem@davemloft.net>" ??? ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [Patch net 1/2] sch_pie: schedule the timer after all init succeed 2014-10-25 0:17 ` [Patch net 1/2] sch_pie: schedule the timer after all init succeed Eric Dumazet @ 2014-10-25 0:28 ` Cong Wang 2014-10-25 0:39 ` Eric Dumazet 0 siblings, 1 reply; 19+ messages in thread From: Cong Wang @ 2014-10-25 0:28 UTC (permalink / raw) To: Eric Dumazet; +Cc: Cong Wang, netdev, David Miller, Vijay Subramanian On Fri, Oct 24, 2014 at 5:17 PM, Eric Dumazet <eric.dumazet@gmail.com> wrote: > > Not sure why you use "Cc: David S. Miller <davem@davemloft.net>", > given that all networking patches have "Signed-off-by: David S. Miller > <davem@davemloft.net>" ??? > I don't understand what you are talking about: If you are suggesting DaveM is too obvious to Cc, well, Cc'ing him doesn't harm anything. I know he is on netdev list, but just in case vger.kernel.org is broken or etc... If you are suggesting to Cc his another email address, please update MAINTAINERS. ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [Patch net 1/2] sch_pie: schedule the timer after all init succeed 2014-10-25 0:28 ` Cong Wang @ 2014-10-25 0:39 ` Eric Dumazet 2014-10-25 0:42 ` Cong Wang 0 siblings, 1 reply; 19+ messages in thread From: Eric Dumazet @ 2014-10-25 0:39 UTC (permalink / raw) To: Cong Wang; +Cc: Cong Wang, netdev, David Miller, Vijay Subramanian On Fri, 2014-10-24 at 17:28 -0700, Cong Wang wrote: > On Fri, Oct 24, 2014 at 5:17 PM, Eric Dumazet <eric.dumazet@gmail.com> wrote: > > > > Not sure why you use "Cc: David S. Miller <davem@davemloft.net>", > > given that all networking patches have "Signed-off-by: David S. Miller > > <davem@davemloft.net>" ??? > > > > I don't understand what you are talking about: > > If you are suggesting DaveM is too obvious to Cc, well, Cc'ing him > doesn't harm anything. I know he is on netdev list, but just in case > vger.kernel.org is broken or etc... > > If you are suggesting to Cc his another email address, please update > MAINTAINERS. You send the patch to David Miller, as a recipient, so that he can read it, and apply it. You don't need to add one line in the changelog. Why would it be needed ? Think about it. ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [Patch net 1/2] sch_pie: schedule the timer after all init succeed 2014-10-25 0:39 ` Eric Dumazet @ 2014-10-25 0:42 ` Cong Wang 0 siblings, 0 replies; 19+ messages in thread From: Cong Wang @ 2014-10-25 0:42 UTC (permalink / raw) To: Eric Dumazet; +Cc: Cong Wang, netdev, David Miller, Vijay Subramanian On Fri, Oct 24, 2014 at 5:39 PM, Eric Dumazet <eric.dumazet@gmail.com> wrote: > > You don't need to add one line in the changelog. > > Why would it be needed ? Think about it. Simply because git-send-email reads Cc: so that I don't have to specify in cmdline. That's all. OMG, first time to hear people complaining on Cc'ing too much! lol :) ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [Patch net 1/2] sch_pie: schedule the timer after all init succeed 2014-10-24 23:55 [Patch net 1/2] sch_pie: schedule the timer after all init succeed Cong Wang 2014-10-24 23:55 ` [Patch net 2/2] net_sched: always call ->destroy when ->init() fails Cong Wang 2014-10-25 0:17 ` [Patch net 1/2] sch_pie: schedule the timer after all init succeed Eric Dumazet @ 2014-10-29 18:28 ` David Miller 2 siblings, 0 replies; 19+ messages in thread From: David Miller @ 2014-10-29 18:28 UTC (permalink / raw) To: xiyou.wangcong; +Cc: netdev, vijaynsu From: Cong Wang <xiyou.wangcong@gmail.com> Date: Fri, 24 Oct 2014 16:55:58 -0700 > Cc: Vijay Subramanian <vijaynsu@cisco.com> > Cc: David S. Miller <davem@davemloft.net> > Signed-off-by: Cong Wang <xiyou.wangcong@gmail.com> Applied, thanks. ^ permalink raw reply [flat|nested] 19+ messages in thread
end of thread, other threads:[~2014-10-30 3:09 UTC | newest] Thread overview: 19+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2014-10-24 23:55 [Patch net 1/2] sch_pie: schedule the timer after all init succeed Cong Wang 2014-10-24 23:55 ` [Patch net 2/2] net_sched: always call ->destroy when ->init() fails Cong Wang 2014-10-25 0:14 ` Eric Dumazet 2014-10-25 0:36 ` Cong Wang 2014-10-25 0:40 ` Eric Dumazet 2014-10-25 0:42 ` Eric Dumazet 2014-10-25 0:36 ` Patrick McHardy 2014-10-25 0:38 ` Cong Wang 2014-10-25 0:44 ` Patrick McHardy 2014-10-25 0:53 ` Cong Wang 2014-10-25 1:04 ` Patrick McHardy 2014-10-28 19:58 ` David Miller 2014-10-29 18:29 ` David Miller 2014-10-30 3:09 ` John Fastabend 2014-10-25 0:17 ` [Patch net 1/2] sch_pie: schedule the timer after all init succeed Eric Dumazet 2014-10-25 0:28 ` Cong Wang 2014-10-25 0:39 ` Eric Dumazet 2014-10-25 0:42 ` Cong Wang 2014-10-29 18:28 ` David Miller
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).