[PATCH net-next 0/5] net_sched: Adjust qdisc_change() for command "#tc qdisc change/replace ..."

[PATCH net-next 0/5] net_sched: Adjust qdisc_change() for command "#tc qdisc change/replace ..."

[Yang Yingliang]

Current commands "#tc qdisc replace..." and "#tc qdisc change..."
are not doing what they're supposed to do.

E.g.

With "#tc qdisc replace ...", it won't clear old option if not specified in
qdisc of netem.

   # 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 option "rate" is still there.

With "#tc qdisc change ... ", it will clear old options if not specified in
qdisc of tbf.

  # tc qdisc add 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
The old peakrate and minburst are cleared.

This patchset adds a flag parameter in qdisc_change and a replace func in struct Qdisc_ops.
If the flag has NLM_F_REPLACE, we call the replace function, or call the change function in
qdisc_change(). And, add tbf_replace() and netem_replace() needed by tbf and netem.

Yang Yingliang (5):
  net_sched: add flag parameter in qdisc_change
  net_sched: add replace func in struct Qdisc_ops
  sch_tbf: change name "tbf_change" to "tbf_replace"
  sch_tbf: add tbf_change for #tc qdisc change ...
  sch_netem: add netem_replace for #tc qdisc replace ...

 include/net/sch_generic.h |   1 +
 net/sched/sch_api.c       |  16 ++++--
 net/sched/sch_netem.c     | 127 ++++++++++++++++++++++++++++++++++++++++++++++
 net/sched/sch_tbf.c       | 120 ++++++++++++++++++++++++++++++++++++++++++-
 4 files changed, 258 insertions(+), 6 deletions(-)

-- 
1.8.0

[PATCH net-next 1/5] net_sched: add flag parameter in qdisc_change

[Yang Yingliang]

Add a new parameter for checking if it's "change" or "replace".

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

diff --git a/net/sched/sch_api.c b/net/sched/sch_api.c
index 1313145e3b86..961cb0f1bd0c 100644
--- a/net/sched/sch_api.c
+++ b/net/sched/sch_api.c
@@ -993,7 +993,7 @@ err_out4:
 	goto err_out3;
 }
 
