Re: CVS commit: src/sys/net

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

On 18/02/2016 11:29, Ryota Ozaki wrote:
> Okay, I've changed my mind. Let's commit your patch anyway and
> keep an eye on the lossage messages to know how often and how
> many lossage happens. If there are issues, we can improve then.
> It's not too late.

The irony is that overnight I changed my mind also!
The challenge was to achieve a longer queue in the same size or less.
According to size(1), the end result of this patch is exactly the same
size as my previous one :)

This has been achieved with bitmask array macros to densely pack a 2 bit
integer into a larger integer as a backing store.
I decided to use a uint16_t which allows for 8 events in the queue.
Simply changing the integral type will magically expand the queue, so it
could be changed to a uint64_t to allow 32 events in the queue but this
is probably overkill.
For really size challenged systems, this could be changed to a uint8_t
to allow 4 events in the queue ....... which should still be big enough
normally, but you wanted 10 so the closest match was 8.

I've managed to keep the assignment of the link state to the interface
within the initial if_link_state_change() call which my prior patch
moved out.

Also, only one queued event is processed at a time, subsequent ones have
to wait for another softint(9) to occur.
After all, the host may have >1 interface and we don't want to hog the
network processing a larger queue.

> I'm sorry for disturbing you.

No, no. Thankyou for challenging me :)

Maybe the macros here could be put to a more generic use (or there are
already generic macros for this which I missed?) but unsure how right now.

All being said, I learned a lot about bit shifting doing this and I
might have made a mistake, but sample test cases for my macros appear to
work fine. I would appreciate an OK from someone before I commit this.

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	19 Feb 2016 03:13:45 -0000
@@ -603,7 +603,8 @@
 	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 = -1; /* all bits set, see link_state_change() */
+	ifp->if_link_queue_state = LINK_STATE_UNKNOWN;
 
 	ifp->if_capenable = 0;
 	ifp->if_csum_flags_tx = 0;
