Re: [PATCH] pkt_sched: Destroy gen estimators under rtnl_lock().
From: Jarek Poplawski <hidden>
Date: 2008-08-21 12:55:08
On Thu, Aug 21, 2008 at 10:48:34PM +1000, Herbert Xu wrote:
On Thu, Aug 21, 2008 at 10:35:38PM +1000, Herbert Xu wrote:quoted
You're right, this doesn't work at all. In fact it's been broken even before we removed the root lock. The problem is that we used to have one big linked list for each device. That was protected by the device qdisc lock. Now we have one list for each txq and qdisc_lookup walks every single txq. This means that no single qdisc root lock can protect this anymore.How about going back to a single list per-device again? This list is only used on the slow path (well anything that tries to walk a potentially unbounded linked list is slow :), and qdisc_lookup walks through everything anyway. We'll need to then add a new lock to protect this list, until we remove requeue. Actually just doing the locking will be sufficient. Something like this totally untested patch (I've abused your tx global lock):
Alas I've to have a break now, anyway I think for now "my" proposal should be safer since it worked like this before... But of course after good checking your patch should be better. Jarek P.
quoted hunk ↗ jump to hunk
diff --git a/net/sched/sch_api.c b/net/sched/sch_api.c index ef0efec..3f5f9b9 100644 --- a/net/sched/sch_api.c +++ b/net/sched/sch_api.c@@ -202,16 +202,25 @@ struct Qdisc *qdisc_match_from_root(struct Qdisc *root, u32 handle) struct Qdisc *qdisc_lookup(struct net_device *dev, u32 handle) { unsigned int i; + struct Qdisc *q; + + spin_lock_bh(&dev->tx_global_lock); for (i = 0; i < dev->num_tx_queues; i++) { struct netdev_queue *txq = netdev_get_tx_queue(dev, i); - struct Qdisc *q, *txq_root = txq->qdisc_sleeping; + struct Qdisc *txq_root = txq->qdisc_sleeping; q = qdisc_match_from_root(txq_root, handle); if (q) - return q; + goto unlock; } - return qdisc_match_from_root(dev->rx_queue.qdisc_sleeping, handle); + + q = qdisc_match_from_root(dev->rx_queue.qdisc_sleeping, handle); + +unlock: + spin_unlock_bh(&dev->tx_global_lock); + + return q; } static struct Qdisc *qdisc_leaf(struct Qdisc *p, u32 classid)diff --git a/net/sched/sch_generic.c b/net/sched/sch_generic.c index c3ed4d4..292a373 100644 --- a/net/sched/sch_generic.c +++ b/net/sched/sch_generic.c@@ -526,8 +526,10 @@ void qdisc_destroy(struct Qdisc *qdisc) !atomic_dec_and_test(&qdisc->refcnt)) return; + spin_lock_bh(&dev->tx_global_lock); if (qdisc->parent) list_del(&qdisc->list); + spin_unlock_bh(&dev->tx_global_lock); #ifdef CONFIG_NET_SCHED qdisc_put_stab(qdisc->stab);Cheers, -- Visit Openswan at http://www.openswan.org/ Email: Herbert Xu ~{PmV>HI~} [off-list ref] Home Page: http://gondor.apana.org.au/~herbert/ PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt