[PATCH iproute2 00/10] RED and GRED fixes, plus TCA_GRED_LIMIT support

[PATCH iproute2 00/10] RED and GRED fixes, plus TCA_GRED_LIMIT support

[David Ward]

Please note that the final patch in this series adds tc command support for the
TCA_GRED_LIMIT attribute, which was accepted into net-next. However, it follows
the other patches in this series, which are bug fixes and improvements that can
be applied to the master branch of iproute2.

Signed-off-by: David Ward <david.ward@ll.mit.edu>

David Ward (10):
  tc: red, gred: Rename overloaded variable wlog
  tc: red, gred: Fix format specifier in burst size warning
  tc: red, gred: Notify when using the default value for "bandwidth"
  tc: red: Mark "bandwidth" parameter as optional in usage text
  tc: gred: Fix whitespace issues in code
  tc: gred: Print usage text if no arguments appear after "gred"
  tc: gred: Improve parameter/statistics output
  tc: gred: Handle unsigned values properly in option parsing/printing
  tc: gred: Adopt the term VQ in the command syntax and output
  tc: gred: Add support for TCA_GRED_LIMIT attribute

 include/linux/pkt_sched.h |    3 +-
 tc/q_gred.c               |  204 +++++++++++++++++++++++++--------------------
 tc/q_red.c                |   27 +++---
 3 files changed, 131 insertions(+), 103 deletions(-)

[PATCH iproute2 01/10] tc: red, gred: Rename overloaded variable wlog

[David Ward]

It is used when parsing three different parameters, only one of
which is Wlog. Change the name to make the code less confusing.

Signed-off-by: David Ward <david.ward@ll.mit.edu>
---
 tc/q_gred.c |   17 ++++++++---------
 tc/q_red.c  |   16 ++++++++--------
 2 files changed, 16 insertions(+), 17 deletions(-)

