sch_netem: Remove classful functionality

PATCH In review Jarek Poplawski 4 participants 10 messages Oct 31, 2008 – Nov 4, 2008

Subsystems (4)

Networking Network drivers Traffic control (tc) Netem network emulator

lore

Share & tools

lore

Jarek Poplawski

author

2008-10-31 13:20 UTC #0 lore raw

PATCH 1/2 sch_netem: Remove classful functionality

Patrick McHardy noticed that: "a lot of the functionality of netem requires the inner tfifo anyways and rate-limiting is usually done on top of netem. So I would suggest so either hard-wire the tfifo qdisc or at least make the assumption that inner qdiscs are work-conserving.", and later: "- a lot of other qdiscs still don't work as inner qdiscs of netem [...]".

So, according to his suggestion, this patch removes classful options of netem. The main reason of this change is to remove ops->requeue() method, which is currently used only by netem.

Signed-off-by Jarek Poplawski <[email protected]>

net/sched/sch_netem.c +0 ~~-87~~

index 3080bd6..2ad0959 100644
@@ -622,95 +622,8 @@ nla_put_failure:
 	return -1;
 }
 
-static int netem_dump_class(struct Qdisc *sch, unsigned long cl,
-			  struct sk_buff *skb, struct tcmsg *tcm)
-{
-	struct netem_sched_data *q = qdisc_priv(sch);
-
-	if (cl != 1) 	/* only one class */
-		return -ENOENT;
-
-	tcm->tcm_handle |= TC_H_MIN(1);
-	tcm->tcm_info = q->qdisc->handle;
-
-	return 0;
-}
-
-static int netem_graft(struct Qdisc *sch, unsigned long arg, struct Qdisc *new,
-		     struct Qdisc **old)
-{
-	struct netem_sched_data *q = qdisc_priv(sch);
-
-	if (new == NULL)
-		new = &noop_qdisc;
-
-	sch_tree_lock(sch);
-	*old = xchg(&q->qdisc, new);
-	qdisc_tree_decrease_qlen(*old, (*old)->q.qlen);
-	qdisc_reset(*old);
-	sch_tree_unlock(sch);
-
-	return 0;
-}
-
-static struct Qdisc *netem_leaf(struct Qdisc *sch, unsigned long arg)
-{
-	struct netem_sched_data *q = qdisc_priv(sch);
-	return q->qdisc;
-}
-
-static unsigned long netem_get(struct Qdisc *sch, u32 classid)
-{
-	return 1;
-}
-
-static void netem_put(struct Qdisc *sch, unsigned long arg)
-{
-}
-
-static int netem_change_class(struct Qdisc *sch, u32 classid, u32 parentid,
-			    struct nlattr **tca, unsigned long *arg)
-{
-	return -ENOSYS;
-}
-
-static int netem_delete(struct Qdisc *sch, unsigned long arg)
-{
-	return -ENOSYS;
-}
-
-static void netem_walk(struct Qdisc *sch, struct qdisc_walker *walker)
-{
-	if (!walker->stop) {
-		if (walker->count >= walker->skip)
-			if (walker->fn(sch, 1, walker) < 0) {
-				walker->stop = 1;
-				return;
-			}
-		walker->count++;
-	}
-}
-
-static struct tcf_proto **netem_find_tcf(struct Qdisc *sch, unsigned long cl)
-{
-	return NULL;
-}
-
-static const struct Qdisc_class_ops netem_class_ops = {
-	.graft		=	netem_graft,
-	.leaf		=	netem_leaf,
-	.get		=	netem_get,
-	.put		=	netem_put,
-	.change		=	netem_change_class,
-	.delete		=	netem_delete,
-	.walk		=	netem_walk,
-	.tcf_chain	=	netem_find_tcf,
-	.dump		=	netem_dump_class,
-};
-
 static struct Qdisc_ops netem_qdisc_ops __read_mostly = {
 	.id		=	"netem",
-	.cl_ops		=	&netem_class_ops,
 	.priv_size	=	sizeof(struct netem_sched_data),
 	.enqueue	=	netem_enqueue,
 	.dequeue	=	netem_dequeue,

Signature · 1 line

1.5.6.5

Stephen Hemminger

2008-10-31 16:45 UTC #1 lore raw

Quoted 7 lines · On Fri, 31 Oct 2008 13:20:10 +0000 Jarek Poplawski <[email protected]> wrote:

Patrick McHardy noticed that: "a lot of the functionality of netem requires the inner tfifo anyways and rate-limiting is usually done on top of netem. So I would suggest so either hard-wire the tfifo qdisc or at least make the assumption that inner qdiscs are work-conserving.", and later: "- a lot of other qdiscs still don't work as inner qdiscs of netem [...]".

Quoted 6 lines

So, according to his suggestion, this patch removes classful options of netem. The main reason of this change is to remove ops->requeue() method, which is currently used only by netem.
Signed-off-by Jarek Poplawski <[email protected]>

Okay, only if you add the following functionality back:
* rate control, it doesn't have TBF, some other rate limiting is fine.

* option for pure fifo or reordering based on jitter.

I have no problem with changing implementation as long as the same feature set used by users remain.

David Miller

maintainer

2008-11-02 07:37 UTC #2 lore raw

In reply to Jarek Poplawski

From: Jarek Poplawski <[email protected]>
Date: Fri, 31 Oct 2008 13:20:10 +0000

Quoted 7 lines

Patrick McHardy noticed that: "a lot of the functionality of netem requires the inner tfifo anyways and rate-limiting is usually done on top of netem. So I would suggest so either hard-wire the tfifo qdisc or at least make the assumption that inner qdiscs are work-conserving.", and later: "- a lot of other qdiscs still don't work as inner qdiscs of netem [...]".

Quoted 5 lines

So, according to his suggestion, this patch removes classful options of netem. The main reason of this change is to remove ops->requeue() method, which is currently used only by netem.
Signed-off-by Jarek Poplawski <[email protected]>

Jarek, I applied this patch and your second one to net-next-2.6

But I did this only because I trust that you will address Stephen's feedback wrt. making existing netem functionality available in some way.

Otherwise I'll have to revert these changes.

Thanks!

Jarek Poplawski

author

2008-11-03 08:29 UTC #3 lore raw

Quoted 16 lines · David Miller

From: Jarek Poplawski <[email protected]>
Date: Fri, 31 Oct 2008 13:20:10 +0000
Quoted 7 lines
Patrick McHardy noticed that: "a lot of the functionality of netem requires the inner tfifo anyways and rate-limiting is usually done on top of netem. So I would suggest so either hard-wire the tfifo qdisc or at least make the assumption that inner qdiscs are work-conserving.", and later: "- a lot of other qdiscs still don't work as inner qdiscs of netem [...]".
Quoted 5 lines
So, according to his suggestion, this patch removes classful options of netem. The main reason of this change is to remove ops->requeue() method, which is currently used only by netem.
Signed-off-by Jarek Poplawski <[email protected]>

Quoted 7 lines

Jarek, I applied this patch and your second one to net-next-2.6
But I did this only because I trust that you will address Stephen's feedback wrt. making existing netem functionality available in some way.
Otherwise I'll have to revert these changes.

Hmm... I thought there was kind of RFC for this, and it looked like Patrick's idea won 100% of votes, but I'm not good in counting...

http://marc.info/?l=linux-netdev&m=122469801712438&w=2
http://marc.info/?l=linux-netdev&m=122469674709761&w=2

Anyway, IMHO adding TBF etc. functionalities to tfifo doesn't make much sense, and if they are really needed it's better to revert these patches and chose one of the other ways of doing reorder proposed in this earlier thread.

Thanks,
Jarek P.

Patrick McHardy

2008-11-03 11:20 UTC #4 lore raw

Quoted 15 lines · Jarek Poplawski

Quoted 10 lines · David Miller
From: Jarek Poplawski <[email protected]>
Date: Fri, 31 Oct 2008 13:20:10 +0000
Jarek, I applied this patch and your second one to net-next-2.6
But I did this only because I trust that you will address Stephen's feedback wrt. making existing netem functionality available in some way.
Otherwise I'll have to revert these changes.
Hmm... I thought there was kind of RFC for this, and it looked like Patrick's idea won 100% of votes, but I'm not good in counting...

Quoted 7 lines

http://marc.info/?l=linux-netdev&m=122469801712438&w=2
http://marc.info/?l=linux-netdev&m=122469674709761&w=2
Anyway, IMHO adding TBF etc. functionalities to tfifo doesn't make much sense, and if they are really needed it's better to revert these patches and chose one of the other ways of doing reorder proposed in this earlier thread.

Whats wrong with simply using TBF as parent qdisc of netem?

Stephen Hemminger

2008-11-03 17:06 UTC #5 lore raw

Quoted 19 lines · On Mon, 03 Nov 2008 12:20:25 +0100 Patrick McHardy <[email protected]> wrote:

Quoted 18 lines · Jarek Poplawski
Quoted 10 lines · David Miller
From: Jarek Poplawski <[email protected]>
Date: Fri, 31 Oct 2008 13:20:10 +0000
Jarek, I applied this patch and your second one to net-next-2.6
But I did this only because I trust that you will address Stephen's feedback wrt. making existing netem functionality available in some way.
Otherwise I'll have to revert these changes.
Hmm... I thought there was kind of RFC for this, and it looked like Patrick's idea won 100% of votes, but I'm not good in counting...
http://marc.info/?l=linux-netdev&m=122469801712438&w=2
http://marc.info/?l=linux-netdev&m=122469674709761&w=2

Quoted 6 lines

Quoted 4 lines
Anyway, IMHO adding TBF etc. functionalities to tfifo doesn't make much sense, and if they are really needed it's better to revert these patches and chose one of the other ways of doing reorder proposed in this earlier thread.
Whats wrong with simply using TBF as parent qdisc of netem?

It works but does something slightly different.

netem inside TBF is like long delay network followed by choke on last hop TBF inside netem was like choke on uplink followed by long delay network.

Since most users use the for network evaluation, and the standard for network evaluation is dummynet; I plan to add dummynet compatiable rate control.

Jarek Poplawski

author

2008-11-04 09:56 UTC #6 lore raw

Quoted 27 lines · Stephen Hemminger

Quoted 24 lines · On Mon, 03 Nov 2008 12:20:25 +0100 Patrick McHardy <[email protected]> wrote:
Quoted 22 lines · Jarek Poplawski
Quoted 10 lines · David Miller
From: Jarek Poplawski <[email protected]>
Date: Fri, 31 Oct 2008 13:20:10 +0000
Jarek, I applied this patch and your second one to net-next-2.6
But I did this only because I trust that you will address Stephen's feedback wrt. making existing netem functionality available in some way.
Otherwise I'll have to revert these changes.
Hmm... I thought there was kind of RFC for this, and it looked like Patrick's idea won 100% of votes, but I'm not good in counting...
http://marc.info/?l=linux-netdev&m=122469801712438&w=2
http://marc.info/?l=linux-netdev&m=122469674709761&w=2
Anyway, IMHO adding TBF etc. functionalities to tfifo doesn't make much sense, and if they are really needed it's better to revert these patches and chose one of the other ways of doing reorder proposed in this earlier thread.

Quoted 6 lines

Quoted 1 line
Whats wrong with simply using TBF as parent qdisc of netem?
It works but does something slightly different.
netem inside TBF is like long delay network followed by choke on last hop TBF inside netem was like choke on uplink followed by long delay network.

David, this makes sense to me, so please revert these two patches. (Then, I think we're still at RFC stage.)

Thanks.
Jarek P.

Stephen Hemminger

2008-11-04 16:25 UTC #7 lore raw

Quoted 32 lines · On Tue, 4 Nov 2008 09:56:42 +0000 Jarek Poplawski <[email protected]> wrote:

Quoted 31 lines · Stephen Hemminger
Quoted 19 lines · On Mon, 03 Nov 2008 12:20:25 +0100 Patrick McHardy <[email protected]> wrote:
Quoted 18 lines · Jarek Poplawski
Quoted 10 lines · David Miller
From: Jarek Poplawski <[email protected]>
Date: Fri, 31 Oct 2008 13:20:10 +0000
Jarek, I applied this patch and your second one to net-next-2.6
But I did this only because I trust that you will address Stephen's feedback wrt. making existing netem functionality available in some way.
Otherwise I'll have to revert these changes.
Hmm... I thought there was kind of RFC for this, and it looked like Patrick's idea won 100% of votes, but I'm not good in counting...
http://marc.info/?l=linux-netdev&m=122469801712438&w=2
http://marc.info/?l=linux-netdev&m=122469674709761&w=2
Quoted 6 lines
Quoted 4 lines
Anyway, IMHO adding TBF etc. functionalities to tfifo doesn't make much sense, and if they are really needed it's better to revert these patches and chose one of the other ways of doing reorder proposed in this earlier thread.
Whats wrong with simply using TBF as parent qdisc of netem?
It works but does something slightly different.

Quoted 8 lines

Quoted 2 lines
netem inside TBF is like long delay network followed by choke on last hop TBF inside netem was like choke on uplink followed by long delay network.
David, this makes sense to me, so please revert these two patches. (Then, I think we're still at RFC stage.)
Thanks.
Jarek P.

Keep Jarek's patch, I am working on dummynet compatiable rate control.

Patrick McHardy

2008-11-04 21:42 UTC #8 lore raw

Quoted 8 lines · Stephen Hemminger

Quoted 6 lines
David, this makes sense to me, so please revert these two patches. (Then, I think we're still at RFC stage.)
Thanks.
Jarek P.
Keep Jarek's patch, I am working on dummynet compatiable rate control.

I'm still wondering what these incompatibilies are, in verifiable terms.

The dummynet algorithm looks like a not particulary cleverly implemented TBF, but I can't see why it would behave any differently than a suitably configured Linux TBF (cell size == 1) except for implementation differences outside its scope (clock, timers etc).

Patrick McHardy

2008-11-04 11:04 UTC #9 lore raw

In reply to Stephen Hemminger

Quoted 9 lines · Stephen Hemminger

Quoted 1 line · On Mon, 03 Nov 2008 12:20:25 +0100 Patrick McHardy <[email protected]> wrote:
Whats wrong with simply using TBF as parent qdisc of netem?
It works but does something slightly different.
netem inside TBF is like long delay network followed by choke on last hop TBF inside netem was like choke on uplink followed by long delay network.

The behaviour visible on either side is identical though.

TBF only affects the ->dequeue path, in which netem only handles delays. In both cases the total delay is the maximum of the absolute delay imposed by netem + the delay resulting from rate-limiting (the maximum because TBF can recharge while netem is delaying). Both values are independant from each other, so the maximum is always the same.