| Message ID | 20081031132010.GA18895@ff.dom.local |
|---|---|
| State | Accepted, archived |
| Delegated to: | David Miller |
| Headers | show |
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. -- 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
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! -- 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
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. -- 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
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? -- 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
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. -- 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
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. -- 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
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. -- 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
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. -- 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
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). -- 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
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,
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>