[PATCH net-next 0/3] sch_netem: some improvements

netdev.vger.kernel.org archive mirror
 help / color / mirror / Atom feed

* [PATCH net-next 0/3] sch_netem: some improvements
@ 2014-01-23  9:31 Yang Yingliang
  2014-01-23  9:31 ` [PATCH net-next 1/3] sch_netem: return errcode before setting params Yang Yingliang
                   ` (2 more replies)
  0 siblings, 3 replies; 5+ messages in thread
From: Yang Yingliang @ 2014-01-23  9:31 UTC (permalink / raw)
  To: netdev; +Cc: stephen, davem

This patchset do three improvements to sch_netem.

 patch 1/3: To avoid breaking old settings when we change failed,
	    do return errcode before doing setup.
 pacth 2/3: Replace some functions' parameters, these functions
	    only need struct netem_sched_data *q.
 patch 3/3: Replace magic numbers with enumerate for better readability.

Yang Yingliang (3):
  sch_netem: return errcode before setting params
  sch_netem: change some func's param from "struct Qdisc *" to "struct
    netem_sched_data *"
  sch_netem: replace magic numbers with enumerate in GE model

 net/sched/sch_netem.c | 74 ++++++++++++++++++++++++++++++++-------------------
 1 file changed, 46 insertions(+), 28 deletions(-)

-- 
1.8.0

^ permalink raw reply	[flat|nested] 5+ messages in thread

* [PATCH net-next 1/3] sch_netem: return errcode before setting params
  2014-01-23  9:31 [PATCH net-next 0/3] sch_netem: some improvements Yang Yingliang
@ 2014-01-23  9:31 ` Yang Yingliang
  2014-01-23 17:25   ` Stephen Hemminger
  2014-01-23  9:31 ` [PATCH net-next 2/3] sch_netem: change some func's param from "struct Qdisc *" to "struct netem_sched_data *" Yang Yingliang
  2014-01-23  9:31 ` [PATCH net-next 3/3] sch_netem: replace magic numbers with enumerate in GE model Yang Yingliang
  2 siblings, 1 reply; 5+ messages in thread
From: Yang Yingliang @ 2014-01-23  9:31 UTC (permalink / raw)
  To: netdev; +Cc: stephen, davem

get_dist_table() and get_loss_clg() may be failed. These
two functions should be called after setting the members
of qdisc_priv(sch), or it will break the old settings while
either of them is failed.

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

diff --git a/net/sched/sch_netem.c b/net/sched/sch_netem.c
index a2bfc37..c90de21 100644
--- a/net/sched/sch_netem.c
+++ b/net/sched/sch_netem.c
@@ -819,6 +819,8 @@ static int netem_change(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)
@@ -829,6 +831,32 @@ static int netem_change(struct Qdisc *sch, struct nlattr *opt)
 	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(sch, tb[TCA_NETEM_LOSS]);
+		if (ret) {
+			q->loss_model = old_loss_model;
+			return ret;
+		}
+	} else {
+		q->loss_model = CLG_RANDOM;
+	}
+
+	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;
+		}
+	}
+
 	sch->limit = qopt->limit;
 
 	q->latency = qopt->latency;
@@ -848,12 +876,6 @@ static int netem_change(struct Qdisc *sch, struct nlattr *opt)
 	if (tb[TCA_NETEM_CORR])
 		get_correlation(sch, tb[TCA_NETEM_CORR]);
 
-	if (tb[TCA_NETEM_DELAY_DIST]) {
-		ret = get_dist_table(sch, tb[TCA_NETEM_DELAY_DIST]);
-		if (ret)
-			return ret;
-	}
-
 	if (tb[TCA_NETEM_REORDER])
 		get_reorder(sch, tb[TCA_NETEM_REORDER]);
 
@@ -870,10 +892,6 @@ static int netem_change(struct Qdisc *sch, struct nlattr *opt)
 	if (tb[TCA_NETEM_ECN])
 		q->ecn = nla_get_u32(tb[TCA_NETEM_ECN]);
 
-	q->loss_model = CLG_RANDOM;
-	if (tb[TCA_NETEM_LOSS])
-		ret = get_loss_clg(sch, tb[TCA_NETEM_LOSS]);
-
 	return ret;
 }
 
-- 
1.8.0

^ permalink raw reply related	[flat|nested] 5+ messages in thread

