[PATCH 1/2] sch_netem: Remove classful functionality

[PATCH 1/2] sch_netem: Remove classful functionality

[Jarek Poplawski]

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 <jarkao2@gmail.com>

diff --git a/net/sched/sch_netem.c b/net/sched/sch_netem.c
index 3080bd6..2ad0959 100644
--- a/net/sched/sch_netem.c
+++ b/net/sched/sch_netem.c
@@ -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,
-- 
1.5.6.5

Re: [PATCH 1/2] sch_netem: Remove classful functionality

[Stephen Hemminger]

On Fri, 31 Oct 2008 13:20:10 +0000
Jarek Poplawski <jarkao2@gmail.com> 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 [...]".
> 
> 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 <jarkao2@gmail.com>
>

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.

Re: [PATCH 1/2] sch_netem: Remove classful functionality

[David Miller]

From: Jarek Poplawski <jarkao2@gmail.com>
Date: Fri, 31 Oct 2008 13:20:10 +0000

> 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 <jarkao2@gmail.com>

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!

Re: [PATCH 1/2] sch_netem: Remove classful functionality

[Jarek Poplawski]

On Sun, Nov 02, 2008 at 12:37:00AM -0700, David Miller wrote:
> From: Jarek Poplawski <jarkao2@gmail.com>
> Date: Fri, 31 Oct 2008 13:20:10 +0000
> 
> > 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 <jarkao2@gmail.com>
> 
> 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.

Re: [PATCH 1/2] sch_netem: Remove classful functionality

[Patrick McHardy]

Jarek Poplawski wrote:
> On Sun, Nov 02, 2008 at 12:37:00AM -0700, David Miller wrote:
>> From: Jarek Poplawski <jarkao2@gmail.com>
>> 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.

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

Re: [PATCH 1/2] sch_netem: Remove classful functionality

[Stephen Hemminger]

On Mon, 03 Nov 2008 12:20:25 +0100
Patrick McHardy <kaber@trash.net> wrote:

> Jarek Poplawski wrote:
> > On Sun, Nov 02, 2008 at 12:37:00AM -0700, David Miller wrote:
> >> From: Jarek Poplawski <jarkao2@gmail.com>
> >> 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.
> 
> 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.

Re: [PATCH 1/2] sch_netem: Remove classful functionality

[Jarek Poplawski]

On Mon, Nov 03, 2008 at 09:06:30AM -0800, Stephen Hemminger wrote:
> On Mon, 03 Nov 2008 12:20:25 +0100
> Patrick McHardy <kaber@trash.net> wrote:
> 
> > Jarek Poplawski wrote:
> > > On Sun, Nov 02, 2008 at 12:37:00AM -0700, David Miller wrote:
> > >> From: Jarek Poplawski <jarkao2@gmail.com>
> > >> 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.
> > 
> > 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.

Re: [PATCH 1/2] sch_netem: Remove classful functionality

[Patrick McHardy]

Stephen Hemminger wrote:
> On Mon, 03 Nov 2008 12:20:25 +0100
> Patrick McHardy <kaber@trash.net> 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.

Re: [PATCH 1/2] sch_netem: Remove classful functionality

[Stephen Hemminger]

On Tue, 4 Nov 2008 09:56:42 +0000
Jarek Poplawski <jarkao2@gmail.com> wrote:

> On Mon, Nov 03, 2008 at 09:06:30AM -0800, Stephen Hemminger wrote:
> > On Mon, 03 Nov 2008 12:20:25 +0100
> > Patrick McHardy <kaber@trash.net> wrote:
> > 
> > > Jarek Poplawski wrote:
> > > > On Sun, Nov 02, 2008 at 12:37:00AM -0700, David Miller wrote:
> > > >> From: Jarek Poplawski <jarkao2@gmail.com>
> > > >> 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.
> > > 
> > > 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.

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

Re: [PATCH 1/2] sch_netem: Remove classful functionality

[Patrick McHardy]

Stephen Hemminger wrote:
>> 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).