PATCH idea - netlink and link changes

PATCH idea - netlink and link changes

[Tim Hockin]

[-- Attachment #1: Type: text/plain, Size: 566 bytes --]

I'm not subscribed to netdev or linux-net, I was asked to forward this 
patch to these lists.  Please CC: me on any commentary.


Looking for feedback on this quickie patch.  This enables netlink to
deliver link-change events, as reported by drivers via
netif_carrier_{on,off}.

* We could use a different RTM than NEWLINK (RTM_LINKCHANGE?) if we care
* We could do notifier_call_chain(netdev_chain) with NETDEV_UP or
NETDEV_LINKCHANGE (new) if we care

comments?

-- 
Tim Hockin
Systems Software Engineer
Sun Microsystems, Linux Kernel Engineering
thockin@sun.com


[-- Attachment #2: link_change_via_netlink.diff --]
[-- Type: text/plain, Size: 5101 bytes --]

===== drivers/net/net_init.c 1.9 vs edited =====
--- 1.9/drivers/net/net_init.c	Mon Feb  4 23:44:55 2002
+++ edited/drivers/net/net_init.c	Thu Oct 10 11:44:18 2002
@@ -93,6 +93,7 @@
 		dev->priv = (void *) (((long)(dev + 1) + 31) & ~31);
 
 	setup(dev);
+	INIT_WORK(&dev->link_work, NULL, NULL);
 	strcpy(dev->name, mask);
 
 	return dev;
@@ -163,6 +164,7 @@
 	 */
 	
 	setup(dev);
+	INIT_WORK(&dev->link_work, NULL, NULL);
 	
 	if (new_device) {
 		int err;
===== include/linux/netdevice.h 1.24 vs edited =====
--- 1.24/include/linux/netdevice.h	Fri Oct  4 19:17:35 2002
+++ edited/include/linux/netdevice.h	Thu Oct 10 14:56:23 2002
@@ -35,6 +35,7 @@
 
 #ifdef __KERNEL__
 #include <linux/config.h>
+#include <linux/workqueue.h>
 #ifdef CONFIG_NET_PROFILE
 #include <net/profile.h>
 #endif
@@ -437,6 +438,7 @@
 	/* this will get initialized at each interface type init routine */
 	struct divert_blk	*divert;
 #endif /* CONFIG_NET_DIVERT */
+	struct work_struct	link_work;
 };
 
 
@@ -637,17 +639,20 @@
 }
 
 extern void __netdev_watchdog_up(struct net_device *dev);
+extern void netdev_do_link_work(struct net_device *dev);
 
 static inline void netif_carrier_on(struct net_device *dev)
 {
 	clear_bit(__LINK_STATE_NOCARRIER, &dev->state);
 	if (netif_running(dev))
 		__netdev_watchdog_up(dev);
+	netdev_do_link_work(dev);
 }
 
 static inline void netif_carrier_off(struct net_device *dev)
 {
 	set_bit(__LINK_STATE_NOCARRIER, &dev->state);
+	netdev_do_link_work(dev);
 }
 
 /* Hot-plugging. */
===== include/linux/workqueue.h 1.3 vs edited =====
--- 1.3/include/linux/workqueue.h	Thu Oct  3 14:29:58 2002
+++ edited/include/linux/workqueue.h	Thu Oct 10 16:50:36 2002
@@ -7,6 +7,7 @@
 
 #include <linux/timer.h>
 #include <linux/linkage.h>
+#include <linux/spinlock.h>
 
 struct workqueue_struct;
 
@@ -17,12 +18,15 @@
 	void *data;
 	void *wq_data;
 	struct timer_list timer;
+	spinlock_t lock;
 };
 
 #define __WORK_INITIALIZER(n, f, d) {				\
         .entry	= { &(n).entry, &(n).entry },			\
+	.pending = 0,						\
 	.func = (f),						\
-	.data = (d) }
+	.data = (d),						\
+	.lock = SPIN_LOCK_UNLOCKED }
 
 #define DECLARE_WORK(n, f, d)					\
 	struct work_struct n = __WORK_INITIALIZER(n, f, d)
@@ -45,6 +49,7 @@
 		(_work)->pending = 0;				\
 		PREPARE_WORK((_work), (_func), (_data));	\
 		init_timer(&(_work)->timer);			\
+		spin_lock_init(&(_work)->lock);			\
 	} while (0)
 
 extern struct workqueue_struct *create_workqueue(const char *name);
@@ -56,10 +61,13 @@
 
 extern int FASTCALL(schedule_work(struct work_struct *work));
 extern int FASTCALL(schedule_delayed_work(struct work_struct *work, unsigned long delay));
+extern int FASTCALL(cancel_work(struct work_struct *work));
 extern void flush_scheduled_work(void);
 extern int current_is_keventd(void);
 
 extern void init_workqueues(void);
+
+#define work_pending(_work)	test_bit(0, &(_work)->pending)
 
 #endif
 
===== kernel/workqueue.c 1.3 vs edited =====
--- 1.3/kernel/workqueue.c	Thu Oct  3 05:37:56 2002
+++ edited/kernel/workqueue.c	Thu Oct 10 17:12:28 2002
@@ -140,10 +140,13 @@
 		void *data = work->data;
 
 		list_del_init(cwq->worklist.next);
-		spin_unlock_irqrestore(&cwq->lock, flags);
+		spin_lock(&work->lock);
+		spin_unlock(&cwq->lock);
 
 		BUG_ON(work->wq_data != cwq);
 		clear_bit(0, &work->pending);
