* [PATCH 1/2] net_sched: call qlen_notify only if child qdisc is empty
@ 2017-08-15 13:39 Konstantin Khlebnikov
2017-08-15 13:40 ` [PATCH 2/2] net_sched/hfsc: opencode trivial set_active() and set_passive() Konstantin Khlebnikov
` (2 more replies)
0 siblings, 3 replies; 6+ messages in thread
From: Konstantin Khlebnikov @ 2017-08-15 13:39 UTC (permalink / raw)
To: netdev, David S. Miller, Cong Wang
Cc: Jiri Kosina, Eric Dumazet, Jamal Hadi Salim
This callback is used for deactivating class in parent qdisc.
This is cheaper to test queue length right here.
Also this allows to catch draining screwed backlog and prevent
second deactivation of already inactive parent class which will
crash kernel for sure. Kernel with print warning at destruction
of child qdisc where no packets but backlog is not zero.
Signed-off-by: Konstantin Khlebnikov <khlebnikov@yandex-team.ru>
---
net/sched/sch_api.c | 10 +++++++++-
net/sched/sch_cbq.c | 3 +--
net/sched/sch_drr.c | 3 +--
net/sched/sch_hfsc.c | 6 ++----
net/sched/sch_htb.c | 3 +--
net/sched/sch_qfq.c | 3 +--
6 files changed, 15 insertions(+), 13 deletions(-)
diff --git a/net/sched/sch_api.c b/net/sched/sch_api.c
index bd24a550e0f9..18da45c0769c 100644
--- a/net/sched/sch_api.c
+++ b/net/sched/sch_api.c
@@ -752,6 +752,7 @@ void qdisc_tree_reduce_backlog(struct Qdisc *sch, unsigned int n,
const struct Qdisc_class_ops *cops;
unsigned long cl;
u32 parentid;
+ bool notify;
int drops;
if (n == 0 && len == 0)
@@ -764,6 +765,13 @@ void qdisc_tree_reduce_backlog(struct Qdisc *sch, unsigned int n,
if (sch->flags & TCQ_F_NOPARENT)
break;
+ /* Notify parent qdisc only if child qdisc becomes empty.
+ *
+ * If child was empty even before update then backlog
+ * counter is screwed and we skip notification because
+ * parent class is already passive.
+ */
+ notify = !sch->q.qlen && !WARN_ON_ONCE(!n);
/* TODO: perform the search on a per txq basis */
sch = qdisc_lookup(qdisc_dev(sch), TC_H_MAJ(parentid));
if (sch == NULL) {
@@ -771,7 +779,7 @@ void qdisc_tree_reduce_backlog(struct Qdisc *sch, unsigned int n,
break;
}
cops = sch->ops->cl_ops;
- if (cops->qlen_notify) {
+ if (notify && cops->qlen_notify) {
cl = cops->get(sch, parentid);
cops->qlen_notify(sch, cl);
cops->put(sch, cl);
diff --git a/net/sched/sch_cbq.c b/net/sched/sch_cbq.c
index 780db43300b1..1bdb0106f342 100644
--- a/net/sched/sch_cbq.c
+++ b/net/sched/sch_cbq.c
@@ -1385,8 +1385,7 @@ static void cbq_qlen_notify(struct Qdisc *sch, unsigned long arg)
{
struct cbq_class *cl = (struct cbq_class *)arg;
- if (cl->q->q.qlen == 0)
- cbq_deactivate_class(cl);
+ cbq_deactivate_class(cl);
}
static unsigned long cbq_get(struct Qdisc *sch, u32 classid)
diff --git a/net/sched/sch_drr.c b/net/sched/sch_drr.c
index a413dc1c2098..1d2f6235dfcf 100644
--- a/net/sched/sch_drr.c
+++ b/net/sched/sch_drr.c
@@ -246,8 +246,7 @@ static void drr_qlen_notify(struct Qdisc *csh, unsigned long arg)
{
struct drr_class *cl = (struct drr_class *)arg;
- if (cl->qdisc->q.qlen == 0)
- list_del(&cl->alist);
+ list_del(&cl->alist);
}
static int drr_dump_class(struct Qdisc *sch, unsigned long arg,
diff --git a/net/sched/sch_hfsc.c b/net/sched/sch_hfsc.c
index fd15200f8627..14c99870cdb6 100644
--- a/net/sched/sch_hfsc.c
+++ b/net/sched/sch_hfsc.c
@@ -1221,10 +1221,8 @@ hfsc_qlen_notify(struct Qdisc *sch, unsigned long arg)
{
struct hfsc_class *cl = (struct hfsc_class *)arg;
- if (cl->qdisc->q.qlen == 0) {
- update_vf(cl, 0, 0);
- set_passive(cl);
- }
+ update_vf(cl, 0, 0);
+ set_passive(cl);
}
static unsigned long
diff --git a/net/sched/sch_htb.c b/net/sched/sch_htb.c
index 5d65ec5207e9..dcf3c85e1f4f 100644
--- a/net/sched/sch_htb.c
+++ b/net/sched/sch_htb.c
@@ -1186,8 +1186,7 @@ static void htb_qlen_notify(struct Qdisc *sch, unsigned long arg)
{
struct htb_class *cl = (struct htb_class *)arg;
- if (cl->un.leaf.q->q.qlen == 0)
- htb_deactivate(qdisc_priv(sch), cl);
+ htb_deactivate(qdisc_priv(sch), cl);
}
static unsigned long htb_get(struct Qdisc *sch, u32 classid)
diff --git a/net/sched/sch_qfq.c b/net/sched/sch_qfq.c
index 0e16dfda0bd7..9caa959f91e1 100644
--- a/net/sched/sch_qfq.c
+++ b/net/sched/sch_qfq.c
@@ -1428,8 +1428,7 @@ static void qfq_qlen_notify(struct Qdisc *sch, unsigned long arg)
struct qfq_sched *q = qdisc_priv(sch);
struct qfq_class *cl = (struct qfq_class *)arg;
- if (cl->qdisc->q.qlen == 0)
- qfq_deactivate_class(q, cl);
+ qfq_deactivate_class(q, cl);
}
static int qfq_init_qdisc(struct Qdisc *sch, struct nlattr *opt)
^ permalink raw reply related [flat|nested] 6+ messages in thread* [PATCH 2/2] net_sched/hfsc: opencode trivial set_active() and set_passive()
2017-08-15 13:39 [PATCH 1/2] net_sched: call qlen_notify only if child qdisc is empty Konstantin Khlebnikov
@ 2017-08-15 13:40 ` Konstantin Khlebnikov
2017-08-16 17:57 ` David Miller
2017-08-16 17:22 ` [PATCH 1/2] net_sched: call qlen_notify only if child qdisc is empty Cong Wang
2017-08-16 17:56 ` David Miller
2 siblings, 1 reply; 6+ messages in thread
From: Konstantin Khlebnikov @ 2017-08-15 13:40 UTC (permalink / raw)
To: netdev, David S. Miller, Cong Wang
Cc: Jiri Kosina, Eric Dumazet, Jamal Hadi Salim
Any move comment abount update_vf() into right place.
Signed-off-by: Konstantin Khlebnikov <khlebnikov@yandex-team.ru>
---
net/sched/sch_hfsc.c | 45 ++++++++++++++++-----------------------------
1 file changed, 16 insertions(+), 29 deletions(-)
diff --git a/net/sched/sch_hfsc.c b/net/sched/sch_hfsc.c
index 14c99870cdb6..15f09cb9f1ff 100644
--- a/net/sched/sch_hfsc.c
+++ b/net/sched/sch_hfsc.c
@@ -829,28 +829,6 @@ update_vf(struct hfsc_class *cl, unsigned int len, u64 cur_time)
}
}
-static void
-set_active(struct hfsc_class *cl, unsigned int len)
-{
- if (cl->cl_flags & HFSC_RSC)
- init_ed(cl, len);
- if (cl->cl_flags & HFSC_FSC)
- init_vf(cl, len);
-
-}
-
-static void
-set_passive(struct hfsc_class *cl)
-{
- if (cl->cl_flags & HFSC_RSC)
- eltree_remove(cl);
-
- /*
- * vttree is now handled in update_vf() so that update_vf(cl, 0, 0)
- * needs to be called explicitly to remove a class from vttree.
- */
-}
-
static unsigned int
qdisc_peek_len(struct Qdisc *sch)
{
@@ -1221,8 +1199,12 @@ hfsc_qlen_notify(struct Qdisc *sch, unsigned long arg)
{
struct hfsc_class *cl = (struct hfsc_class *)arg;
+ /* vttree is now handled in update_vf() so that update_vf(cl, 0, 0)
+ * needs to be called explicitly to remove a class from vttree.
+ */
update_vf(cl, 0, 0);
- set_passive(cl);
+ if (cl->cl_flags & HFSC_RSC)
+ eltree_remove(cl);
}
static unsigned long
@@ -1583,7 +1565,12 @@ hfsc_enqueue(struct sk_buff *skb, struct Qdisc *sch, struct sk_buff **to_free)
}
if (cl->qdisc->q.qlen == 1) {
- set_active(cl, qdisc_pkt_len(skb));
+ unsigned int len = qdisc_pkt_len(skb);
+
+ if (cl->cl_flags & HFSC_RSC)
+ init_ed(cl, len);
+ if (cl->cl_flags & HFSC_FSC)
+ init_vf(cl, len);
/*
* If this is the first packet, isolate the head so an eventual
* head drop before the first dequeue operation has no chance
@@ -1647,18 +1634,18 @@ hfsc_dequeue(struct Qdisc *sch)
if (realtime)
cl->cl_cumul += qdisc_pkt_len(skb);
- if (cl->qdisc->q.qlen != 0) {
- if (cl->cl_flags & HFSC_RSC) {
+ if (cl->cl_flags & HFSC_RSC) {
+ if (cl->qdisc->q.qlen != 0) {
/* update ed */
next_len = qdisc_peek_len(cl->qdisc);
if (realtime)
update_ed(cl, next_len);
else
update_d(cl, next_len);
+ } else {
+ /* the class becomes passive */
+ eltree_remove(cl);
}
- } else {
- /* the class becomes passive */
- set_passive(cl);
}
qdisc_bstats_update(sch, skb);
^ permalink raw reply related [flat|nested] 6+ messages in thread* Re: [PATCH 1/2] net_sched: call qlen_notify only if child qdisc is empty
2017-08-15 13:39 [PATCH 1/2] net_sched: call qlen_notify only if child qdisc is empty Konstantin Khlebnikov
2017-08-15 13:40 ` [PATCH 2/2] net_sched/hfsc: opencode trivial set_active() and set_passive() Konstantin Khlebnikov
@ 2017-08-16 17:22 ` Cong Wang
2017-08-16 17:32 ` Konstantin Khlebnikov
2017-08-16 17:56 ` David Miller
2 siblings, 1 reply; 6+ messages in thread
From: Cong Wang @ 2017-08-16 17:22 UTC (permalink / raw)
To: Konstantin Khlebnikov
Cc: Linux Kernel Network Developers, David S. Miller, Jiri Kosina,
Eric Dumazet, Jamal Hadi Salim
On Tue, Aug 15, 2017 at 6:39 AM, Konstantin Khlebnikov
<khlebnikov@yandex-team.ru> wrote:
> This callback is used for deactivating class in parent qdisc.
> This is cheaper to test queue length right here.
>
> Also this allows to catch draining screwed backlog and prevent
> second deactivation of already inactive parent class which will
> crash kernel for sure. Kernel with print warning at destruction
> of child qdisc where no packets but backlog is not zero.
Good cleanup. Please explicitly mark patches like this as
for net-next.
Just one comment below.
> diff --git a/net/sched/sch_api.c b/net/sched/sch_api.c
> index bd24a550e0f9..18da45c0769c 100644
> --- a/net/sched/sch_api.c
> +++ b/net/sched/sch_api.c
> @@ -752,6 +752,7 @@ void qdisc_tree_reduce_backlog(struct Qdisc *sch, unsigned int n,
> const struct Qdisc_class_ops *cops;
> unsigned long cl;
> u32 parentid;
> + bool notify;
> int drops;
>
> if (n == 0 && len == 0)
> @@ -764,6 +765,13 @@ void qdisc_tree_reduce_backlog(struct Qdisc *sch, unsigned int n,
>
> if (sch->flags & TCQ_F_NOPARENT)
> break;
> + /* Notify parent qdisc only if child qdisc becomes empty.
> + *
> + * If child was empty even before update then backlog
> + * counter is screwed and we skip notification because
> + * parent class is already passive.
> + */
> + notify = !sch->q.qlen && !WARN_ON_ONCE(!n);
Since 'n' never changes in this function, can we just move
this WARN_ON_ONCE() right before the loop??
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 1/2] net_sched: call qlen_notify only if child qdisc is empty
2017-08-16 17:22 ` [PATCH 1/2] net_sched: call qlen_notify only if child qdisc is empty Cong Wang
@ 2017-08-16 17:32 ` Konstantin Khlebnikov
0 siblings, 0 replies; 6+ messages in thread
From: Konstantin Khlebnikov @ 2017-08-16 17:32 UTC (permalink / raw)
To: Cong Wang
Cc: Linux Kernel Network Developers, David S. Miller, Jiri Kosina,
Eric Dumazet, Jamal Hadi Salim
On 16.08.2017 20:22, Cong Wang wrote:
> On Tue, Aug 15, 2017 at 6:39 AM, Konstantin Khlebnikov
> <khlebnikov@yandex-team.ru> wrote:
>> This callback is used for deactivating class in parent qdisc.
>> This is cheaper to test queue length right here.
>>
>> Also this allows to catch draining screwed backlog and prevent
>> second deactivation of already inactive parent class which will
>> crash kernel for sure. Kernel with print warning at destruction
>> of child qdisc where no packets but backlog is not zero.
>
> Good cleanup. Please explicitly mark patches like this as
> for net-next.
>
> Just one comment below.
>
>
>> diff --git a/net/sched/sch_api.c b/net/sched/sch_api.c
>> index bd24a550e0f9..18da45c0769c 100644
>> --- a/net/sched/sch_api.c
>> +++ b/net/sched/sch_api.c
>> @@ -752,6 +752,7 @@ void qdisc_tree_reduce_backlog(struct Qdisc *sch, unsigned int n,
>> const struct Qdisc_class_ops *cops;
>> unsigned long cl;
>> u32 parentid;
>> + bool notify;
>> int drops;
>>
>> if (n == 0 && len == 0)
>> @@ -764,6 +765,13 @@ void qdisc_tree_reduce_backlog(struct Qdisc *sch, unsigned int n,
>>
>> if (sch->flags & TCQ_F_NOPARENT)
>> break;
>> + /* Notify parent qdisc only if child qdisc becomes empty.
>> + *
>> + * If child was empty even before update then backlog
>> + * counter is screwed and we skip notification because
>> + * parent class is already passive.
>> + */
>> + notify = !sch->q.qlen && !WARN_ON_ONCE(!n);
>
> Since 'n' never changes in this function, can we just move
> this WARN_ON_ONCE() right before the loop??
>
Here warning evaluated only if qlen is zero.
'n' could be zero not together with non-zero 'len' and zero 'qlen'.
This means queue already was empty before reduce but caller somehow reduced it even further.
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 1/2] net_sched: call qlen_notify only if child qdisc is empty
2017-08-15 13:39 [PATCH 1/2] net_sched: call qlen_notify only if child qdisc is empty Konstantin Khlebnikov
2017-08-15 13:40 ` [PATCH 2/2] net_sched/hfsc: opencode trivial set_active() and set_passive() Konstantin Khlebnikov
2017-08-16 17:22 ` [PATCH 1/2] net_sched: call qlen_notify only if child qdisc is empty Cong Wang
@ 2017-08-16 17:56 ` David Miller
2 siblings, 0 replies; 6+ messages in thread
From: David Miller @ 2017-08-16 17:56 UTC (permalink / raw)
To: khlebnikov; +Cc: netdev, xiyou.wangcong, jkosina, edumazet, jhs
From: Konstantin Khlebnikov <khlebnikov@yandex-team.ru>
Date: Tue, 15 Aug 2017 16:39:59 +0300
> This callback is used for deactivating class in parent qdisc.
> This is cheaper to test queue length right here.
>
> Also this allows to catch draining screwed backlog and prevent
> second deactivation of already inactive parent class which will
> crash kernel for sure. Kernel with print warning at destruction
> of child qdisc where no packets but backlog is not zero.
>
> Signed-off-by: Konstantin Khlebnikov <khlebnikov@yandex-team.ru>
Applied.
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2017-08-16 17:57 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2017-08-15 13:39 [PATCH 1/2] net_sched: call qlen_notify only if child qdisc is empty Konstantin Khlebnikov
2017-08-15 13:40 ` [PATCH 2/2] net_sched/hfsc: opencode trivial set_active() and set_passive() Konstantin Khlebnikov
2017-08-16 17:57 ` David Miller
2017-08-16 17:22 ` [PATCH 1/2] net_sched: call qlen_notify only if child qdisc is empty Cong Wang
2017-08-16 17:32 ` Konstantin Khlebnikov
2017-08-16 17:56 ` David Miller
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).