* Re: [PATCH net-next 1/3] sch_netem: return errcode before setting params
  2014-01-23  9:31 ` [PATCH net-next 1/3] sch_netem: return errcode before setting params Yang Yingliang
@ 2014-01-23 17:25   ` Stephen Hemminger
  0 siblings, 0 replies; 5+ messages in thread
From: Stephen Hemminger @ 2014-01-23 17:25 UTC (permalink / raw)
  To: Yang Yingliang; +Cc: netdev, davem

I am not sure how important it is that netem updates to multiple
parameters act as a single transaction. The only failures possible
are out of memory and user configuration error.

I prefer the simplicity of the original code.


> +			/* recover clg and loss_model, in case of
> +			 * q->clg and q->loss_model were modified
> +			 * in get_loss_clg() */

This comment needs to be formatted as:
	/* multiline comment
         * example
         */


 

^ permalink raw reply	[flat|nested] 5+ messages in thread

* [PATCH net-next 2/3] sch_netem: change some func's param from "struct Qdisc *" to "struct netem_sched_data *"
  2014-01-23  9:31 [PATCH net-next 0/3] sch_netem: some improvements Yang Yingliang
  2014-01-23  9:31 ` [PATCH net-next 1/3] sch_netem: return errcode before setting params Yang Yingliang
@ 2014-01-23  9:31 ` Yang Yingliang
  2014-01-23  9:31 ` [PATCH net-next 3/3] sch_netem: replace magic numbers with enumerate in GE model Yang Yingliang
  2 siblings, 0 replies; 5+ messages in thread
From: Yang Yingliang @ 2014-01-23  9:31 UTC (permalink / raw)
  To: netdev; +Cc: stephen, davem

In netem_change(), we have already get "struct netem_sched_data *q".
Replace params of get_correlation() and other similar functions with
"struct netem_sched_data *q".

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

diff --git a/net/sched/sch_netem.c b/net/sched/sch_netem.c
index c90de21..7e15f08 100644
--- a/net/sched/sch_netem.c
+++ b/net/sched/sch_netem.c
@@ -689,9 +689,8 @@ static int get_dist_table(struct Qdisc *sch, const struct nlattr *attr)
 	return 0;
 }
 
-static void get_correlation(struct Qdisc *sch, const struct nlattr *attr)
+static void get_correlation(struct netem_sched_data *q, const struct nlattr *attr)
 {
-	struct netem_sched_data *q = qdisc_priv(sch);
 	const struct tc_netem_corr *c = nla_data(attr);
 
 	init_crandom(&q->delay_cor, c->delay_corr);
@@ -699,27 +698,24 @@ static void get_correlation(struct Qdisc *sch, const struct nlattr *attr)
 	init_crandom(&q->dup_cor, c->dup_corr);
 }
 
-static void get_reorder(struct Qdisc *sch, const struct nlattr *attr)
+static void get_reorder(struct netem_sched_data *q, const struct nlattr *attr)
 {
-	struct netem_sched_data *q = qdisc_priv(sch);
 	const struct tc_netem_reorder *r = nla_data(attr);
 
 	q->reorder = r->probability;
 	init_crandom(&q->reorder_cor, r->correlation);
 }
 
-static void get_corrupt(struct Qdisc *sch, const struct nlattr *attr)
+static void get_corrupt(struct netem_sched_data *q, const struct nlattr *attr)
 {
-	struct netem_sched_data *q = qdisc_priv(sch);
 	const struct tc_netem_corrupt *r = nla_data(attr);
 
 	q->corrupt = r->probability;
 	init_crandom(&q->corrupt_cor, r->correlation);
 }
 
-static void get_rate(struct Qdisc *sch, const struct nlattr *attr)
+static void get_rate(struct netem_sched_data *q, const struct nlattr *attr)
 {
-	struct netem_sched_data *q = qdisc_priv(sch);
 	const struct tc_netem_rate *r = nla_data(attr);
 
 	q->rate = r->rate;
@@ -730,9 +726,8 @@ static void get_rate(struct Qdisc *sch, const struct nlattr *attr)
 	q->cell_overhead = r->cell_overhead;
 }
 
-static int get_loss_clg(struct Qdisc *sch, const struct nlattr *attr)
+static int get_loss_clg(struct netem_sched_data *q, const struct nlattr *attr)
 {
-	struct netem_sched_data *q = qdisc_priv(sch);
 	const struct nlattr *la;
 	int rem;
 
@@ -836,7 +831,7 @@ static int netem_change(struct Qdisc *sch, struct nlattr *opt)
 	old_loss_model = q->loss_model;
 
 	if (tb[TCA_NETEM_LOSS]) {
-		ret = get_loss_clg(sch, tb[TCA_NETEM_LOSS]);
+		ret = get_loss_clg(q, tb[TCA_NETEM_LOSS]);
 		if (ret) {
 			q->loss_model = old_loss_model;
 			return ret;
@@ -874,16 +869,16 @@ static int netem_change(struct Qdisc *sch, struct nlattr *opt)
 		q->reorder = ~0;
 
 	if (tb[TCA_NETEM_CORR])
-		get_correlation(sch, tb[TCA_NETEM_CORR]);
+		get_correlation(q, tb[TCA_NETEM_CORR]);
 
 	if (tb[TCA_NETEM_REORDER])
-		get_reorder(sch, tb[TCA_NETEM_REORDER]);
+		get_reorder(q, tb[TCA_NETEM_REORDER]);
 
 	if (tb[TCA_NETEM_CORRUPT])
-		get_corrupt(sch, tb[TCA_NETEM_CORRUPT]);
+		get_corrupt(q, tb[TCA_NETEM_CORRUPT]);
 
 	if (tb[TCA_NETEM_RATE])
-		get_rate(sch, tb[TCA_NETEM_RATE]);
+		get_rate(q, tb[TCA_NETEM_RATE]);
 
 	if (tb[TCA_NETEM_RATE64])
 		q->rate = max_t(u64, q->rate,
-- 
1.8.0

^ permalink raw reply related	[flat|nested] 5+ messages in thread

* [PATCH net-next 3/3] sch_netem: replace magic numbers with enumerate in GE model
  2014-01-23  9:31 [PATCH net-next 0/3] sch_netem: some improvements Yang Yingliang
  2014-01-23  9:31 ` [PATCH net-next 1/3] sch_netem: return errcode before setting params Yang Yingliang
  2014-01-23  9:31 ` [PATCH net-next 2/3] sch_netem: change some func's param from "struct Qdisc *" to "struct netem_sched_data *" Yang Yingliang
@ 2014-01-23  9:31 ` Yang Yingliang
  2 siblings, 0 replies; 5+ messages in thread
