[PATCH net-next 0/4] sch_netem: some improvements and cleanups.

[PATCH net-next 0/4] sch_netem: some improvements and cleanups.

[Yang Yingliang]

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

Yang Yingliang (4):
  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
  sch_netem: replace spin_(un)lock_bh with sch_tree_(un)lock

 net/sched/sch_netem.c | 82 ++++++++++++++++++++++++++++++---------------------
 1 file changed, 49 insertions(+), 33 deletions(-)

-- 
1.8.0

[PATCH net-next 1/4] sch_netem: return errcode before setting params

[Yang Yingliang]

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 | 39 +++++++++++++++++++++++++++++----------
 1 file changed, 29 insertions(+), 10 deletions(-)

diff --git a/net/sched/sch_netem.c b/net/sched/sch_netem.c
index de1059a..b341943 100644
--- a/net/sched/sch_netem.c
+++ b/net/sched/sch_netem.c
@@ -821,6 +821,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)
@@ -831,6 +833,33 @@ 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;
@@ -850,12 +879,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]);
 
@@ -872,10 +895,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

[PATCH net-next 2/4] sch_netem: change some func's param from "struct Qdisc *" to "struct netem_sched_data *"

[Yang Yingliang]

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 b341943..4a5eb28 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;
@@ -732,9 +728,8 @@ static void get_rate(struct Qdisc *sch, const struct nlattr *attr)
 		q->cell_size_reciprocal = (struct reciprocal_value) { 0 };
 }
 
-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;
 
@@ -838,7 +833,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;
@@ -877,16 +872,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

[PATCH net-next 3/4] sch_netem: replace magic numbers with enumerate in GE model

[Yang Yingliang]

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 4a5eb28..4fced67 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

[PATCH net-next 4/4] sch_netem: replace spin_(un)lock_bh with sch_tree_(un)lock

[Yang Yingliang]

spin_(un)lock_bh(root_lock) is same as sch_tree_(un)lock.

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

diff --git a/net/sched/sch_netem.c b/net/sched/sch_netem.c
index 4fced67..62811d5 100644
--- a/net/sched/sch_netem.c
+++ b/net/sched/sch_netem.c
@@ -665,7 +665,6 @@ static int get_dist_table(struct Qdisc *sch, const struct nlattr *attr)
 	struct netem_sched_data *q = qdisc_priv(sch);
 	size_t n = nla_len(attr)/sizeof(__s16);
 	const __s16 *data = nla_data(attr);
-	spinlock_t *root_lock;
 	struct disttable *d;
 	int i;
 	size_t s;
@@ -684,11 +683,9 @@ static int get_dist_table(struct Qdisc *sch, const struct nlattr *attr)
 	for (i = 0; i < n; i++)
 		d->table[i] = data[i];
 
-	root_lock = qdisc_root_sleeping_lock(sch);
-
-	spin_lock_bh(root_lock);
+	sch_tree_lock(sch);
 	swap(q->delay_dist, d);
-	spin_unlock_bh(root_lock);
+	sch_tree_unlock(sch);
 
 	dist_free(d);
 	return 0;
-- 
1.8.0

Re: [PATCH net-next 4/4] sch_netem: replace spin_(un)lock_bh with sch_tree_(un)lock

[David Miller]

From: Yang Yingliang <yangyingliang@huawei.com>
Date: Wed, 12 Feb 2014 10:58:15 +0800

> spin_(un)lock_bh(root_lock) is same as sch_tree_(un)lock.
> 
> Signed-off-by: Yang Yingliang <yangyingliang@huawei.com>
 ...
> @@ -684,11 +683,9 @@ static int get_dist_table(struct Qdisc *sch, const struct nlattr *attr)
>  	for (i = 0; i < n; i++)
>  		d->table[i] = data[i];
>  
> -	root_lock = qdisc_root_sleeping_lock(sch);
> -
> -	spin_lock_bh(root_lock);
> +	sch_tree_lock(sch);
>  	swap(q->delay_dist, d);
> -	spin_unlock_bh(root_lock);
> +	sch_tree_unlock(sch);
>  
>  	dist_free(d);
>  	return 0;

This is more expensive than the existing code.

We will now calculate qdisc_root_sleeping_lock() twice which is at
least two pointer dereferences each.

Without explicitly open-coding this, the compiler cannot cache the
result, because the spin lock operations have memory barriers (if
implemented inline) or are considered to potentially modify all memory
(if implemented as function calls).

Re: [PATCH net-next 4/4] sch_netem: replace spin_(un)lock_bh with sch_tree_(un)lock

[Yang Yingliang]

On 2014/2/14 6:46, David Miller wrote:
> From: Yang Yingliang <yangyingliang@huawei.com>
> Date: Wed, 12 Feb 2014 10:58:15 +0800
> 
>> spin_(un)lock_bh(root_lock) is same as sch_tree_(un)lock.
>>
>> Signed-off-by: Yang Yingliang <yangyingliang@huawei.com>
>  ...
>> @@ -684,11 +683,9 @@ static int get_dist_table(struct Qdisc *sch, const struct nlattr *attr)
>>  	for (i = 0; i < n; i++)
>>  		d->table[i] = data[i];
>>  
>> -	root_lock = qdisc_root_sleeping_lock(sch);
>> -
>> -	spin_lock_bh(root_lock);
>> +	sch_tree_lock(sch);
>>  	swap(q->delay_dist, d);
>> -	spin_unlock_bh(root_lock);
>> +	sch_tree_unlock(sch);
>>  
>>  	dist_free(d);
>>  	return 0;
> 
> This is more expensive than the existing code.
> 
> We will now calculate qdisc_root_sleeping_lock() twice which is at
> least two pointer dereferences each.
> 
> Without explicitly open-coding this, the compiler cannot cache the
> result, because the spin lock operations have memory barriers (if
> implemented inline) or are considered to potentially modify all memory
> (if implemented as function calls).
> 
> 
OK, thanks!
I'll send v2 without this patch.