[PATCH v3 1/2] net_sched: add size table functions

[PATCH v3 1/2] net_sched: add size table functions

[Jussi Kivilinna]

Patch adds size table that is similiar to rate table, with difference that
size table stores link layer packet size. It's needed for HFSC link
layer adaption patch as it converts skb->len to link layer packet size
directly, unlike HTB/CFQ/etc that convert packet length to link layer
transfer time using rate tables.

Signed-off-by: Jussi Kivilinna <jussi.kivilinna@mbnet.fi>
---

 include/linux/pkt_sched.h |   10 ++++++++++
 include/net/pkt_sched.h   |    3 +++
 include/net/sch_generic.h |   23 ++++++++++++++++++++++
 net/sched/sch_api.c       |   47 +++++++++++++++++++++++++++++++++++++++++++++
 4 files changed, 83 insertions(+), 0 deletions(-)

diff --git a/include/linux/pkt_sched.h b/include/linux/pkt_sched.h
index dbb7ac3..5bf1444 100644
--- a/include/linux/pkt_sched.h
+++ b/include/linux/pkt_sched.h
@@ -85,6 +85,16 @@ struct tc_ratespec
 
 #define TC_RTAB_SIZE	1024
 
+struct tc_sizespec {
+	unsigned char	cell_log;
+	unsigned char	size_log;
+	short		overhead;
+	short		cell_align;
+	unsigned short	mpu;
+};
+
+#define TC_STAB_SIZE	1024
+
 /* FIFO section */
 
 struct tc_fifo_qopt
diff --git a/include/net/pkt_sched.h b/include/net/pkt_sched.h
index 46fb4d8..b032488 100644
--- a/include/net/pkt_sched.h
+++ b/include/net/pkt_sched.h
@@ -78,7 +78,10 @@ extern struct Qdisc *qdisc_lookup(struct net_device *dev, u32 handle);
 extern struct Qdisc *qdisc_lookup_class(struct net_device *dev, u32 handle);
 extern struct qdisc_rate_table *qdisc_get_rtab(struct tc_ratespec *r,
 		struct nlattr *tab);
+extern struct qdisc_size_table *qdisc_get_stab(struct tc_sizespec *s,
+		struct nlattr *tab);
 extern void qdisc_put_rtab(struct qdisc_rate_table *tab);
+extern void qdisc_put_stab(struct qdisc_size_table *tab);
 
 extern void __qdisc_run(struct net_device *dev);
 
diff --git a/include/net/sch_generic.h b/include/net/sch_generic.h
index ab502ec..32b6865 100644
--- a/include/net/sch_generic.h
+++ b/include/net/sch_generic.h
@@ -23,6 +23,13 @@ struct qdisc_rate_table
 	int		refcnt;
 };
 
+struct qdisc_size_table {
+	struct tc_sizespec szopts;
+	u16		data[512];
+	struct qdisc_size_table *next;
+	int		refcnt;
+};
+
 struct Qdisc
 {
 	int 			(*enqueue)(struct sk_buff *skb, struct Qdisc *dev);
@@ -316,6 +323,22 @@ static inline u32 qdisc_l2t(struct qdisc_rate_table* rtab, unsigned int pktlen)
 	return rtab->data[slot];
 }
 
+/* Length to link layer size lookup in a qdisc_size_table, to determine how
+   what size packet takes on link layer.
+ */
+static inline u32 qdisc_linklayer_sz(struct qdisc_size_table *stab, u32 pktlen)
+{
+	int slot = pktlen + stab->szopts.cell_align + stab->szopts.overhead;
+	unsigned char size_log = stab->szopts.size_log;
+	if (unlikely(slot < 0))
+		slot = 0;
+	slot >>= stab->szopts.cell_log;
+	if (unlikely(slot > 511))
+		return ((u32)stab->data[511] << size_log) * (slot >> 9) +
+			((u32)stab->data[slot & 0x1FF] << size_log);
+	return (u32)stab->data[slot] << size_log;
+}
+
 #ifdef CONFIG_NET_CLS_ACT
 static inline struct sk_buff *skb_act_clone(struct sk_buff *skb, gfp_t gfp_mask)
 {
diff --git a/net/sched/sch_api.c b/net/sched/sch_api.c
index c40773c..8dfe28c 100644
--- a/net/sched/sch_api.c
+++ b/net/sched/sch_api.c
@@ -277,6 +277,53 @@ void qdisc_put_rtab(struct qdisc_rate_table *tab)
 }
 EXPORT_SYMBOL(qdisc_put_rtab);
 
