On Tue, Jan 09, 2007 at 06:13:55PM -0500, Jeff Garzik wrote:
> Andy Gospodarek wrote:
> >-void bond_alb_handle_active_change(struct bonding *bond, struct slave
> >*new_slave)
> >+void bond_alb_handle_active_change(struct bonding *bond, struct slave
> >*new_slave, int rtnl_locked)
> > {
> > struct slave *swap_slave;
> > int i;
>
> Although this is not a direct NAK (haven't read the full patch yet),
> conditional locking behavior like this is /very/ fragile, and in Linux
> is generally discouraged. Vendor drivers in particular have a history
> of constantly getting this wrong, and it makes locking more difficult to
> review.
>
> Jeff
>
I'd be happy to propose something that doesn't do conditional locking
like this. I saw this route as a way to take the rtnl lock only when it
was absolutely necessary. After spending some time trying to get it
right I can understand why it is so discouraged. I'd also rather not
provide a 'bad example' for how to do things. :-)
If others are OK with it, I'd be happy to propose a patch like Stephen
suggested where the lock is taken in the mii/arp monitor routines.
Though the locking would be unnecessary in some cases it would be much
cleaner and easier to maintain.
-
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
