Re: CVS commit: src/sys/net

[Roy Marples <roy%marples.name@localhost>]

On 16/02/2016 08:38, Roy Marples wrote:
> On Tuesday 16 February 2016 13:37:28 you wrote:
>>> Oh, I see. We shouldn't drop any events of link state changes.
>>
>> i don't see any point in keeping a huge series of link changes
>> if they can be collapsed into an equivalent sequence.  with VMs
>> and other user-controlled systems it's possible to flood such a
>> link state checking engine.
> 
> The queue can be kept quite short because we don't care about any prior 
> entries each time state changes to LINK_STATE_DOWN.
> Also, we don't care about any prior entries (other than LINK_STATE_DOWN) when 
> setting LINK_STATE_UNKNOWN.
> Finally we set LINK_STATE_UP.
> 
> So the queue should only ever hold a maximum of 3 items. Could even work it 
> like so

I found the time to work on this, here is the patch I've been running
for an hour or so upping and downing interfaces.

OK to commit?

Roy

Index: sys/net/if.c
===================================================================
RCS file: /cvsroot/src/sys/net/if.c,v
retrieving revision 1.324
diff -u -r1.324 if.c
--- sys/net/if.c	15 Feb 2016 08:08:04 -0000	1.324
+++ sys/net/if.c	16 Feb 2016 13:22:43 -0000
@@ -603,7 +603,6 @@
 	ifp->if_broadcastaddr = 0; /* reliably crash if used uninitialized */
 
 	ifp->if_link_state = LINK_STATE_UNKNOWN;
-	ifp->if_old_link_state = LINK_STATE_UNKNOWN;
 
 	ifp->if_capenable = 0;
 	ifp->if_csum_flags_tx = 0;
@@ -1562,41 +1561,73 @@
 	}
 }
 
+/* Priority list for link state changes */
+static const int
+if_link_state_prio[LINK_STATE_MAX] = {
+	0,	/* LINK_STATE_UNKNOWN */
+	-1,	/* LINK_STATE_DOWN */
+	1,	/* LINK_STATE_UP */
+};
+
 /*
  * Handle a change in the interface link state.
- * XXX: We should listen to the routing socket in-kernel rather
- * than calling in6_if_link_* functions directly from here.
  */
 void
 if_link_state_change(struct ifnet *ifp, int link_state)
 {
-	int s;
+	int s, i;
+
+	KASSERT(link_state >= 0 && link_state < LINK_STATE_MAX);
 
 	s = splnet();
-	if (ifp->if_link_state == link_state) {
-		splx(s);
-		return;
+	if (ifp->if_link_state == link_state && ifp->if_link_queue_len == 0)
+		goto out;
+
+	for (i = 0; i < ifp->if_link_queue_len; i++) {
+		if (if_link_state_prio[link_state] <=
+		    if_link_state_prio[ifp->if_link_queue[i]])
+		{
+			ifp->if_link_queue[i] = link_state;
+			ifp->if_link_queue_len = i + 1;
+			/* Because we are trimming the queue,
+			 * there is no need to call softint_schedule again. */
+			goto out;
+		}
 	}
 
-	ifp->if_link_state = link_state;
+	KASSERT(ifp->if_link_queue_len + 1 < __arraycount(ifp->if_link_queue));
+	ifp->if_link_queue[ifp->if_link_queue_len++] = link_state;
 	softint_schedule(ifp->if_link_si);
 
+out:
 	splx(s);
 }
 
-
 static void
 if_link_state_change_si(void *arg)
 {
 	struct ifnet *ifp = arg;
-	int s;
+	int s, i;
 	int link_state, old_link_state;
 	struct domain *dp;
 
 	s = splnet();
-	link_state = ifp->if_link_state;
-	old_link_state = ifp->if_old_link_state;
-	ifp->if_old_link_state = ifp->if_link_state;
+	KASSERT(ifp->if_link_queue_len >= 0); /* unlikely */
+	if (ifp->if_link_queue_len == 0)
+		goto out;
+
+	/* Pop the link state change from the queue */
+	link_state = ifp->if_link_queue[0];
+	ifp->if_link_queue_len--;
+	for (i = 0; i < ifp->if_link_queue_len; i++)
+		ifp->if_link_queue[i] = ifp->if_link_queue[i + 1];
+
+	/* Ensure link state is actually changing */
+	if (ifp->if_link_state == link_state)
+		goto out;
+
+	old_link_state = ifp->if_link_state;
+	ifp->if_link_state = link_state;
 
 #ifdef DEBUG
 	log(LOG_DEBUG, "%s: link state %s (was %s)\n", ifp->if_xname,
@@ -1640,6 +1671,7 @@
 			dp->dom_if_link_state_change(ifp, link_state);
 	}
 
+out:
 	splx(s);
 }
 
Index: sys/net/if.h
===================================================================
RCS file: /cvsroot/src/sys/net/if.h,v
retrieving revision 1.197
diff -u -r1.197 if.h
--- sys/net/if.h	16 Feb 2016 01:31:26 -0000	1.197
+++ sys/net/if.h	16 Feb 2016 13:22:43 -0000
@@ -191,10 +191,12 @@
 
 /*
  * Values for if_link_state.
+ * When changing these values, consider if_link_state_prio in if.c.
  */
 #define	LINK_STATE_UNKNOWN	0	/* link invalid/unknown */
 #define	LINK_STATE_DOWN		1	/* link is down */
 #define	LINK_STATE_UP		2	/* link is up */
+#define	LINK_STATE_MAX		3	/* size of link states */
 
 /*
  * Structure defining a queue for a network interface.
@@ -351,7 +353,8 @@
 	struct krwlock	*if_afdata_lock;
 	struct if_percpuq	*if_percpuq; /* We should remove it in the future */
 	void	*if_link_si;		/* softint to handle link state changes */
-	int	if_old_link_state;	/* previous link state */
+	int	if_link_queue[LINK_STATE_MAX];	/* link state change queue */
+	int	if_link_queue_len;	/* length of link state change queue */
 #endif
 } ifnet_t;