Re: [Bonding-devel] [PATCH 3/3] bonding: Improve IGMP join processing

["Andy Gospodarek" <gospo@redhat.com>]

On Tue, Mar 06, 2007 at 03:15:41PM -0800, Jay Vosburgh wrote:
> 
> David Stevens <dlstevens@us.ibm.com> wrote:
> 
> >It looks to me like "rejoin" is essentially ip_mc_up(), and it'd be better
> >to call that than add a nearly identical function.
> 
> 	Won't ip_mc_up() acquire an additional reference (via
> ip_mc_inc_group) to the IGMP_ALL_HOSTS im->users that would never be
> released (in the case of bonding calling the function out of the blue)?
> 
> 	In looking at it, the ip_mc_rejoin_group function (the new one
> added with the patch) is a lot more like igmp_group_added() than
> ip_mc_up().  I'm not sure if the extra bits in igmp_group_added() are
> worthy of concern; I'm thinking not, since im->loaded shouldn't be zero
> coming in for the bonding case.
> 
> 	I think the meat that the "rejoin" wants is what's in
> igmpv3_send_cr(), which appears to do the actual sending stuff.  I'm not
> sure if that's better to call directly (and risk locking adventures) or
> to just trip the timer via igmp_ifc_event().
> 
> 	Anyway, it looks like all of this needs to be done under RTNL,
> which isn't the case, so I need to go off and look into reworking it
> again.
> 
> 	Andy: do you have any work in progress on the sleep / rtnl stuff
> we've been discussing?

Jay, 

I do, but unfortunately it's much closer to the code I'd proposed
originally than the code you sent me.  The more I audited your code, the
more I like the design -- until I discovered that every time you pause
the timers you need to flush the workqueue.  This is bad since you are
regularly stopping the timers in places where the rtnl lock is taken
and the currently running work item may need to that lock to complete.
With a small enough monitor interval I could deadlock pretty quickly.
Without the benefit of the full stop, I couldn't justify the major
conversion just yet (plus is feels like keeping a list of the timers is
re-implementing what workqueues are designed to do for you).

I've got a patch that seems decent so far, but its really just at
timer->workqueue conversion with some bits thrown in correctly stop the
queues when taking the interface down or when removing the module.

> >Also, real interfaces already do gratuitous IGMP advertisements when
> >they are bounced (the reason there is an ip_mc_up()). Could bonding,
> >when failing over, simply mark the master interface as down, switch, and
> >then mark the master as up again? In addition to doing the right
> >thing for both IPv4 and IPv6 multicasting w/o any code changes in those
> >layers, it may have similar benefits for ARP and neighbor discovery, 
> >right?
> 
> 	Marking the master down would, I believe, issue notifiers that
> the device has gone down.  Various things, network manager sort of
> applications in particular, listen to those, so I'm not sure it's a good
> idea.  I think there are other side effects as well, I'm thinking it
> would flush routes associated with the interface as well.

I agree with Jay here.  I hate that bonding has to have so much
knowledge about upper layer protocols, but for the ones that are
stateful like IGMP we will need fixes like the one proposed.