Patchwork [take,2] pkt_sched: Fix qdisc_watchdog() vs. dev_deactivate() race

login
register
mail settings
Submitter Jarek Poplawski
Date Sept. 23, 2008, 8:02 a.m.
Message ID <20080923080240.GA4692@ff.dom.local>
Download mbox | patch
Permalink /patch/1027/
State Accepted
Headers show

Comments

Jarek Poplawski - Sept. 23, 2008, 8:02 a.m.
On Mon, Sep 22, 2008 at 10:16:58PM -0700, David Miller wrote:
> From: Jarek Poplawski <jarkao2@gmail.com>
> Date: Sun, 21 Sep 2008 13:15:51 +0200
> 
> > That's why I think you should reconsider this simple solution for now,
> > until somebody proves this is wrong or something else is better.
> 
> Ok, that sounds reasonable.  I've added those three patches to net-next-2.6
> and will push those out after some build tests.

OK, then we have to say B and try this all. BTW, I guess, after this
change we could have similar effect as reported by Alexander Duyck
while testing his solution for this problem, namely the higher drop
rate in some cases, which I can only explain as: less time in
requeuing more time for new enqueuing. Of course, if I'm right, this
"bug" should be rather "fixed" with longer queues or some other
throttle mechanism.

Thanks,
Jarek P.

-------------------->

pkt_sched: Remove the tx queue state check in qdisc_run()

The current check wrongly uses the state of one (currently the first)
tx queue for all tx queues in case of non-default qdiscs. This check
mainly prevented requeuing loop with __netif_schedule(), but now it's
controlled inside __qdisc_run(), while dequeuing. The wrongness of
this check was first noticed by Herbert Xu.

Signed-off-by: Jarek Poplawski <jarkao2@gmail.com>

---

 include/net/pkt_sched.h |    5 +----
 1 files changed, 1 insertions(+), 4 deletions(-)

--
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
David Miller - Sept. 23, 2008, 8:06 a.m.
From: Jarek Poplawski <jarkao2@gmail.com>
Date: Tue, 23 Sep 2008 08:02:40 +0000

> OK, then we have to say B and try this all. BTW, I guess, after this
> change we could have similar effect as reported by Alexander Duyck
> while testing his solution for this problem, namely the higher drop
> rate in some cases, which I can only explain as: less time in
> requeuing more time for new enqueuing. Of course, if I'm right, this
> "bug" should be rather "fixed" with longer queues or some other
> throttle mechanism.
 ...
> pkt_sched: Remove the tx queue state check in qdisc_run()
> 
> The current check wrongly uses the state of one (currently the first)
> tx queue for all tx queues in case of non-default qdiscs. This check
> mainly prevented requeuing loop with __netif_schedule(), but now it's
> controlled inside __qdisc_run(), while dequeuing. The wrongness of
> this check was first noticed by Herbert Xu.
> 
> Signed-off-by: Jarek Poplawski <jarkao2@gmail.com>

Agreed and applied, thanks Jarek.
--
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

Patch

diff --git a/include/net/pkt_sched.h b/include/net/pkt_sched.h
index b786a5b..4082f39 100644
--- a/include/net/pkt_sched.h
+++ b/include/net/pkt_sched.h
@@ -90,10 +90,7 @@  extern void __qdisc_run(struct Qdisc *q);
 
 static inline void qdisc_run(struct Qdisc *q)
 {
-	struct netdev_queue *txq = q->dev_queue;
-
-	if (!netif_tx_queue_stopped(txq) &&
-	    !test_and_set_bit(__QDISC_STATE_RUNNING, &q->state))
+	if (!test_and_set_bit(__QDISC_STATE_RUNNING, &q->state))
 		__qdisc_run(q);
 }