[Patch net 1/2] sch_pie: schedule the timer after all init succeed

[Patch net 1/2] sch_pie: schedule the timer after all init succeed

[Cong Wang]

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

[Patch net 2/2] net_sched: always call ->destroy when ->init() fails

[Cong Wang]

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

Re: [Patch net 2/2] net_sched: always call ->destroy when ->init() fails

[Eric Dumazet]

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.

Re: [Patch net 1/2] sch_pie: schedule the timer after all init succeed

[Eric Dumazet]

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>" ???

Re: [Patch net 1/2] sch_pie: schedule the timer after all init succeed

[Cong Wang]

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.

Re: [Patch net 2/2] net_sched: always call ->destroy when ->init() fails

[Cong Wang]

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.

Re: [Patch net 2/2] net_sched: always call ->destroy when ->init() fails

[Patrick McHardy]

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().

Re: [Patch net 2/2] net_sched: always call ->destroy when ->init() fails

[Cong Wang]

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. :)

Re: [Patch net 1/2] sch_pie: schedule the timer after all init succeed

[Eric Dumazet]

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.

Re: [Patch net 2/2] net_sched: always call ->destroy when ->init() fails

[Eric Dumazet]

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 ?

Re: [Patch net 2/2] net_sched: always call ->destroy when ->init() fails

[Eric Dumazet]

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 ;)

Re: [Patch net 1/2] sch_pie: schedule the timer after all init succeed

[Cong Wang]

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 :)

Re: [Patch net 2/2] net_sched: always call ->destroy when ->init() fails

[Patrick McHardy]

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);

Re: [Patch net 2/2] net_sched: always call ->destroy when ->init() fails

[Cong Wang]

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.

Re: [Patch net 2/2] net_sched: always call ->destroy when ->init() fails

[Patrick McHardy]

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.

Re: [Patch net 2/2] net_sched: always call ->destroy when ->init() fails

[David Miller]

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.

Re: [Patch net 1/2] sch_pie: schedule the timer after all init succeed

[David Miller]

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.

Re: [Patch net 2/2] net_sched: always call ->destroy when ->init() fails

[David Miller]

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.

Re: [Patch net 2/2] net_sched: always call ->destroy when ->init() fails

[John Fastabend]

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