From: Cong Wang <hidden> Date: 2015-08-26 22:41:33
The main goal of this patchset is to improve the behavior of setting
the default qdisc. Current behavior has no error check, no check for
ingress and _can_ crash the kernel with some buggy implementation.
We only have flags for each instance of qdisc's, for flags like
if a qdisc is a fifo qdisc, they can simply be moved into qdisc->ops,
as shown by patch 1, 2, 5. Patch 4 just uses this for error checking
when setting default qdisc.
Cc: Jamal Hadi Salim <jhs@mojatatu.com>
Signed-off-by: Cong Wang <redacted>
---
Cong Wang (5):
net_sched: move some qdisc flag into qdisc ops
net_sched: move TCQ_F_MQROOT into qdisc ops
net_sched: use a flag to indicate fifo qdiscs instead of the name
net_sched: forbid setting default qdisc to inappropriate ones
net_sched: move ingress flag into qdisc ops
include/net/sch_generic.h | 9 ++++++---
net/sched/sch_api.c | 40 +++++++++++++++++++++++++++-------------
net/sched/sch_fifo.c | 6 ++++--
net/sched/sch_fq.c | 1 +
net/sched/sch_fq_codel.c | 1 +
net/sched/sch_generic.c | 11 ++++++-----
net/sched/sch_ingress.c | 1 +
net/sched/sch_mq.c | 2 +-
net/sched/sch_mqprio.c | 2 +-
net/sched/sch_sfq.c | 1 +
10 files changed, 49 insertions(+), 25 deletions(-)
--
1.8.3.1
From: Cong Wang <hidden> Date: 2015-08-26 22:41:34
For those static flags, that is never changed dynamically,
we could just move them into qdisc->ops. This will be used
by the following patches.
Cc: Jamal Hadi Salim <jhs@mojatatu.com>
Signed-off-by: Cong Wang <redacted>
---
include/net/sch_generic.h | 3 ++-
net/sched/sch_api.c | 4 ++--
net/sched/sch_generic.c | 10 +++++-----
3 files changed, 9 insertions(+), 8 deletions(-)
From: Cong Wang <hidden> Date: 2015-08-26 22:41:35
It is just another static flag which can be moved.
Cc: Jamal Hadi Salim <jhs@mojatatu.com>
Signed-off-by: Cong Wang <redacted>
---
include/net/sch_generic.h | 2 +-
net/sched/sch_api.c | 6 +++---
net/sched/sch_mq.c | 2 +-
net/sched/sch_mqprio.c | 2 +-
4 files changed, 6 insertions(+), 6 deletions(-)
From: Cong Wang <hidden> Date: 2015-08-26 22:41:36
Relying on its name is a bad practice.
Cc: Jamal Hadi Salim <jhs@mojatatu.com>
Signed-off-by: Cong Wang <redacted>
---
include/net/sch_generic.h | 1 +
net/sched/sch_fifo.c | 6 ++++--
net/sched/sch_generic.c | 1 +
3 files changed, 6 insertions(+), 2 deletions(-)
@@ -143,8 +146,7 @@ int fifo_set_limit(struct Qdisc *q, unsigned int limit)structnlattr*nla;intret=-ENOMEM;-/* Hack to avoid sending change message to non-FIFO */-if(strncmp(q->ops->id+1,"fifo",4)!=0)+if(!(q->ops->flags&QDISC_F_FIFO))return0;nla=kmalloc(nla_attr_size(sizeof(structtc_fifo_qopt)),GFP_KERNEL);
From: Cong Wang <hidden> Date: 2015-08-26 22:41:37
Currently there is no check for if a qdisc is appropriate
to be used as the default qdisc. This causes we get no
error even we set the default qdisc to an inappropriate one
but an error will be shown up later. This is not good.
Also, for qdisc's like HTB, kernel will just crash when
we use it as default qdisc, because some data structures are
not even initialized yet before checking opt == NULL, the cleanup
doing ->reset() or ->destroy() on them will just crash.
Let's fail as early as we can.
Cc: Jamal Hadi Salim <jhs@mojatatu.com>
Cc: Stephen Hemminger <stephen@networkplumber.org>
Signed-off-by: Cong Wang <redacted>
---
include/net/sch_generic.h | 1 +
net/sched/sch_api.c | 12 ++++++++++--
net/sched/sch_fifo.c | 6 +++---
net/sched/sch_fq.c | 1 +
net/sched/sch_fq_codel.c | 1 +
net/sched/sch_generic.c | 2 +-
net/sched/sch_mq.c | 2 +-
net/sched/sch_sfq.c | 1 +
8 files changed, 19 insertions(+), 7 deletions(-)
@@ -243,13 +244,20 @@ int qdisc_set_default(const char *name)}if(ops){+if(!(ops->flags&QDISC_F_DEFAULTABLE)){+err=-EINVAL;+gotounlock;+}/* Set new default */module_put(default_qdisc_ops->owner);default_qdisc_ops=ops;+}else{+err=-ENOENT;}-write_unlock(&qdisc_mod_lock);-returnops?0:-ENOENT;+unlock:+write_unlock(&qdisc_mod_lock);+returnerr;}/* We know handle. Find qdisc among all qdisc's attached to device
From: Stephen Hemminger <stephen@networkplumber.org> Date: 2015-08-27 00:08:00
On Wed, 26 Aug 2015 15:41:26 -0700
Cong Wang [off-list ref] wrote:
Currently there is no check for if a qdisc is appropriate
to be used as the default qdisc. This causes we get no
error even we set the default qdisc to an inappropriate one
but an error will be shown up later. This is not good.
Also, for qdisc's like HTB, kernel will just crash when
we use it as default qdisc, because some data structures are
not even initialized yet before checking opt == NULL, the cleanup
doing ->reset() or ->destroy() on them will just crash.
From: Cong Wang <hidden> Date: 2015-08-27 00:14:56
On Wed, Aug 26, 2015 at 5:08 PM, Stephen Hemminger
[off-list ref] wrote:
On Wed, 26 Aug 2015 15:41:26 -0700
Cong Wang [off-list ref] wrote:
quoted
Currently there is no check for if a qdisc is appropriate
to be used as the default qdisc. This causes we get no
error even we set the default qdisc to an inappropriate one
but an error will be shown up later. This is not good.
Also, for qdisc's like HTB, kernel will just crash when
we use it as default qdisc, because some data structures are
not even initialized yet before checking opt == NULL, the cleanup
doing ->reset() or ->destroy() on them will just crash.
Why not fix the buggy one's instead?
They are not exactly buggy, since they are fine in other ->init() calling
cases.
As in the first paragraph you quoted from me, it is more like
a usability issue, for example ingress qdisc can be set as default
without any error at any time.
From: David Miller <davem@davemloft.net> Date: 2015-08-27 22:30:50
From: Cong Wang <redacted>
Date: Wed, 26 Aug 2015 15:41:26 -0700
Currently there is no check for if a qdisc is appropriate
to be used as the default qdisc. This causes we get no
error even we set the default qdisc to an inappropriate one
but an error will be shown up later. This is not good.
Also, for qdisc's like HTB, kernel will just crash when
we use it as default qdisc, because some data structures are
not even initialized yet before checking opt == NULL, the cleanup
doing ->reset() or ->destroy() on them will just crash.
Let's fail as early as we can.
Cc: Jamal Hadi Salim <jhs@mojatatu.com>
Cc: Stephen Hemminger <stephen@networkplumber.org>
Signed-off-by: Cong Wang <redacted>
I don't like this.
The situation is that some sophisticated qdiscs can function without
explicit parameters, some cannot.
That is the problem you need to solve. For example, if "opts" is NULL
HTB should use a reasonable set of defaults instead of failing.
Furthermore, you can improve the behavior when this happens.
When qdisc_create_dflt() returns NULL because ops->init() fails, do
something reasonable.
I'm not applying this patch series, it papers over the issue rather
than actually addressing it properly.
From: Cong Wang <hidden> Date: 2015-08-27 22:39:13
On Thu, Aug 27, 2015 at 3:30 PM, David Miller [off-list ref] wrote:
I don't like this.
The situation is that some sophisticated qdiscs can function without
explicit parameters, some cannot.
This is exactly what this patch tries to solve... I already mark those
with a DEFAULTABLE flag.
That is the problem you need to solve. For example, if "opts" is NULL
HTB should use a reasonable set of defaults instead of failing.
Furthermore, you can improve the behavior when this happens.
When qdisc_create_dflt() returns NULL because ops->init() fails, do
something reasonable.
I'm not applying this patch series, it papers over the issue rather
than actually addressing it properly.
I wish I never mention that crash, which leads you to think I am trying
to fix a crash rather than a more important issue, usability. See below.
Forget about the crash, consider the current behavior:
# echo htb > default_qdisc
# succeed without any error
(then add a root qdisc and remove it)
# failure shown here in dmesg
And compare it with the behavior after my patch:
# echo htb > default_qdisc
Invalid arguments
I think this is clearly an improvement.
Thanks.
From: David Miller <davem@davemloft.net> Date: 2015-08-27 22:42:03
From: Cong Wang <redacted>
Date: Thu, 27 Aug 2015 15:39:12 -0700
On Thu, Aug 27, 2015 at 3:30 PM, David Miller [off-list ref] wrote:
quoted
I don't like this.
The situation is that some sophisticated qdiscs can function without
explicit parameters, some cannot.
This is exactly what this patch tries to solve... I already mark those
with a DEFAULTABLE flag.
It is not solving it, if you were solving it you would make all qdisc's
capable of being default instead of giving them what is essentially
"this is broken" flag.
I wish I never mention that crash, which leads you to think I am trying
to fix a crash rather than a more important issue, usability. See below.
Forget about the crash, consider the current behavior:
# echo htb > default_qdisc
# succeed without any error
(then add a root qdisc and remove it)
# failure shown here in dmesg
And compare it with the behavior after my patch:
# echo htb > default_qdisc
Invalid arguments
I think this is clearly an improvement.
Long term it's the wrong fix, trust me.
If you fix it properly, by making every qdisc capable of being ->init()'d
without explicit parameters, it will be the best behavior overall.
From: Cong Wang <hidden> Date: 2015-08-27 22:47:56
On Thu, Aug 27, 2015 at 3:42 PM, David Miller [off-list ref] wrote:
Long term it's the wrong fix, trust me.
So we have plan to convert some non-defaultable qdisc to defaultable?
I don't see a reason here.
If you fix it properly, by making every qdisc capable of being ->init()'d
without explicit parameters, it will be the best behavior overall.
The problem is ->init() is not even called when setting it as default,
since setting a default qdisc doesn't need to create a qdisc. This is
why the flag has to be in ops->flags rather than qdisc->flags.
From: David Miller <davem@davemloft.net> Date: 2015-08-27 23:18:51
From: Cong Wang <redacted>
Date: Thu, 27 Aug 2015 15:47:55 -0700
On Thu, Aug 27, 2015 at 3:42 PM, David Miller [off-list ref] wrote:
quoted
If you fix it properly, by making every qdisc capable of being ->init()'d
without explicit parameters, it will be the best behavior overall.
The problem is ->init() is not even called when setting it as default,
since setting a default qdisc doesn't need to create a qdisc. This is
why the flag has to be in ops->flags rather than qdisc->flags.
Just sounds like another shortcoming of how default qdiscs are handled,
rather than a reason to not fix things properly.
From: Cong Wang <hidden> Date: 2015-08-28 01:49:10
On Thu, Aug 27, 2015 at 4:18 PM, David Miller [off-list ref] wrote:
From: Cong Wang <redacted>
Date: Thu, 27 Aug 2015 15:47:55 -0700
quoted
On Thu, Aug 27, 2015 at 3:42 PM, David Miller [off-list ref] wrote:
quoted
If you fix it properly, by making every qdisc capable of being ->init()'d
without explicit parameters, it will be the best behavior overall.
The problem is ->init() is not even called when setting it as default,
since setting a default qdisc doesn't need to create a qdisc. This is
why the flag has to be in ops->flags rather than qdisc->flags.
Just sounds like another shortcoming of how default qdiscs are handled,
It has to, due to its definition. I don't see any other way except we change
the meaning of the default qdisc.
rather than a reason to not fix things properly.
If you mean the crash, my patch can fix it too by simply rejecting the
invalid and buggy case like HTB, even though I do have local patches
to fix it directly.
Like I said, the more important question is not if it crashes, it is if we
should reject invalid case as early as possible or just wait for an error
to happen later.
From: David Miller <davem@davemloft.net> Date: 2015-08-28 04:24:01
From: Cong Wang <redacted>
Date: Thu, 27 Aug 2015 18:49:09 -0700
On Thu, Aug 27, 2015 at 4:18 PM, David Miller [off-list ref] wrote:
quoted
From: Cong Wang <redacted>
Date: Thu, 27 Aug 2015 15:47:55 -0700
quoted
On Thu, Aug 27, 2015 at 3:42 PM, David Miller [off-list ref] wrote:
quoted
If you fix it properly, by making every qdisc capable of being ->init()'d
without explicit parameters, it will be the best behavior overall.
The problem is ->init() is not even called when setting it as default,
since setting a default qdisc doesn't need to create a qdisc. This is
why the flag has to be in ops->flags rather than qdisc->flags.
Just sounds like another shortcoming of how default qdiscs are handled,
It has to, due to its definition. I don't see any other way except we change
the meaning of the default qdisc.
We are talking past eachother.
If a default qdisc like HTB is choosen, we invoke the ->init() function
and we change the HTB ->init() function to do something reasonable
if a NULL set of configuration attributes is given. ie. make HTB use
some defaults.
Please explain to me why this won't fix the problem.
From: Jamal Hadi Salim <jhs@mojatatu.com> Date: 2015-08-28 12:26:56
On 08/28/15 00:23, David Miller wrote:
If a default qdisc like HTB is choosen, we invoke the ->init() function
and we change the HTB ->init() function to do something reasonable
if a NULL set of configuration attributes is given. ie. make HTB use
some defaults.
That may work. Or to reduce ambiguity introduce qdisc->set_default().
cheers,
jamal
From: Cong Wang <hidden> Date: 2015-08-28 21:39:45
On Thu, Aug 27, 2015 at 9:23 PM, David Miller [off-list ref] wrote:
From: Cong Wang <redacted>
Date: Thu, 27 Aug 2015 18:49:09 -0700
quoted
On Thu, Aug 27, 2015 at 4:18 PM, David Miller [off-list ref] wrote:
quoted
From: Cong Wang <redacted>
Date: Thu, 27 Aug 2015 15:47:55 -0700
quoted
On Thu, Aug 27, 2015 at 3:42 PM, David Miller [off-list ref] wrote:
quoted
If you fix it properly, by making every qdisc capable of being ->init()'d
without explicit parameters, it will be the best behavior overall.
The problem is ->init() is not even called when setting it as default,
since setting a default qdisc doesn't need to create a qdisc. This is
why the flag has to be in ops->flags rather than qdisc->flags.
Just sounds like another shortcoming of how default qdiscs are handled,
It has to, due to its definition. I don't see any other way except we change
the meaning of the default qdisc.
We are talking past eachother.
If a default qdisc like HTB is choosen, we invoke the ->init() function
and we change the HTB ->init() function to do something reasonable
if a NULL set of configuration attributes is given. ie. make HTB use
some defaults.
Please explain to me why this won't fix the problem.
It does, and it is exactly what my local patch does.
The problem is setting HTB as default is already invalid from the
beginning, so HTB->init() is not supposed be called since we can
reject it earlier, and this is my whole point.
If HTB is not a good example, as using HTB as default might
make some sense, please try ingress qdisc, no error at _any_ time,
and apparently defaulting to ingress is totally non-sense.
From: David Miller <davem@davemloft.net> Date: 2015-08-28 23:20:40
From: Cong Wang <redacted>
Date: Fri, 28 Aug 2015 14:39:45 -0700
If HTB is not a good example, as using HTB as default might
make some sense, please try ingress qdisc, no error at _any_ time,
and apparently defaulting to ingress is totally non-sense.
I agree that ingress should have some special flag that prevents
it from being an egress qdisc.
But HTB definitely should be allowed.
From: Jamal Hadi Salim <jhs@mojatatu.com> Date: 2015-08-30 19:07:27
On 08/28/15 19:20, David Miller wrote:
But HTB definitely should be allowed.
Problem with most non-work conserving schedulers is what the meaning
of default resources means; example, for HTB:
What is the default bandwidth you allocate to a class of users?
cheers,
jamal
From: Cong Wang <hidden> Date: 2015-09-02 06:05:46
On Sun, Aug 30, 2015 at 12:07 PM, Jamal Hadi Salim [off-list ref] wrote:
On 08/28/15 19:20, David Miller wrote:
quoted
But HTB definitely should be allowed.
Problem with most non-work conserving schedulers is what the meaning
of default resources means; example, for HTB:
What is the default bandwidth you allocate to a class of users?
Exactly, that is why it has to need at least one parameter for bandwidth,
while default qdisc requires no parameter.
On Sun, Aug 30, 2015 at 12:07 PM, Jamal Hadi Salim [off-list ref] wrote:
quoted
On 08/28/15 19:20, David Miller wrote:
quoted
But HTB definitely should be allowed.
Problem with most non-work conserving schedulers is what the meaning
of default resources means; example, for HTB:
What is the default bandwidth you allocate to a class of users?
Exactly, that is why it has to need at least one parameter for bandwidth,
while default qdisc requires no parameter.
On Sun, Aug 30, 2015 at 12:07 PM, Jamal Hadi Salim [off-list ref] wrote:
quoted
On 08/28/15 19:20, David Miller wrote:
quoted
But HTB definitely should be allowed.
Problem with most non-work conserving schedulers is what the meaning
of default resources means; example, for HTB:
What is the default bandwidth you allocate to a class of users?
Exactly, that is why it has to need at least one parameter for bandwidth,
while default qdisc requires no parameter.
Ok I'm convinced.
Ok, I will update the changelog to clarify this and resend.
Thanks.