[PATCH net-next 0/7] clear old options when old qdisc's replaced

[PATCH net-next 0/7] clear old options when old qdisc's replaced

[Yang Yingliang]

I've added a netem qdisc with rate option, then I replace this qdisc
without rate option but with latency option. The rate option is still
there.

E.g.
  # tc qdisc add dev eth4 handle 1: root netem rate 10mbit
  # tc qdisc show
    qdisc netem 1: dev eth4 root refcnt 2 limit 1000 rate 10Mbit

  # tc qdisc replace dev eth4 handle 1: root netem latency 10ms
  # tc qdisc show
    qdisc netem 1: dev eth4 root refcnt 2 limit 1000 delay 10.0ms rate 10Mbit

The old options need be cleared after the qdisc is replaced.

Yang Yingliang (7):
  sch_netem: clear old clgstate when old qdisc's replaced
  sch_netem: clear old dist table when old qdisc's replaced
  sch_netem: clear old reorder when old qdisc's replaced
  sch_netem: clear old correlation when old qdisc's replaced
  sch_netem: clear old corrupt when old qdisc's replaced
  sch_netem: clear old rate when old qdisc's replaced
  sch_netem: clear old ecn when old qdisc's replaced

 net/sched/sch_netem.c | 44 ++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 44 insertions(+)

-- 
1.8.0

[PATCH net-next 1/7] sch_netem: clear old clgstate when old qdisc's replaced

[Yang Yingliang]

If we set a netem qdisc with clgstate options, while we
use "#tc qdisc replace ..." that without clgstate options
to replace the old qdisc, the old clgstate's value is still
there. We need clear these values after qdisc's replaced.

Signed-off-by: Yang Yingliang <yangyingliang@huawei.com>
---
 net/sched/sch_netem.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/net/sched/sch_netem.c b/net/sched/sch_netem.c
index 4fced67..6f2cc04 100644
--- a/net/sched/sch_netem.c
+++ b/net/sched/sch_netem.c
@@ -845,6 +845,7 @@ static int netem_change(struct Qdisc *sch, struct nlattr *opt)
 		}
 	} else {
 		q->loss_model = CLG_RANDOM;
+		memset(&q->clg, 0, sizeof(q->clg));
 	}
 
 	if (tb[TCA_NETEM_DELAY_DIST]) {
-- 
1.8.0

[PATCH net-next 2/7] sch_netem: clear old dist table when old qdisc's replaced

[Yang Yingliang]

If we set a netem qdisc with dist table option, while we
use "#tc qdisc replace ..." that without dist table option
to replace the old qdisc, the old dist table is still there.
We need clear dist table after qdisc's replaced.

Signed-off-by: Yang Yingliang <yangyingliang@huawei.com>
---
 net/sched/sch_netem.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/net/sched/sch_netem.c b/net/sched/sch_netem.c
index 6f2cc04..fd2206d 100644
--- a/net/sched/sch_netem.c
+++ b/net/sched/sch_netem.c
@@ -859,6 +859,9 @@ static int netem_change(struct Qdisc *sch, struct nlattr *opt)
 			q->loss_model = old_loss_model;
 			return ret;
 		}
+	} else {
+		dist_free(q->delay_dist);
+		q->delay_dist = NULL;
 	}
 
 	sch->limit = qopt->limit;
-- 
1.8.0

[PATCH net-next 3/7] sch_netem: clear old reorder when old qdisc's replaced

[Yang Yingliang]

If we set a netem qdisc with reorder option, while we
use "#tc qdisc replace ..." that without reorder option
to replace the old qdisc, the old reorder is still there.
We need clear old reorder after qdisc's replaced.

Signed-off-by: Yang Yingliang <yangyingliang@huawei.com>
---
 net/sched/sch_netem.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/net/sched/sch_netem.c b/net/sched/sch_netem.c
index fd2206d..bb06dd8 100644
--- a/net/sched/sch_netem.c
+++ b/net/sched/sch_netem.c
@@ -711,6 +711,12 @@ static void get_reorder(struct netem_sched_data *q, const struct nlattr *attr)
 	init_crandom(&q->reorder_cor, r->correlation);
 }
 