+static struct qdisc_size_table *qdisc_stab_list;
+
+struct qdisc_size_table *qdisc_get_stab(struct tc_sizespec *s,
+					struct nlattr *tab)
+{
+	struct qdisc_size_table *stab;
+
+	for (stab = qdisc_stab_list; stab; stab = stab->next) {
+		if (memcmp(&stab->szopts, s, sizeof(struct tc_sizespec)) == 0) {
+			stab->refcnt++;
+			return stab;
+		}
+	}
+
+	if (tab == NULL || nla_len(tab) != TC_STAB_SIZE)
+		return NULL;
+
+	stab = kmalloc(sizeof(*stab), GFP_KERNEL);
+	if (stab) {
+		stab->szopts = *s;
+		stab->refcnt = 1;
+		memcpy(stab->data, nla_data(tab), TC_STAB_SIZE);
+		stab->next = qdisc_stab_list;
+		qdisc_stab_list = stab;
+	}
+	return stab;
+}
+EXPORT_SYMBOL(qdisc_get_stab);
+
+void qdisc_put_stab(struct qdisc_size_table *tab)
+{
+	struct qdisc_size_table *stab, **stabp;
+
+	if (!tab || --tab->refcnt)
+		return;
+
+	for (stabp = &qdisc_stab_list; (stab = *stabp) != NULL;
+							stabp = &stab->next) {
+		if (stab == tab) {
+			*stabp = stab->next;
+			kfree(stab);
+			return;
+		}
+	}
+}
+EXPORT_SYMBOL(qdisc_put_stab);
+
 static enum hrtimer_restart qdisc_watchdog(struct hrtimer *timer)
 {
 	struct qdisc_watchdog *wd = container_of(timer, struct qdisc_watchdog,

[PATCH v3 2/2] hfsc: add link layer overhead adaption

[Jussi Kivilinna]

CBQ and HTB have options for emulating overhead of underlying link layer
(mpu/overhead/linklayer options). This patch makes sch_hfsc use size table
to emulate link layer overhead.

Patch uses size table to convert packet length to emulated link layer packet
length. Converted packet length is passed to hfsc calculations instead of
real. If size table isn't passed to kernel, hfsc works as before.

Signed-off-by: Jussi Kivilinna <jussi.kivilinna@mbnet.fi>
---

 include/linux/pkt_sched.h |    2 +
 net/sched/sch_hfsc.c      |  116 +++++++++++++++++++++++++++++++++++----------
 2 files changed, 93 insertions(+), 25 deletions(-)

diff --git a/include/linux/pkt_sched.h b/include/linux/pkt_sched.h
index 5bf1444..6a2cbc4 100644
--- a/include/linux/pkt_sched.h
+++ b/include/linux/pkt_sched.h
@@ -326,6 +326,8 @@ enum
 	TCA_HFSC_RSC,
 	TCA_HFSC_FSC,
 	TCA_HFSC_USC,
+	TCA_HFSC_SZOPTS,
+	TCA_HFSC_STAB,
 	__TCA_HFSC_MAX,
 };
 
diff --git a/net/sched/sch_hfsc.c b/net/sched/sch_hfsc.c
index fdfaa3f..de958aa 100644
--- a/net/sched/sch_hfsc.c
+++ b/net/sched/sch_hfsc.c
@@ -128,6 +128,8 @@ struct hfsc_class
 	struct list_head siblings;	/* sibling classes */
 	struct list_head children;	/* child classes */
 	struct Qdisc	*qdisc;		/* leaf qdisc */
+	struct qdisc_size_table *stab;	/* size table used for link layer
+					   overhead adaption */
 
 	struct rb_node el_node;		/* qdisc's eligible tree member */
 	struct rb_root vt_tree;		/* active children sorted by cl_vt */
@@ -496,6 +498,14 @@ sc2isc(struct tc_service_curve *sc, struct internal_sc *isc)
 	isc->ism2 = m2ism(sc->m2);
 }
 
