SFQ: backport some features from ESFQ (try 5)

SFQ: backport some features from ESFQ (try 5)

[Corey Hickey]

Patchset try 2 addresses the review by Michael Buesch.
Patchset try 3 addresses the review by Patrick McHardy.
Patchset try 4 has a few cosmetic improvements.
Patchset try 5 addresses further review by Patrick McHardy.

This set of patches is substantially the same as my previous try, with
changes made according to Patrick's recommendations.

Iproute2 patches will follow shortly.



The following is the original patch text.

This set of patches adds some of ESFQ's modifications to the original 
SFQ. Thus far, I have received support for this approach rather than for 
trying to get ESFQ included as a separate qdisc.

http://mailman.ds9a.nl/pipermail/lartc/2007q2/021056.html

My patches here implement "tc qdisc change", user-configurable depth 
(number of flows), and user-configurable divisor (for setting hash table 
size). I've left out the remaining ESFQ features (usage of jhash and 
different hashing methods) because Patrick McHardy intends to submit a 
patch that will supersede that functionality; see the URL above.

Default values remain the same, and SFQ's default behavior remains the 
same, so there should be no user disruption.

Thanks for your consideration,
Corey


 include/linux/pkt_sched.h |   23 ++-
 net/sched/sch_sfq.c       |  434 +++++++++++++++++++++++++++++++--------------
 2 files changed, 319 insertions(+), 138 deletions(-)


[PATCH 1/8] Preparatory refactoring part 1.
[PATCH 2/8] Preparatory refactoring part 2.
[PATCH 3/8] Make "depth" (number of queues) user-configurable
[PATCH 4/8] Add divisor.
[PATCH 5/8] Make qdisc changeable.
[PATCH 6/8] Remove comments about hardcoded values.
[PATCH 7/8] Rework perturb_period.
[PATCH 8/8] Use nested compat attributes to pass parameters.

[PATCH 1/8] Preparatory refactoring part 1.

[Corey Hickey]

Make a new function sfq_q_enqueue() that operates directly on the
queue data. This will be useful for implementing sfq_change() in
a later patch. A pleasant side-effect is reducing most of the
duplicate code in sfq_enqueue() and sfq_requeue().

Similarly, make a new function sfq_q_dequeue().

Signed-off-by: Corey Hickey <bugfood-ml@fatooh.org>
---
 net/sched/sch_sfq.c |  119 ++++++++++++++++++++++++++++++---------------------
 1 files changed, 70 insertions(+), 49 deletions(-)

diff --git a/net/sched/sch_sfq.c b/net/sched/sch_sfq.c
index b542c87..10e2f3d 100644
--- a/net/sched/sch_sfq.c
+++ b/net/sched/sch_sfq.c
@@ -78,6 +78,9 @@
 #define SFQ_DEPTH		128
 #define SFQ_HASH_DIVISOR	1024
 
+#define SFQ_HEAD 0
+#define SFQ_TAIL 1
+
 /* This type should contain at least SFQ_DEPTH*2 values */
 typedef unsigned char sfq_index;
 
@@ -241,9 +244,8 @@ static unsigned int sfq_drop(struct Qdisc *sch)
 }
 
 static int
-sfq_enqueue(struct sk_buff *skb, struct Qdisc* sch)
+sfq_q_enqueue(struct sk_buff *skb, struct sfq_sched_data *q, int end)
 {
-	struct sfq_sched_data *q = qdisc_priv(sch);
 	unsigned hash = sfq_hash(q, skb);
 	sfq_index x;
 
@@ -252,15 +254,37 @@ sfq_enqueue(struct sk_buff *skb, struct Qdisc* sch)
 		q->ht[hash] = x = q->dep[SFQ_DEPTH].next;
 		q->hash[x] = hash;
 	}
-	/* If selected queue has length q->limit, this means that
-	 * all another queues are empty and that we do simple tail drop,
-	 * i.e. drop _this_ packet.
-	 */
-	if (q->qs[x].qlen >= q->limit)
-		return qdisc_drop(skb, sch);
 
-	sch->qstats.backlog += skb->len;
-	__skb_queue_tail(&q->qs[x], skb);
+	if (end == SFQ_TAIL) {
+		/* If selected queue has length q->limit, this means that
+		 * all other queues are empty and that we do simple tail drop,
+		 * i.e. drop _this_ packet.
+		 */
+		if (q->qs[x].qlen >= q->limit) {
+			unsigned int drop_len = skb->len;
+
+			kfree_skb(skb);
+			return drop_len;
+		}
+		__skb_queue_tail(&q->qs[x], skb);
+	} else { /* end == SFQ_HEAD */
+		__skb_queue_head(&q->qs[x], skb);
+		/* If selected queue has length q->limit+1, this means that
+		 * all other queues are empty and we do simple tail drop.
+		 * This packet is still requeued at head of queue, tail packet
+		 * is dropped.
+		 */
+		if (q->qs[x].qlen > q->limit) {
+			unsigned int drop_len;
+
+			skb = q->qs[x].prev;
+			drop_len = skb->len;
+			__skb_unlink(skb, &q->qs[x]);
+			kfree_skb(skb);
+			return drop_len;
+		}
+	}
+
 	sfq_inc(q, x);
 	if (q->qs[x].qlen == 1) {		/* The flow is new */
 		if (q->tail == SFQ_DEPTH) {	/* It is the first flow */
@@ -273,6 +297,21 @@ sfq_enqueue(struct sk_buff *skb, struct Qdisc* sch)
 			q->tail = x;
 		}
 	}