+static void reorder_reset(struct netem_sched_data *q)
+{
+	q->reorder = 0;
+	memset(&q->reorder_cor, 0, sizeof(struct crndstate));
+}
+
 static void get_corrupt(struct netem_sched_data *q, const struct nlattr *attr)
 {
 	const struct tc_netem_corrupt *r = nla_data(attr);
@@ -879,12 +885,16 @@ static int netem_change(struct Qdisc *sch, struct nlattr *opt)
 	 */
 	if (q->gap)
 		q->reorder = ~0;
+	else
+		q->reorder = 0;
 
 	if (tb[TCA_NETEM_CORR])
 		get_correlation(q, tb[TCA_NETEM_CORR]);
 
 	if (tb[TCA_NETEM_REORDER])
 		get_reorder(q, tb[TCA_NETEM_REORDER]);
+	else
+		reorder_reset(q);
 
 	if (tb[TCA_NETEM_CORRUPT])
 		get_corrupt(q, tb[TCA_NETEM_CORRUPT]);
-- 
1.8.0

[PATCH net-next 4/7] sch_netem: clear old correlation when old qdisc's replaced

[Yang Yingliang]

If we set a netem qdisc with correlation option, while we
use "#tc qdisc replace ..." that without correlation option
to replace the old qdisc, the old correlation is still there.
We need clear old correlation after qdisc's replaced.

Signed-off-by: Yang Yingliang <yangyingliang@huawei.com>
---
 net/sched/sch_netem.c | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/net/sched/sch_netem.c b/net/sched/sch_netem.c
index bb06dd8..33e7cef 100644
--- a/net/sched/sch_netem.c
+++ b/net/sched/sch_netem.c
@@ -703,6 +703,13 @@ static void get_correlation(struct netem_sched_data *q, const struct nlattr *att
 	init_crandom(&q->dup_cor, c->dup_corr);
 }
 
+static void correlation_reset(struct netem_sched_data *q)
+{
+	memset(&q->delay_cor, 0, sizeof(struct crndstate));
+	memset(&q->loss_cor, 0, sizeof(struct crndstate));
+	memset(&q->dup_cor, 0, sizeof(struct crndstate));
+}
+
 static void get_reorder(struct netem_sched_data *q, const struct nlattr *attr)
 {
 	const struct tc_netem_reorder *r = nla_data(attr);
@@ -890,6 +897,8 @@ static int netem_change(struct Qdisc *sch, struct nlattr *opt)
 
 	if (tb[TCA_NETEM_CORR])
 		get_correlation(q, tb[TCA_NETEM_CORR]);
+	else
+		correlation_reset(q);
 
 	if (tb[TCA_NETEM_REORDER])
 		get_reorder(q, tb[TCA_NETEM_REORDER]);
-- 
1.8.0

[PATCH net-next 5/7] sch_netem: clear old corrupt when old qdisc's replaced

[Yang Yingliang]

If we set a netem qdisc with corrupt option, while we
use "#tc qdisc replace ..." that without corrupt option
to replace the old qdisc, the old corrupt is still there.
We need clear old corrupt after qdisc's replaced.

Signed-off-by: Yang Yingliang <yangyingliang@huawei.com>
---
 net/sched/sch_netem.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/net/sched/sch_netem.c b/net/sched/sch_netem.c
index 33e7cef..2f630e9 100644
--- a/net/sched/sch_netem.c
+++ b/net/sched/sch_netem.c
@@ -732,6 +732,12 @@ static void get_corrupt(struct netem_sched_data *q, const struct nlattr *attr)
 	init_crandom(&q->corrupt_cor, r->correlation);
 }
 
+static void corrupt_reset(struct netem_sched_data *q)
+{
+	q->corrupt = 0;
+	memset(&q->corrupt_cor, 0, sizeof(struct crndstate));
+}
+
 static void get_rate(struct netem_sched_data *q, const struct nlattr *attr)
 {
 	const struct tc_netem_rate *r = nla_data(attr);
@@ -907,6 +913,8 @@ static int netem_change(struct Qdisc *sch, struct nlattr *opt)
 
 	if (tb[TCA_NETEM_CORRUPT])
 		get_corrupt(q, tb[TCA_NETEM_CORRUPT]);
+	else
+		corrupt_reset(q);
 
 	if (tb[TCA_NETEM_RATE])
 		get_rate(q, tb[TCA_NETEM_RATE]);
-- 
1.8.0

[PATCH net-next 6/7] sch_netem: clear old rate when old qdisc's replaced

[Yang Yingliang]

If we set a netem qdisc with rate option, while we
use "#tc qdisc replace ..." that without rate option
to replace the old qdisc, the old rate is still there.
We need clear old rate after qdisc's replaced.

Signed-off-by: Yang Yingliang <yangyingliang@huawei.com>
---
 net/sched/sch_netem.c | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/net/sched/sch_netem.c b/net/sched/sch_netem.c
index 2f630e9..92f6ba6 100644
--- a/net/sched/sch_netem.c
+++ b/net/sched/sch_netem.c
@@ -752,6 +752,15 @@ static void get_rate(struct netem_sched_data *q, const struct nlattr *attr)
 		q->cell_size_reciprocal = (struct reciprocal_value) { 0 };
 }
 
+static void rate_reset(struct netem_sched_data *q)
+{
+	q->rate = 0;
+	q->packet_overhead = 0;
+	q->cell_size = 0;
+	q->cell_size_reciprocal = (struct reciprocal_value) { 0 };
+	q->cell_overhead = 0;
+}
+
 static int get_loss_clg(struct netem_sched_data *q, const struct nlattr *attr)
 {
 	const struct nlattr *la;
@@ -918,6 +927,8 @@ static int netem_change(struct Qdisc *sch, struct nlattr *opt)
 
 	if (tb[TCA_NETEM_RATE])
 		get_rate(q, tb[TCA_NETEM_RATE]);
+	else
+		rate_reset(q);
 
 	if (tb[TCA_NETEM_RATE64])
 		q->rate = max_t(u64, q->rate,
-- 
1.8.0

[PATCH net-next 7/7] sch_netem: clear old ecn when old qdisc's replaced

[Yang Yingliang]

If we set a netem qdisc with ecn option, while we
use "#tc qdisc replace ..." that without ecn option
to replace the old qdisc, the old ecn is still there.
We need clear old ecn after qdisc's replaced.

Signed-off-by: Yang Yingliang <yangyingliang@huawei.com>
---
 net/sched/sch_netem.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/net/sched/sch_netem.c b/net/sched/sch_netem.c
index 92f6ba6..52ae2ef 100644
--- a/net/sched/sch_netem.c
+++ b/net/sched/sch_netem.c
@@ -936,6 +936,8 @@ static int netem_change(struct Qdisc *sch, struct nlattr *opt)
 
 	if (tb[TCA_NETEM_ECN])
 		q->ecn = nla_get_u32(tb[TCA_NETEM_ECN]);
+	else
+		q->ecn = 0;
 
 	return ret;
 }
-- 
1.8.0

Re: [PATCH net-next 6/7] sch_netem: clear old rate when old qdisc's replaced

[Eric Dumazet]

On Fri, 2014-02-14 at 16:36 +0800, Yang Yingliang wrote:
> If we set a netem qdisc with rate option, while we
> use "#tc qdisc replace ..." that without rate option
> to replace the old qdisc, the old rate is still there.
> We need clear old rate after qdisc's replaced.

Wait... Have you tested :

tc qdisc change ...

This is far more needed than 'replace' : 

You (meaning user scripts) can implement replace by delete + create, but
'tc qdisc change' needs current code.

Re: [PATCH net-next 0/7] clear old options when old qdisc's replaced

[Eric Dumazet]

On Fri, 2014-02-14 at 16:36 +0800, Yang Yingliang wrote:
> I've added a netem qdisc with rate option, then I replace this qdisc
> without rate option but with latency option. The rate option is still
> there.
> 
> E.g.
>   # tc qdisc add dev eth4 handle 1: root netem rate 10mbit
>   # tc qdisc show
>     qdisc netem 1: dev eth4 root refcnt 2 limit 1000 rate 10Mbit
> 
>   # tc qdisc replace dev eth4 handle 1: root netem latency 10ms
>   # tc qdisc show
>     qdisc netem 1: dev eth4 root refcnt 2 limit 1000 delay 10.0ms rate 10Mbit
> 
> The old options need be cleared after the qdisc is replaced.

Not at all. Test your changes with "tc qdisc change ... "

Re: [PATCH net-next 0/7] clear old options when old qdisc's replaced

[Yang Yingliang]

On 2014/2/14 20:44, Eric Dumazet wrote:
> On Fri, 2014-02-14 at 16:36 +0800, Yang Yingliang wrote:
>> I've added a netem qdisc with rate option, then I replace this qdisc
>> without rate option but with latency option. The rate option is still
>> there.
>>
>> E.g.
>>   # tc qdisc add dev eth4 handle 1: root netem rate 10mbit
>>   # tc qdisc show
>>     qdisc netem 1: dev eth4 root refcnt 2 limit 1000 rate 10Mbit
>>
>>   # tc qdisc replace dev eth4 handle 1: root netem latency 10ms
>>   # tc qdisc show
>>     qdisc netem 1: dev eth4 root refcnt 2 limit 1000 delay 10.0ms rate 10Mbit
>>
>> The old options need be cleared after the qdisc is replaced.
> 
> Not at all. Test your changes with "tc qdisc change ... "
> 
> 
With "tc qdisc change ... ", it will only change the options that specified in command.
It's proper, I think.

   # tc qdisc add dev eth4 handle 1: root netem rate 10mbit
   # tc qdisc show
     qdisc netem 1: dev eth4 root refcnt 2 limit 1000 rate 10Mbit

   # tc qdisc replace dev eth4 handle 1: root netem latency 10ms
   # tc qdisc show
     qdisc netem 1: dev eth4 root refcnt 2 limit 1000 delay 10.0ms rate 10Mbit

"rate" is not cleared.

But with other qdiscs, such as tbf and htb etc, their usage not complete same as netem's,
when we use "tc qdisc change ... ", the old options will be cleared. Maybe we should make
their useages of "change" same as netem's?

E.g.
  # tc qdisc replace dev eth4 handle 1: root tbf rate 10mbit burst 10kb latency 50ms mtu 64kb peakrate 20mbit
  # tc qdisc show
    qdisc tbf 1: dev eth4 root refcnt 2 rate 10Mbit burst 10Kb peakrate 20Mbit minburst 64Kb lat 50.0ms
  # tc qdisc change dev eth4 handle 1: root tbf rate 20mbit burst 10kb latency 50ms
  # tc qdisc show
    qdisc tbf 1: dev eth4 root refcnt 2 rate 20Mbit burst 10Kb lat 50.0ms

"peakrate" and "minburst" are cleared.

  # tc class add dev eth4 parent 1:0 classid 1:1 htb rate 10mbit ceil 10mbit burst 10kb cburst 10kb
  # tc class show dev eth4
    class htb 1:1 root prio 0 rate 10Mbit ceil 10Mbit burst 10Kb cburst 10Kb
  # tc class change dev eth4 parent 1:0 classid 1:1 htb rate 20mbit
  # tc class show dev eth4
    class htb 1:1 root prio 0 rate 20Mbit ceil 20Mbit burst 1600b cburst 1600b

"cburst" and "burst" are cleared.


Regards,
Yang

Re: [PATCH net-next 0/7] clear old options when old qdisc's replaced

[Eric Dumazet]

On Sat, 2014-02-15 at 10:20 +0800, Yang Yingliang wrote:
> On 2014/2/14 20:44, Eric Dumazet wrote:
> > On Fri, 2014-02-14 at 16:36 +0800, Yang Yingliang wrote:
> >> I've added a netem qdisc with rate option, then I replace this qdisc
> >> without rate option but with latency option. The rate option is still
> >> there.
> >>
> >> E.g.
> >>   # tc qdisc add dev eth4 handle 1: root netem rate 10mbit
> >>   # tc qdisc show
> >>     qdisc netem 1: dev eth4 root refcnt 2 limit 1000 rate 10Mbit
> >>
> >>   # tc qdisc replace dev eth4 handle 1: root netem latency 10ms
> >>   # tc qdisc show
> >>     qdisc netem 1: dev eth4 root refcnt 2 limit 1000 delay 10.0ms rate 10Mbit
> >>
> >> The old options need be cleared after the qdisc is replaced.
> > 
> > Not at all. Test your changes with "tc qdisc change ... "
> > 
> > 
> With "tc qdisc change ... ", it will only change the options that specified in command.
> It's proper, I think.
> 
>    # tc qdisc add dev eth4 handle 1: root netem rate 10mbit
>    # tc qdisc show
>      qdisc netem 1: dev eth4 root refcnt 2 limit 1000 rate 10Mbit
> 
>    # tc qdisc replace dev eth4 handle 1: root netem latency 10ms
>    # tc qdisc show
>      qdisc netem 1: dev eth4 root refcnt 2 limit 1000 delay 10.0ms rate 10Mbit
> 
> "rate" is not cleared.
> 
> But with other qdiscs, such as tbf and htb etc, their usage not complete same as netem's,
> when we use "tc qdisc change ... ", the old options will be cleared. Maybe we should make
> their useages of "change" same as netem's?

qdisc change should only change the parameters you specified in the
command line.

For example, if I want to change my fq qdisc pacing I use :

tc qdisc change dev eth0 root fq pacing

or

tc qdisc change dev eth0 root fq nopacing

In your patches, you for example clear ecn if not specified, its wrong.

        if (tb[TCA_NETEM_ECN])
                q->ecn = nla_get_u32(tb[TCA_NETEM_ECN]);
+       else
+               q->ecn = 0;