Re: CVS commit: src/sys/net

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

On Tuesday 16 February 2016 22:20:37 Taylor R Campbell wrote:
> Except for an issue with queueing discussed privately (scheduling a
> softint that is already scheduled won't cause it to run again, so
> if_link_state_change_si needs to process the whole queue in one go),
> that approach looks fine to me, although as we also discussed
> privately we can easily compact it into a three-bit mask with a
> trivial update instead of a whole array of states.

Patch attached to solve change from a priority array into a bit mask approach 
where we swallow the whole queue in a softint.
Thanks to Taylor for some reviews and suggestions.

> This has the consequence that if the link goes up/down in quick
> succession, and then up/down in quick succession, &c., with the queue
> processed after each up/down transition, userland will never be
> notified.  However, if the link goes down/up, then down/up, &c., the
> userland will be notified of all the transitions.  Roy claims that
> that's OK, and I'm inclined to believe the author of dhcpcd about
> this.

If you don't read the patch, here is the comment I added to the
if_link_state_change() function:

* Queue a change in the interface link state.
*
* The queue itself is very limited:
*   no state can be queued more than once
*   a higher priority state will remove lower priority states
*
* The queue state priority in descending order is DOWN, UNKNOWN, UP.
* Rationale is that if the kernel can't process the link state change queue
* fast enough then userland has no chance of catching up.
* Any lossage is logged to the console.
* The queue state priority is ordered so that link state aware programs
* will still have the correct end result regardless of any lossage.

Any comments or objections?

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	17 Feb 2016 01:36:07 -0000
@@ -603,7 +603,7 @@
 	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_link_queue = 0;
 
 	ifp->if_capenable = 0;
 	ifp->if_csum_flags_tx = 0;
@@ -1563,9 +1563,18 @@
 }
 
 /*
- * 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.
+ * Queue a change in the interface link state.
+ *
+ * The queue itself is very limited:
+ *   no state can be queued more than once
+ *   a higher priority state will remove lower priority states
+ *
+ * The queue state priority in descending order is DOWN, UNKNOWN, UP.
+ * Rationale is that if the kernel can't process the link state change queue
+ * fast enough then userland has no chance of catching up.
+ * The queue state priority is ordered so that link state aware programs
+ * will still have the correct end result regardless of any lossage.
+ * Any lossage is logged to the console.
  */
 void
 if_link_state_change(struct ifnet *ifp, int link_state)
@@ -1573,30 +1582,68 @@
 	int s;
 
 	s = splnet();
-	if (ifp->if_link_state == link_state) {
-		splx(s);
-		return;
+
+	/* If state is the same and there is no current queue, do nothing. */
+	if (ifp->if_link_state == link_state && ifp->if_link_queue == 0x00)
+		goto out;
+
+#define LINK_STATE_LOST "%s: lost %s notification\n"
+	switch (link_state) {
+	case LINK_STATE_DOWN:
+		if (isset(&ifp->if_link_queue, LINK_STATE_UP)) {
+			printf(LINK_STATE_LOST,
+			    ifp->if_xname, "LINK_STATE_UP");
+			clrbit(&ifp->if_link_queue, LINK_STATE_UP);
+		}
+		if (isset(&ifp->if_link_queue, LINK_STATE_UNKNOWN)) {
+			printf(LINK_STATE_LOST,
+			    ifp->if_xname, "LINK_STATE_UNKNOWN");
+			clrbit(&ifp->if_link_queue, LINK_STATE_UNKNOWN);
+		}
+		setbit(&ifp->if_link_queue, LINK_STATE_DOWN);
+		break;
+	case LINK_STATE_UNKNOWN:
+		if (isset(&ifp->if_link_queue, LINK_STATE_UP)) {
+			printf(LINK_STATE_LOST,
+			    ifp->if_xname, "LINK_STATE_UP");
+			clrbit(&ifp->if_link_queue, LINK_STATE_UP);
+		}
+		setbit(&ifp->if_link_queue, LINK_STATE_UNKNOWN);
+		break;
+	case LINK_STATE_UP:
+		setbit(&ifp->if_link_queue, LINK_STATE_UP);
+		break;
+	default:
+#ifdef DEBUG
+		printf("%s: invalid link state %d\n",
+		    ifp->if_xname, link_state);
+#endif
+		goto out;
 	}
+#undef LINK_STATE_LOST
 
-	ifp->if_link_state = link_state;
 	softint_schedule(ifp->if_link_si);
 
+out:
 	splx(s);
 }
 
-
+/*
+ * Handle a change in the interface link state.
+ * Must be called at splnet().
+ */
 static void
-if_link_state_change_si(void *arg)
+if_link_state_change0(struct ifnet *ifp, int link_state)
 {
-	struct ifnet *ifp = arg;
-	int s;
-	int link_state, old_link_state;
+	int 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;
+	/* Ensure link state is actually changing */
+	if (ifp->if_link_state == link_state)
+		return;
+
+	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,
@@ -1639,7 +1686,25 @@
 		if (dp->dom_if_link_state_change != NULL)
 			dp->dom_if_link_state_change(ifp, link_state);
 	}
+}
 
+/*
+ * Process the interface link state change queue.
+ */
+static void
+if_link_state_change_si(void *arg)
+{
+	struct ifnet *ifp = arg;
+	int s;
+
+	s = splnet();
+	if (isset(&ifp->if_link_queue, LINK_STATE_DOWN))
+		if_link_state_change0(ifp, LINK_STATE_DOWN);
+	if (isset(&ifp->if_link_queue, LINK_STATE_UNKNOWN))
+		if_link_state_change0(ifp, LINK_STATE_UNKNOWN);
+	if (isset(&ifp->if_link_queue, LINK_STATE_UP))
+		if_link_state_change0(ifp, LINK_STATE_UP);
+	ifp->if_link_queue = 0;
 	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	17 Feb 2016 01:36:08 -0000
@@ -351,7 +351,7 @@
 	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 */
+	uint8_t	if_link_queue;		/* masked link state change queue */
 #endif
 } ifnet_t;