@@ -1563,51 +1564,141 @@
 }
 
 /*
- * 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.
+ * bitmask macros to manage a densely packed link_state change queue.
+ * Because we need to store LINK_STATE_UNKNOWN(0), LINK_STATE_DOWN(1) and
+ * LINK_STATE_UP(2) we need 2 bits for each state change.
+ * As a state change to store is 0, treat all bits set as an unset item.
+ */
+#define LQ_ITEM_BITS		2
+#define LQ_ITEM_MASK		((1 << LQ_ITEM_BITS) - 1)
+#define LQ_MASK(i)		(LQ_ITEM_MASK << (i) * LQ_ITEM_BITS)
+#define LINK_STATE_UNSET	LQ_ITEM_MASK
+#define LQ_ITEM(q, i)		(((q) & LQ_MASK((i))) >> (i) * LQ_ITEM_BITS)
+#define LQ_STORE(q, i, v)						      \
+	do {								      \
+		(q) &= ~LQ_MASK((i));					      \
+		(q) |= (v) << (i) * LQ_ITEM_BITS;			      \
+	} while (0 /* CONSTCOND */)
+#define LQ_MAX(q)		((sizeof((q)) * NBBY) / LQ_ITEM_BITS)
+#define LQ_POP(q, v)							      \
+	do {								      \
+		(v) = LQ_ITEM((q), 0);					      \
+		(q) >>= LQ_ITEM_BITS;					      \
+		(q) |= LINK_STATE_UNSET << (LQ_MAX((q)) - 1) * LQ_ITEM_BITS;  \
+	} while (0 /* CONSTCOND */)
+#define LQ_PUSH(q, v)							      \
+	do {								      \
+		(q) >>= LQ_ITEM_BITS;					      \
+		(q) |= (v) << (LQ_MAX((q)) - 1) * LQ_ITEM_BITS;		      \
+	} while (0 /* CONSTCOND */)
+#define LQ_FIND_UNSET(q, i)						      \
+	for ((i) = 0; i < LQ_MAX((q)); (i)++) {				      \
+		if (LQ_ITEM((q), (i)) == LINK_STATE_UNSET)		      \
+			break;						      \
+	}
+/*
+ * Handle a change in the interface link state and
+ * queue notifications.
  */
 void
 if_link_state_change(struct ifnet *ifp, int link_state)
 {
-	int s;
+	int s, idx;
+#ifdef DEBUG
+	int old_link_state;
+#endif
 
-	s = splnet();
-	if (ifp->if_link_state == link_state) {
-		splx(s);
+	/* Ensure change is to a valid state */
+	switch (link_state) {
+	case LINK_STATE_UNKNOWN:	/* FALLTHROUGH */
+	case LINK_STATE_DOWN:		/* FALLTHROUGH */
+	case LINK_STATE_UP:
+		break;
+	default:
+#ifdef DEBUG
+		printf("%s: invalid link state %d\n",
+		    ifp->if_xname, link_state);
+#endif
 		return;
 	}
 
-	ifp->if_link_state = link_state;
-	softint_schedule(ifp->if_link_si);
-
-	splx(s);
-}
-
-
-static void
-if_link_state_change_si(void *arg)
-{
-	struct ifnet *ifp = arg;
-	int s;
-	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;
 
+	/* Ensure link state is actually changing */
+	if (ifp->if_link_state == link_state)
+		goto out;
+
+#ifdef DEBUG
+	old_link_state = ifp->if_link_state;
+#endif
+	ifp->if_link_state = link_state;
 #ifdef DEBUG
 	log(LOG_DEBUG, "%s: link state %s (was %s)\n", ifp->if_xname,
 		link_state == LINK_STATE_UP ? "UP" :
 		link_state == LINK_STATE_DOWN ? "DOWN" :
 		"UNKNOWN",
-		 old_link_state == LINK_STATE_UP ? "UP" :
+		old_link_state == LINK_STATE_UP ? "UP" :
 		old_link_state == LINK_STATE_DOWN ? "DOWN" :
 		"UNKNOWN");
 #endif
 
+	/* Find the last unset event in the queue. */
+	LQ_FIND_UNSET(ifp->if_link_queue, idx);
+
+	/* Handle queue overflow. */
+	if (idx == LQ_MAX(ifp->if_link_queue)) {
+		uint8_t lost, protect;
+
+		/*
+		 * The DOWN state must be protected from being pushed off
+		 * the queue to ensure that userland will always be
+		 * in a sane state.
+		 * Because DOWN is protected, there is no need to protect
+		 * UNKNOWN.
+		 * It should be invalid to change from any other state to
+		 * UNKNOWN anyway ...
+		 */
+		lost = LQ_ITEM(ifp->if_link_queue, 0);
+		if (lost == LINK_STATE_DOWN) {
+			protect = LINK_STATE_DOWN;
+			lost = LQ_ITEM(ifp->if_link_queue, 1);
+		} else
+			protect = LINK_STATE_UNSET;
+
+		printf("%s: lost %s notification\n", ifp->if_xname,
+		    lost == LINK_STATE_UP ? "UP" :
+		    lost == LINK_STATE_DOWN ? "DOWN" :
+		    "UNKNOWN");
+		LQ_PUSH(ifp->if_link_queue, (uint8_t)link_state);
+		if (protect != LINK_STATE_UNSET)
+			LQ_STORE(ifp->if_link_queue, 0, protect);
+	} else
+		LQ_STORE(ifp->if_link_queue, idx, (uint8_t)link_state);
+
+	softint_schedule(ifp->if_link_si);
+
+out:
+	splx(s);
+}
+
+/*
+ * Handle interface link state change notifications.
+ * Must be called at splnet().
+ */
+static void
+if_link_state_change0(struct ifnet *ifp, int link_state)
+{
+	struct domain *dp;
+
+	/*
+	 * Don't raise consecutive states.
+	 * Note that the link queue state may not match the
+	 * interface link state.
+	 * XXX: should this be a warning about lost notification?
+	 */
+	if (ifp->if_link_queue_state == link_state)
+		return;
+
 	/*
 	 * When going from UNKNOWN to UP, we need to mark existing
 	 * addresses as tentative and restart DAD as we may have
@@ -1618,7 +1709,7 @@
 	 * away.
 	 */
 	if (link_state == LINK_STATE_UP &&
-	    old_link_state == LINK_STATE_UNKNOWN)
+	    ifp->if_link_queue_state == LINK_STATE_UNKNOWN)
 	{
 		DOMAIN_FOREACH(dp) {
 			if (dp->dom_if_link_state_change != NULL)
@@ -1626,6 +1717,7 @@
 				    LINK_STATE_DOWN);
 		}
 	}
+	ifp->if_link_queue_state = link_state;
 
 	/* Notify that the link state has changed. */
 	rt_ifmsg(ifp);
@@ -1639,6 +1731,27 @@
 		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;
+	uint8_t state;
+
+	s = splnet();
+
+	/* Pop a link state change from the queue and process it. */
+	LQ_POP(ifp->if_link_queue, state);
+	if_link_state_change0(ifp, state);
+
+	/* If there is a link state change to come, schedule it. */
+	if (LQ_ITEM(ifp->if_link_queue, 0) != LINK_STATE_UNSET)
+		softint_schedule(ifp->if_link_si);
 
 	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	19 Feb 2016 03:13:45 -0000
@@ -351,7 +351,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 */
+	uint16_t	if_link_queue;	/* masked link state change queue */
+	int	if_link_queue_state;	/* last link queue state */
 #endif
 } ifnet_t;