diff --git a/tc/q_gred.c b/tc/q_gred.c
index 88bd094..625bcf9 100644
--- a/tc/q_gred.c
+++ b/tc/q_gred.c
@@ -123,7 +123,7 @@ static int gred_parse_opt(struct qdisc_util *qu, int argc, char **argv, struct n
 	unsigned avpkt = 0;
 	double probability = 0.02;
 	unsigned rate = 0;
-	int wlog;
+	int parm;
 	__u8 sbuf[256];
 	struct rtattr *tail;
 	__u32 max_P;
@@ -227,27 +227,26 @@ static int gred_parse_opt(struct qdisc_util *qu, int argc, char **argv, struct n
 		burst = (2 * opt.qth_min + opt.qth_max) / (3 * avpkt);
 		fprintf(stderr, "GRED: set burst to %u\n", burst);
 	}
-
-	if ((wlog = tc_red_eval_ewma(opt.qth_min, burst, avpkt)) < 0) {
+	if ((parm = tc_red_eval_ewma(opt.qth_min, burst, avpkt)) < 0) {
 		fprintf(stderr, "GRED: failed to calculate EWMA constant.\n");
 		return -1;
 	}
-	if (wlog >= 10)
+	if (parm >= 10)
 		fprintf(stderr, "GRED: WARNING. Burst %d seems to be too "
 		    "large.\n", burst);
-	opt.Wlog = wlog;
-	if ((wlog = tc_red_eval_P(opt.qth_min, opt.qth_max, probability)) < 0) {
+	opt.Wlog = parm;
+	if ((parm = tc_red_eval_P(opt.qth_min, opt.qth_max, probability)) < 0) {
 		fprintf(stderr, "GRED: failed to calculate probability.\n");
 		return -1;
 	}
-	opt.Plog = wlog;
-	if ((wlog = tc_red_eval_idle_damping(opt.Wlog, avpkt, rate, sbuf)) < 0)
+	opt.Plog = parm;
+	if ((parm = tc_red_eval_idle_damping(opt.Wlog, avpkt, rate, sbuf)) < 0)
 	    {
 		fprintf(stderr, "GRED: failed to calculate idle damping "
 		    "table.\n");
 		return -1;
 	}
-	opt.Scell_log = wlog;
+	opt.Scell_log = parm;
 
 	tail = NLMSG_TAIL(n);
 	addattr_l(n, 1024, TCA_OPTIONS, NULL, 0);
diff --git a/tc/q_red.c b/tc/q_red.c
index 89e7320..5a74c50 100644
--- a/tc/q_red.c
+++ b/tc/q_red.c
@@ -40,7 +40,7 @@ static int red_parse_opt(struct qdisc_util *qu, int argc, char **argv, struct nl
 	unsigned avpkt = 0;
 	double probability = 0.02;
 	unsigned rate = 0;
-	int wlog;
+	int parm;
 	__u8 sbuf[256];
 	__u32 max_P;
 	struct rtattr *tail;
@@ -126,23 +126,23 @@ static int red_parse_opt(struct qdisc_util *qu, int argc, char **argv, struct nl
 		opt.qth_min = opt.qth_max / 3;
 	if (!burst)
 		burst = (2 * opt.qth_min + opt.qth_max) / (3 * avpkt);
-	if ((wlog = tc_red_eval_ewma(opt.qth_min, burst, avpkt)) < 0) {
+	if ((parm = tc_red_eval_ewma(opt.qth_min, burst, avpkt)) < 0) {
 		fprintf(stderr, "RED: failed to calculate EWMA constant.\n");
 		return -1;
 	}
-	if (wlog >= 10)
+	if (parm >= 10)
 		fprintf(stderr, "RED: WARNING. Burst %d seems to be too large.\n", burst);
-	opt.Wlog = wlog;
-	if ((wlog = tc_red_eval_P(opt.qth_min, opt.qth_max, probability)) < 0) {
+	opt.Wlog = parm;
+	if ((parm = tc_red_eval_P(opt.qth_min, opt.qth_max, probability)) < 0) {
 		fprintf(stderr, "RED: failed to calculate probability.\n");
 		return -1;
 	}
-	opt.Plog = wlog;
-	if ((wlog = tc_red_eval_idle_damping(opt.Wlog, avpkt, rate, sbuf)) < 0) {
+	opt.Plog = parm;
+	if ((parm = tc_red_eval_idle_damping(opt.Wlog, avpkt, rate, sbuf)) < 0) {
 		fprintf(stderr, "RED: failed to calculate idle damping table.\n");
 		return -1;
 	}
-	opt.Scell_log = wlog;
+	opt.Scell_log = parm;
 
 	tail = NLMSG_TAIL(n);
 	addattr_l(n, 1024, TCA_OPTIONS, NULL, 0);
-- 
1.7.1

Re: [PATCH iproute2 01/10] tc: red, gred: Rename overloaded variable wlog

[Stephen Hemminger]

On Mon, 18 May 2015 11:35:05 -0400
David Ward <david.ward@ll.mit.edu> wrote:

> It is used when parsing three different parameters, only one of
> which is Wlog. Change the name to make the code less confusing.
> 
> Signed-off-by: David Ward <david.ward@ll.mit.edu>


All applied, and I went ahead and fixed a couple of old style issues in RED/GRED
code as well.

[PATCH iproute2 02/10] tc: red, gred: Fix format specifier in burst size warning

[David Ward]

burst is an unsigned value.

Signed-off-by: David Ward <david.ward@ll.mit.edu>
---
 tc/q_gred.c |    2 +-
 tc/q_red.c  |    2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/tc/q_gred.c b/tc/q_gred.c
index 625bcf9..d876f83 100644
--- a/tc/q_gred.c
+++ b/tc/q_gred.c
@@ -232,7 +232,7 @@ static int gred_parse_opt(struct qdisc_util *qu, int argc, char **argv, struct n
 		return -1;
 	}
 	if (parm >= 10)
-		fprintf(stderr, "GRED: WARNING. Burst %d seems to be too "
+		fprintf(stderr, "GRED: WARNING. Burst %u seems to be too "
 		    "large.\n", burst);
 	opt.Wlog = parm;
 	if ((parm = tc_red_eval_P(opt.qth_min, opt.qth_max, probability)) < 0) {
diff --git a/tc/q_red.c b/tc/q_red.c
index 5a74c50..6555eb2 100644
--- a/tc/q_red.c
+++ b/tc/q_red.c
@@ -131,7 +131,7 @@ static int red_parse_opt(struct qdisc_util *qu, int argc, char **argv, struct nl
 		return -1;
 	}
 	if (parm >= 10)
-		fprintf(stderr, "RED: WARNING. Burst %d seems to be too large.\n", burst);
+		fprintf(stderr, "RED: WARNING. Burst %u seems to be too large.\n", burst);
 	opt.Wlog = parm;
 	if ((parm = tc_red_eval_P(opt.qth_min, opt.qth_max, probability)) < 0) {
 		fprintf(stderr, "RED: failed to calculate probability.\n");
-- 
1.7.1

[PATCH iproute2 03/10] tc: red, gred: Notify when using the default value for "bandwidth"

[David Ward]

The "bandwidth" parameter is optional, but ensure the user is aware
of its default value, to proactively avoid configuration problems.

Signed-off-by: David Ward <david.ward@ll.mit.edu>
---
 tc/q_gred.c |    7 ++++---
 tc/q_red.c  |    7 ++++---
 2 files changed, 8 insertions(+), 6 deletions(-)

diff --git a/tc/q_gred.c b/tc/q_gred.c
index d876f83..5e0dfcf 100644
--- a/tc/q_gred.c
+++ b/tc/q_gred.c
@@ -214,9 +214,6 @@ static int gred_parse_opt(struct qdisc_util *qu, int argc, char **argv, struct n
 		argc--; argv++;
 	}
 
-	if (rate == 0)
-		get_rate(&rate, "10Mbit");
-
 	if (!opt.qth_min || !opt.qth_max || !opt.limit || !avpkt ||
 	    (opt.DP<0)) {
 		fprintf(stderr, "Required parameter (min, max, limit, "
@@ -227,6 +224,10 @@ static int gred_parse_opt(struct qdisc_util *qu, int argc, char **argv, struct n
 		burst = (2 * opt.qth_min + opt.qth_max) / (3 * avpkt);
 		fprintf(stderr, "GRED: set burst to %u\n", burst);
 	}
+	if (!rate) {
+		get_rate(&rate, "10Mbit");
+		fprintf(stderr, "GRED: set bandwidth to 10Mbit\n");
+	}
 	if ((parm = tc_red_eval_ewma(opt.qth_min, burst, avpkt)) < 0) {
 		fprintf(stderr, "GRED: failed to calculate EWMA constant.\n");
 		return -1;
diff --git a/tc/q_red.c b/tc/q_red.c
index 6555eb2..1bf2bd9 100644
--- a/tc/q_red.c
+++ b/tc/q_red.c
@@ -109,9 +109,6 @@ static int red_parse_opt(struct qdisc_util *qu, int argc, char **argv, struct nl
 		argc--; argv++;
 	}
 
-	if (rate == 0)
-		get_rate(&rate, "10Mbit");
-
 	if (!opt.limit || !avpkt) {
 		fprintf(stderr, "RED: Required parameter (limit, avpkt) is missing\n");
 		return -1;
@@ -126,6 +123,10 @@ static int red_parse_opt(struct qdisc_util *qu, int argc, char **argv, struct nl
 		opt.qth_min = opt.qth_max / 3;
 	if (!burst)
 		burst = (2 * opt.qth_min + opt.qth_max) / (3 * avpkt);
+	if (!rate) {
+		get_rate(&rate, "10Mbit");
+		fprintf(stderr, "RED: set bandwidth to 10Mbit\n");
+	}
 	if ((parm = tc_red_eval_ewma(opt.qth_min, burst, avpkt)) < 0) {
 		fprintf(stderr, "RED: failed to calculate EWMA constant.\n");
 		return -1;
-- 
1.7.1

[PATCH iproute2 04/10] tc: red: Mark "bandwidth" parameter as optional in usage text

[David Ward]

Signed-off-by: David Ward <david.ward@ll.mit.edu>
---
 tc/q_red.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/tc/q_red.c b/tc/q_red.c
index 1bf2bd9..abd86c7 100644
--- a/tc/q_red.c
+++ b/tc/q_red.c
@@ -29,7 +29,7 @@
 static void explain(void)
 {
 	fprintf(stderr, "Usage: ... red limit BYTES [min BYTES] [max BYTES] avpkt BYTES [burst PACKETS]\n");
-	fprintf(stderr, "               [adaptive] [probability PROBABILITY] bandwidth KBPS\n");
+	fprintf(stderr, "               [adaptive] [probability PROBABILITY] [bandwidth KBPS]\n");
 	fprintf(stderr, "               [ecn] [harddrop]\n");
 }
 
-- 
1.7.1

[PATCH iproute2 05/10] tc: gred: Fix whitespace issues in code

[David Ward]

Signed-off-by: David Ward <david.ward@ll.mit.edu>
---
 tc/q_gred.c |    5 ++---
 1 files changed, 2 insertions(+), 3 deletions(-)

diff --git a/tc/q_gred.c b/tc/q_gred.c
index 5e0dfcf..a3dc722 100644
--- a/tc/q_gred.c
+++ b/tc/q_gred.c
@@ -143,8 +143,7 @@ static int gred_parse_opt(struct qdisc_util *qu, int argc, char **argv, struct n
 				fprintf(stderr, "Illegal \"setup\"\n");
 				return -1;
 			}
-		return init_gred(qu,argc-1, argv+1,n);
-
+			return init_gred(qu, argc-1, argv+1, n);
 		} else if (strcmp(*argv, "min") == 0) {
 			NEXT_ARG();
 			if (get_size(&opt.qth_min, *argv)) {
@@ -172,7 +171,7 @@ static int gred_parse_opt(struct qdisc_util *qu, int argc, char **argv, struct n
 			ok++;
 		} else if (strcmp(*argv, "burst") == 0) {
 			NEXT_ARG();
-                        if (get_unsigned(&burst, *argv, 0)) {
+			if (get_unsigned(&burst, *argv, 0)) {
 				fprintf(stderr, "Illegal \"burst\"\n");
 				return -1;
 			}
-- 
1.7.1

[PATCH iproute2 06/10] tc: gred: Print usage text if no arguments appear after "gred"

[David Ward]

This is more helpful to the user, since the command takes two forms,
and the message that would otherwise appear about missing parameters
assumes one of those forms.

Signed-off-by: David Ward <david.ward@ll.mit.edu>
---
 tc/q_gred.c |    4 ++++
 1 files changed, 4 insertions(+), 0 deletions(-)

diff --git a/tc/q_gred.c b/tc/q_gred.c
index a3dc722..65caeee 100644
--- a/tc/q_gred.c
+++ b/tc/q_gred.c
@@ -213,6 +213,10 @@ static int gred_parse_opt(struct qdisc_util *qu, int argc, char **argv, struct n
 		argc--; argv++;
 	}
 
+	if (!ok) {
+		explain();
+		return -1;
+	}
 	if (!opt.qth_min || !opt.qth_max || !opt.limit || !avpkt ||
 	    (opt.DP<0)) {
 		fprintf(stderr, "Required parameter (min, max, limit, "
-- 
1.7.1

[PATCH iproute2 07/10] tc: gred: Improve parameter/statistics output

[David Ward]

Make the output more consistent with the RED qdisc, and only show
details/statistics if the appropriate flag is set when calling tc.

Show the parameters used with "gred setup". Add missing statistics
"pdrop" and "other". Fix format specifiers for unsigned values.

Signed-off-by: David Ward <david.ward@ll.mit.edu>
---
 tc/q_gred.c |   63 +++++++++++++++++++++++++++++++++-------------------------
 1 files changed, 36 insertions(+), 27 deletions(-)

diff --git a/tc/q_gred.c b/tc/q_gred.c
index 65caeee..d3e5ec7 100644
--- a/tc/q_gred.c
+++ b/tc/q_gred.c
@@ -265,14 +265,13 @@ static int gred_parse_opt(struct qdisc_util *qu, int argc, char **argv, struct n
 static int gred_print_opt(struct qdisc_util *qu, FILE *f, struct rtattr *opt)
 {
 	struct rtattr *tb[TCA_GRED_MAX + 1];
+	struct tc_gred_sopt *sopt;
 	struct tc_gred_qopt *qopt;
 	__u32 *max_p = NULL;
-	int i;
+	unsigned i;
 	SPRINT_BUF(b1);
 	SPRINT_BUF(b2);
 	SPRINT_BUF(b3);
-	SPRINT_BUF(b4);
-	SPRINT_BUF(b5);
 
 	if (opt == NULL)
 		return 0;
@@ -286,40 +285,50 @@ static int gred_print_opt(struct qdisc_util *qu, FILE *f, struct rtattr *opt)
 	    RTA_PAYLOAD(tb[TCA_GRED_MAX_P]) >= sizeof(__u32) * MAX_DPs)
 		max_p = RTA_DATA(tb[TCA_GRED_MAX_P]);
 
+	sopt = RTA_DATA(tb[TCA_GRED_DPS]);
 	qopt = RTA_DATA(tb[TCA_GRED_PARMS]);
-	if (RTA_PAYLOAD(tb[TCA_GRED_PARMS])  < sizeof(*qopt)*MAX_DPs) {
+	if (RTA_PAYLOAD(tb[TCA_GRED_DPS]) < sizeof(*sopt) ||
+	    RTA_PAYLOAD(tb[TCA_GRED_PARMS]) < sizeof(*qopt)*MAX_DPs) {
 		fprintf(f,"\n GRED received message smaller than expected\n");
 		return -1;
-		}
+	}
 
 /* Bad hack! should really return a proper message as shown above*/
 
+	fprintf(f, "DPs %u default %u %s",
+		sopt->DPs,
+		sopt->def_DP,
+		sopt->grio ? "grio " : "");
+
 	for (i=0;i<MAX_DPs;i++, qopt++) {
 		if (qopt->DP >= MAX_DPs) continue;
-		fprintf(f, "\n DP:%d (prio %d) Average Queue %s Measured "
-		    "Queue %s  ",
+		fprintf(f, "\n DP %u prio %hhu limit %s min %s max %s ",
 			qopt->DP,
 			qopt->prio,
-			sprint_size(qopt->qave, b4),
-			sprint_size(qopt->backlog, b5));
-		fprintf(f, "\n\t Packet drops: %d (forced %d early %d)  ",
-			qopt->forced+qopt->early,
-			qopt->forced,
-			qopt->early);
-		fprintf(f, "\n\t Packet totals: %u (bytes %u)  ",
-			qopt->packets,
-			qopt->bytesin);
-		if (show_details)
-			fprintf(f, "\n limit %s min %s max %s ",
-				sprint_size(qopt->limit, b1),
-				sprint_size(qopt->qth_min, b2),
-				sprint_size(qopt->qth_max, b3));
-		fprintf(f, "ewma %u ", qopt->Wlog);
-		if (max_p)
-			fprintf(f, "probability %lg ", max_p[i] / pow(2, 32));
-		else
-			fprintf(f, "Plog %u ", qopt->Plog);
-		fprintf(f, "Scell_log %u", qopt->Scell_log);
+			sprint_size(qopt->limit, b1),
+			sprint_size(qopt->qth_min, b2),
+			sprint_size(qopt->qth_max, b3));
+		if (show_details) {
+			fprintf(f, "ewma %u ", qopt->Wlog);
+			if (max_p)
+				fprintf(f, "probability %lg ", max_p[i] / pow(2, 32));
+			else
+				fprintf(f, "Plog %u ", qopt->Plog);
+			fprintf(f, "Scell_log %u ", qopt->Scell_log);
+		}
+		if (show_stats) {
+			fprintf(f, "\n  Queue size: average %s current %s ",
+				sprint_size(qopt->qave, b1),
+				sprint_size(qopt->backlog, b2));
+			fprintf(f, "\n  Dropped packets: forced %u early %u pdrop %u other %u ",
+				qopt->forced,
+				qopt->early,
+				qopt->pdrop,
+				qopt->other);
+			fprintf(f, "\n  Total packets: %u (%s) ",
+				qopt->packets,
+				sprint_size(qopt->bytesin, b1));
+		}
 	}
 	return 0;
 }
-- 
1.7.1

[PATCH iproute2 08/10] tc: gred: Handle unsigned values properly in option parsing/printing

[David Ward]

DPs, def_DP, and DP are unsigned values that are sent and received
in TCA_GRED_* netlink attributes; handle them properly when they
are parsed or printed. Use MAX_DPs as the initial value for def_DP
and DP, and fix the operator used for bounds checking them.

Signed-off-by: David Ward <david.ward@ll.mit.edu>
---
 tc/q_gred.c |   65 ++++++++++++++++++++++++++++------------------------------
 1 files changed, 31 insertions(+), 34 deletions(-)

diff --git a/tc/q_gred.c b/tc/q_gred.c
index d3e5ec7..0192d9f 100644
--- a/tc/q_gred.c
+++ b/tc/q_gred.c
@@ -53,34 +53,34 @@ static int init_gred(struct qdisc_util *qu, int argc, char **argv,
 
 	struct rtattr *tail;
 	struct tc_gred_sopt opt = { 0 };
-	int dps = 0;
-	int def_dp = -1;
+
+	opt.def_DP = MAX_DPs;
 
 	while (argc > 0) {
 		DPRINTF(stderr,"init_gred: invoked with %s\n",*argv);
 		if (strcmp(*argv, "DPs") == 0) {
 			NEXT_ARG();
-			DPRINTF(stderr,"init_gred: next_arg with %s\n",*argv);
-			dps = strtol(*argv, (char **)NULL, 10);
-			if (dps < 0 || dps >MAX_DPs) {
-				fprintf(stderr, "DPs =%d\n", dps);
+			if (get_unsigned(&opt.DPs, *argv, 10)) {
 				fprintf(stderr, "Illegal \"DPs\"\n");
-				fprintf(stderr, "GRED: only %d DPs are "
-					"currently supported\n",MAX_DPs);
+				return -1;
+			} else if (opt.DPs > MAX_DPs) {
+				fprintf(stderr, "GRED: only %u DPs are "
+					"currently supported\n", MAX_DPs);
 				return -1;
 			}
 		} else if (strcmp(*argv, "default") == 0) {
-			NEXT_ARG();
-			def_dp = strtol(*argv, (char **)NULL, 10);
-			if (dps == 0) {
-				fprintf(stderr, "\"default DP\" must be "
-					"defined after DPs\n");
+			if (opt.DPs == 0) {
+				fprintf(stderr, "\"default\" must be defined "
+					"after \"DPs\"\n");
 				return -1;
 			}
-			if (def_dp < 0 || def_dp > dps) {
-				fprintf(stderr,
-					"\"default DP\" must be less than %d\n",
-					opt.DPs);
+			NEXT_ARG();
+			if (get_unsigned(&opt.def_DP, *argv, 10)) {
+				fprintf(stderr, "Illegal \"default\"\n");
+				return -1;
+			} else if (opt.def_DP >= opt.DPs) {
+				fprintf(stderr, "\"default\" must be less than "
+					"\"DPs\"\n");
 				return -1;
 			}
 		} else if (strcmp(*argv, "grio") == 0) {
@@ -96,15 +96,12 @@ static int init_gred(struct qdisc_util *qu, int argc, char **argv,
 		argc--; argv++;
 	}
 
-	if (!dps || def_dp == -1) {
+	if (!opt.DPs || opt.def_DP == MAX_DPs) {
 		fprintf(stderr, "Illegal gred setup parameters \n");
 		return -1;
 	}
 
-	opt.DPs = dps;
-	opt.def_DP = def_dp;
-
-	DPRINTF("TC_GRED: sending DPs=%d default=%d\n",opt.DPs,opt.def_DP);
+	DPRINTF("TC_GRED: sending DPs=%u def_DP=%u\n",opt.DPs,opt.def_DP);
 	n->nlmsg_flags|=NLM_F_CREATE;
 	tail = NLMSG_TAIL(n);
 	addattr_l(n, 1024, TCA_OPTIONS, NULL, 0);
@@ -118,7 +115,7 @@ static int init_gred(struct qdisc_util *qu, int argc, char **argv,
 static int gred_parse_opt(struct qdisc_util *qu, int argc, char **argv, struct nlmsghdr *n)
 {
 	int ok=0;
-	struct tc_gred_qopt opt;
+	struct tc_gred_qopt opt = { 0 };
 	unsigned burst = 0;
 	unsigned avpkt = 0;
 	double probability = 0.02;
@@ -128,7 +125,7 @@ static int gred_parse_opt(struct qdisc_util *qu, int argc, char **argv, struct n
 	struct rtattr *tail;
 	__u32 max_P;
 
-	memset(&opt, 0, sizeof(opt));
+	opt.DP = MAX_DPs;
 
 	while (argc > 0) {
 		if (strcmp(*argv, "limit") == 0) {
@@ -160,14 +157,14 @@ static int gred_parse_opt(struct qdisc_util *qu, int argc, char **argv, struct n
 			ok++;
 		} else if (strcmp(*argv, "DP") == 0) {
 			NEXT_ARG();
-			opt.DP=strtol(*argv, (char **)NULL, 10);
-			DPRINTF ("\n ******* DP =%u\n",opt.DP);
-			if (opt.DP >MAX_DPs) { /* need a better error check */
-				fprintf(stderr, "DP =%u \n",opt.DP);
+			if (get_unsigned(&opt.DP, *argv, 10)) {
 				fprintf(stderr, "Illegal \"DP\"\n");
-				fprintf(stderr, "GRED: only %d DPs are currently supported\n",MAX_DPs);
 				return -1;
-			}
+			} else if (opt.DP >= MAX_DPs) {
+				fprintf(stderr, "GRED: only %u DPs are "
+					"currently supported\n", MAX_DPs);
+				return -1;
+			} /* need a better error check */
 			ok++;
 		} else if (strcmp(*argv, "burst") == 0) {
 			NEXT_ARG();
@@ -217,10 +214,10 @@ static int gred_parse_opt(struct qdisc_util *qu, int argc, char **argv, struct n
 		explain();
 		return -1;
 	}
-	if (!opt.qth_min || !opt.qth_max || !opt.limit || !avpkt ||
-	    (opt.DP<0)) {
-		fprintf(stderr, "Required parameter (min, max, limit, "
-		    "avpkt, DP) is missing\n");
+	if (opt.DP == MAX_DPs || !opt.limit || !opt.qth_min || !opt.qth_max ||
+	    !avpkt) {
+		fprintf(stderr, "Required parameter (DP, limit, min, max, "
+			"avpkt) is missing\n");
 		return -1;
 	}
 	if (!burst) {
-- 
1.7.1

[PATCH iproute2 09/10] tc: gred: Adopt the term VQ in the command syntax and output

[David Ward]

In the GRED kernel source code, both of the terms "drop parameters"
(DP) and "virtual queue" (VQ) are used to refer to the same thing.
Each "DP" is better understood as a "set of drop parameters", since
it has values for limit, min, max, avpkt, etc. This terminology can
result in confusion when creating a GRED qdisc having multiple DPs.
Netlink attributes and struct members with the DP name seem to have
been left intact for compatibility, while the term VQ was otherwise
adopted in the code, which is more intuitive.

Use the VQ term in the tc command syntax and output (but maintain
compatibility with the old syntax).

Rewrite the usage text to be concise and similar to other qdiscs.

Signed-off-by: David Ward <david.ward@ll.mit.edu>
---
 tc/q_gred.c |   37 ++++++++++++++++++-------------------
 1 files changed, 18 insertions(+), 19 deletions(-)

diff --git a/tc/q_gred.c b/tc/q_gred.c
index 0192d9f..463d725 100644
--- a/tc/q_gred.c
+++ b/tc/q_gred.c
@@ -37,14 +37,11 @@
 
 static void explain(void)
 {
-	fprintf(stderr, "Usage: ... gred DP drop-probability limit BYTES "
-	    "min BYTES max BYTES\n");
-	fprintf(stderr, "    avpkt BYTES burst PACKETS probability PROBABILITY "
-	    "bandwidth KBPS\n");
-	fprintf(stderr, "    [prio value]\n");
-	fprintf(stderr," OR ...\n");
-	fprintf(stderr," gred setup DPs <num of DPs> default <default DP> "
-	    "[grio]\n");
+	fprintf(stderr, "Usage: tc qdisc { add | replace | change } ... gred setup vqs NUMBER\n");
+	fprintf(stderr, "           default DEFAULT_VQ [ grio ]\n");
+	fprintf(stderr, "       tc qdisc change ... gred vq VQ [ prio VALUE ] limit BYTES\n");
+	fprintf(stderr, "           min BYTES max BYTES avpkt BYTES [ burst PACKETS ]\n");
+	fprintf(stderr, "           [ probability PROBABILITY ] [ bandwidth KBPS ]\n");
 }
 
 static int init_gred(struct qdisc_util *qu, int argc, char **argv,
@@ -58,20 +55,21 @@ static int init_gred(struct qdisc_util *qu, int argc, char **argv,
 
 	while (argc > 0) {
 		DPRINTF(stderr,"init_gred: invoked with %s\n",*argv);
-		if (strcmp(*argv, "DPs") == 0) {
+		if (strcmp(*argv, "vqs") == 0 ||
+		    strcmp(*argv, "DPs") == 0) {
 			NEXT_ARG();
 			if (get_unsigned(&opt.DPs, *argv, 10)) {
-				fprintf(stderr, "Illegal \"DPs\"\n");
+				fprintf(stderr, "Illegal \"vqs\"\n");
 				return -1;
 			} else if (opt.DPs > MAX_DPs) {
-				fprintf(stderr, "GRED: only %u DPs are "
+				fprintf(stderr, "GRED: only %u VQs are "
 					"currently supported\n", MAX_DPs);
 				return -1;
 			}
 		} else if (strcmp(*argv, "default") == 0) {
 			if (opt.DPs == 0) {
 				fprintf(stderr, "\"default\" must be defined "
-					"after \"DPs\"\n");
+					"after \"vqs\"\n");
 				return -1;
 			}
 			NEXT_ARG();
@@ -80,7 +78,7 @@ static int init_gred(struct qdisc_util *qu, int argc, char **argv,
 				return -1;
 			} else if (opt.def_DP >= opt.DPs) {
 				fprintf(stderr, "\"default\" must be less than "
-					"\"DPs\"\n");
+					"\"vqs\"\n");
 				return -1;
 			}
 		} else if (strcmp(*argv, "grio") == 0) {
@@ -155,13 +153,14 @@ static int gred_parse_opt(struct qdisc_util *qu, int argc, char **argv, struct n
 				return -1;
 			}
 			ok++;
-		} else if (strcmp(*argv, "DP") == 0) {
+		} else if (strcmp(*argv, "vq") == 0 ||
+			   strcmp(*argv, "DP") == 0) {
 			NEXT_ARG();
 			if (get_unsigned(&opt.DP, *argv, 10)) {
-				fprintf(stderr, "Illegal \"DP\"\n");
+				fprintf(stderr, "Illegal \"vq\"\n");
 				return -1;
 			} else if (opt.DP >= MAX_DPs) {
-				fprintf(stderr, "GRED: only %u DPs are "
+				fprintf(stderr, "GRED: only %u VQs are "
 					"currently supported\n", MAX_DPs);
 				return -1;
 			} /* need a better error check */
@@ -216,7 +215,7 @@ static int gred_parse_opt(struct qdisc_util *qu, int argc, char **argv, struct n
 	}
 	if (opt.DP == MAX_DPs || !opt.limit || !opt.qth_min || !opt.qth_max ||
 	    !avpkt) {
-		fprintf(stderr, "Required parameter (DP, limit, min, max, "
+		fprintf(stderr, "Required parameter (vq, limit, min, max, "
 			"avpkt) is missing\n");
 		return -1;
 	}
@@ -292,14 +291,14 @@ static int gred_print_opt(struct qdisc_util *qu, FILE *f, struct rtattr *opt)
 
 /* Bad hack! should really return a proper message as shown above*/
 
-	fprintf(f, "DPs %u default %u %s",
+	fprintf(f, "vqs %u default %u %s",
 		sopt->DPs,
 		sopt->def_DP,
 		sopt->grio ? "grio " : "");
 
 	for (i=0;i<MAX_DPs;i++, qopt++) {
 		if (qopt->DP >= MAX_DPs) continue;
-		fprintf(f, "\n DP %u prio %hhu limit %s min %s max %s ",
+		fprintf(f, "\n vq %u prio %hhu limit %s min %s max %s ",
 			qopt->DP,
 			qopt->prio,
 			sprint_size(qopt->limit, b1),
-- 
1.7.1

[PATCH iproute2/net-next] tc: gred: Add support for TCA_GRED_LIMIT attribute

[David Ward]

Allow the qdisc limit to be set, which is particularly useful when
the default VQ is not configured with RED parameters.

Signed-off-by: David Ward <david.ward@ll.mit.edu>
---
 include/linux/pkt_sched.h |    3 ++-
 tc/q_gred.c               |   20 +++++++++++++++++++-
 2 files changed, 21 insertions(+), 2 deletions(-)

diff --git a/include/linux/pkt_sched.h b/include/linux/pkt_sched.h
index 534b847..700f50a 100644
--- a/include/linux/pkt_sched.h
+++ b/include/linux/pkt_sched.h
@@ -268,7 +268,8 @@ enum {
        TCA_GRED_STAB,
        TCA_GRED_DPS,
        TCA_GRED_MAX_P,
-	   __TCA_GRED_MAX,
+       TCA_GRED_LIMIT,
+       __TCA_GRED_MAX,
 };
 
 #define TCA_GRED_MAX (__TCA_GRED_MAX - 1)
diff --git a/tc/q_gred.c b/tc/q_gred.c
index 463d725..f31daa3 100644
--- a/tc/q_gred.c
+++ b/tc/q_gred.c
@@ -38,7 +38,7 @@
 static void explain(void)
 {
 	fprintf(stderr, "Usage: tc qdisc { add | replace | change } ... gred setup vqs NUMBER\n");
-	fprintf(stderr, "           default DEFAULT_VQ [ grio ]\n");
+	fprintf(stderr, "           default DEFAULT_VQ [ grio ] [ limit BYTES ]\n");
 	fprintf(stderr, "       tc qdisc change ... gred vq VQ [ prio VALUE ] limit BYTES\n");
 	fprintf(stderr, "           min BYTES max BYTES avpkt BYTES [ burst PACKETS ]\n");
 	fprintf(stderr, "           [ probability PROBABILITY ] [ bandwidth KBPS ]\n");
@@ -50,6 +50,7 @@ static int init_gred(struct qdisc_util *qu, int argc, char **argv,
 
 	struct rtattr *tail;
 	struct tc_gred_sopt opt = { 0 };
+	__u32 limit = 0;
 
 	opt.def_DP = MAX_DPs;
 
@@ -83,6 +84,12 @@ static int init_gred(struct qdisc_util *qu, int argc, char **argv,
 			}
 		} else if (strcmp(*argv, "grio") == 0) {
 			opt.grio = 1;
+		} else if (strcmp(*argv, "limit") == 0) {
+			NEXT_ARG();
+			if (get_size(&limit, *argv)) {
+				fprintf(stderr, "Illegal \"limit\"\n");
+				return -1;
+			}
 		} else if (strcmp(*argv, "help") == 0) {
 			explain();
 			return -1;
@@ -104,6 +111,8 @@ static int init_gred(struct qdisc_util *qu, int argc, char **argv,
 	tail = NLMSG_TAIL(n);
 	addattr_l(n, 1024, TCA_OPTIONS, NULL, 0);
 	addattr_l(n, 1024, TCA_GRED_DPS, &opt, sizeof(struct tc_gred_sopt));
+	if (limit)
+		addattr32(n, 1024, TCA_GRED_LIMIT, limit);
 	tail->rta_len = (void *) NLMSG_TAIL(n) - (void *) tail;
 	return 0;
 }
@@ -264,6 +273,7 @@ static int gred_print_opt(struct qdisc_util *qu, FILE *f, struct rtattr *opt)
 	struct tc_gred_sopt *sopt;
 	struct tc_gred_qopt *qopt;
 	__u32 *max_p = NULL;
+	__u32 *limit = NULL;
 	unsigned i;
 	SPRINT_BUF(b1);
 	SPRINT_BUF(b2);
@@ -281,6 +291,10 @@ static int gred_print_opt(struct qdisc_util *qu, FILE *f, struct rtattr *opt)
 	    RTA_PAYLOAD(tb[TCA_GRED_MAX_P]) >= sizeof(__u32) * MAX_DPs)
 		max_p = RTA_DATA(tb[TCA_GRED_MAX_P]);
 
+	if (tb[TCA_GRED_LIMIT] &&
+	    RTA_PAYLOAD(tb[TCA_GRED_LIMIT]) == sizeof(__u32))
+		limit = RTA_DATA(tb[TCA_GRED_LIMIT]);
+
 	sopt = RTA_DATA(tb[TCA_GRED_DPS]);
 	qopt = RTA_DATA(tb[TCA_GRED_PARMS]);
 	if (RTA_PAYLOAD(tb[TCA_GRED_DPS]) < sizeof(*sopt) ||
@@ -296,6 +310,10 @@ static int gred_print_opt(struct qdisc_util *qu, FILE *f, struct rtattr *opt)
 		sopt->def_DP,
 		sopt->grio ? "grio " : "");
 
+	if (limit)
+		fprintf(f, "limit %s ",
+			sprint_size(*limit, b1));
+
 	for (i=0;i<MAX_DPs;i++, qopt++) {
 		if (qopt->DP >= MAX_DPs) continue;
 		fprintf(f, "\n vq %u prio %hhu limit %s min %s max %s ",
-- 
1.7.1