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;
