On 16.08.2017 20:22, Cong Wang wrote:
On Tue, Aug 15, 2017 at 6:39 AM, Konstantin Khlebnikov
<khlebni...@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.
