Skip to contentSkip to conversation

PATCH idea - netlink and link changes

PATCH Tim Hockin 4 participants 7 messages Oct 11, 2002 – Oct 14, 2002

Subsystems (3)
Networking Network drivers Workqueue
lore
Share & tools
lore

Tim Hockin

author
#0 lore raw

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?

Signature · 4 lines
Tim Hockin
Systems Software Engineer
Sun Microsystems, Linux Kernel Engineering
[email protected]

===== drivers/net/net_init.c 1.9 vs edited =====

drivers/net/net_init.c +2 -0
@@ -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 =====

include/linux/netdevice.h +5 -0
@@ -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 =====

include/linux/workqueue.h +9 -1
@@ -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 =====

kernel/workqueue.c +27 -1
@@ -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 =====

net/netsyms.c +1 -0
@@ -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 =====

net/core/dev.c +14 -0
@@ -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
#1 lore raw

Hi,

Quoted 2 lines

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

#2 lore raw
In reply to Tim Hockin

Hi Tim,

Quoted 3 lines

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

#3 lore raw

From: Stefan Rompf <[email protected]>
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.

#4 lore raw
In reply to Stefan Rompf
Quoted 4 lines · Stefan Rompf

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

author
#5 lore raw
Quoted 9 lines · jamal
Quoted 4 lines · Stefan Rompf

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..

Signature · 4 lines
Tim Hockin
Systems Software Engineer
Sun Microsystems, Linux Kernel Engineering
[email protected]
#6 lore raw

Hi,

Quoted 4 lines
Quoted 1 line

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