-static int qdisc_change(struct Qdisc *sch, struct nlattr **tca)
+static int qdisc_change(struct Qdisc *sch, struct nlattr **tca, u16 flags)
 {
 	struct qdisc_size_table *ostab, *stab = NULL;
 	int err = 0;
@@ -1248,7 +1248,7 @@ replay:
 		return -EEXIST;
 	if (tca[TCA_KIND] && nla_strcmp(tca[TCA_KIND], q->ops->id))
 		return -EINVAL;
-	err = qdisc_change(q, tca);
+	err = qdisc_change(q, tca, n->nlmsg_flags);
 	if (err == 0)
 		qdisc_notify(net, skb, n, clid, NULL, q);
 	return err;
-- 
1.8.0

[PATCH net-next 2/5] net_sched: add replace func in struct Qdisc_ops

[Yang Yingliang]

Add replace func, if NLM_F_REPLACE is set, use replace func.
Otherwise, use change func.

Signed-off-by: Yang Yingliang <yangyingliang@huawei.com>
---
 include/net/sch_generic.h |  1 +
 net/sched/sch_api.c       | 12 +++++++++---
 2 files changed, 10 insertions(+), 3 deletions(-)

diff --git a/include/net/sch_generic.h b/include/net/sch_generic.h
index d062f81c692f..ef8a41bcb9a7 100644
--- a/include/net/sch_generic.h
+++ b/include/net/sch_generic.h
@@ -170,6 +170,7 @@ struct Qdisc_ops {
 	void			(*reset)(struct Qdisc *);
 	void			(*destroy)(struct Qdisc *);
 	int			(*change)(struct Qdisc *, struct nlattr *arg);
+	int			(*replace)(struct Qdisc *, struct nlattr *arg);
 	void			(*attach)(struct Qdisc *);
 
 	int			(*dump)(struct Qdisc *, struct sk_buff *);
diff --git a/net/sched/sch_api.c b/net/sched/sch_api.c
index 961cb0f1bd0c..8f0f8e9d5475 100644
--- a/net/sched/sch_api.c
+++ b/net/sched/sch_api.c
@@ -999,9 +999,15 @@ static int qdisc_change(struct Qdisc *sch, struct nlattr **tca, u16 flags)
 	int err = 0;
 
 	if (tca[TCA_OPTIONS]) {
-		if (sch->ops->change == NULL)
-			return -EINVAL;
-		err = sch->ops->change(sch, tca[TCA_OPTIONS]);
+		if (flags & NLM_F_REPLACE) {
+			if (sch->ops->replace == NULL)
+				return -EINVAL;
+			err = sch->ops->replace(sch, tca[TCA_OPTIONS]);
+		} else {
+			if (sch->ops->change == NULL)
+				return -EINVAL;
+			err = sch->ops->change(sch, tca[TCA_OPTIONS]);
+		}
 		if (err)
 			return err;
 	}
-- 
1.8.0

[PATCH net-next 3/5] sch_tbf: change name "tbf_change" to "tbf_replace"

[Yang Yingliang]

tbf_change will clear all options when we change the parameters
that specified in the command line. So it should be "tbf_replace",
then add a new "tbf_change" in next patch for just changing the
parameters.

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

diff --git a/net/sched/sch_tbf.c b/net/sched/sch_tbf.c
index 87a02bfa707b..e9f373effd5f 100644
--- a/net/sched/sch_tbf.c
+++ b/net/sched/sch_tbf.c
@@ -308,7 +308,7 @@ static const struct nla_policy tbf_policy[TCA_TBF_MAX + 1] = {
 	[TCA_TBF_PBURST] = { .type = NLA_U32 },
 };
 
-static int tbf_change(struct Qdisc *sch, struct nlattr *opt)
+static int tbf_replace(struct Qdisc *sch, struct nlattr *opt)
 {
 	int err;
 	struct tbf_sched_data *q = qdisc_priv(sch);
@@ -435,7 +435,7 @@ static int tbf_init(struct Qdisc *sch, struct nlattr *opt)
 	qdisc_watchdog_init(&q->watchdog, sch);
 	q->qdisc = &noop_qdisc;
 
-	return tbf_change(sch, opt);
+	return tbf_replace(sch, opt);
 }
 
 static void tbf_destroy(struct Qdisc *sch)
@@ -560,7 +560,7 @@ static struct Qdisc_ops tbf_qdisc_ops __read_mostly = {
 	.init		=	tbf_init,
 	.reset		=	tbf_reset,
 	.destroy	=	tbf_destroy,
-	.change		=	tbf_change,
+	.replace	=	tbf_replace,
 	.dump		=	tbf_dump,
 	.owner		=	THIS_MODULE,
 };
-- 
1.8.0

[PATCH net-next 4/5] sch_tbf: add tbf_change for #tc qdisc change ...

[Yang Yingliang]

Add tbf_change() function which is called by command #tc qdisc change...
It's different from tbf_replace. In tbf_change(), it will only change
the options that specified in command.

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

diff --git a/net/sched/sch_tbf.c b/net/sched/sch_tbf.c
index e9f373effd5f..ef39d284d06a 100644
--- a/net/sched/sch_tbf.c
+++ b/net/sched/sch_tbf.c
@@ -308,6 +308,123 @@ static const struct nla_policy tbf_policy[TCA_TBF_MAX + 1] = {
 	[TCA_TBF_PBURST] = { .type = NLA_U32 },
 };
 
+static int tbf_change(struct Qdisc *sch, struct nlattr *opt)
+{
+	int err;
+	struct tbf_sched_data *q = qdisc_priv(sch);
+	struct nlattr *tb[TCA_TBF_MAX + 1];
+	struct tc_tbf_qopt *qopt;
+	struct Qdisc *child = NULL;
+	struct psched_ratecfg rate;
+	struct psched_ratecfg peak;
+	u64 max_size;
+	s64 buffer, mtu;
+	u64 rate64 = 0, prate64 = 0;
+
+	err = nla_parse_nested(tb, TCA_TBF_MAX, opt, tbf_policy);
+	if (err < 0)
+		return err;
+
+	err = -EINVAL;
+	if (tb[TCA_TBF_PARMS] == NULL)
+		goto done;
+
+	qopt = nla_data(tb[TCA_TBF_PARMS]);
+	if (qopt->rate.linklayer == TC_LINKLAYER_UNAWARE)
+		qdisc_put_rtab(qdisc_get_rtab(&qopt->rate,
+					      tb[TCA_TBF_RTAB]));
+
+	if (qopt->peakrate.linklayer == TC_LINKLAYER_UNAWARE)
+			qdisc_put_rtab(qdisc_get_rtab(&qopt->peakrate,
+						      tb[TCA_TBF_PTAB]));
+
+	buffer = min_t(u64, PSCHED_TICKS2NS(qopt->buffer), ~0U);
+	mtu = min_t(u64, PSCHED_TICKS2NS(qopt->mtu), ~0U);
+
+	if (tb[TCA_TBF_RATE64])
+		rate64 = nla_get_u64(tb[TCA_TBF_RATE64]);
+	psched_ratecfg_precompute(&rate, &qopt->rate, rate64);
+
+	if (tb[TCA_TBF_BURST]) {
+		max_size = nla_get_u32(tb[TCA_TBF_BURST]);
+		buffer = psched_l2t_ns(&rate, max_size);
+	} else {
+		max_size = min_t(u64, psched_ns_t2l(&rate, buffer), ~0U);
+	}
+
+	if (qopt->peakrate.rate) {
+		if (tb[TCA_TBF_PRATE64])
+			prate64 = nla_get_u64(tb[TCA_TBF_PRATE64]);
+		psched_ratecfg_precompute(&peak, &qopt->peakrate, prate64);
+		if (peak.rate_bytes_ps <= rate.rate_bytes_ps) {
+			pr_warn_ratelimited("sch_tbf: peakrate %llu is lower than or equals to rate %llu !\n",
+					peak.rate_bytes_ps, rate.rate_bytes_ps);
+			err = -EINVAL;
+			goto done;
+		}
+
+		if (tb[TCA_TBF_PBURST]) {
+			u32 pburst = nla_get_u32(tb[TCA_TBF_PBURST]);
+			max_size = min_t(u32, max_size, pburst);
+			mtu = psched_l2t_ns(&peak, pburst);
+		} else {
+			max_size = min_t(u64, max_size, psched_ns_t2l(&peak, mtu));
+		}
+	}
+
+	if (max_size < psched_mtu(qdisc_dev(sch)))
+		pr_warn_ratelimited("sch_tbf: burst %llu is lower than device %s mtu (%u) !\n",
+				    max_size, qdisc_dev(sch)->name,
+				    psched_mtu(qdisc_dev(sch)));
+
+	if (!max_size) {
+		err = -EINVAL;
+		goto done;
+	}
+
+	if (q->qdisc != &noop_qdisc) {
+		err = fifo_set_limit(q->qdisc, qopt->limit);
+		if (err)
+			goto done;
+	} else if (qopt->limit > 0) {
+		child = fifo_create_dflt(sch, &bfifo_qdisc_ops, qopt->limit);
+		if (IS_ERR(child)) {
+			err = PTR_ERR(child);
+			goto done;
+		}
+	}
+
+	sch_tree_lock(sch);
+	if (child) {
+		qdisc_tree_decrease_qlen(q->qdisc, q->qdisc->q.qlen);
+		qdisc_destroy(q->qdisc);
+		q->qdisc = child;
+	}
+	if (qopt->limit)
+		q->limit = qopt->limit;
+	if (tb[TCA_TBF_PBURST])
+		q->mtu = mtu;
+	else if (qopt->mtu)
+		q->mtu = PSCHED_TICKS2NS(qopt->mtu);
+	q->max_size = max_size;
+	if (tb[TCA_TBF_BURST])
+		q->buffer = buffer;
+	else if (qopt->buffer)
+		q->buffer = PSCHED_TICKS2NS(qopt->buffer);
+	q->tokens = q->buffer;
+	q->ptokens = q->mtu;
+
+	if (qopt->rate.rate)
+		memcpy(&q->rate, &rate, sizeof(struct psched_ratecfg));
+	if (qopt->peakrate.rate)
+		memcpy(&q->peak, &peak, sizeof(struct psched_ratecfg));
+
+	sch_tree_unlock(sch);
+	err = 0;
+done:
+	return err;
+}
+
 static int tbf_replace(struct Qdisc *sch, struct nlattr *opt)
 {
 	int err;
@@ -560,6 +677,7 @@ static struct Qdisc_ops tbf_qdisc_ops __read_mostly = {
 	.init		=	tbf_init,
 	.reset		=	tbf_reset,
 	.destroy	=	tbf_destroy,
+	.change		=	tbf_change,
 	.replace	=	tbf_replace,
 	.dump		=	tbf_dump,
 	.owner		=	THIS_MODULE,
-- 
1.8.0

[PATCH net-next 5/5] sch_netem: add netem_replace for #tc qdisc replace ...

[Yang Yingliang]

Command "#tc qdisc replace ..." cannot really replace the old qdisc.
The old options are still there after replacing the old qdisc.

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 rate option is still there.

This patch adds netem_replace() function which will clear old options for
command "#tc qdisc replace ...".

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

diff --git a/net/sched/sch_netem.c b/net/sched/sch_netem.c
index f1669a00f571..af8dc71065e6 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);
@@ -711,6 +718,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);
@@ -719,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);
@@ -733,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;
@@ -898,6 +926,104 @@ static int netem_change(struct Qdisc *sch, struct nlattr *opt)
 	return ret;
 }
 
