lagg0.2 style vlans on lagg(4) interface

[Niki Denev]

On Thu, May 22, 2008 at 3:42 PM, Bruce M. Simpson <bms at freebsd.org> wrote:
> Hi,
>
> It looks like this patch will cause gratuitous ARP to be queued even when
> the interface is not IFF_UP, is this intentional?
>
> Niki Denev wrote:
>>
>> I think arp_gratuit() needs a better name.
>>
>
> arp_announce() ?
>
>> Is if_ethersubr.c:ether_ifattach() good place to register the EVENT hook?
>>
>
> ARP is also used by FDDI and IEEE 802.5, as well as anything which emulates
> this. Taking the call to arp_ifinit() out of if_setlladdr() is likely to
> break this code.
>
>> And if yes, what would be the best way to handle failure to register
>> the hook, as the function is void?
>>  Should I worry about that, or just print a warning message and continue?
>>
>
> I see the C++-style comments - perhaps someone who knows event handlers
> better than I can comment, I believe it's using one of the shared kernel
> malloc pools with M_WAIT.
>
> It looks like this won't run afoul of locking, but it is a change to a
> fairly central path which needs to be considered carefully as it affects
> consumers other than Ethernet drivers.
>
> cheers
> BMS
>
>

Well, yes, this change will have side effects...
My initial problem was that vlan interfaces link layer address is not
updated when
the parents link layer address is changed, for example when adding the
first member in
a link aggregation lagg(4) interface.

This patch seems to fix this for me :

diff -ur /usr/src/.zfs/snapshot/orig/sys/net/if.c /usr/src/sys/net/if.c
--- /usr/src/.zfs/snapshot/orig/sys/net/if.c	2008-04-29 23:43:08.000000000 +0300
+++ /usr/src/sys/net/if.c	2008-05-22 17:15:15.681205282 +0300
@@ -2636,6 +2636,7 @@
 			(*ifp->if_ioctl)(ifp, SIOCSIFFLAGS, (caddr_t)&ifr);
 			IFF_UNLOCKGIANT(ifp);
 		}
+		EVENTHANDLER_INVOKE(ifaddr_event, ifp);
 #ifdef INET
 		/*
 		 * Also send gratuitous ARPs to notify other nodes about
@@ -2646,6 +2647,8 @@
 				arp_ifinit(ifp, ifa);
 		}
 #endif
+	} else {
+		EVENTHANDLER_INVOKE(ifaddr_event, ifp);
 	}
 	return (0);
 }
diff -ur /usr/src/.zfs/snapshot/orig/sys/net/if_lagg.c
/usr/src/sys/net/if_lagg.c
--- /usr/src/.zfs/snapshot/orig/sys/net/if_lagg.c	2008-04-19
14:45:20.055330961 +0300
+++ /usr/src/sys/net/if_lagg.c	2008-05-23 09:51:22.911830591 +0300
@@ -303,6 +303,7 @@
 	/* Let the protocol know the MAC has changed */
 	if (sc->sc_lladdr != NULL)
 		(*sc->sc_lladdr)(sc);
+	EVENTHANDLER_INVOKE(ifaddr_event, ifp);
 }

 static void
@@ -364,6 +365,7 @@
 	/* Update the lladdr even if pending, it may have changed */
 	llq->llq_ifp = ifp;
 	bcopy(lladdr, llq->llq_lladdr, ETHER_ADDR_LEN);
+	EVENTHANDLER_INVOKE(ifaddr_event, ifp);

 	if (!pending)
 		SLIST_INSERT_HEAD(&sc->sc_llq_head, llq, llq_entries);
diff -ur /usr/src/.zfs/snapshot/orig/sys/net/if_vlan.c
/usr/src/sys/net/if_vlan.c
--- /usr/src/.zfs/snapshot/orig/sys/net/if_vlan.c	2007-10-28
18:24:16.000000000 +0200
+++ /usr/src/sys/net/if_vlan.c	2008-05-22 17:32:40.849337824 +0300
@@ -137,6 +137,7 @@
 static MALLOC_DEFINE(M_VLAN, VLANNAME, "802.1Q Virtual LAN Interface");

 static eventhandler_tag ifdetach_tag;
+static eventhandler_tag ifaddr_tag;

 /*
  * We have a global mutex, that is used to serialize configuration
@@ -190,6 +191,7 @@
 static	void vlan_link_state(struct ifnet *ifp, int link);
 static	void vlan_capabilities(struct ifvlan *ifv);
 static	void vlan_trunk_capabilities(struct ifnet *ifp);
+static	int vlan_lladdr_update(void *arg, struct ifnet *ifp);

 static	struct ifnet *vlan_clone_match_ethertag(struct if_clone *,
     const char *, int *);
@@ -528,6 +530,10 @@
 		    vlan_ifdetach, NULL, EVENTHANDLER_PRI_ANY);
 		if (ifdetach_tag == NULL)
 			return (ENOMEM);
+		ifaddr_tag = EVENTHANDLER_REGISTER(ifaddr_event, vlan_lladdr_update,
+		    NULL, EVENTHANDLER_PRI_ANY);
+		if (ifaddr_tag == NULL)
+			return (ENOMEM);
 		VLAN_LOCK_INIT();
 		vlan_input_p = vlan_input;
 		vlan_link_state_p = vlan_link_state;
@@ -546,6 +552,7 @@
 	case MOD_UNLOAD:
 		if_clone_detach(&vlan_cloner);
 		EVENTHANDLER_DEREGISTER(ifnet_departure_event, ifdetach_tag);
+		EVENTHANDLER_DEREGISTER(ifaddr_event, ifaddr_tag);
 		vlan_input_p = NULL;
 		vlan_link_state_p = NULL;
 		vlan_trunk_cap_p = NULL;
@@ -1280,6 +1287,35 @@
 	TRUNK_UNLOCK(trunk);
 }

+/*
+ * Update vlan interface link layer address on
+ * parent interface link layer address change.
+ */
+static int
+vlan_lladdr_update(void *arg __unused, struct ifnet *ifp)
+{
+	struct ifvlantrunk *trunk = ifp->if_vlantrunk;
+	struct ifvlan *ifv;
+	int i;
+
+	if (trunk) {
+		TRUNK_LOCK(trunk);
+#ifdef VLAN_ARRAY
+		for (i=0; i < VLAN_ARRAY_SIZE; i++)
+			if (trunk->vlans[i] != NULL) {
+				ifv = trunk->vlans[i];
+#else
+		for (i=0; i < (1 << trunk->hwidth); i++) {
+			LIST_FOREACH(ifv, &trunk->hash[i], ifv_list)
+#endif
+				bcopy(IF_LLADDR(ifp), IF_LLADDR(ifv->ifv_ifp),
+					ETHER_ADDR_LEN);
+		}
+		TRUNK_UNLOCK(trunk);
+	}
+	return (0);
+}
+
 static int
 vlan_ioctl(struct ifnet *ifp, u_long cmd, caddr_t data)
 {

I have left the gratuitious arp sending alone in this one,
also I wonder if there is a need for another kernel event like
iflladdr_event?
Another problem is that many drivers handle things internally by
making bcopy's or memcpy's of the lladdr and not generating events,
which probably should be done by some function.

Niki