+
+	return 0;
+}
+
+static int
+sfq_enqueue(struct sk_buff *skb, struct Qdisc* sch)
+{
+	struct sfq_sched_data *q = qdisc_priv(sch);
+
+	if (sfq_q_enqueue(skb, q, SFQ_TAIL)) {
+		sch->qstats.drops++;
+		return NET_XMIT_DROP;
+	}
+
+	sch->qstats.backlog += skb->len;
 	if (++sch->q.qlen <= q->limit) {
 		sch->bstats.bytes += skb->len;
 		sch->bstats.packets++;
@@ -287,58 +326,27 @@ static int
 sfq_requeue(struct sk_buff *skb, struct Qdisc* sch)
 {
 	struct sfq_sched_data *q = qdisc_priv(sch);
-	unsigned hash = sfq_hash(q, skb);
-	sfq_index x;
+	unsigned int drop_len;
 
-	x = q->ht[hash];
-	if (x == SFQ_DEPTH) {
-		q->ht[hash] = x = q->dep[SFQ_DEPTH].next;
-		q->hash[x] = hash;
-	}
 	sch->qstats.backlog += skb->len;
-	__skb_queue_head(&q->qs[x], skb);
-	/* If selected queue has length q->limit+1, this means that
-	 * all another queues are empty and we do simple tail drop.
-	 * This packet is still requeued at head of queue, tail packet
-	 * is dropped.
-	 */
-	if (q->qs[x].qlen > q->limit) {
-		skb = q->qs[x].prev;
-		__skb_unlink(skb, &q->qs[x]);
+	if ((drop_len = sfq_q_enqueue(skb, q, SFQ_HEAD))) {
+		sch->qstats.backlog -= drop_len;
 		sch->qstats.drops++;
-		sch->qstats.backlog -= skb->len;
-		kfree_skb(skb);
 		return NET_XMIT_CN;
 	}
-	sfq_inc(q, x);
-	if (q->qs[x].qlen == 1) {		/* The flow is new */
-		if (q->tail == SFQ_DEPTH) {	/* It is the first flow */
-			q->tail = x;
-			q->next[x] = x;
-			q->allot[x] = q->quantum;
-		} else {
-			q->next[x] = q->next[q->tail];
-			q->next[q->tail] = x;
-			q->tail = x;
-		}
-	}
+
 	if (++sch->q.qlen <= q->limit) {
 		sch->qstats.requeues++;
 		return 0;
 	}
 
-	sch->qstats.drops++;
 	sfq_drop(sch);
 	return NET_XMIT_CN;
 }
 
-
-
-
-static struct sk_buff *
-sfq_dequeue(struct Qdisc* sch)
+static struct
+sk_buff *sfq_q_dequeue(struct sfq_sched_data *q)
 {
-	struct sfq_sched_data *q = qdisc_priv(sch);
 	struct sk_buff *skb;
 	sfq_index a, old_a;
 
@@ -351,8 +359,6 @@ sfq_dequeue(struct Qdisc* sch)
 	/* Grab packet */
 	skb = __skb_dequeue(&q->qs[a]);
 	sfq_dec(q, a);
-	sch->q.qlen--;
-	sch->qstats.backlog -= skb->len;
 
 	/* Is the slot empty? */
 	if (q->qs[a].qlen == 0) {
@@ -369,6 +375,21 @@ sfq_dequeue(struct Qdisc* sch)
 		a = q->next[a];
 		q->allot[a] += q->quantum;
 	}
+
+	return skb;
+}
+
+static struct sk_buff
+*sfq_dequeue(struct Qdisc* sch)
+{
+	struct sfq_sched_data *q = qdisc_priv(sch);
+	struct sk_buff *skb;
+
+	skb = sfq_q_dequeue(q);
+	if (skb == NULL)
+		return NULL;
+	sch->q.qlen--;
+	sch->qstats.backlog -= skb->len;
 	return skb;
 }
 
-- 
1.5.3.4

[PATCH 2/8] Preparatory refactoring part 2.

[Corey Hickey]

Factor code out of sfq_init() so that the new function can be used
by sfq_change() later.

Actually, as the diff itself shows, most of the sfq_q_init() code
comes from the original sfq_change(), but sfq_change() is only
called by sfq_init() right now. Thus, it is safe to remove
sfq_change(); "tc qdisc change" doesn't yet work for sfq anyway.

Setting default parameters is moved into a separate function for
clarity.

Signed-off-by: Corey Hickey <bugfood-ml@fatooh.org>
---
 net/sched/sch_sfq.c |   88 +++++++++++++++++++++++++++-----------------------
 1 files changed, 47 insertions(+), 41 deletions(-)

diff --git a/net/sched/sch_sfq.c b/net/sched/sch_sfq.c
index 10e2f3d..8ea816a 100644
--- a/net/sched/sch_sfq.c
+++ b/net/sched/sch_sfq.c
@@ -413,43 +413,41 @@ static void sfq_perturbation(unsigned long arg)
 		mod_timer(&q->perturb_timer, jiffies + q->perturb_period);
 }
 
-static int sfq_change(struct Qdisc *sch, struct rtattr *opt)
+static void
+sfq_default_parameters(struct Qdisc *sch)
 {
 	struct sfq_sched_data *q = qdisc_priv(sch);
-	struct tc_sfq_qopt *ctl = RTA_DATA(opt);
-	unsigned int qlen;
-
-	if (opt->rta_len < RTA_LENGTH(sizeof(*ctl)))
-		return -EINVAL;
-
-	sch_tree_lock(sch);
-	q->quantum = ctl->quantum ? : psched_mtu(sch->dev);
-	q->perturb_period = ctl->perturb_period*HZ;
-	if (ctl->limit)
-		q->limit = min_t(u32, ctl->limit, SFQ_DEPTH - 1);
 
-	qlen = sch->q.qlen;
-	while (sch->q.qlen > q->limit)
-		sfq_drop(sch);
-	qdisc_tree_decrease_qlen(sch, qlen - sch->q.qlen);
-
-	del_timer(&q->perturb_timer);
-	if (q->perturb_period) {
-		mod_timer(&q->perturb_timer, jiffies + q->perturb_period);
-		get_random_bytes(&q->perturbation, 4);
-	}
-	sch_tree_unlock(sch);
-	return 0;
+	q->quantum        = psched_mtu(sch->dev);
+	q->perturbation   = 0;
+	q->perturb_period = 0;
+	q->limit          = SFQ_DEPTH - 1;
 }
 
-static int sfq_init(struct Qdisc *sch, struct rtattr *opt)
+static int
+sfq_q_init(struct sfq_sched_data *q, struct rtattr *opt)
 {
-	struct sfq_sched_data *q = qdisc_priv(sch);
 	int i;
 
-	init_timer(&q->perturb_timer);
-	q->perturb_timer.data = (unsigned long)sch;
-	q->perturb_timer.function = sfq_perturbation;
+	/* At this point, parameters are set to either defaults (sfq_init) or
+	 * the previous values (sfq_change). So, overwrite the parameters as
+	 * specified. */
+	if (opt) {
+		struct tc_sfq_qopt *ctl = RTA_DATA(opt);
+
+		if (opt->rta_len < RTA_LENGTH(sizeof(*ctl)))
+			return -EINVAL;
+
+		if (ctl->quantum)
+			q->quantum = ctl->quantum;
+		if (ctl->perturb_period)
+			q->perturb_period = ctl->perturb_period * HZ;
+		if (ctl->limit)
+			q->limit = ctl->limit;
+	}
+	q->limit = min_t(u32, q->limit, SFQ_DEPTH - 1);
+	q->tail = SFQ_DEPTH;
+	q->max_depth = 0;
 
 	for (i=0; i<SFQ_HASH_DIVISOR; i++)
 		q->ht[i] = SFQ_DEPTH;
@@ -458,23 +456,31 @@ static int sfq_init(struct Qdisc *sch, struct rtattr *opt)
 		q->dep[i+SFQ_DEPTH].next = i+SFQ_DEPTH;
 		q->dep[i+SFQ_DEPTH].prev = i+SFQ_DEPTH;
 	}
-	q->limit = SFQ_DEPTH - 1;
-	q->max_depth = 0;
-	q->tail = SFQ_DEPTH;
-	if (opt == NULL) {
-		q->quantum = psched_mtu(sch->dev);
-		q->perturb_period = 0;
-		get_random_bytes(&q->perturbation, 4);
-	} else {
-		int err = sfq_change(sch, opt);
-		if (err)
-			return err;
-	}
 	for (i=0; i<SFQ_DEPTH; i++)
 		sfq_link(q, i);
 	return 0;
 }
 
+static int sfq_init(struct Qdisc *sch, struct rtattr *opt)
+{
+	struct sfq_sched_data *q = qdisc_priv(sch);
+	int err;
+
+	sfq_default_parameters(sch);
+	if ((err = sfq_q_init(q, opt)))
+		return err;
+
+	init_timer(&q->perturb_timer);
+	q->perturb_timer.data = (unsigned long)sch;
+	q->perturb_timer.function = sfq_perturbation;
+	if (q->perturb_period) {
+		q->perturb_timer.expires = jiffies + q->perturb_period;
+		add_timer(&q->perturb_timer);
+	}
+
+	return 0;
+}
+
 static void sfq_destroy(struct Qdisc *sch)
 {
 	struct sfq_sched_data *q = qdisc_priv(sch);
-- 
1.5.3.4

[PATCH 3/8] Make "depth" (number of queues) user-configurable

[Corey Hickey]

* replace #define with a parameter
* use old hardcoded value as a default
* kcalloc() arrays in sfq_q_init()
* free() arrays in new function sfq_q_destroy()
* move sfq_destroy() to near sfq_q_destroy(), for clarity

Signed-off-by: Corey Hickey <bugfood-ml@fatooh.org>
---
 net/sched/sch_sfq.c |  104 +++++++++++++++++++++++++++++++++++----------------
 1 files changed, 72 insertions(+), 32 deletions(-)

diff --git a/net/sched/sch_sfq.c b/net/sched/sch_sfq.c
index 8ea816a..1c1bf08 100644
--- a/net/sched/sch_sfq.c
+++ b/net/sched/sch_sfq.c
@@ -75,14 +75,16 @@
 
 	It is easy to increase these values, but not in flight.  */
 
-#define SFQ_DEPTH		128
+#define SFQ_DEPTH_DEFAULT	128
 #define SFQ_HASH_DIVISOR	1024
 
 #define SFQ_HEAD 0
 #define SFQ_TAIL 1
 
-/* This type should contain at least SFQ_DEPTH*2 values */
-typedef unsigned char sfq_index;
+/* This type must contain greater than depth*2 values, so depth is constrained 
+ * accordingly. */
+typedef unsigned int sfq_index;
+#define SFQ_MAX_DEPTH (UINT_MAX / 2 - 1)
 
 struct sfq_head
 {
@@ -96,6 +98,7 @@ struct sfq_sched_data
 	int		perturb_period;
 	unsigned	quantum;	/* Allotment per round: MUST BE >= MTU */
 	int		limit;
+	unsigned	depth;
 
 /* Variables */
 	struct timer_list perturb_timer;
@@ -104,11 +107,11 @@ struct sfq_sched_data
 	sfq_index	max_depth;	/* Maximal depth */
 
 	sfq_index	ht[SFQ_HASH_DIVISOR];	/* Hash table */
-	sfq_index	next[SFQ_DEPTH];	/* Active slots link */
-	short		allot[SFQ_DEPTH];	/* Current allotment per slot */
-	unsigned short	hash[SFQ_DEPTH];	/* Hash value indexed by slots */
-	struct sk_buff_head	qs[SFQ_DEPTH];		/* Slot queue */
-	struct sfq_head	dep[SFQ_DEPTH*2];	/* Linked list of slots, indexed by depth */
+	sfq_index	*next;			/* Active slots link */
+	short		*allot;			/* Current allotment per slot */
+	unsigned short	*hash;			/* Hash value indexed by slots */
+	struct sk_buff_head	*qs;		/* Slot queue */
+	struct sfq_head	*dep;			/* Linked list of slots, indexed by depth */
 };
 
 static __inline__ unsigned sfq_fold_hash(struct sfq_sched_data *q, u32 h, u32 h1)
@@ -160,7 +163,7 @@ static unsigned sfq_hash(struct sfq_sched_data *q, struct sk_buff *skb)
 static inline void sfq_link(struct sfq_sched_data *q, sfq_index x)
 {
 	sfq_index p, n;
-	int d = q->qs[x].qlen + SFQ_DEPTH;
+	int d = q->qs[x].qlen + q->depth;
 
 	p = d;
 	n = q->dep[d].next;
@@ -211,7 +214,7 @@ static unsigned int sfq_drop(struct Qdisc *sch)
 	   drop a packet from it */
 
 	if (d > 1) {
-		sfq_index x = q->dep[d+SFQ_DEPTH].next;
+		sfq_index x = q->dep[d + q->depth].next;
 		skb = q->qs[x].prev;
 		len = skb->len;
 		__skb_unlink(skb, &q->qs[x]);
@@ -234,7 +237,7 @@ static unsigned int sfq_drop(struct Qdisc *sch)
 		kfree_skb(skb);
 		sfq_dec(q, d);
 		sch->q.qlen--;
-		q->ht[q->hash[d]] = SFQ_DEPTH;
+		q->ht[q->hash[d]] = q->depth;
 		sch->qstats.drops++;
 		sch->qstats.backlog -= len;
 		return len;
@@ -250,8 +253,8 @@ sfq_q_enqueue(struct sk_buff *skb, struct sfq_sched_data *q, int end)
 	sfq_index x;
 
 	x = q->ht[hash];
-	if (x == SFQ_DEPTH) {
-		q->ht[hash] = x = q->dep[SFQ_DEPTH].next;
+	if (x == q->depth) {
+		q->ht[hash] = x = q->dep[q->depth].next;
 		q->hash[x] = hash;
 	}
 
@@ -287,7 +290,7 @@ sfq_q_enqueue(struct sk_buff *skb, struct sfq_sched_data *q, int end)
 
 	sfq_inc(q, x);
 	if (q->qs[x].qlen == 1) {		/* The flow is new */
-		if (q->tail == SFQ_DEPTH) {	/* It is the first flow */
+		if (q->tail == q->depth) {	/* It is the first flow */
 			q->tail = x;
 			q->next[x] = x;
 			q->allot[x] = q->quantum;
@@ -351,7 +354,7 @@ sk_buff *sfq_q_dequeue(struct sfq_sched_data *q)
 	sfq_index a, old_a;
 
 	/* No active slots */
-	if (q->tail == SFQ_DEPTH)
+	if (q->tail == q->depth)
 		return NULL;
 
 	a = old_a = q->next[q->tail];
@@ -362,10 +365,10 @@ sk_buff *sfq_q_dequeue(struct sfq_sched_data *q)
 
 	/* Is the slot empty? */
 	if (q->qs[a].qlen == 0) {
-		q->ht[q->hash[a]] = SFQ_DEPTH;
+		q->ht[q->hash[a]] = q->depth;
 		a = q->next[a];
 		if (a == old_a) {
-			q->tail = SFQ_DEPTH;
+			q->tail = q->depth;
 			return skb;
 		}
 		q->next[q->tail] = a;
@@ -413,6 +416,23 @@ static void sfq_perturbation(unsigned long arg)
 		mod_timer(&q->perturb_timer, jiffies + q->perturb_period);
 }
 
+static void sfq_q_destroy(struct sfq_sched_data *q)
+{
+	kfree(q->dep);
+	kfree(q->next);
+	kfree(q->allot);
+	kfree(q->hash);
+	kfree(q->qs);
+}
+
+static void sfq_destroy(struct Qdisc *sch)
+{
+	struct sfq_sched_data *q = qdisc_priv(sch);
+
+	del_timer(&q->perturb_timer);
+	sfq_q_destroy(q);
+}
+
 static void
 sfq_default_parameters(struct Qdisc *sch)
 {
@@ -421,7 +441,8 @@ sfq_default_parameters(struct Qdisc *sch)
 	q->quantum        = psched_mtu(sch->dev);
 	q->perturbation   = 0;
 	q->perturb_period = 0;
-	q->limit          = SFQ_DEPTH - 1;
+	q->depth          = SFQ_DEPTH_DEFAULT;
+	q->limit          = SFQ_DEPTH_DEFAULT - 1;
 }
 
 static int
@@ -442,23 +463,48 @@ sfq_q_init(struct sfq_sched_data *q, struct rtattr *opt)
 			q->quantum = ctl->quantum;
 		if (ctl->perturb_period)
 			q->perturb_period = ctl->perturb_period * HZ;
+		if (ctl->flows)
+			q->depth = ctl->flows;
 		if (ctl->limit)
 			q->limit = ctl->limit;
+
+		if (q->depth > SFQ_MAX_DEPTH)
+			return -EINVAL;
 	}
-	q->limit = min_t(u32, q->limit, SFQ_DEPTH - 1);
-	q->tail = SFQ_DEPTH;
+	q->limit = min_t(u32, q->limit, q->depth - 1);
+	q->tail = q->depth;
 	q->max_depth = 0;
 
+	q->dep = kcalloc(1 + q->depth*2, sizeof(struct sfq_head), GFP_KERNEL);
+	if (!q->dep)
+		goto err_case;
+	q->next = kcalloc(q->depth, sizeof(sfq_index), GFP_KERNEL);
+	if (!q->next)
+		goto err_case;
+	q->allot = kcalloc(q->depth, sizeof(short), GFP_KERNEL);
+	if (!q->allot)
+		goto err_case;
+	q->hash = kcalloc(q->depth, sizeof(unsigned short), GFP_KERNEL);
+	if (!q->hash)
+		goto err_case;
+	q->qs = kcalloc(q->depth, sizeof(struct sk_buff_head), GFP_KERNEL);
+	if (!q->qs)
+		goto err_case;
+
 	for (i=0; i<SFQ_HASH_DIVISOR; i++)
-		q->ht[i] = SFQ_DEPTH;
-	for (i=0; i<SFQ_DEPTH; i++) {
+		q->ht[i] = q->depth;
+	for (i=0; i < q->depth; i++) {
 		skb_queue_head_init(&q->qs[i]);
-		q->dep[i+SFQ_DEPTH].next = i+SFQ_DEPTH;
-		q->dep[i+SFQ_DEPTH].prev = i+SFQ_DEPTH;
+		q->dep[i + q->depth].next = i + q->depth;
+		q->dep[i + q->depth].prev = i + q->depth;
 	}
-	for (i=0; i<SFQ_DEPTH; i++)
+
+	for (i=0; i < q->depth; i++)
 		sfq_link(q, i);
 	return 0;
+err_case:
+	sfq_q_destroy(q);
+	return -ENOBUFS;
 }
 
 static int sfq_init(struct Qdisc *sch, struct rtattr *opt)
@@ -481,12 +527,6 @@ static int sfq_init(struct Qdisc *sch, struct rtattr *opt)
 	return 0;
 }
 
-static void sfq_destroy(struct Qdisc *sch)
-{
-	struct sfq_sched_data *q = qdisc_priv(sch);
-	del_timer(&q->perturb_timer);
-}
-
 static int sfq_dump(struct Qdisc *sch, struct sk_buff *skb)
 {
 	struct sfq_sched_data *q = qdisc_priv(sch);
@@ -498,7 +538,7 @@ static int sfq_dump(struct Qdisc *sch, struct sk_buff *skb)
 
 	opt.limit = q->limit;
 	opt.divisor = SFQ_HASH_DIVISOR;
-	opt.flows = q->limit;
+	opt.flows = q->depth;
 
 	RTA_PUT(skb, TCA_OPTIONS, sizeof(opt), &opt);
 
-- 
1.5.3.4

[PATCH 4/8] Add divisor.

[Corey Hickey]

Make hash divisor user-configurable.

Signed-off-by: Corey Hickey <bugfood-ml@fatooh.org>
---
 net/sched/sch_sfq.c |   27 +++++++++++++++++++++------
 1 files changed, 21 insertions(+), 6 deletions(-)

diff --git a/net/sched/sch_sfq.c b/net/sched/sch_sfq.c
index 1c1bf08..c74d5ce 100644
--- a/net/sched/sch_sfq.c
+++ b/net/sched/sch_sfq.c
@@ -76,7 +76,7 @@
 	It is easy to increase these values, but not in flight.  */
 
 #define SFQ_DEPTH_DEFAULT	128
-#define SFQ_HASH_DIVISOR	1024
+#define SFQ_DIVISOR_DEFAULT	10
 
 #define SFQ_HEAD 0
 #define SFQ_TAIL 1
@@ -86,6 +86,10 @@
 typedef unsigned int sfq_index;
 #define SFQ_MAX_DEPTH (UINT_MAX / 2 - 1)
 
+/* In practice, the actual divisor size is limited by kcalloc, but we still
+ * don't want to left shift by more than 31. */
+#define SFQ_MAX_DIVISOR 31
+
 struct sfq_head
 {
 	sfq_index	next;
@@ -99,6 +103,7 @@ struct sfq_sched_data
 	unsigned	quantum;	/* Allotment per round: MUST BE >= MTU */
 	int		limit;
 	unsigned	depth;
+	unsigned	hash_divisor;
 
 /* Variables */
 	struct timer_list perturb_timer;
@@ -106,7 +111,7 @@ struct sfq_sched_data
 	sfq_index	tail;		/* Index of current slot in round */
 	sfq_index	max_depth;	/* Maximal depth */
 
-	sfq_index	ht[SFQ_HASH_DIVISOR];	/* Hash table */
+	sfq_index	*ht;			/* Hash table */
 	sfq_index	*next;			/* Active slots link */
 	short		*allot;			/* Current allotment per slot */
 	unsigned short	*hash;			/* Hash value indexed by slots */
@@ -116,7 +121,9 @@ struct sfq_sched_data
 
 static __inline__ unsigned sfq_fold_hash(struct sfq_sched_data *q, u32 h, u32 h1)
 {
-	return jhash_2words(h, h1, q->perturbation) & (SFQ_HASH_DIVISOR - 1);
+	unsigned mask = (1<<q->hash_divisor) - 1;
+
+	return jhash_2words(h, h1, q->perturbation) & mask;
 }
 
 static unsigned sfq_hash(struct sfq_sched_data *q, struct sk_buff *skb)
@@ -418,6 +425,7 @@ static void sfq_perturbation(unsigned long arg)
 
 static void sfq_q_destroy(struct sfq_sched_data *q)
 {
+	kfree(q->ht);
 	kfree(q->dep);
 	kfree(q->next);
 	kfree(q->allot);
@@ -441,6 +449,7 @@ sfq_default_parameters(struct Qdisc *sch)
 	q->quantum        = psched_mtu(sch->dev);
 	q->perturbation   = 0;
 	q->perturb_period = 0;
+	q->hash_divisor   = SFQ_DIVISOR_DEFAULT;
 	q->depth          = SFQ_DEPTH_DEFAULT;
 	q->limit          = SFQ_DEPTH_DEFAULT - 1;
 }
@@ -463,18 +472,24 @@ sfq_q_init(struct sfq_sched_data *q, struct rtattr *opt)
 			q->quantum = ctl->quantum;
 		if (ctl->perturb_period)
 			q->perturb_period = ctl->perturb_period * HZ;
+		if (ctl->divisor)
+			q->hash_divisor = ctl->divisor;
 		if (ctl->flows)
 			q->depth = ctl->flows;
 		if (ctl->limit)
 			q->limit = ctl->limit;
 
-		if (q->depth > SFQ_MAX_DEPTH)
+		if (q->depth        > SFQ_MAX_DEPTH ||
+		    q->hash_divisor > SFQ_MAX_DIVISOR)
 			return -EINVAL;
 	}
 	q->limit = min_t(u32, q->limit, q->depth - 1);
 	q->tail = q->depth;
 	q->max_depth = 0;
 
+	q->ht = kcalloc(1<<q->hash_divisor, sizeof(sfq_index), GFP_KERNEL);
+	if (!q->ht)
+		goto err_case;
 	q->dep = kcalloc(1 + q->depth*2, sizeof(struct sfq_head), GFP_KERNEL);
 	if (!q->dep)
 		goto err_case;
@@ -491,7 +506,7 @@ sfq_q_init(struct sfq_sched_data *q, struct rtattr *opt)
 	if (!q->qs)
 		goto err_case;
 
-	for (i=0; i<SFQ_HASH_DIVISOR; i++)
+	for (i=0; i < 1<<q->hash_divisor; i++)
 		q->ht[i] = q->depth;
 	for (i=0; i < q->depth; i++) {
 		skb_queue_head_init(&q->qs[i]);
@@ -537,7 +552,7 @@ static int sfq_dump(struct Qdisc *sch, struct sk_buff *skb)
 	opt.perturb_period = q->perturb_period/HZ;
 
 	opt.limit = q->limit;
-	opt.divisor = SFQ_HASH_DIVISOR;
+	opt.divisor = q->hash_divisor;
 	opt.flows = q->depth;
 
 	RTA_PUT(skb, TCA_OPTIONS, sizeof(opt), &opt);
-- 
1.5.3.4

[PATCH 5/8] Make qdisc changeable.

[Corey Hickey]

Re-implement sfq_change() and enable Qdisc_opts.change so "tc qdisc
change" will work.

Signed-off-by: Corey Hickey <bugfood-ml@fatooh.org>
---
 net/sched/sch_sfq.c |   67 ++++++++++++++++++++++++++++++++++++++++++++++++++-
 1 files changed, 66 insertions(+), 1 deletions(-)

diff --git a/net/sched/sch_sfq.c b/net/sched/sch_sfq.c
index c74d5ce..ca8716f 100644
--- a/net/sched/sch_sfq.c
+++ b/net/sched/sch_sfq.c
@@ -454,6 +454,17 @@ sfq_default_parameters(struct Qdisc *sch)
 	q->limit          = SFQ_DEPTH_DEFAULT - 1;
 }
 
+static void
+sfq_copy_parameters(struct sfq_sched_data *dst, struct sfq_sched_data *src)
+{
+	dst->quantum        = src->quantum;
+	dst->perturbation   = src->perturbation;
+	dst->perturb_period = src->perturb_period;
+	dst->hash_divisor   = src->hash_divisor;
+	dst->limit          = src->limit;
+	dst->depth          = src->depth;
+}
+
 static int
 sfq_q_init(struct sfq_sched_data *q, struct rtattr *opt)
 {
@@ -542,6 +553,60 @@ static int sfq_init(struct Qdisc *sch, struct rtattr *opt)
 	return 0;
 }
 
+static int sfq_change(struct Qdisc *sch, struct rtattr *opt)
+{
+	struct sfq_sched_data *q = qdisc_priv(sch);
+	struct sfq_sched_data tmp;
+	struct sk_buff *skb;
+	unsigned int qlen;
+	int err;
+
+	/* set up tmp queue */
+	memset(&tmp, 0, sizeof(struct sfq_sched_data));
+	sfq_copy_parameters(&tmp, q);
+	if ((err = sfq_q_init(&tmp, opt)))
+		return err;
+
+	/* handle perturbation */
+	/* This code avoids resetting the perturb_timer unless perturb_period
+	 * is changed. Note that the rest of this function leaves
+	 * q->perturb_timer alone, whereas all other members of q get
+	 * overwritten from tmp. */
+	if (!tmp.perturb_period) {
+		tmp.perturbation = 0;
+		del_timer(&q->perturb_timer);
+	} else if (tmp.perturb_period != q->perturb_period) {
+		mod_timer(&q->perturb_timer, jiffies + tmp.perturb_period);
+	}
+
+	/* move packets from the old queue to the tmp queue */
+	sch_tree_lock(sch);
+	qlen = sch->q.qlen;
+	while (sch->q.qlen >= tmp.limit - 1)
+		sfq_drop(sch);
+	qdisc_tree_decrease_qlen(sch, qlen - sch->q.qlen);
+	while ((skb = sfq_q_dequeue(q)) != NULL)
+		sfq_q_enqueue(skb, &tmp, SFQ_TAIL);
+
+	/* clean up the old queue */
+	sfq_q_destroy(q);
+
+	/* copy elements of the tmp queue into the old queue */
+	sfq_copy_parameters(q, &tmp);
+	q->tail      = tmp.tail;
+	q->max_depth = tmp.max_depth;
+	q->ht        = tmp.ht;
+	q->dep       = tmp.dep;
+	q->next      = tmp.next;
+	q->allot     = tmp.allot;
+	q->hash      = tmp.hash;
+	q->qs        = tmp.qs;
+
+	/* finish up */
+	sch_tree_unlock(sch);
+	return 0;
+}
+
 static int sfq_dump(struct Qdisc *sch, struct sk_buff *skb)
 {
 	struct sfq_sched_data *q = qdisc_priv(sch);
@@ -576,7 +641,7 @@ static struct Qdisc_ops sfq_qdisc_ops = {
 	.init		=	sfq_init,
 	.reset		=	sfq_reset,
 	.destroy	=	sfq_destroy,
-	.change		=	NULL,
+	.change		=	sfq_change,
 	.dump		=	sfq_dump,
 	.owner		=	THIS_MODULE,
 };
-- 
1.5.3.4

[PATCH 6/8] Remove comments about hardcoded values.

[Corey Hickey]

None of these are true anymore (hooray!).

Signed-off-by: Corey Hickey <bugfood-ml@fatooh.org>
---
 include/linux/pkt_sched.h |    8 --------
 net/sched/sch_sfq.c       |   13 +------------
 2 files changed, 1 insertions(+), 20 deletions(-)

diff --git a/include/linux/pkt_sched.h b/include/linux/pkt_sched.h
index 919af93..d754a3d 100644
--- a/include/linux/pkt_sched.h
+++ b/include/linux/pkt_sched.h
@@ -148,14 +148,6 @@ struct tc_sfq_qopt
 	unsigned	flows;		/* Maximal number of flows  */
 };
 
-/*
- *  NOTE: limit, divisor and flows are hardwired to code at the moment.
- *
- *	limit=flows=128, divisor=1024;
- *
- *	The only reason for this is efficiency, it is possible
- *	to change these parameters in compile time.
- */
 
 /* RED section */
 
diff --git a/net/sched/sch_sfq.c b/net/sched/sch_sfq.c
index ca8716f..7b11086 100644
--- a/net/sched/sch_sfq.c
+++ b/net/sched/sch_sfq.c
@@ -62,18 +62,7 @@
 
 	We still need true WFQ for top level CSZ, but using WFQ
 	for the best effort traffic is absolutely pointless:
-	SFQ is superior for this purpose.
-
-	IMPLEMENTATION:
-	This implementation limits maximal queue length to 128;
-	maximal mtu to 2^15-1; number of hash buckets to 1024.
-	The only goal of this restrictions was that all data
-	fit into one 4K page :-). Struct sfq_sched_data is
-	organized in anti-cache manner: all the data for a bucket
-	are scattered over different locations. This is not good,
-	but it allowed me to put it into 4K.
-
-	It is easy to increase these values, but not in flight.  */
+	SFQ is superior for this purpose. */
 
 #define SFQ_DEPTH_DEFAULT	128
 #define SFQ_DIVISOR_DEFAULT	10
-- 
1.5.3.4

[PATCH 7/8] Rework perturb_period.

[Corey Hickey]

perturb_period is the only parameter that doesn't match 1:1 with the
value from userspace. Multiplying perturb_period by HZ when used rather
than when assigned makes it easy and clean to use a small function for
setting parameters (in a subsequent patch).

perturb_period is currently a signed integer, but I can't see any good
reason why this is so--a negative perturbation period will add a timer
that expires in the past, causing constant perturbation, which makes
hashing useless.

Strictly speaking, this will break binary compatibility with older
versions of tc, but that ought not to be a problem because (a) there's
no valid use for a negative perturb_period, and (b) negative values
will be seen as high values (> INT_MAX), which don't work anyway.

Signed-off-by: Corey Hickey <bugfood-ml@fatooh.org>
---
 include/linux/pkt_sched.h |    2 +-
 net/sched/sch_sfq.c       |   14 ++++++++------
 2 files changed, 9 insertions(+), 7 deletions(-)

diff --git a/include/linux/pkt_sched.h b/include/linux/pkt_sched.h
index d754a3d..14a08ad 100644
--- a/include/linux/pkt_sched.h
+++ b/include/linux/pkt_sched.h
@@ -142,7 +142,7 @@ enum
 struct tc_sfq_qopt
 {
 	unsigned	quantum;	/* Bytes per round allocated to flow */
-	int		perturb_period;	/* Period of hash perturbation */
+	unsigned	perturb_period;	/* Period of hash perturbation */
 	__u32		limit;		/* Maximal packets in queue */
 	unsigned	divisor;	/* Hash divisor  */
 	unsigned	flows;		/* Maximal number of flows  */
diff --git a/net/sched/sch_sfq.c b/net/sched/sch_sfq.c
index 7b11086..2764a54 100644
--- a/net/sched/sch_sfq.c
+++ b/net/sched/sch_sfq.c
@@ -70,6 +70,8 @@
 #define SFQ_HEAD 0
 #define SFQ_TAIL 1
 
+#define SFQ_PERTURB(period) (jiffies + (unsigned long)period * HZ)
+
 /* This type must contain greater than depth*2 values, so depth is constrained 
  * accordingly. */
 typedef unsigned int sfq_index;
@@ -88,7 +90,7 @@ struct sfq_head
 struct sfq_sched_data
 {
 /* Parameters */
-	int		perturb_period;
+	unsigned	perturb_period;
 	unsigned	quantum;	/* Allotment per round: MUST BE >= MTU */
 	int		limit;
 	unsigned	depth;
@@ -409,7 +411,7 @@ static void sfq_perturbation(unsigned long arg)
 	get_random_bytes(&q->perturbation, 4);
 
 	if (q->perturb_period)
-		mod_timer(&q->perturb_timer, jiffies + q->perturb_period);
+		mod_timer(&q->perturb_timer, SFQ_PERTURB(q->perturb_period));
 }
 
 static void sfq_q_destroy(struct sfq_sched_data *q)
@@ -471,7 +473,7 @@ sfq_q_init(struct sfq_sched_data *q, struct rtattr *opt)
 		if (ctl->quantum)
 			q->quantum = ctl->quantum;
 		if (ctl->perturb_period)
-			q->perturb_period = ctl->perturb_period * HZ;
+			q->perturb_period = ctl->perturb_period;
 		if (ctl->divisor)
 			q->hash_divisor = ctl->divisor;
 		if (ctl->flows)
@@ -535,7 +537,7 @@ static int sfq_init(struct Qdisc *sch, struct rtattr *opt)
 	q->perturb_timer.data = (unsigned long)sch;
 	q->perturb_timer.function = sfq_perturbation;
 	if (q->perturb_period) {
-		q->perturb_timer.expires = jiffies + q->perturb_period;
+		q->perturb_timer.expires = SFQ_PERTURB(q->perturb_period);
 		add_timer(&q->perturb_timer);
 	}
 
@@ -565,7 +567,7 @@ static int sfq_change(struct Qdisc *sch, struct rtattr *opt)
 		tmp.perturbation = 0;
 		del_timer(&q->perturb_timer);
 	} else if (tmp.perturb_period != q->perturb_period) {
-		mod_timer(&q->perturb_timer, jiffies + tmp.perturb_period);
+		mod_timer(&q->perturb_timer, SFQ_PERTURB(tmp.perturb_period));
 	}
 
 	/* move packets from the old queue to the tmp queue */
@@ -603,7 +605,7 @@ static int sfq_dump(struct Qdisc *sch, struct sk_buff *skb)
 	struct tc_sfq_qopt opt;
 
 	opt.quantum = q->quantum;
-	opt.perturb_period = q->perturb_period/HZ;
+	opt.perturb_period = q->perturb_period;
 
 	opt.limit = q->limit;
 	opt.divisor = q->hash_divisor;
-- 
1.5.3.4

[PATCH 8/8] Use nested compat attributes to pass parameters.

[Corey Hickey]

This fixes the ambiguity between, for example:
tc qdisc change ... perturb 0
tc qdisc change ...

Without this patch, there is no way for SFQ to differentiate between
a parameter specified to be 0 and a parameter that was omitted.

Signed-off-by: Corey Hickey <bugfood-ml@fatooh.org>
---
 include/linux/pkt_sched.h |   13 ++++++++
 net/sched/sch_sfq.c       |   69 ++++++++++++++++++++++++++++++++++----------
 2 files changed, 66 insertions(+), 16 deletions(-)

diff --git a/include/linux/pkt_sched.h b/include/linux/pkt_sched.h
index 14a08ad..b1a1a52 100644
--- a/include/linux/pkt_sched.h
+++ b/include/linux/pkt_sched.h
@@ -148,6 +148,19 @@ struct tc_sfq_qopt
 	unsigned	flows;		/* Maximal number of flows  */
 };
 
+enum
+{
+	TCA_SFQ_UNSPEC,
+	TCA_SFQ_COMPAT,
+	TCA_SFQ_QUANTUM,
+	TCA_SFQ_PERTURB,
+	TCA_SFQ_LIMIT,
+	TCA_SFQ_DIVISOR,
+	TCA_SFQ_FLOWS,
+	__TCA_SFQ_MAX,
+};
+
+#define TCA_SFQ_MAX (__TCA_SFQ_MAX - 1)
 
 /* RED section */
 
diff --git a/net/sched/sch_sfq.c b/net/sched/sch_sfq.c
index 2764a54..5c25b05 100644
--- a/net/sched/sch_sfq.c
+++ b/net/sched/sch_sfq.c
@@ -456,6 +456,29 @@ sfq_copy_parameters(struct sfq_sched_data *dst, struct sfq_sched_data *src)
 	dst->depth          = src->depth;
 }
 
+/* SFQ parameters exist as individual rtattr attributes, with a nested
+ * "struct tc_sfq_qopt" for compatibility with older userspace tools. If an
+ * individual attribute is set, we want to use it; otherwise, fall back to the
+ * nested struct.
+ * There is one caveat: if a member of the nested struct is 0, we cannot
+ * determine if that parameter is supposed to be 0 or if it is merely unset.
+ * So, only set a parameter if the corresponding struct member (u32 compat) is
+ * nonzero. When setting a parameter to 0, it is necessary to use the
+ * individual attribute. */
+static inline int
+sfq_get_parameter(u32 *dst, struct rtattr *tb[TCA_SFQ_MAX], int attr,
+		  u32 compat)
+{
+	struct rtattr *rta = tb[(attr - 1)];
+	if (rta)
+		*dst = RTA_GET_U32(rta);
+	else if (compat)
+		*dst = compat;
+
+	rtattr_failure:
+		return -EINVAL;
+}
+	
 static int
 sfq_q_init(struct sfq_sched_data *q, struct rtattr *opt)
 {
@@ -465,21 +488,24 @@ sfq_q_init(struct sfq_sched_data *q, struct rtattr *opt)
 	 * the previous values (sfq_change). So, overwrite the parameters as
 	 * specified. */
 	if (opt) {
-		struct tc_sfq_qopt *ctl = RTA_DATA(opt);
-
-		if (opt->rta_len < RTA_LENGTH(sizeof(*ctl)))
-			return -EINVAL;
-
-		if (ctl->quantum)
-			q->quantum = ctl->quantum;
-		if (ctl->perturb_period)
-			q->perturb_period = ctl->perturb_period;
-		if (ctl->divisor)
-			q->hash_divisor = ctl->divisor;
-		if (ctl->flows)
-			q->depth = ctl->flows;
-		if (ctl->limit)
-			q->limit = ctl->limit;
+		struct tc_sfq_qopt *ctl;
+		struct rtattr *tb[TCA_SFQ_MAX];
+
+		if (rtattr_parse_nested_compat(tb, TCA_SFQ_MAX, opt, ctl,
+				sizeof(*ctl)))
+			goto rtattr_failure;
+
+		if (sfq_get_parameter(&(q->quantum),        tb, TCA_SFQ_QUANTUM,
+				ctl->quantum)        ||
+		    sfq_get_parameter(&(q->perturb_period), tb, TCA_SFQ_PERTURB,
+				ctl->perturb_period) ||
+		    sfq_get_parameter(&(q->hash_divisor),   tb, TCA_SFQ_DIVISOR,
+				ctl->divisor)        ||
+		    sfq_get_parameter(&(q->depth),          tb, TCA_SFQ_FLOWS,
+				ctl->flows)          ||
+		    sfq_get_parameter(&(q->limit),          tb, TCA_SFQ_LIMIT,
+				ctl->limit))
+			goto rtattr_failure;
 
 		if (q->depth        > SFQ_MAX_DEPTH ||
 		    q->hash_divisor > SFQ_MAX_DIVISOR)
@@ -519,6 +545,8 @@ sfq_q_init(struct sfq_sched_data *q, struct rtattr *opt)
 	for (i=0; i < q->depth; i++)
 		sfq_link(q, i);
 	return 0;
+rtattr_failure:
+	return -EINVAL;
 err_case:
 	sfq_q_destroy(q);
 	return -ENOBUFS;
@@ -602,17 +630,26 @@ static int sfq_dump(struct Qdisc *sch, struct sk_buff *skb)
 {
 	struct sfq_sched_data *q = qdisc_priv(sch);
 	unsigned char *b = skb_tail_pointer(skb);
+	struct rtattr *nest;
 	struct tc_sfq_qopt opt;
 
 	opt.quantum = q->quantum;
 	opt.perturb_period = q->perturb_period;
-
 	opt.limit = q->limit;
 	opt.divisor = q->hash_divisor;
 	opt.flows = q->depth;
 
+	nest = RTA_NEST_COMPAT(skb, TCA_OPTIONS, sizeof(opt), &opt);
+
+	RTA_PUT_U32(skb, TCA_SFQ_QUANTUM, q->quantum);
+	RTA_PUT_U32(skb, TCA_SFQ_PERTURB, q->perturb_period);
+	RTA_PUT_U32(skb, TCA_SFQ_LIMIT,   q->limit);
+	RTA_PUT_U32(skb, TCA_SFQ_DIVISOR, q->hash_divisor);
+	RTA_PUT_U32(skb, TCA_SFQ_FLOWS,   q->depth);
 	RTA_PUT(skb, TCA_OPTIONS, sizeof(opt), &opt);
 
+	RTA_NEST_COMPAT_END(skb, nest);
+
 	return skb->len;
 
 rtattr_failure:
-- 
1.5.3.4

Re: [PATCH 8/8] Use nested compat attributes to pass parameters.

[Corey Hickey]

Corey Hickey wrote:
> +/* SFQ parameters exist as individual rtattr attributes, with a nested
> + * "struct tc_sfq_qopt" for compatibility with older userspace tools. If an
> + * individual attribute is set, we want to use it; otherwise, fall back to the
> + * nested struct.
> + * There is one caveat: if a member of the nested struct is 0, we cannot
> + * determine if that parameter is supposed to be 0 or if it is merely unset.
> + * So, only set a parameter if the corresponding struct member (u32 compat) is
> + * nonzero. When setting a parameter to 0, it is necessary to use the
> + * individual attribute. */
> +static inline int
> +sfq_get_parameter(u32 *dst, struct rtattr *tb[TCA_SFQ_MAX], int attr,
> +		  u32 compat)
> +{
> +	struct rtattr *rta = tb[(attr - 1)];
> +	if (rta)
> +		*dst = RTA_GET_U32(rta);
> +	else if (compat)
> +		*dst = compat;
> +
> +	rtattr_failure:
> +		return -EINVAL;
> +}
> +	
>  static int
>  sfq_q_init(struct sfq_sched_data *q, struct rtattr *opt)
>  {
> @@ -465,21 +488,24 @@ sfq_q_init(struct sfq_sched_data *q, struct rtattr *opt)
>  	 * the previous values (sfq_change). So, overwrite the parameters as
>  	 * specified. */
>  	if (opt) {
> -		struct tc_sfq_qopt *ctl = RTA_DATA(opt);
> -
> -		if (opt->rta_len < RTA_LENGTH(sizeof(*ctl)))
> -			return -EINVAL;
> -
> -		if (ctl->quantum)
> -			q->quantum = ctl->quantum;
> -		if (ctl->perturb_period)
> -			q->perturb_period = ctl->perturb_period;
> -		if (ctl->divisor)
> -			q->hash_divisor = ctl->divisor;
> -		if (ctl->flows)
> -			q->depth = ctl->flows;
> -		if (ctl->limit)
> -			q->limit = ctl->limit;
> +		struct tc_sfq_qopt *ctl;
> +		struct rtattr *tb[TCA_SFQ_MAX];
> +
> +		if (rtattr_parse_nested_compat(tb, TCA_SFQ_MAX, opt, ctl,
> +				sizeof(*ctl)))
> +			goto rtattr_failure;
> +
> +		if (sfq_get_parameter(&(q->quantum),        tb, TCA_SFQ_QUANTUM,
> +				ctl->quantum)        ||
> +		    sfq_get_parameter(&(q->perturb_period), tb, TCA_SFQ_PERTURB,
> +				ctl->perturb_period) ||
> +		    sfq_get_parameter(&(q->hash_divisor),   tb, TCA_SFQ_DIVISOR,
> +				ctl->divisor)        ||
> +		    sfq_get_parameter(&(q->depth),          tb, TCA_SFQ_FLOWS,
> +				ctl->flows)          ||
> +		    sfq_get_parameter(&(q->limit),          tb, TCA_SFQ_LIMIT,
> +				ctl->limit))
> +			goto rtattr_failure;
>  

You may note that this part ended up being rather ugly, and I wouldn't
blame anyone for wanting it to be improved. I don't see any good
solutions; the alternatives I can provide are:

1. Use a macro, as I had originally written for this patch:
http://marc.info/?l=linux-netdev&m=119102007907626&w=2
(search for GET_PARAM)
The macro itself doesn't look pretty, but the usage is fairly clean.

2. Use neither macro nor function.
There would be five repetitions of very similar code, with five lines
per repetition: not very nice, but the formatting would still look better.

3. Only use a separate rtattr for perturb, since the other parameters
work fine already. From the start, I had wanted to keep parameter
parsing consistent, but it may not be worth it. This would definitely be
the cleanest approach for readability, and the most simple.


Sorry to bother you with such a superficial problem, but I really don't
know what would be preferable.

Thanks,
Corey

Re: SFQ: backport some features from ESFQ (try 5)

[Corey Hickey]

Corey Hickey wrote:
> Patchset try 2 addresses the review by Michael Buesch.
> Patchset try 3 addresses the review by Patrick McHardy.
> Patchset try 4 has a few cosmetic improvements.
> Patchset try 5 addresses further review by Patrick McHardy.
> 
> This set of patches is substantially the same as my previous try, with
> changes made according to Patrick's recommendations.

Any comments? Patrick? I know you're busy, but you've been sending
reviews for my patches so far and yet another one would be much appreciated.

Thanks,
Corey

Re: SFQ: backport some features from ESFQ (try 5)

[Denys Fedoryshchenko]

On Thu, 15 Nov 2007 18:59:05 -0800, Corey Hickey wrote
> Corey Hickey wrote:
> > Patchset try 2 addresses the review by Michael Buesch.
> > Patchset try 3 addresses the review by Patrick McHardy.
> > Patchset try 4 has a few cosmetic improvements.
> > Patchset try 5 addresses further review by Patrick McHardy.
> > 
> > This set of patches is substantially the same as my previous try, with
> > changes made according to Patrick's recommendations.
> 
> Any comments? Patrick? I know you're busy, but you've been sending
> reviews for my patches so far and yet another one would be much appreciated.
> 
> Thanks,
> Corey
> -
> 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
For me also it is very interesting to have SFQ with this patches, cause right 
now SFQ suitable only for low-bandwidth customers. With this patch i can use 
it much more, and QOS will become much more flexible for me.

--
Denys Fedoryshchenko
Technical Manager
Virtual ISP S.A.L.