+static int netem_replace(struct Qdisc *sch, struct nlattr *opt)
+{
+	struct netem_sched_data *q = qdisc_priv(sch);
+	struct nlattr *tb[TCA_NETEM_MAX + 1];
+	struct tc_netem_qopt *qopt;
+	struct clgstate old_clg;
+	int old_loss_model = CLG_RANDOM;
+	int ret;
+
+	if (opt == NULL)
+		return -EINVAL;
+
+	qopt = nla_data(opt);
+	ret = parse_attr(tb, TCA_NETEM_MAX, opt, netem_policy, sizeof(*qopt));
+	if (ret < 0)
+		return ret;
+
+	/* backup q->clg and q->loss_model */
+	old_clg = q->clg;
+	old_loss_model = q->loss_model;
+
+	if (tb[TCA_NETEM_LOSS]) {
+		ret = get_loss_clg(q, tb[TCA_NETEM_LOSS]);
+		if (ret) {
+			q->loss_model = old_loss_model;
+			return ret;
+		}
+	} else {
+		q->loss_model = CLG_RANDOM;
+		memset(&q->clg, 0, sizeof(q->clg));
+	}
+
+	if (tb[TCA_NETEM_DELAY_DIST]) {
+		ret = get_dist_table(sch, tb[TCA_NETEM_DELAY_DIST]);
+		if (ret) {
+			/* recover clg and loss_model, in case of
+			 * q->clg and q->loss_model were modified
+			 * in get_loss_clg()
+			 */
+			q->clg = old_clg;
+			q->loss_model = old_loss_model;
+			return ret;
+		}
+	} else {
+		dist_free(q->delay_dist);
+		q->delay_dist = NULL;
+	}
+
+	sch->limit = qopt->limit;
+
+	q->latency = qopt->latency;
+	q->jitter = qopt->jitter;
+	q->limit = qopt->limit;
+	q->gap = qopt->gap;
+	q->counter = 0;
+	q->loss = qopt->loss;
+	q->duplicate = qopt->duplicate;
+
+	/* for compatibility with earlier versions.
+	 * if gap is set, need to assume 100% probability
+	 */
+	if (q->gap)
+		q->reorder = ~0;
+	else
+		q->reorder = 0;
+
+	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]);
+	else
+		reorder_reset(q);
+
+	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]);
+	else
+		rate_reset(q);
+
+	if (tb[TCA_NETEM_RATE64])
+		q->rate = max_t(u64, q->rate,
+				nla_get_u64(tb[TCA_NETEM_RATE64]));
+
+	if (tb[TCA_NETEM_ECN])
+		q->ecn = nla_get_u32(tb[TCA_NETEM_ECN]);
+	else
+		q->ecn = 0;
+
+	return ret;
+}
+
 static int netem_init(struct Qdisc *sch, struct nlattr *opt)
 {
 	struct netem_sched_data *q = qdisc_priv(sch);
@@ -1115,6 +1241,7 @@ static struct Qdisc_ops netem_qdisc_ops __read_mostly = {
 	.reset		=	netem_reset,
 	.destroy	=	netem_destroy,
 	.change		=	netem_change,
+	.replace	=	netem_replace,
 	.dump		=	netem_dump,
 	.owner		=	THIS_MODULE,
 };
-- 
1.8.0

Re: [PATCH net-next 0/5] net_sched: Adjust qdisc_change() for command "#tc qdisc change/replace ..."

[Hagen Paul Pfeifer]

* Yang Yingliang | 2014-03-06 21:08:36 [+0800]:

>Current commands "#tc qdisc replace..." and "#tc qdisc change..."
>are not doing what they're supposed to do.

... <snip>

Problem: thousands of scripts may expect exactly this unexpected behavior.

Hagen

Re: [PATCH net-next 2/5] net_sched: add replace func in struct Qdisc_ops

[Eric Dumazet]

On Thu, 2014-03-06 at 21:08 +0800, Yang Yingliang wrote:
> Add replace func, if NLM_F_REPLACE is set, use replace func.
> Otherwise, use change func.
> 
> Signed-off-by: Yang Yingliang <yangyingliang@huawei.com>
> ---
>  include/net/sch_generic.h |  1 +
>  net/sched/sch_api.c       | 12 +++++++++---
>  2 files changed, 10 insertions(+), 3 deletions(-)
> 
> diff --git a/include/net/sch_generic.h b/include/net/sch_generic.h
> index d062f81c692f..ef8a41bcb9a7 100644
> --- a/include/net/sch_generic.h
> +++ b/include/net/sch_generic.h
> @@ -170,6 +170,7 @@ struct Qdisc_ops {
>  	void			(*reset)(struct Qdisc *);
>  	void			(*destroy)(struct Qdisc *);
>  	int			(*change)(struct Qdisc *, struct nlattr *arg);
> +	int			(*replace)(struct Qdisc *, struct nlattr *arg);
>  	void			(*attach)(struct Qdisc *);
>  
>  	int			(*dump)(struct Qdisc *, struct sk_buff *);
> diff --git a/net/sched/sch_api.c b/net/sched/sch_api.c
> index 961cb0f1bd0c..8f0f8e9d5475 100644
> --- a/net/sched/sch_api.c
> +++ b/net/sched/sch_api.c
> @@ -999,9 +999,15 @@ static int qdisc_change(struct Qdisc *sch, struct nlattr **tca, u16 flags)
>  	int err = 0;
>  
>  	if (tca[TCA_OPTIONS]) {
> -		if (sch->ops->change == NULL)
> -			return -EINVAL;
> -		err = sch->ops->change(sch, tca[TCA_OPTIONS]);
> +		if (flags & NLM_F_REPLACE) {
> +			if (sch->ops->replace == NULL)
> +				return -EINVAL;
> +			err = sch->ops->replace(sch, tca[TCA_OPTIONS]);
> +		} else {
> +			if (sch->ops->change == NULL)
> +				return -EINVAL;
> +			err = sch->ops->change(sch, tca[TCA_OPTIONS]);
> +		}
>  		if (err)
>  			return err;
>  	}

NACK

This breaks existing userland scripts.

Have you tried :

tc qdisc replace dev eth0 root fq

Re: [PATCH net-next 0/5] net_sched: Adjust qdisc_change() for command "#tc qdisc change/replace ..."

[Hiroaki SHIMODA]

On Thu, 6 Mar 2014 21:08:36 +0800
Yang Yingliang <yangyingliang@huawei.com> wrote:

> Current commands "#tc qdisc replace..." and "#tc qdisc change..."
> are not doing what they're supposed to do.
> 
> E.g.
> 
> With "#tc qdisc replace ...", it won't clear old option if not specified in
> qdisc of netem.
> 
>    # 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 option "rate" is still there.

It looks like you are trying to replace existing qdisc 1: with
the same qdisc 1:. So the effect is the same as change.
If you want to do replace the existing qdisc, you should specify
other handle or different qdisc.

> With "#tc qdisc change ... ", it will clear old options if not specified in
> qdisc of tbf.
> 
>   # tc qdisc add 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
> The old peakrate and minburst are cleared.

You are assumed to implicitly specify "peakrate 0" at change.
It seems that you are arguing the tc command, not kernel code.
There would exist userland commmand whose rate and peakrate
command options are always mandatory.
How they support its command options is up to userland I think.

> This patchset adds a flag parameter in qdisc_change and a replace func in struct Qdisc_ops.
> If the flag has NLM_F_REPLACE, we call the replace function, or call the change function in
> qdisc_change(). And, add tbf_replace() and netem_replace() needed by tbf and netem.
> 
> Yang Yingliang (5):
>   net_sched: add flag parameter in qdisc_change
>   net_sched: add replace func in struct Qdisc_ops
>   sch_tbf: change name "tbf_change" to "tbf_replace"
>   sch_tbf: add tbf_change for #tc qdisc change ...
>   sch_netem: add netem_replace for #tc qdisc replace ...
> 
>  include/net/sch_generic.h |   1 +
>  net/sched/sch_api.c       |  16 ++++--
>  net/sched/sch_netem.c     | 127 ++++++++++++++++++++++++++++++++++++++++++++++
>  net/sched/sch_tbf.c       | 120 ++++++++++++++++++++++++++++++++++++++++++-
>  4 files changed, 258 insertions(+), 6 deletions(-)
> 
> -- 
> 1.8.0
> 
> 
> --
> 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

Re: [PATCH net-next 2/5] net_sched: add replace func in struct Qdisc_ops

[Yang Yingliang]

On 2014/3/6 22:55, Eric Dumazet wrote:
> On Thu, 2014-03-06 at 21:08 +0800, Yang Yingliang wrote:
>> Add replace func, if NLM_F_REPLACE is set, use replace func.
>> Otherwise, use change func.
>>
>> Signed-off-by: Yang Yingliang <yangyingliang@huawei.com>
>> ---
>>  include/net/sch_generic.h |  1 +
>>  net/sched/sch_api.c       | 12 +++++++++---
>>  2 files changed, 10 insertions(+), 3 deletions(-)
>>
>> diff --git a/include/net/sch_generic.h b/include/net/sch_generic.h
>> index d062f81c692f..ef8a41bcb9a7 100644
>> --- a/include/net/sch_generic.h
>> +++ b/include/net/sch_generic.h
>> @@ -170,6 +170,7 @@ struct Qdisc_ops {
>>  	void			(*reset)(struct Qdisc *);
>>  	void			(*destroy)(struct Qdisc *);
>>  	int			(*change)(struct Qdisc *, struct nlattr *arg);
>> +	int			(*replace)(struct Qdisc *, struct nlattr *arg);
>>  	void			(*attach)(struct Qdisc *);
>>  
>>  	int			(*dump)(struct Qdisc *, struct sk_buff *);
>> diff --git a/net/sched/sch_api.c b/net/sched/sch_api.c
>> index 961cb0f1bd0c..8f0f8e9d5475 100644
>> --- a/net/sched/sch_api.c
>> +++ b/net/sched/sch_api.c
>> @@ -999,9 +999,15 @@ static int qdisc_change(struct Qdisc *sch, struct nlattr **tca, u16 flags)
>>  	int err = 0;
>>  
>>  	if (tca[TCA_OPTIONS]) {
>> -		if (sch->ops->change == NULL)
>> -			return -EINVAL;
>> -		err = sch->ops->change(sch, tca[TCA_OPTIONS]);
>> +		if (flags & NLM_F_REPLACE) {
>> +			if (sch->ops->replace == NULL)
>> +				return -EINVAL;
>> +			err = sch->ops->replace(sch, tca[TCA_OPTIONS]);
>> +		} else {
>> +			if (sch->ops->change == NULL)
>> +				return -EINVAL;
>> +			err = sch->ops->change(sch, tca[TCA_OPTIONS]);
>> +		}
>>  		if (err)
>>  			return err;
>>  	}
> 
> NACK
> 
> This breaks existing userland scripts.

Yes, it is.

Maybe I need do some change for this code.

Thanks!

> 
> Have you tried :
> 
> tc qdisc replace dev eth0 root fq
> 

Re: [PATCH net-next 0/5] net_sched: Adjust qdisc_change() for command "#tc qdisc change/replace ..."

[Yang Yingliang]

On 2014/3/6 21:16, Hagen Paul Pfeifer wrote:
> * Yang Yingliang | 2014-03-06 21:08:36 [+0800]:
> 
>> Current commands "#tc qdisc replace..." and "#tc qdisc change..."
>> are not doing what they're supposed to do.
> 
> ... <snip>
> 
> Problem: thousands of scripts may expect exactly this unexpected behavior.
> 
> Hagen

Yes, it breaks behavior of "tc qdisc change".

I will try another way.

Thanks!

Re: [PATCH net-next 0/5] net_sched: Adjust qdisc_change() for command "#tc qdisc change/replace ..."

[Yang Yingliang]

On 2014/3/7 5:08, Hiroaki SHIMODA wrote:
> On Thu, 6 Mar 2014 21:08:36 +0800
> Yang Yingliang <yangyingliang@huawei.com> wrote:
> 
>> Current commands "#tc qdisc replace..." and "#tc qdisc change..."
>> are not doing what they're supposed to do.
>>
>> E.g.
>>
>> With "#tc qdisc replace ...", it won't clear old option if not specified in
>> qdisc of netem.
>>
>>    # 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 option "rate" is still there.
> 
> It looks like you are trying to replace existing qdisc 1: with
> the same qdisc 1:. So the effect is the same as change.
> If you want to do replace the existing qdisc, you should specify
> other handle or different qdisc.
> 
>> With "#tc qdisc change ... ", it will clear old options if not specified in
>> qdisc of tbf.
>>
>>   # tc qdisc add 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
>> The old peakrate and minburst are cleared.
> 
> You are assumed to implicitly specify "peakrate 0" at change.
> It seems that you are arguing the tc command, not kernel code.
> There would exist userland commmand whose rate and peakrate
> command options are always mandatory.
> How they support its command options is up to userland I think.
> 

peakrate is not specified, it should not be changed, I think.
peakrate is not mandatory.

I think tc command has no big problem and the real problem is kernel
code treats "change" as "replace", sometimes it treats "replace" as
"change".

Thanks!