Date: Sun, 8 Nov 2015 21:25:35 +0000 From: Steven Hartland <killing@multiplay.co.uk> To: freebsd-net@freebsd.org Subject: [PATCH] if_lagg driver enhancements. Message-ID: <563FBDCF.1050509@multiplay.co.uk>
next in thread | raw e-mail | index | archive | help
> IMHO, the patch introduces a layering violation, which is bad. This would > lead to problems in future. From a quick look I don't see any right now, > and patch is compatible with carp(4) just accidentially :) > > I would suggest the following approach: > > 1) Network protocols should register theirselves on the ifnet_link_event > EVENTHANDLER(9). > 2) The inet4 should send gratutious ARP on this event. > 3) The inet6 should send NA. > > As you see the patch would be entirely not about lagg(4) :) > > We've got some minor obstacles on the suggested way: > > - The if_link_state_change() function suppresses any processing if the link > hasn't changed, for example UP -> UP event. > > We can overcome this by adding a new pseudo state LINK_STATE_UPAGAIN (or > LINK_STATE_UPCHANGED or LINK_STATE_UPANOTHER or any better name you can > imagine). This pseudo state can't be stored in the ifp->if_link_state, but > it can be used to keep the state LINK_STATE_UP, but force triggering link > state hooks. > > I think this approach is more clean and error prone. It can lead only to > extraneous gratutious ARP sent in some cases, which is not critical. Hi Gleb I know this is an old one, but I've picked it up as I'm going to need it fixed for a project that goes live before Christmas. TBH I'm quite surprised that this is still an issue, as it makes failover mode pretty useless, in its current form. As I understand you're proposal, it was to have each protocol e.g. ipv4, ipv6 register an ifnet_link_event handler. This when combined with lagg_linkstate using LINK_STATE_UPAGAIN (or some other name for it) to ensure that ifnet_link_event is fired so that even when lagg failover mode transitions from backup link to master link live, hence laggX going from UP(backup) -> UP(master) a broadcast is still triggered. Would this be a correct summary of your proposal? One thing that springs to mind is to avoid introducing a new if_link_state value and instead have lagg invoke the event directly in this special case. This is it would avoid the unnecessary do_link_state_change call and all the processing associated with that which is unneeded and could have unintended side effects. Alternatively still have the additional link_state but have if_link_state_change process it directly without enqueueing the task for do_link_state_change, which keeps the layer separation more strictly. What do you think? Regards Steve
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?563FBDCF.1050509>