Re: [PATCH] pkt_sched: Destroy gen estimators under rtnl_lock().

[Jarek Poplawski <jarkao2@gmail.com>]

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:
> >
> > 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.

> 
> 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~} <herbert@gondor.apana.org.au>
> Home Page: http://gondor.apana.org.au/~herbert/
> PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt