Stefan Rompf wrote:
Hi David,
attached is the recent backport of link state notification for the
stable kernel series. The feature is optional and can be configured away
to nothing, and tested on several systems.
It might be a backport, but I haven't seen a 2.5.x version posted in a
while. If this is going into the stable series, it should already be
merged in 2.5.x at the same time, if not first.
I'd really like to have it in 2.4.21, and as your the one to decide
before Marcelo gets it, can you have a look?
s/before/if/ :)
comments follow. note these are all minor objections: I think the
patch needs revising, but I also think it is ok overall and support its
eventual inclusion [after tidying]. Not that my opinion matters much
here compared to DaveM's (and Jamal's and Alexey's and...)...
overall, I have one question: how does netdev_state_change(dev)
propagate link status to userspace? Via the IFF_RUNNING flag's
presence/absence when IFF_UP is true?
diff -uNr linux-2.4.20rc1/include/linux/netdevice.h
linux/include/linux/netdevice.h
--- linux-2.4.20rc1/include/linux/netdevice.h 2002-11-05 00:31:42.000000000
+0100
+++ linux/include/linux/netdevice.h 2002-11-13 22:32:46.000000000 +0100
@@ -630,6 +631,10 @@
* who is responsible for serialization of these calls.
*/
+#ifdef CONFIG_LINKWATCH
+extern void linkwatch_fire_event(struct net_device *dev);
+#endif
remove the ifdef, not needed and ifdefs are discouraged in all Linux code.
diff -uNr linux-2.4.20rc1/net/Config.in linux/net/Config.in
--- linux-2.4.20rc1/net/Config.in 2002-08-03 02:39:46.000000000 +0200
+++ linux/net/Config.in 2002-11-13 22:32:46.000000000 +0100
@@ -48,6 +48,7 @@
bool ' Per-VC IP filter kludge' CONFIG_ATM_BR2684_IPFILTER
fi
fi
+ bool 'Device link state notification (EXPERIMENTAL)' CONFIG_LINKWATCH
ifyou mark it experimental, then it should be
dep_bool ... CONFIG_LINKWATCH $CONFIG_EXPERIMENTAL
diff -uNr linux-2.4.20rc1/net/core/Makefile linux/net/core/Makefile
--- linux-2.4.20rc1/net/core/Makefile 2002-08-03 02:39:46.000000000 +0200
+++ linux/net/core/Makefile 2002-11-13 22:33:32.000000000 +0100
@@ -31,4 +31,6 @@
# Ugly. I wish all wireless drivers were moved in drivers/net/wireless
obj-$(CONFIG_NET_PCMCIA_RADIO) += wireless.o
+obj-$(CONFIG_LINKWATCH) += link_watch.o
+
include $(TOPDIR)/Rules.make
add it to export-objs too (because of below change)
diff -uNr linux-2.4.20rc1/net/core/dev.c linux/net/core/dev.c
--- linux-2.4.20rc1/net/core/dev.c 2002-11-05 00:31:42.000000000 +0100
+++ linux/net/core/dev.c 2002-11-13 22:32:46.000000000 +0100
@@ -194,6 +194,12 @@
#endif
+#ifdef CONFIG_LINKWATCH
+extern void linkwatch_init(void);
+extern void linkwatch_run_queue(void);
+#endif
ug :)
the ifdef can be killed, and the prototypes should be in netdevice.h (or
any other header), not in mainline code.
@@ -2675,6 +2693,10 @@
dv_init();
#endif /* CONFIG_NET_DIVERT */
+#ifdef CONFIG_LINKWATCH
+ linkwatch_init();
+#endif
no need for an ifdef, make linkwatch_init() a no-op static inline for
the !CONFIG_LINKWATCH case.
+static unsigned long linkwatch_flags = 0;
+static unsigned long linkwatch_nextevent = 0;
statics are automatically initialized to zero, don't do it explicitly.
you waste a couple bytes in the kernel image storing a zero value.
+
+static void linkwatch_event(void *dummy);
+static void linkwatch_timer(unsigned long dummy);
+
+static struct tq_struct linkwatch_tq;
+static struct timer_list linkwatch_ti;
+
+static LIST_HEAD(lweventlist);
+static spinlock_t lweventlist_lock = SPIN_LOCK_UNLOCKED;
why initialize some complex structs programmatically and some
explicitly? IMO for consistency you should initialize lweventlist_lock
in linkwatch_init(). (or vice versa)
+/* Must be called with the rtnl semaphore held */
+void linkwatch_run_queue(void) {
+ LIST_HEAD(head);
+ struct list_head *n, *next;
+
+ spin_lock_irq(&lweventlist_lock);
+ list_splice_init(&lweventlist, &head);
+ spin_unlock_irq(&lweventlist_lock);
spin_lock_irq{save,restore} would probably better serve you here...
+
+ list_for_each_safe(n, next, &head) {
+ struct lw_event *event = list_entry(n, struct lw_event, list);
+ struct net_device *dev = event->dev;
+
+ if (event == &singleevent) {
+ clear_bit(LW_SE_USED, &linkwatch_flags);
+ } else {
+ kfree(event);
+ }
style: braces not needed
+
+ /* We are about to handle this device,
+ * so new events can be accepted
+ */
+ clear_bit(__LINK_STATE_LINKWATCH_PENDING, &dev->state);
+
+ if (dev->flags & IFF_UP) {
+ netdev_state_change(dev);
+ }
likewise. (pointlessly makes code longer)
+
+ dev_put(dev);
+ }
+}
+
+
+static void linkwatch_event(void *dummy)
+{
+ /* Limit the number of linkwatch events to one
+ * per second so that a runaway driver does not
+ * cause a storm of messages on the netlink
+ * socket
+ */
+ linkwatch_nextevent = jiffies + HZ;
+ clear_bit(LW_RUNNING, &linkwatch_flags);
the high availability folks would prefer a half-second (HZ >> 1).
+void __init linkwatch_init(void) {
+ linkwatch_ti.function = linkwatch_timer;
+ init_timer(&linkwatch_ti);
+ INIT_TQUEUE(&linkwatch_tq, linkwatch_event, NULL);
+}
set the timer function after the initialize the timer :)
+
diff -uNr linux-2.4.20rc1/net/netsyms.c linux/net/netsyms.c
--- linux-2.4.20rc1/net/netsyms.c 2002-11-05 00:31:42.000000000 +0100
+++ linux/net/netsyms.c 2002-11-13 22:33:32.000000000 +0100
@@ -591,6 +591,10 @@
EXPORT_SYMBOL(softnet_data);
+#ifdef CONFIG_LINKWATCH
+EXPORT_SYMBOL(linkwatch_fire_event);
+#endif
move this to link_watch.c so that the featureset is fully encapsulated.
Jeff
|