From: Yang Yingliang @ 2014-01-23  9:31 UTC (permalink / raw)
  To: netdev; +Cc: stephen, davem

Replace some magic numbers which describe states of GE model
loss generator with enumerate.

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

diff --git a/net/sched/sch_netem.c b/net/sched/sch_netem.c
index 7e15f08..2479709 100644
--- a/net/sched/sch_netem.c
+++ b/net/sched/sch_netem.c
@@ -117,6 +117,11 @@ struct netem_sched_data {
 		LOST_IN_BURST_PERIOD,
 	} _4_state_model;
 
+	enum {
+		GOOD_STATE = 1,
+		BAD_STATE,
+	} GE_state_model;
+
 	/* Correlated Loss Generation models */
 	struct clgstate {
 		/* state of the Markov chain */
@@ -272,15 +277,15 @@ static bool loss_gilb_ell(struct netem_sched_data *q)
 	struct clgstate *clg = &q->clg;
 
 	switch (clg->state) {
-	case 1:
+	case GOOD_STATE:
 		if (prandom_u32() < clg->a1)
-			clg->state = 2;
+			clg->state = BAD_STATE;
 		if (prandom_u32() < clg->a4)
 			return true;
 		break;
-	case 2:
+	case BAD_STATE:
 		if (prandom_u32() < clg->a2)
-			clg->state = 1;
+			clg->state = GOOD_STATE;
 		if (prandom_u32() > clg->a3)
 			return true;
 	}
-- 
1.8.0

^ permalink raw reply related	[flat|nested] 5+ messages in thread

end of thread, other threads:[~2014-01-23 17:25 UTC | newest]

Thread overview: 5+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2014-01-23  9:31 [PATCH net-next 0/3] sch_netem: some improvements Yang Yingliang
2014-01-23  9:31 ` [PATCH net-next 1/3] sch_netem: return errcode before setting params Yang Yingliang
2014-01-23 17:25   ` Stephen Hemminger
2014-01-23  9:31 ` [PATCH net-next 2/3] sch_netem: change some func's param from "struct Qdisc *" to "struct netem_sched_data *" Yang Yingliang
2014-01-23  9:31 ` [PATCH net-next 3/3] sch_netem: replace magic numbers with enumerate in GE model Yang Yingliang

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