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

[Thomas Graf <tgraf@suug.ch>]

> >We get a RTM_DELQDISC request so you'll increment refcnt in
> >qdisc_lookup and decrement it right before you call qdisc_destroy
> >so it actually can be deleted. The rcu callback works fine
> >and will set up a another rcu callback for the destroying of
> >the inner qdiscs. Right at this time you get a RTM_GETQDISC for
> >that inner qdisc so you'll lock on it, then the rcu callback
> >comes in and cannot delete the inner qdisc anymore. Do you want
> >to sleep in softirq context?
> >
> This can't happen. Once the inner qdiscs refcnt drops to zero
> they are removed from the list, before the rcu callback is scheduled.
> Once the RCU callback is scheduled it can't be found anymore.

Right, but what about when the RTM_GETQDISC comes in before the first
rcu callback is invoked? I'm not sure if this can happen in practice
though.

Anyways, I do think we should force the task to be completed, or
at least all the list unlinking, before the rtnl semaphore is given
back.  I'm fine with postponing the deletion of the object but not
to postpone list manipulations even if we cannot reproduce it now.

We might be able to get a working solution for now but
this unneeded async behaviour will definitely cause much troubles
in the future.

I think no function that can be invoked from softirq context should
ever have the chance to sleep even if there is no path at the
moment. Why bother about this when we can move the possible sleep into
a user context only area?

> BTW: An alternative, quite unintrusive solution is to prevent anyone
> from finding the inner qdiscs after the outer one has been destroyed.
> This can be done be keeping inner qdiscs on qdisc->qdisc_list and only
> keep the top-level qdisc in struct net_device. Of course, this makes
> walking all qdiscs more complicated. A generation counter for the
> top-level qdisc should also work.

I agree, that's rather non trivial.

I'll wait for your patch but it's wrong in my eyes ;->