+/* convert packet length to link layer packet length */
+static unsigned int get_linklayer_len(struct hfsc_class *cl, unsigned int len)
+{
+	if (unlikely(!len) || likely(!cl->stab))
+		return len;
+	return qdisc_linklayer_sz(cl->stab, len);
+}
+
 /*
  * initialize the runtime service curve with the given internal
  * service curve starting at (x, y).
@@ -987,9 +997,11 @@ hfsc_change_usc(struct hfsc_class *cl, struct tc_service_curve *usc,
 }
 
 static const struct nla_policy hfsc_policy[TCA_HFSC_MAX + 1] = {
-	[TCA_HFSC_RSC]	= { .len = sizeof(struct tc_service_curve) },
-	[TCA_HFSC_FSC]	= { .len = sizeof(struct tc_service_curve) },
-	[TCA_HFSC_USC]	= { .len = sizeof(struct tc_service_curve) },
+	[TCA_HFSC_RSC]		= { .len = sizeof(struct tc_service_curve) },
+	[TCA_HFSC_FSC]		= { .len = sizeof(struct tc_service_curve) },
+	[TCA_HFSC_USC]		= { .len = sizeof(struct tc_service_curve) },
+	[TCA_HFSC_SZOPTS]	= { .len = sizeof(struct tc_sizespec) },
+	[TCA_HFSC_STAB]		= { .type = NLA_BINARY, .len = TC_STAB_SIZE },
 };
 
 static int
@@ -1000,18 +1012,21 @@ hfsc_change_class(struct Qdisc *sch, u32 classid, u32 parentid,
 	struct hfsc_class *cl = (struct hfsc_class *)*arg;
 	struct hfsc_class *parent = NULL;
 	struct nlattr *opt = tca[TCA_OPTIONS];
-	struct nlattr *tb[TCA_HFSC_MAX + 1];
+	struct nlattr *tb[TCA_HFSC_STAB + 1];
 	struct tc_service_curve *rsc = NULL, *fsc = NULL, *usc = NULL;
+	struct tc_sizespec *szopts = NULL;
+	struct qdisc_size_table *stab = NULL;
 	u64 cur_time;
 	int err;
 
 	if (opt == NULL)
 		return -EINVAL;
 
-	err = nla_parse_nested(tb, TCA_HFSC_MAX, opt, hfsc_policy);
+	err = nla_parse_nested(tb, TCA_HFSC_STAB, opt, hfsc_policy);
 	if (err < 0)
 		return err;
 
+	err = -EINVAL;
 	if (tb[TCA_HFSC_RSC]) {
 		rsc = nla_data(tb[TCA_HFSC_RSC]);
 		if (rsc->m1 == 0 && rsc->m2 == 0)
@@ -1030,12 +1045,18 @@ hfsc_change_class(struct Qdisc *sch, u32 classid, u32 parentid,
 			usc = NULL;
 	}
 
+	if (tb[TCA_HFSC_SZOPTS]) {
+		szopts = nla_data(tb[TCA_HFSC_SZOPTS]);
+		stab = qdisc_get_stab(szopts, tb[TCA_HFSC_STAB]);
+	}
+
 	if (cl != NULL) {
 		if (parentid) {
+			err = -EINVAL;
 			if (cl->cl_parent && cl->cl_parent->classid != parentid)
-				return -EINVAL;
+				goto failure;
 			if (cl->cl_parent == NULL && parentid != TC_H_ROOT)
-				return -EINVAL;
+				goto failure;
 		}
 		cur_time = psched_get_time();
 
@@ -1047,9 +1068,14 @@ hfsc_change_class(struct Qdisc *sch, u32 classid, u32 parentid,
 		if (usc != NULL)
 			hfsc_change_usc(cl, usc, cur_time);
 
+		if (cl->stab)
+			qdisc_put_stab(cl->stab);
+		cl->stab = stab;
+
 		if (cl->qdisc->q.qlen != 0) {
 			if (cl->cl_flags & HFSC_RSC)
-				update_ed(cl, qdisc_peek_len(cl->qdisc));
+				update_ed(cl, get_linklayer_len(cl,
+						qdisc_peek_len(cl->qdisc)));
 			if (cl->cl_flags & HFSC_FSC)
 				update_vf(cl, 0, cur_time);
 		}
@@ -1062,27 +1088,39 @@ hfsc_change_class(struct Qdisc *sch, u32 classid, u32 parentid,
 		return 0;
 	}
 
-	if (parentid == TC_H_ROOT)
-		return -EEXIST;
+	if (parentid == TC_H_ROOT) {
+		err = -EEXIST;
+		goto failure;
+	}
 
 	parent = &q->root;
 	if (parentid) {
 		parent = hfsc_find_class(parentid, sch);
-		if (parent == NULL)
-			return -ENOENT;
+		if (parent == NULL) {
+			err = -ENOENT;
+			goto failure;
+		}
 	}
 
-	if (classid == 0 || TC_H_MAJ(classid ^ sch->handle) != 0)
-		return -EINVAL;
-	if (hfsc_find_class(classid, sch))
-		return -EEXIST;
+	if (classid == 0 || TC_H_MAJ(classid ^ sch->handle) != 0) {
+		err = -EINVAL;
+		goto failure;
+	}
+	if (hfsc_find_class(classid, sch)) {
+		err = -EEXIST;
+		goto failure;
+	}
 
-	if (rsc == NULL && fsc == NULL)
-		return -EINVAL;
+	if (rsc == NULL && fsc == NULL) {
+		err = -EINVAL;
+		goto failure;
+	}
 
 	cl = kzalloc(sizeof(struct hfsc_class), GFP_KERNEL);
-	if (cl == NULL)
-		return -ENOBUFS;
+	if (cl == NULL) {
+		err = -ENOBUFS;
+		goto failure;
+	}
 
 	if (rsc != NULL)
 		hfsc_change_rsc(cl, rsc, 0);
@@ -1109,6 +1147,9 @@ hfsc_change_class(struct Qdisc *sch, u32 classid, u32 parentid,
 		hfsc_purge_queue(sch, parent);
 	hfsc_adjust_levels(parent);
 	cl->cl_pcvtoff = parent->cl_cvtoff;
+	if (cl->stab)
+		qdisc_put_stab(cl->stab);
+	cl->stab = stab;
 	sch_tree_unlock(sch);
 
 	if (tca[TCA_RATE])
@@ -1116,6 +1157,10 @@ hfsc_change_class(struct Qdisc *sch, u32 classid, u32 parentid,
 				  &sch->dev->queue_lock, tca[TCA_RATE]);
 	*arg = (unsigned long)cl;
 	return 0;
+failure:
+	if (stab)
+		qdisc_put_stab(stab);
+	return err;
 }
 
 static void
@@ -1126,6 +1171,8 @@ hfsc_destroy_class(struct Qdisc *sch, struct hfsc_class *cl)
 	tcf_destroy_chain(cl->filter_list);
 	qdisc_destroy(cl->qdisc);
 	gen_kill_estimator(&cl->bstats, &cl->rate_est);
+	if (cl->stab)
+		qdisc_put_stab(cl->stab);
 	if (cl != &q->root)
 		kfree(cl);
 }
@@ -1338,6 +1385,21 @@ hfsc_dump_curves(struct sk_buff *skb, struct hfsc_class *cl)
 	return -1;
 }
 
+static inline int
+hfsc_dump_szopts(struct sk_buff *skb, struct hfsc_class *cl)
+{
+	if (!cl->stab)
+		return 0;
+
+	NLA_PUT(skb, TCA_HFSC_SZOPTS, sizeof(cl->stab->szopts),
+		&cl->stab->szopts);
+
+	return skb->len;
+
+ nla_put_failure:
+	return -1;
+}
+
 static int
 hfsc_dump_class(struct Qdisc *sch, unsigned long arg, struct sk_buff *skb,
 		struct tcmsg *tcm)
@@ -1355,6 +1417,8 @@ hfsc_dump_class(struct Qdisc *sch, unsigned long arg, struct sk_buff *skb,
 		goto nla_put_failure;
 	if (hfsc_dump_curves(skb, cl) < 0)
 		goto nla_put_failure;
+	if (hfsc_dump_szopts(skb, cl) < 0)
+		goto nla_put_failure;
 	nla_nest_end(skb, nest);
 	return skb->len;
 
@@ -1588,7 +1652,7 @@ hfsc_enqueue(struct sk_buff *skb, struct Qdisc *sch)
 	}
 
 	if (cl->qdisc->q.qlen == 1)
-		set_active(cl, len);
+		set_active(cl, get_linklayer_len(cl, len));
 
 	cl->bstats.packets++;
 	cl->bstats.bytes += len;
@@ -1606,7 +1670,7 @@ hfsc_dequeue(struct Qdisc *sch)
 	struct hfsc_class *cl;
 	struct sk_buff *skb;
 	u64 cur_time;
-	unsigned int next_len;
+	unsigned int next_len, cur_len;
 	int realtime = 0;
 
 	if (sch->q.qlen == 0)
@@ -1643,14 +1707,16 @@ hfsc_dequeue(struct Qdisc *sch)
 		return NULL;
 	}
 
-	update_vf(cl, skb->len, cur_time);
+	cur_len = get_linklayer_len(cl, skb->len);
+	update_vf(cl, cur_len, cur_time);
 	if (realtime)
-		cl->cl_cumul += skb->len;
+		cl->cl_cumul += cur_len;
 
 	if (cl->qdisc->q.qlen != 0) {
 		if (cl->cl_flags & HFSC_RSC) {
 			/* update ed */
