netdev
[Top] [All Lists]

Re: [PATCH] PKT_SCHED: Initialize list field in dummy qdiscs

To: Thomas Graf <tgraf@xxxxxxx>
Subject: Re: [PATCH] PKT_SCHED: Initialize list field in dummy qdiscs
From: Patrick McHardy <kaber@xxxxxxxxx>
Date: Fri, 05 Nov 2004 17:12:26 +0100
Cc: davem@xxxxxxxxxxxxx, netdev@xxxxxxxxxxx, spam@xxxxxxxxxxxxx, kuznet@xxxxxxxxxxxxx, jmorris@xxxxxxxxxx
In-reply-to: <20041105141640.GQ19714@rei.reeler.org>
References: <418B4C7C.8000402@crocom.com.pl> <20041105115430.GP19714@rei.reeler.org> <418B4C7C.8000402@crocom.com.pl> <20041105141640.GQ19714@rei.reeler.org>
Sender: netdev-bounce@xxxxxxxxxxx
User-agent: Mozilla/5.0 (X11; U; Linux x86_64; en-US; rv:1.7.3) Gecko/20041008 Debian/1.7.3-5
Thomas Graf wrote:

-   if (dev) {
-       struct Qdisc *q, **qp;
-       for (qp = &qdisc->dev->qdisc_list; (q=*qp) != NULL; qp = &q->next) {
-           if (q == qdisc) {
-               *qp = q->next;
-               break;
-           }
-       }
-   }
+   list_del(&qdisc->list);

Nice cleanup, although it assumes that everyone calling qdisc_destroy provides a
Qdisc which is actually linked or at least has an initialized list node. This
was not the true for noqueue and noop dummy qdiscs.

Nice catch. I can't understand how you triggered that oops,
though. The noop- and noqueue-qdiscs used without qdisc_create_*
are not refcounted, so I would expect:

void qdisc_destroy(struct Qdisc *qdisc)
{
       if (!atomic_dec_and_test(&qdisc->refcnt))
               return;

to underflow and return until refcnt finally reaches 0 again.
Can you explain, please ?

Regards
Patrick

Signed-off-by: Thomas Graf <tgraf@xxxxxxx>

--- linux-2.6.10-rc1-bk14.orig/net/sched/sch_generic.c  2004-11-05 
01:11:17.000000000 +0100
+++ linux-2.6.10-rc1-bk14/net/sched/sch_generic.c       2004-11-05 
15:05:54.000000000 +0100
@@ -280,6 +280,7 @@
        .dequeue        =       noop_dequeue,
        .flags          =       TCQ_F_BUILTIN,
        .ops            =       &noop_qdisc_ops,    
+       .list           =       LIST_HEAD_INIT(noop_qdisc.list),
};

struct Qdisc_ops noqueue_qdisc_ops = {
@@ -298,6 +299,7 @@
        .dequeue        =       noop_dequeue,
        .flags          =       TCQ_F_BUILTIN,
        .ops            =       &noqueue_qdisc_ops,
+       .list           =       LIST_HEAD_INIT(noqueue_qdisc.list),
};





<Prev in Thread] Current Thread [Next in Thread>