+		spin_unlock_irqrestore(&work->lock, flags);
+		
 		f(data);
 
 		/*
@@ -354,6 +357,28 @@
 	flush_workqueue(keventd_wq);
 }
 
+int cancel_work(struct work_struct *work)
+{
+	unsigned long flags;
+	struct cpu_workqueue_struct *cwq;
+	int cancelled = 0;
+
+	/* this should be safe to read - it only races with queue_*work() */
+	cwq = work->wq_data;
+
+	spin_lock_irqsave(&cwq->lock, flags);
+	spin_lock(&work->lock);
+	if (work_pending(work)) {
+		list_del_init(&work->entry);
+		clear_bit(0, &work->pending);
+		cancelled = 1;
+	}
+	spin_unlock(&work->lock);
+	spin_unlock_irqrestore(&cwq->lock, flags);
+
+	return cancelled;
+}
+
 int current_is_keventd(void)
 {
 	struct cpu_workqueue_struct *cwq;
@@ -386,4 +411,5 @@
 EXPORT_SYMBOL(schedule_work);
 EXPORT_SYMBOL(schedule_delayed_work);
 EXPORT_SYMBOL(flush_scheduled_work);
+EXPORT_SYMBOL(cancel_work);
 
===== net/netsyms.c 1.29 vs edited =====
--- 1.29/net/netsyms.c	Fri Oct  4 19:17:35 2002
+++ edited/net/netsyms.c	Thu Oct 10 14:57:53 2002
@@ -471,6 +471,7 @@
 EXPORT_SYMBOL(register_netdevice);
 EXPORT_SYMBOL(unregister_netdevice);
 EXPORT_SYMBOL(netdev_state_change);
+EXPORT_SYMBOL(netdev_do_link_work);
 EXPORT_SYMBOL(dev_new_index);
 EXPORT_SYMBOL(dev_get_by_index);
 EXPORT_SYMBOL(__dev_get_by_index);
===== net/core/dev.c 1.41 vs edited =====
--- 1.41/net/core/dev.c	Mon Oct  7 05:31:06 2002
+++ edited/net/core/dev.c	Thu Oct 10 16:05:06 2002
@@ -629,6 +629,20 @@
 	}
 }
 
+static void do_link_work(void *cookie)
+{
+	struct net_device *dev = cookie;
+	rtnl_lock();
+	rtmsg_ifinfo(RTM_NEWLINK, dev, IFF_RUNNING);
+	rtnl_unlock();
+}
+
+void netdev_do_link_work(struct net_device *dev)
+{
+	cancel_work(&dev->link_work);
+	PREPARE_WORK(&dev->link_work, do_link_work, dev);
+	schedule_work(&dev->link_work);
+}
 
 #ifdef CONFIG_KMOD
 

Re: PATCH idea - netlink and link changes

[Stefan Rompf]

Hi,

> I'm not subscribed to netdev or linux-net, I was asked to forward this
> patch to these lists.  Please CC: me on any commentary.

funny. Today I've finished a new release of my link change patch that
lay dormant for months. Now there is a choice ;-)

Cheers, Stefan

Re: PATCH idea - netlink and link changes

[David S. Miller]

   From: Stefan Rompf <srompf@isg.de>
   Date: Sat, 12 Oct 2002 11:23:39 +0200
   
   IMHO we should merge the patches, taking your one workqueue per device
   approach and my usage of IFF_RUNNING as a mirror bit. What do other
   netdev people think?

I'm waiting for Jamal to catch up on his email and comment
on this stuff, he had very clear ideas about what should
be happening here.

Re: PATCH idea - netlink and link changes

[Stefan Rompf]

Hi Tim,

> Looking for feedback on this quickie patch.  This enables netlink to
> deliver link-change events, as reported by drivers via
> netif_carrier_{on,off}.

I've taken a closer look at the approach and like it more than my own
implementation. What you are currently missing is some code to assure
that a device does not have an event queued after removal - look what my
patch does in netdev_unregister(). Patching f.e. the vlan driver to do
some netif_carrier_on/off in the stop method is a good test case. Also,
do not forget to call the network notifier chain beside emitting a
netlink message - the kernel isinterested in the event, too.

IMHO we should merge the patches, taking your one workqueue per device
approach and my usage of IFF_RUNNING as a mirror bit. What do other
netdev people think?

Cheers, Stefan

Re: PATCH idea - netlink and link changes

[jamal]

On Sat, 12 Oct 2002, Stefan Rompf wrote:

> IMHO we should merge the patches, taking your one workqueue per device
> approach and my usage of IFF_RUNNING as a mirror bit. What do other
> netdev people think?
>

Only one work queue please, not one per device.

cheers,
jamal

Re: PATCH idea - netlink and link changes

[Tim Hockin]

jamal wrote:
> 
> On Sat, 12 Oct 2002, Stefan Rompf wrote:
> 
> 
>>IMHO we should merge the patches, taking your one workqueue per device
>>approach and my usage of IFF_RUNNING as a mirror bit. What do other
>>netdev people think?
>>
> Only one work queue please, not one per device.

What of two devices that lose link at the same time?  Say a hub or 
switch dies..


-- 
Tim Hockin
Systems Software Engineer
Sun Microsystems, Linux Kernel Engineering
thockin@sun.com

Re: PATCH idea - netlink and link changes

[Stefan Rompf]

Hi,

> > Only one work queue please, not one per device.
> 
> What of two devices that lose link at the same time?  Say a hub or
> switch dies..

one workqueue != losing notifications. The thread is just growing quite
large, best look it up on http://marc.theaimsgroup.com/?l=linux-netdev
or some other archive.

Cheers, Stefan