-			next_len = qdisc_peek_len(cl->qdisc);
+			next_len = get_linklayer_len(cl,
+						qdisc_peek_len(cl->qdisc));
 			if (realtime)
 				update_ed(cl, next_len);
 			else

Re: [PATCH v3 2/2] hfsc: add link layer overhead adaption

[Patrick McHardy]

Jussi Kivilinna wrote:
> CBQ and HTB have options for emulating overhead of underlying link layer
> (mpu/overhead/linklayer options). This patch makes sch_hfsc use size table
> to emulate link layer overhead.
> 
> Patch uses size table to convert packet length to emulated link layer packet
> length. Converted packet length is passed to hfsc calculations instead of
> real. If size table isn't passed to kernel, hfsc works as before.
> 
> +/* convert packet length to link layer packet length */
> +static unsigned int get_linklayer_len(struct hfsc_class *cl, unsigned int len)
> +{
> +	if (unlikely(!len) || likely(!cl->stab))
> +		return len;
> +	return qdisc_linklayer_sz(cl->stab, len);
> +}

This means HFSC will use other packet sizes as inner qdiscs, policers,
statistics etc, which I don't like very much. My original patch used
the size tables to calculate the size when enqueing to the root qdisc
and stored it in the cb, so all qdiscs in the hierarchy can use the
same size.

