Re: [PATCH] rtnl: Simplify ASSERT_RTNL

[Patrick McHardy <kaber@trash.net>]

Herbert Xu wrote:
> On Tue, Oct 02, 2007 at 05:29:11PM +0200, Patrick McHardy wrote:
> 
>>I think this doesn't completely fix it, when dev_unicast_add is
>>interrupted by dev_mc_add before the unicast changes are performed,
>>they will get committed in the dev_mc_add context, so we might still
>>call change_flags with BH disabled. Taking the TX lock around the
>>dev->uc_count and dev->uc_promisc checks and changes in __dev_set_rx_mode
>>should fix this.
> 
> 
> Good catch.  Digging back in history it seems that you added
> the change_rx_flags function so that the driver didn't have to
> do it under TX lock, right?


Yes, and to make sure the RTNL is held.

> The problem with this is that the stack can now call
> change_rx_flags and set_multicast_list simultaneously
> which presents a potential headache for the driver
> author (if they were to use change_rx_flags).


The change_rx_flags function was intended to be used by software
devices that want to synchronize flags to a different device,
in which case they shouldn't interfere since set_multicast_list
would only be used for the multicast list and not the flags.

> It seems to me what we could do is in fact separate out the
> part that adds the address and the part that syncs it with
> hardware.


That sounds like a really good idea to get rid of all the
confusion.

> That way we can call the hardware from a process context later
> and use the RTNL to guarantee that we only enter the driver
> once.
> 
> So dev_mc_add would look like:
> 
> 1) Hold some form of lock L.
> 2) Modify mc list A (a copy of the current mc list).
> 3) Drop lock.
> 4) Schedule an update to the hardware.
> 
> The update to the hardware would look lie:
> 
> 1) Hold RTNL.
> 2) Hold lock L.
> 3) Copy list A to list B (B would be our current list).
> 4) Drop lock L.
> 5) Call the hardware.
> 6) Drop RTNL.
> 
> For compatibility, set_multicast_list would still be invoked
> under the TX lock while set_rx_mode would do exactly the same
> thing but would only hold the RTNL.
> 
> What do you think about this approach?


Sounds great :)