> @@ -987,9 +997,11 @@ hfsc_change_usc(struct hfsc_class *cl, struct tc_service_curve *usc,
>  }
>  
>  static const struct nla_policy hfsc_policy[TCA_HFSC_MAX + 1] = {
> -	[TCA_HFSC_RSC]	= { .len = sizeof(struct tc_service_curve) },
> -	[TCA_HFSC_FSC]	= { .len = sizeof(struct tc_service_curve) },
> -	[TCA_HFSC_USC]	= { .len = sizeof(struct tc_service_curve) },
> +	[TCA_HFSC_RSC]		= { .len = sizeof(struct tc_service_curve) },
> +	[TCA_HFSC_FSC]		= { .len = sizeof(struct tc_service_curve) },
> +	[TCA_HFSC_USC]		= { .len = sizeof(struct tc_service_curve) },
> +	[TCA_HFSC_SZOPTS]	= { .len = sizeof(struct tc_sizespec) },
> +	[TCA_HFSC_STAB]		= { .type = NLA_BINARY, .len = TC_STAB_SIZE },

Why are these two separate attributes?

Re: [PATCH v3 2/2] hfsc: add link layer overhead adaption

[Jussi Kivilinna]

Quoting "Patrick McHardy" <kaber@trash.net>:
>
> This means HFSC will use other packet sizes as inner qdiscs, policers,
> statistics etc, which I don't like very much. My original patch used
> the size tables to calculate the size when enqueing to the root qdisc
> and stored it in the cb, so all qdiscs in the hierarchy can use the
> same size.

With this patch I have tried to make HFSC support link layer emulation  
in same way as done in HTB with rate table, so that overhead is  
defined on leaf qdisc (if have understood right).

I have used this patch to have different overhead between qdisc leafs  
to account for extra overhead of ipv6-in-ipv4 tunnel (redirecting  
traffic to IMQ device and filtering ipv4&ipv6 to corresponding leafs).  
If overhead/linklayer is defined on root qdisc, this wouldn't be  
possible?

I'm not too happy about statistics being affected either, I'll look in to it.

>> @@ -987,9 +997,11 @@ hfsc_change_usc(struct hfsc_class *cl, struct   
>> tc_service_curve *usc,
>> }
>>  static const struct nla_policy hfsc_policy[TCA_HFSC_MAX + 1] = {
>> -	[TCA_HFSC_RSC]	= { .len = sizeof(struct tc_service_curve) },
>> -	[TCA_HFSC_FSC]	= { .len = sizeof(struct tc_service_curve) },
>> -	[TCA_HFSC_USC]	= { .len = sizeof(struct tc_service_curve) },
>> +	[TCA_HFSC_RSC]		= { .len = sizeof(struct tc_service_curve) },
>> +	[TCA_HFSC_FSC]		= { .len = sizeof(struct tc_service_curve) },
>> +	[TCA_HFSC_USC]		= { .len = sizeof(struct tc_service_curve) },
>> +	[TCA_HFSC_SZOPTS]	= { .len = sizeof(struct tc_sizespec) },
>> +	[TCA_HFSC_STAB]		= { .type = NLA_BINARY, .len = TC_STAB_SIZE },
>
> Why are these two separate attributes?

I copied qdisc_get_rtab() to qdisc_get_stab() and qdisc_get_rtab uses  
nla_data(tab) to get the buffer. Should use single attribute instead?
I have worked on patch that would merge qdisc_get_rtab/stab to generic  
qdisc_get_xtab supporting both types of link layer emulation tables.  
With single attribute here, patch wouldn't work so easily.

  - Jussi

Re: [PATCH v3 2/2] hfsc: add link layer overhead adaption

[Patrick McHardy]

Jussi Kivilinna wrote:
> Quoting "Patrick McHardy" <kaber@trash.net>:
>>
>> This means HFSC will use other packet sizes as inner qdiscs, policers,
>> statistics etc, which I don't like very much. My original patch used
>> the size tables to calculate the size when enqueing to the root qdisc
>> and stored it in the cb, so all qdiscs in the hierarchy can use the
>> same size.
>
> With this patch I have tried to make HFSC support link layer emulation 
> in same way as done in HTB with rate table, so that overhead is 
> defined on leaf qdisc (if have understood right).

The difference is that with this approach you are actually
able to make the entire qdisc hierarchy use the same size
for calculations/statistics/etc. The rate tables only work
in TBF based qdiscs and policers (which was always one of
my complaints).

>
> I have used this patch to have different overhead between qdisc leafs 
> to account for extra overhead of ipv6-in-ipv4 tunnel (redirecting 
> traffic to IMQ device and filtering ipv4&ipv6 to corresponding leafs). 
> If overhead/linklayer is defined on root qdisc, this wouldn't be 
> possible?

You could override the size in inner classes and qdiscs.
Having an optional size table per qdisc would make this
consistent from a user perspective with the top-level
size calculation. (And I agree that including tunnel
overhead makes sense).

>
> I'm not too happy about statistics being affected either, I'll look in 
> to it.
>
>>> @@ -987,9 +997,11 @@ hfsc_change_usc(struct hfsc_class *cl, struct  
>>> tc_service_curve *usc,
>>> }
>>>  static const struct nla_policy hfsc_policy[TCA_HFSC_MAX + 1] = {
>>> -    [TCA_HFSC_RSC]    = { .len = sizeof(struct tc_service_curve) },
>>> -    [TCA_HFSC_FSC]    = { .len = sizeof(struct tc_service_curve) },
>>> -    [TCA_HFSC_USC]    = { .len = sizeof(struct tc_service_curve) },
>>> +    [TCA_HFSC_RSC]        = { .len = sizeof(struct 
>>> tc_service_curve) },
>>> +    [TCA_HFSC_FSC]        = { .len = sizeof(struct 
>>> tc_service_curve) },
>>> +    [TCA_HFSC_USC]        = { .len = sizeof(struct 
>>> tc_service_curve) },
>>> +    [TCA_HFSC_SZOPTS]    = { .len = sizeof(struct tc_sizespec) },
>>> +    [TCA_HFSC_STAB]        = { .type = NLA_BINARY, .len = 
>>> TC_STAB_SIZE },
>>
>> Why are these two separate attributes?
>
> I copied qdisc_get_rtab() to qdisc_get_stab() and qdisc_get_rtab uses 
> nla_data(tab) to get the buffer. Should use single attribute instead?
> I have worked on patch that would merge qdisc_get_rtab/stab to generic 
> qdisc_get_xtab supporting both types of link layer emulation tables. 
> With single attribute here, patch wouldn't work so easily.

Right, I forgot that rtabs also use two attribures. Well,
usually they use one attribute and one structure embedded in
their base structure, but only because they were using RTABs
from the beginning. So that looks OK.

Re: [PATCH v3 2/2] hfsc: add link layer overhead adaption

[Jussi Kivilinna]

Quoting "Patrick McHardy" <kaber@trash.net>:

> Jussi Kivilinna wrote:
>> Quoting "Patrick McHardy" <kaber@trash.net>:
>>>
>>> This means HFSC will use other packet sizes as inner qdiscs, policers,
>>> statistics etc, which I don't like very much. My original patch used
>>> the size tables to calculate the size when enqueing to the root qdisc
>>> and stored it in the cb, so all qdiscs in the hierarchy can use the
>>> same size.
>>
>> With this patch I have tried to make HFSC support link layer   
>> emulation in same way as done in HTB with rate table, so that   
>> overhead is defined on leaf qdisc (if have understood right).
>
> The difference is that with this approach you are actually
> able to make the entire qdisc hierarchy use the same size
> for calculations/statistics/etc. The rate tables only work
> in TBF based qdiscs and policers (which was always one of
> my complaints).
>
>>
>> I have used this patch to have different overhead between qdisc   
>> leafs to account for extra overhead of ipv6-in-ipv4 tunnel   
>> (redirecting traffic to IMQ device and filtering ipv4&ipv6 to   
>> corresponding leafs). If overhead/linklayer is defined on root   
>> qdisc, this wouldn't be possible?
>
> You could override the size in inner classes and qdiscs.
> Having an optional size table per qdisc would make this
> consistent from a user perspective with the top-level
> size calculation. (And I agree that including tunnel
> overhead makes sense).
>

Ok, having one table for whole qdisc tree (with overrides) sounds better.

  - Jussi

Re: [PATCH v3 1/2] net_sched: add size table functions

[David Miller]

From: Jussi Kivilinna <jussi.kivilinna@mbnet.fi>
Date: Fri, 04 Jul 2008 01:03:12 +0300

> Patch adds size table that is similiar to rate table, with difference that
> size table stores link layer packet size. It's needed for HFSC link
> layer adaption patch as it converts skb->len to link layer packet size
> directly, unlike HTB/CFQ/etc that convert packet length to link layer
> transfer time using rate tables.
> 
> Signed-off-by: Jussi Kivilinna <jussi.kivilinna@mbnet.fi>

Ok.

Patrick, can you give some feedback on these two patches?

Thanks!

Re: [PATCH v3 1/2] net_sched: add size table functions

[Patrick McHardy]

David Miller wrote:
> From: Jussi Kivilinna <jussi.kivilinna@mbnet.fi>
> Date: Fri, 04 Jul 2008 01:03:12 +0300
> 
>> Patch adds size table that is similiar to rate table, with difference that
>> size table stores link layer packet size. It's needed for HFSC link
>> layer adaption patch as it converts skb->len to link layer packet size
>> directly, unlike HTB/CFQ/etc that convert packet length to link layer
>> transfer time using rate tables.
>>
>> Signed-off-by: Jussi Kivilinna <jussi.kivilinna@mbnet.fi>
> 
> Ok.
> 
> Patrick, can you give some feedback on these two patches?

Sure, I'll review them tommorrow. Want to finish some VLAN fixes first.