[PATCH 00/11] netpoll cleanups

[PATCH 00/11] netpoll cleanups

[Stephen Hemminger]

These patches simplify netpoll by better locking,
reorganizing code etc.

-- 
Stephen Hemminger <shemminger@linux-foundation.org>

[PATCH 01/11] netpoll: use skb_queue_purge

[Stephen Hemminger]

[-- Attachment #1: netpoll-flush.patch --]
[-- Type: text/plain, Size: 683 bytes --]

Use standard route for flushing queue.

Signed-off-by: Stephen Hemminger <shemminger@linux-foundation.org>

--- a/net/core/netpoll.c	2007-11-03 09:13:16.000000000 -0700
+++ b/net/core/netpoll.c	2007-11-03 09:14:05.000000000 -0700
@@ -816,11 +812,7 @@ void netpoll_cleanup(struct netpoll *np)
 				cancel_rearming_delayed_work(&npinfo->tx_work);
 
 				/* clean after last, unfinished work */
-				if (!skb_queue_empty(&npinfo->txq)) {
-					struct sk_buff *skb;
-					skb = __skb_dequeue(&npinfo->txq);
-					kfree_skb(skb);
-				}
+				__skb_queue_purge(&npinfo->txq);
 				kfree(npinfo);
 				np->dev->npinfo = NULL;
 			}

-- 
Stephen Hemminger <shemminger@linux-foundation.org>

[PATCH 02/11] netpoll: netpoll_poll cleanup

[Stephen Hemminger]

[-- Attachment #1: netpoll-poll-cleanup.patch --]
[-- Type: text/plain, Size: 2006 bytes --]

Restructure code slightly to improve readability:
  * dereference device once
  * change obvious while() loop
  * let poll_napi() handle null list itself

Signed-off-by: Stephen Hemminger <shemminger@linux-foundation.org>


--- a/net/core/netpoll.c	2007-11-03 10:02:50.000000000 -0700
+++ b/net/core/netpoll.c	2007-11-03 11:40:27.000000000 -0700
@@ -139,16 +139,15 @@ static int poll_one_napi(struct netpoll_
 	return budget - work;
 }
 
-static void poll_napi(struct netpoll *np)
+static void poll_napi(struct net_device *dev)
 {
-	struct netpoll_info *npinfo = np->dev->npinfo;
-	struct napi_struct *napi;
 	int budget = 16;
+	struct napi_struct *napi;
 
-	list_for_each_entry(napi, &np->dev->napi_list, dev_list) {
+	list_for_each_entry(napi, &dev->napi_list, dev_list) {
 		if (napi->poll_owner != smp_processor_id() &&
 		    spin_trylock(&napi->poll_lock)) {
-			budget = poll_one_napi(npinfo, napi, budget);
+			budget = poll_one_napi(dev->npinfo, napi, budget);
 			spin_unlock(&napi->poll_lock);
 
 			if (!budget)
@@ -159,30 +158,27 @@ static void poll_napi(struct netpoll *np
 
 static void service_arp_queue(struct netpoll_info *npi)
 {
-	struct sk_buff *skb;
+	if (npi) {
+		struct sk_buff *skb;
 
-	if (unlikely(!npi))
-		return;
-
-	skb = skb_dequeue(&npi->arp_tx);
-
-	while (skb != NULL) {
-		arp_reply(skb);
-		skb = skb_dequeue(&npi->arp_tx);
+		while ( (skb = skb_dequeue(&npi->arp_tx)) )
+			arp_reply(skb);
 	}
 }
 
 void netpoll_poll(struct netpoll *np)
 {
-	if (!np->dev || !netif_running(np->dev) || !np->dev->poll_controller)
+	struct net_device *dev = np->dev;
+
+	if (!dev || !netif_running(dev) || !dev->poll_controller)
 		return;
 
 	/* Process pending work on NIC */
-	np->dev->poll_controller(np->dev);
-	if (!list_empty(&np->dev->napi_list))
-		poll_napi(np);
+	dev->poll_controller(dev);
+
+	poll_napi(dev);
 
-	service_arp_queue(np->dev->npinfo);
+	service_arp_queue(dev->npinfo);
 
 	zap_completion_queue();
 }

-- 
Stephen Hemminger <shemminger@linux-foundation.org>

[PATCH 03/11] netpoll: no need to store local_mac

[Stephen Hemminger]

[-- Attachment #1: netpoll-local-mac.patch --]
[-- Type: text/plain, Size: 2812 bytes --]

The local_mac is managed by the network device, no need to
keep a spare copy and all the management problems that could
cause.

Signed-off-by: Stephen Hemminger <shemminger@linux-foundation.org>

--- a/include/linux/netpoll.h	2007-11-03 09:12:48.000000000 -0700
+++ b/include/linux/netpoll.h	2007-11-03 09:18:34.000000000 -0700
@@ -20,7 +20,7 @@ struct netpoll {
 
 	u32 local_ip, remote_ip;
 	u16 local_port, remote_port;
- 	u8 local_mac[ETH_ALEN], remote_mac[ETH_ALEN];
+ 	u8 remote_mac[ETH_ALEN];
 };
 
 struct netpoll_info {
--- a/net/core/netpoll.c	2007-11-03 09:18:30.000000000 -0700
+++ b/net/core/netpoll.c	2007-11-03 09:19:34.000000000 -0700
@@ -360,8 +360,8 @@ void netpoll_send_udp(struct netpoll *np
 	eth = (struct ethhdr *) skb_push(skb, ETH_HLEN);
 	skb_reset_mac_header(skb);
 	skb->protocol = eth->h_proto = htons(ETH_P_IP);
-	memcpy(eth->h_source, np->local_mac, 6);
-	memcpy(eth->h_dest, np->remote_mac, 6);
+	memcpy(eth->h_source, np->dev->dev_addr, ETH_ALEN);
+	memcpy(eth->h_dest, np->remote_mac, ETH_ALEN);
 
 	skb->dev = np->dev;
 
@@ -431,7 +431,7 @@ static void arp_reply(struct sk_buff *sk
 
 	/* Fill the device header for the ARP frame */
 	if (dev_hard_header(send_skb, skb->dev, ptype,
-			    sha, np->local_mac,
+			    sha, np->dev->dev_addr,
 			    send_skb->len) < 0) {
 		kfree_skb(send_skb);
 		return;
@@ -737,9 +737,6 @@ int netpoll_setup(struct netpoll *np)
 		}
 	}
 
-	if (is_zero_ether_addr(np->local_mac) && ndev->dev_addr)
-		memcpy(np->local_mac, ndev->dev_addr, 6);
-
 	if (!np->local_ip) {
 		rcu_read_lock();
 		in_dev = __in_dev_get_rcu(ndev);
--- a/drivers/net/netconsole.c	2007-10-17 15:25:31.000000000 -0700
+++ b/drivers/net/netconsole.c	2007-11-03 09:26:27.000000000 -0700
@@ -306,9 +306,11 @@ static ssize_t show_remote_ip(struct net
 
 static ssize_t show_local_mac(struct netconsole_target *nt, char *buf)
 {
+	struct net_device *dev = nt->np.dev;
+
 	DECLARE_MAC_BUF(mac);
 	return snprintf(buf, PAGE_SIZE, "%s\n",
-			print_mac(mac, nt->np.local_mac));
+			print_mac(mac, dev->dev_addr));
 }
 
 static ssize_t show_remote_mac(struct netconsole_target *nt, char *buf)
@@ -667,7 +669,7 @@ static int netconsole_netdev_event(struc
 	struct netconsole_target *nt;
 	struct net_device *dev = ptr;
 
-	if (!(event == NETDEV_CHANGEADDR || event == NETDEV_CHANGENAME))
+	if (!(event == NETDEV_CHANGENAME))
 		goto done;
 
 	spin_lock_irqsave(&target_list_lock, flags);
@@ -675,10 +677,6 @@ static int netconsole_netdev_event(struc
 		netconsole_target_get(nt);
 		if (nt->np.dev == dev) {
 			switch (event) {
-			case NETDEV_CHANGEADDR:
-				memcpy(nt->np.local_mac, dev->dev_addr, ETH_ALEN);
-				break;
-
 			case NETDEV_CHANGENAME:
 				strlcpy(nt->np.dev_name, dev->name, IFNAMSIZ);
 				break;

-- 
Stephen Hemminger <shemminger@linux-foundation.org>

[PATCH 04/11] netpoll: alternative implementation of dropping

[Stephen Hemminger]

[-- Attachment #1: netpoll-drop-flag.patch --]
[-- Type: text/plain, Size: 1330 bytes --]

Signed-off-by: Stephen Hemminger <shemminger@linux-foundation.org>

--- a/net/core/netpoll.c	2007-11-03 09:19:34.000000000 -0700
+++ b/net/core/netpoll.c	2007-11-03 09:33:31.000000000 -0700
@@ -40,7 +40,6 @@ static atomic_t trapped;
 
 #define USEC_PER_POLL	50
 #define NETPOLL_RX_ENABLED  1
-#define NETPOLL_RX_DROP     2
 
 #define MAX_SKB_SIZE \
 		(MAX_UDP_CHUNK + sizeof(struct udphdr) + \
@@ -128,13 +127,11 @@ static int poll_one_napi(struct netpoll_
 	if (!test_bit(NAPI_STATE_SCHED, &napi->state))
 		return budget;
 
-	npinfo->rx_flags |= NETPOLL_RX_DROP;
 	atomic_inc(&trapped);
 
 	work = napi->poll(napi, budget);
 
 	atomic_dec(&trapped);
-	npinfo->rx_flags &= ~NETPOLL_RX_DROP;
 
 	return budget - work;
 }
@@ -475,7 +472,7 @@ int __netpoll_rx(struct sk_buff *skb)
 	if (skb->dev->type != ARPHRD_ETHER)
 		goto out;
 
-	/* check if netpoll clients need ARP */
+	/* if receive ARP during middle of NAPI poll, then queue */
 	if (skb->protocol == htons(ETH_P_ARP) &&
 	    atomic_read(&trapped)) {
 		skb_queue_tail(&npi->arp_tx, skb);
@@ -537,6 +534,9 @@ int __netpoll_rx(struct sk_buff *skb)
 	return 1;
 
 out:
+	/* If packet received while already in poll then just
+	 * silently drop.
+	 */
 	if (atomic_read(&trapped)) {
 		kfree_skb(skb);
 		return 1;

-- 
Stephen Hemminger <shemminger@linux-foundation.org>

[PATCH 05/11] netpoll: dont need rx_flags

[Stephen Hemminger]

[-- Attachment #1: netpoll-enable-flag.patch --]
[-- Type: text/plain, Size: 2071 bytes --]

The rx_flags variable is redundant. Turning rx on/off is done
via setting the rx_np pointer.

Signed-off-by: Stephen Hemminger <shemminger@linux-foundation.org>


--- a/include/linux/netpoll.h	2007-11-03 09:32:54.000000000 -0700
+++ b/include/linux/netpoll.h	2007-11-03 09:35:45.000000000 -0700
@@ -25,7 +25,6 @@ struct netpoll {
 
 struct netpoll_info {
 	atomic_t refcnt;
-	int rx_flags;
 	spinlock_t rx_lock;
 	struct netpoll *rx_np; /* netpoll that registered an rx_hook */
 	struct sk_buff_head arp_tx; /* list of arp requests to reply to */
@@ -51,12 +50,11 @@ static inline int netpoll_rx(struct sk_b
 	unsigned long flags;
 	int ret = 0;
 
-	if (!npinfo || (!npinfo->rx_np && !npinfo->rx_flags))
+	if (!npinfo || !npinfo->rx_np)
 		return 0;
 
 	spin_lock_irqsave(&npinfo->rx_lock, flags);
-	/* check rx_flags again with the lock held */
-	if (npinfo->rx_flags && __netpoll_rx(skb))
+	if (__netpoll_rx(skb))
 		ret = 1;
 	spin_unlock_irqrestore(&npinfo->rx_lock, flags);
 
--- a/net/core/netpoll.c	2007-11-03 09:33:31.000000000 -0700
+++ b/net/core/netpoll.c	2007-11-03 09:35:45.000000000 -0700
@@ -39,7 +39,6 @@ static struct sk_buff_head skb_pool;
 static atomic_t trapped;
 
 #define USEC_PER_POLL	50
-#define NETPOLL_RX_ENABLED  1
 
 #define MAX_SKB_SIZE \
 		(MAX_UDP_CHUNK + sizeof(struct udphdr) + \
@@ -675,7 +674,6 @@ int netpoll_setup(struct netpoll *np)
 			goto release;
 		}
 
-		npinfo->rx_flags = 0;
 		npinfo->rx_np = NULL;
 
 		spin_lock_init(&npinfo->rx_lock);
@@ -757,7 +755,6 @@ int netpoll_setup(struct netpoll *np)
 
 	if (np->rx_hook) {
 		spin_lock_irqsave(&npinfo->rx_lock, flags);
-		npinfo->rx_flags |= NETPOLL_RX_ENABLED;
 		npinfo->rx_np = np;
 		spin_unlock_irqrestore(&npinfo->rx_lock, flags);
 	}
@@ -799,7 +796,6 @@ void netpoll_cleanup(struct netpoll *np)
 			if (npinfo->rx_np == np) {
 				spin_lock_irqsave(&npinfo->rx_lock, flags);
 				npinfo->rx_np = NULL;
-				npinfo->rx_flags &= ~NETPOLL_RX_ENABLED;
 				spin_unlock_irqrestore(&npinfo->rx_lock, flags);
 			}
 

-- 
Stephen Hemminger <shemminger@linux-foundation.org>

[PATCH 06/11] netpoll: remove dev_name for npinfo

[Stephen Hemminger]

[-- Attachment #1: netpoll-noname.patch --]
[-- Type: text/plain, Size: 9910 bytes --]

The device name was only in npinfo for netconsole target
configuration, so move it to netconsole.  Netconsole only
needs the value during config, so no need to do all
the device name tracking etc.. 

Make functions for common code for instantiation and 
start up.

Signed-off-by: Stephen Hemminger <shemminger@linux-foundation.org>


--- a/drivers/net/netconsole.c	2007-11-03 09:26:27.000000000 -0700
+++ b/drivers/net/netconsole.c	2007-11-03 10:02:18.000000000 -0700
@@ -96,6 +96,7 @@ struct netconsole_target {
 	struct config_item	item;
 #endif
 	int			enabled;
+	char 			dev_name[IFNAMSIZ];
 	struct netpoll		np;
 };
 
@@ -157,39 +158,67 @@ static void netconsole_target_put(struct
 
 #endif	/* CONFIG_NETCONSOLE_DYNAMIC */
 
+
+
+/*
+ * Allocate and initialize with defaults.
+ * Note that these targets get their config_item fields zeroed-out.
+ */
+static struct netconsole_target *new_target(void)
+{
+	struct netconsole_target *nt;
+
+	nt = kzalloc(sizeof(*nt), GFP_KERNEL);
+	if (nt) {
+		nt->np.name = "netconsole";
+		strlcpy(nt->dev_name, "eth0", IFNAMSIZ);
+		nt->np.local_port = 6665;
+		nt->np.remote_port = 6666;
+		memset(nt->np.remote_mac, 0xff, ETH_ALEN);
+	}
+
+	return nt;
+}
+
+static int start_target(struct netconsole_target *nt)
+{
+	struct net_device *dev;
+	int err;
+
+	dev = dev_get_by_name(&init_net, nt->dev_name);
+	if (!dev)
+		return -ENODEV;
+
+	err = netpoll_setup(&nt->np, dev);
+	if (err)
+		dev_put(dev);
+	else
+		nt->enabled = 1;
+	return err;
+}
+
+
 /* Allocate new target (from boot/module param) and setup netpoll for it */
 static struct netconsole_target *alloc_param_target(char *target_config)
 {
 	int err = -ENOMEM;
 	struct netconsole_target *nt;
 
-	/*
-	 * Allocate and initialize with defaults.
-	 * Note that these targets get their config_item fields zeroed-out.
-	 */
-	nt = kzalloc(sizeof(*nt), GFP_KERNEL);
+	nt = new_target();
 	if (!nt) {
 		printk(KERN_ERR "netconsole: failed to allocate memory\n");
 		goto fail;
 	}
 
-	nt->np.name = "netconsole";
-	strlcpy(nt->np.dev_name, "eth0", IFNAMSIZ);
-	nt->np.local_port = 6665;
-	nt->np.remote_port = 6666;
-	memset(nt->np.remote_mac, 0xff, ETH_ALEN);
-
 	/* Parse parameters and setup netpoll */
-	err = netpoll_parse_options(&nt->np, target_config);
+	err = netpoll_parse_options(&nt->np, target_config, nt->dev_name);
 	if (err)
 		goto fail;
 
-	err = netpoll_setup(&nt->np);
+
+	err = start_target(nt);
 	if (err)
 		goto fail;
-
-	nt->enabled = 1;
-
 	return nt;
 
 fail:
@@ -279,7 +308,8 @@ static ssize_t show_enabled(struct netco
 
 static ssize_t show_dev_name(struct netconsole_target *nt, char *buf)
 {
-	return snprintf(buf, PAGE_SIZE, "%s\n", nt->np.dev_name);
+	return snprintf(buf, PAGE_SIZE, "%s\n",
+			nt->enabled ? nt->np.dev->name : nt->dev_name);
 }
 
 static ssize_t show_local_port(struct netconsole_target *nt, char *buf)
@@ -339,14 +369,13 @@ static ssize_t store_enabled(struct netc
 		return enabled;
 
 	if (enabled) {	/* 1 */
-
 		/*
 		 * Skip netpoll_parse_options() -- all the attributes are
 		 * already configured via configfs. Just print them out.
 		 */
 		netpoll_print_options(&nt->np);
 
-		err = netpoll_setup(&nt->np);
+		err = start_target(nt);
 		if (err)
 			return err;
 
@@ -365,7 +394,7 @@ static ssize_t store_dev_name(struct net
 			      const char *buf,
 			      size_t count)
 {
-	size_t len;
+	char *cp;
 
 	if (nt->enabled) {
 		printk(KERN_ERR "netconsole: target (%s) is enabled, "
@@ -374,12 +403,12 @@ static ssize_t store_dev_name(struct net
 		return -EINVAL;
 	}
 
-	strlcpy(nt->np.dev_name, buf, IFNAMSIZ);
+	strlcpy(nt->dev_name, buf, IFNAMSIZ);
 
 	/* Get rid of possible trailing newline from echo(1) */
-	len = strnlen(nt->np.dev_name, IFNAMSIZ);
-	if (nt->np.dev_name[len - 1] == '\n')
-		nt->np.dev_name[len - 1] = '\0';
+	cp = strnchr(nt->dev_name, '\n', IFNAMSIZ);
+	if (cp)
+		*cp = '\0';
 
 	return strnlen(buf, count);
 }
@@ -591,21 +620,7 @@ static struct config_item *make_netconso
 	unsigned long flags;
 	struct netconsole_target *nt;
 
-	/*
-	 * Allocate and initialize with defaults.
-	 * Target is disabled at creation (enabled == 0).
-	 */
-	nt = kzalloc(sizeof(*nt), GFP_KERNEL);
-	if (!nt) {
-		printk(KERN_ERR "netconsole: failed to allocate memory\n");
-		return NULL;
-	}
-
-	nt->np.name = "netconsole";
-	strlcpy(nt->np.dev_name, "eth0", IFNAMSIZ);
-	nt->np.local_port = 6665;
-	nt->np.remote_port = 6666;
-	memset(nt->np.remote_mac, 0xff, ETH_ALEN);
+	nt = new_target();
 
 	/* Initialize the config_item member */
 	config_item_init_type_name(&nt->item, name, &netconsole_target_type);
@@ -660,40 +675,6 @@ static struct configfs_subsystem netcons
 
 #endif	/* CONFIG_NETCONSOLE_DYNAMIC */
 
-/* Handle network interface device notifications */
-static int netconsole_netdev_event(struct notifier_block *this,
-				   unsigned long event,
-				   void *ptr)
-{
-	unsigned long flags;
-	struct netconsole_target *nt;
-	struct net_device *dev = ptr;
-
-	if (!(event == NETDEV_CHANGENAME))
-		goto done;
-
-	spin_lock_irqsave(&target_list_lock, flags);
-	list_for_each_entry(nt, &target_list, list) {
-		netconsole_target_get(nt);
-		if (nt->np.dev == dev) {
-			switch (event) {
-			case NETDEV_CHANGENAME:
-				strlcpy(nt->np.dev_name, dev->name, IFNAMSIZ);
-				break;
-			}
-		}
-		netconsole_target_put(nt);
-	}
-	spin_unlock_irqrestore(&target_list_lock, flags);
-
-done:
-	return NOTIFY_DONE;
-}
-
-static struct notifier_block netconsole_netdev_notifier = {
-	.notifier_call  = netconsole_netdev_event,
-};
-
 static void write_msg(struct console *con, const char *msg, unsigned int len)
 {
 	int frag, left;
@@ -755,22 +736,15 @@ static int __init init_netconsole(void)
 		}
 	}
 
-	err = register_netdevice_notifier(&netconsole_netdev_notifier);
-	if (err)
-		goto fail;
-
 	err = dynamic_netconsole_init();
 	if (err)
-		goto undonotifier;
+		goto fail;
 
 	register_console(&netconsole);
 	printk(KERN_INFO "netconsole: network logging started\n");
 
 	return err;
 
-undonotifier:
-	unregister_netdevice_notifier(&netconsole_netdev_notifier);
-
 fail:
 	printk(KERN_ERR "netconsole: cleaning up\n");
 
@@ -793,7 +767,6 @@ static void __exit cleanup_netconsole(vo
 
 	unregister_console(&netconsole);
 	dynamic_netconsole_exit();
-	unregister_netdevice_notifier(&netconsole_netdev_notifier);
 
 	/*
 	 * Targets created via configfs pin references on our module
--- a/include/linux/netpoll.h	2007-11-03 09:35:45.000000000 -0700
+++ b/include/linux/netpoll.h	2007-11-03 09:41:57.000000000 -0700
@@ -14,7 +14,6 @@
 
 struct netpoll {
 	struct net_device *dev;
-	char dev_name[IFNAMSIZ];
 	const char *name;
 	void (*rx_hook)(struct netpoll *, int, char *, int);
 
@@ -35,8 +34,8 @@ struct netpoll_info {
 void netpoll_poll(struct netpoll *np);
 void netpoll_send_udp(struct netpoll *np, const char *msg, int len);
 void netpoll_print_options(struct netpoll *np);
-int netpoll_parse_options(struct netpoll *np, char *opt);
-int netpoll_setup(struct netpoll *np);
+int netpoll_parse_options(struct netpoll *np, char *opt, char *name);
+int netpoll_setup(struct netpoll *np, struct net_device *dev);
 int netpoll_trap(void);
 void netpoll_set_trap(int trap);
 void netpoll_cleanup(struct netpoll *np);
--- a/net/core/netpoll.c	2007-11-03 09:35:45.000000000 -0700
+++ b/net/core/netpoll.c	2007-11-03 09:42:43.000000000 -0700
@@ -552,7 +552,7 @@ void netpoll_print_options(struct netpol
 	printk(KERN_INFO "%s: local IP %d.%d.%d.%d\n",
 			 np->name, HIPQUAD(np->local_ip));
 	printk(KERN_INFO "%s: interface %s\n",
-			 np->name, np->dev_name);
+			 np->name, np->dev->name);
 	printk(KERN_INFO "%s: remote port %d\n",
 			 np->name, np->remote_port);
 	printk(KERN_INFO "%s: remote IP %d.%d.%d.%d\n",
@@ -561,7 +561,7 @@ void netpoll_print_options(struct netpol
 	                 np->name, print_mac(mac, np->remote_mac));
 }
 
-int netpoll_parse_options(struct netpoll *np, char *opt)
+int netpoll_parse_options(struct netpoll *np, char *opt, char *dev_name)
 {
 	char *cur=opt, *delim;
 
@@ -588,7 +588,7 @@ int netpoll_parse_options(struct netpoll
 		if ((delim = strchr(cur, ',')) == NULL)
 			goto parse_failed;
 		*delim = 0;
-		strlcpy(np->dev_name, cur, sizeof(np->dev_name));
+		strlcpy(dev_name, cur, IFNAMSIZ);
 		cur = delim;
 	}
 	cur++;
@@ -650,22 +650,13 @@ int netpoll_parse_options(struct netpoll
 	return -1;
 }
 
-int netpoll_setup(struct netpoll *np)
+int netpoll_setup(struct netpoll *np, struct net_device *ndev)
 {
-	struct net_device *ndev = NULL;
 	struct in_device *in_dev;
 	struct netpoll_info *npinfo;
 	unsigned long flags;
 	int err;
 
-	if (np->dev_name)
-		ndev = dev_get_by_name(&init_net, np->dev_name);
-	if (!ndev) {
-		printk(KERN_ERR "%s: %s doesn't exist, aborting.\n",
-		       np->name, np->dev_name);
-		return -ENODEV;
-	}
-
 	np->dev = ndev;
 	if (!ndev->npinfo) {
 		npinfo = kmalloc(sizeof(*npinfo), GFP_KERNEL);
@@ -689,7 +680,7 @@ int netpoll_setup(struct netpoll *np)
 
 	if (!ndev->poll_controller) {
 		printk(KERN_ERR "%s: %s doesn't support polling, aborting.\n",
-		       np->name, np->dev_name);
+		       np->name, ndev->name);
 		err = -ENOTSUPP;
 		goto release;
 	}
@@ -698,7 +689,7 @@ int netpoll_setup(struct netpoll *np)
 		unsigned long atmost, atleast;
 
 		printk(KERN_INFO "%s: device %s not up yet, forcing it\n",
-		       np->name, np->dev_name);
+		       np->name, ndev->name);
 
 		rtnl_lock();
 		err = dev_open(ndev);
@@ -742,7 +733,7 @@ int netpoll_setup(struct netpoll *np)
 		if (!in_dev || !in_dev->ifa_list) {
 			rcu_read_unlock();
 			printk(KERN_ERR "%s: no IP address for %s, aborting\n",
-			       np->name, np->dev_name);
+			       np->name, ndev->name);
 			err = -EDESTADDRREQ;
 			goto release;
 		}
@@ -774,7 +765,6 @@ int netpoll_setup(struct netpoll *np)
 	if (!ndev->npinfo)
 		kfree(npinfo);
 	np->dev = NULL;
-	dev_put(ndev);
 	return err;
 }
 

-- 
Stephen Hemminger <shemminger@linux-foundation.org>

[PATCH 07/11] netpoll: get rid of name parameter

[Stephen Hemminger]

[-- Attachment #1: netpoll-no-name.patch --]
[-- Type: text/plain, Size: 6229 bytes --]

The name was being stored and used only for error messages.
The same effect can be had by just passing it in where needed during
config.

Signed-off-by: Stephen Hemminger <shemminger@linux-foundation.org>


--- a/drivers/net/netconsole.c	2007-11-03 10:03:40.000000000 -0700
+++ b/drivers/net/netconsole.c	2007-11-03 10:12:35.000000000 -0700
@@ -170,7 +170,6 @@ static struct netconsole_target *new_tar
 
 	nt = kzalloc(sizeof(*nt), GFP_KERNEL);
 	if (nt) {
-		nt->np.name = "netconsole";
 		strlcpy(nt->dev_name, "eth0", IFNAMSIZ);
 		nt->np.local_port = 6665;
 		nt->np.remote_port = 6666;
@@ -186,8 +185,13 @@ static int start_target(struct netconsol
 	int err;
 
 	dev = dev_get_by_name(&init_net, nt->dev_name);
-	if (!dev)
+	if (!dev) {
+		printk(KERN_ERR "netconsole: device '%s' does not exist.\n",
+		       nt->dev_name);
 		return -ENODEV;
+	}
+
+	netpoll_print_options("netconsole", &nt->np);
 
 	err = netpoll_setup(&nt->np, dev);
 	if (err)
@@ -212,13 +216,16 @@ static struct netconsole_target *alloc_p
 
 	/* Parse parameters and setup netpoll */
 	err = netpoll_parse_options(&nt->np, target_config, nt->dev_name);
-	if (err)
+	if (err) {
+		printk(KERN_ERR "netconsole: parse options '%s' failed %d\n",
+		       target_config, err);
 		goto fail;
-
+	}
 
 	err = start_target(nt);
 	if (err)
 		goto fail;
+
 	return nt;
 
 fail:
@@ -369,12 +376,6 @@ static ssize_t store_enabled(struct netc
 		return enabled;
 
 	if (enabled) {	/* 1 */
-		/*
-		 * Skip netpoll_parse_options() -- all the attributes are
-		 * already configured via configfs. Just print them out.
-		 */
-		netpoll_print_options(&nt->np);
-
 		err = start_target(nt);
 		if (err)
 			return err;
--- a/include/linux/netpoll.h	2007-11-03 10:03:40.000000000 -0700
+++ b/include/linux/netpoll.h	2007-11-03 10:10:00.000000000 -0700
@@ -14,7 +14,6 @@
 
 struct netpoll {
 	struct net_device *dev;
-	const char *name;
 	void (*rx_hook)(struct netpoll *, int, char *, int);
 
 	u32 local_ip, remote_ip;
@@ -33,7 +32,7 @@ struct netpoll_info {
 
 void netpoll_poll(struct netpoll *np);
 void netpoll_send_udp(struct netpoll *np, const char *msg, int len);
-void netpoll_print_options(struct netpoll *np);
+void netpoll_print_options(const char *prefix, struct netpoll *np);
 int netpoll_parse_options(struct netpoll *np, char *opt, char *name);
 int netpoll_setup(struct netpoll *np, struct net_device *dev);
 int netpoll_trap(void);
--- a/net/core/netpoll.c	2007-11-03 10:03:40.000000000 -0700
+++ b/net/core/netpoll.c	2007-11-03 10:22:44.000000000 -0700
@@ -544,21 +544,21 @@ out:
 	return 0;
 }
 
-void netpoll_print_options(struct netpoll *np)
+void netpoll_print_options(const char *name, struct netpoll *np)
 {
 	DECLARE_MAC_BUF(mac);
 	printk(KERN_INFO "%s: local port %d\n",
-			 np->name, np->local_port);
+			 name, np->local_port);
 	printk(KERN_INFO "%s: local IP %d.%d.%d.%d\n",
-			 np->name, HIPQUAD(np->local_ip));
+			 name, HIPQUAD(np->local_ip));
 	printk(KERN_INFO "%s: interface %s\n",
-			 np->name, np->dev->name);
+			 name, np->dev->name);
 	printk(KERN_INFO "%s: remote port %d\n",
-			 np->name, np->remote_port);
+			 name, np->remote_port);
 	printk(KERN_INFO "%s: remote IP %d.%d.%d.%d\n",
-			 np->name, HIPQUAD(np->remote_ip));
+			 name, HIPQUAD(np->remote_ip));
 	printk(KERN_INFO "%s: remote ethernet address %s\n",
-	                 np->name, print_mac(mac, np->remote_mac));
+	                 name, print_mac(mac, np->remote_mac));
 }
 
 int netpoll_parse_options(struct netpoll *np, char *opt, char *dev_name)
@@ -640,13 +640,9 @@ int netpoll_parse_options(struct netpoll
 		np->remote_mac[5] = simple_strtol(cur, NULL, 16);
 	}
 
-	netpoll_print_options(np);
-
 	return 0;
 
  parse_failed:
-	printk(KERN_INFO "%s: couldn't parse config at %s!\n",
-	       np->name, cur);
 	return -1;
 }
 
@@ -679,8 +675,8 @@ int netpoll_setup(struct netpoll *np, st
 	}
 
 	if (!ndev->poll_controller) {
-		printk(KERN_ERR "%s: %s doesn't support polling, aborting.\n",
-		       np->name, ndev->name);
+		printk(KERN_ERR "netpoll: %s doesn't support polling, aborting.\n",
+		       ndev->name);
 		err = -ENOTSUPP;
 		goto release;
 	}
@@ -688,16 +684,16 @@ int netpoll_setup(struct netpoll *np, st
 	if (!netif_running(ndev)) {
 		unsigned long atmost, atleast;
 
-		printk(KERN_INFO "%s: device %s not up yet, forcing it\n",
-		       np->name, ndev->name);
+		printk(KERN_INFO "netpoll: device %s not up yet, forcing it\n",
+		       ndev->name);
 
 		rtnl_lock();
 		err = dev_open(ndev);
 		rtnl_unlock();
 
 		if (err) {
-			printk(KERN_ERR "%s: failed to open %s\n",
-			       np->name, ndev->name);
+			printk(KERN_ERR "netpoll: failed to open %s\n",
+			       ndev->name);
 			goto release;
 		}
 
@@ -705,9 +701,9 @@ int netpoll_setup(struct netpoll *np, st
 		atmost = jiffies + 4*HZ;
 		while (!netif_carrier_ok(ndev)) {
 			if (time_after(jiffies, atmost)) {
-				printk(KERN_NOTICE
-				       "%s: timeout waiting for carrier\n",
-				       np->name);
+				printk(KERN_NOTICE "netpoll:"
+				       "timeout waiting for carrier on '%s'\n",
+				       ndev->name);
 				break;
 			}
 			cond_resched();
@@ -719,9 +715,9 @@ int netpoll_setup(struct netpoll *np, st
 		 */
 
 		if (time_before(jiffies, atleast)) {
-			printk(KERN_NOTICE "%s: carrier detect appears"
-			       " untrustworthy, waiting 4 seconds\n",
-			       np->name);
+			printk(KERN_NOTICE "netpoll: carrier detect appears"
+			       " untrustworthy '%s', waiting 4 seconds\n",
+			       ndev->name);
 			msleep(4000);
 		}
 	}
@@ -732,16 +728,16 @@ int netpoll_setup(struct netpoll *np, st
 
 		if (!in_dev || !in_dev->ifa_list) {
 			rcu_read_unlock();
-			printk(KERN_ERR "%s: no IP address for %s, aborting\n",
-			       np->name, ndev->name);
+			printk(KERN_ERR "netpoll: no IP address for %s, aborting\n",
+			       ndev->name);
 			err = -EDESTADDRREQ;
 			goto release;
 		}
 
 		np->local_ip = ntohl(in_dev->ifa_list->ifa_local);
 		rcu_read_unlock();
-		printk(KERN_INFO "%s: local IP %d.%d.%d.%d\n",
-		       np->name, HIPQUAD(np->local_ip));
+		printk(KERN_INFO "netpoll: local IP %d.%d.%d.%d\n",
+		       HIPQUAD(np->local_ip));
 	}
 
 	if (np->rx_hook) {

-- 
Stephen Hemminger <shemminger@linux-foundation.org>

[PATCH 08/11] netpoll: NETPOLL_TRAP configuration change

[Stephen Hemminger]

[-- Attachment #1: netpoll-trap-stub.patch --]
[-- Type: text/plain, Size: 3002 bytes --]

NETPOLL_TRAP is actually not used by any in-tree code.
Rather than exposing it in kernel configuration, make it a selectable
option and make sure API is stubbed properly. I.e: if not configured
then netpoll_set_trap is not available.


Signed-off-by: Stephen Hemminger <shemminger@linux-foundation.org>

---
P.s: netpoll stuff should really be EXPORT_SYMBOL_GPL??


--- a/include/linux/netdevice.h	2007-11-03 11:30:57.000000000 -0700
+++ b/include/linux/netdevice.h	2007-11-03 11:30:59.000000000 -0700
@@ -837,8 +837,14 @@ extern int call_netdevice_notifiers(unsi
 extern struct net_device	*dev_get_by_index(struct net *net, int ifindex);
 extern struct net_device	*__dev_get_by_index(struct net *net, int ifindex);
 extern int		dev_restart(struct net_device *dev);
+
 #ifdef CONFIG_NETPOLL_TRAP
 extern int		netpoll_trap(void);
+extern void		netpoll_set_trap(int trap);
+#else
+static inline int netpoll_trap(void) {
+	return 0;
+}
 #endif
 
 static inline int dev_hard_header(struct sk_buff *skb, struct net_device *dev,
@@ -918,12 +924,11 @@ static inline void netif_start_queue(str
  */
 static inline void netif_wake_queue(struct net_device *dev)
 {
-#ifdef CONFIG_NETPOLL_TRAP
 	if (netpoll_trap()) {
 		clear_bit(__LINK_STATE_XOFF, &dev->state);
 		return;
 	}
-#endif
+
 	if (test_and_clear_bit(__LINK_STATE_XOFF, &dev->state))
 		__netif_schedule(dev);
 }
@@ -993,10 +998,8 @@ static inline void netif_start_subqueue(
 static inline void netif_stop_subqueue(struct net_device *dev, u16 queue_index)
 {
 #ifdef CONFIG_NETDEVICES_MULTIQUEUE
-#ifdef CONFIG_NETPOLL_TRAP
 	if (netpoll_trap())
 		return;
-#endif
 	set_bit(__LINK_STATE_XOFF, &dev->egress_subqueue[queue_index].state);
 #endif
 }
@@ -1035,10 +1038,8 @@ static inline int netif_subqueue_stopped
 static inline void netif_wake_subqueue(struct net_device *dev, u16 queue_index)
 {
 #ifdef CONFIG_NETDEVICES_MULTIQUEUE
-#ifdef CONFIG_NETPOLL_TRAP
 	if (netpoll_trap())
 		return;
-#endif
 	if (test_and_clear_bit(__LINK_STATE_XOFF,
 			       &dev->egress_subqueue[queue_index].state))
 		__netif_schedule(dev);
--- a/drivers/net/Kconfig	2007-11-03 11:30:57.000000000 -0700
+++ b/drivers/net/Kconfig	2007-11-03 11:30:59.000000000 -0700
@@ -3118,8 +3118,8 @@ config NETCONSOLE_DYNAMIC
 config NETPOLL
 	def_bool NETCONSOLE
 
+# If needed use select to enable
 config NETPOLL_TRAP
-	bool "Netpoll traffic trapping"
 	default n
 	depends on NETPOLL
 
--- a/net/core/netpoll.c	2007-11-03 11:30:57.000000000 -0700
+++ b/net/core/netpoll.c	2007-11-03 11:31:45.000000000 -0700
@@ -803,6 +803,7 @@ void netpoll_cleanup(struct netpoll *np)
 	np->dev = NULL;
 }
 
+#ifdef CONFIG_NETPOLL_TRAP
 int netpoll_trap(void)
 {
 	return atomic_read(&trapped);
@@ -818,6 +819,8 @@ void netpoll_set_trap(int trap)
 
 EXPORT_SYMBOL(netpoll_set_trap);
 EXPORT_SYMBOL(netpoll_trap);
+#endif
+
 EXPORT_SYMBOL(netpoll_print_options);
 EXPORT_SYMBOL(netpoll_parse_options);
 EXPORT_SYMBOL(netpoll_setup);

-- 
Stephen Hemminger <shemminger@linux-foundation.org>

[PATCH 09/11] netpoll: ethernet devices only

[Stephen Hemminger]

[-- Attachment #1: netpoll-ether-only.patch --]
[-- Type: text/plain, Size: 687 bytes --]

Netpoll only works on Ethernet devices, so check during setup
rather than just failing silently later.

Signed-off-by: Stephen Hemminger <shemminger@linux-foundation.org>

--- a/net/core/netpoll.c	2007-11-03 11:05:33.000000000 -0700
+++ b/net/core/netpoll.c	2007-11-03 11:08:23.000000000 -0700
@@ -653,6 +653,12 @@ int netpoll_setup(struct netpoll *np, st
 	unsigned long flags;
 	int err;
 
+	if (ndev->type != ARPHRD_ETHER) {
+		printk(KERN_ERR "netpoll: %s is not an ethernet device\n",
+		       ndev->name);
+		return -EINVAL;
+	}
+
 	np->dev = ndev;
 	if (!ndev->npinfo) {
 		npinfo = kmalloc(sizeof(*npinfo), GFP_KERNEL);

-- 
Stephen Hemminger <shemminger@linux-foundation.org>

[PATCH 10/11] netpoll: rx optimization

[Stephen Hemminger]

[-- Attachment #1: netpoll-rx.patch --]
[-- Type: text/plain, Size: 3603 bytes --]

This patch makes netpoll work for non-NAPI devices that call
netif_receive_skb. Devices are allowed to call netif_receive_skb
if they are receiving packets in softirq (ie in tasklet).
One side effect of this is that received packets will be looked
at twice for the non-NAPI case, but this is harmless.

Move the locking out of the inline hook and into the internal
function.

Signed-off-by: Stephen Hemminger <shemminger@linux-foundation.org>


--- a/include/linux/netpoll.h	2007-11-03 11:05:33.000000000 -0700
+++ b/include/linux/netpoll.h	2007-11-03 11:08:36.000000000 -0700
@@ -30,6 +30,9 @@ struct netpoll_info {
 	struct delayed_work tx_work;
 };
 
+
+#ifdef CONFIG_NETPOLL
+
 void netpoll_poll(struct netpoll *np);
 void netpoll_send_udp(struct netpoll *np, const char *msg, int len);
 void netpoll_print_options(const char *prefix, struct netpoll *np);
@@ -38,32 +41,18 @@ int netpoll_setup(struct netpoll *np, st
 int netpoll_trap(void);
 void netpoll_set_trap(int trap);
 void netpoll_cleanup(struct netpoll *np);
-int __netpoll_rx(struct sk_buff *skb);
+bool __netpoll_rx(struct sk_buff *skb);
 
 
-#ifdef CONFIG_NETPOLL
-static inline int netpoll_rx(struct sk_buff *skb)
+/* Hijack incoming packet for use by netpoll.
+ * NB: may be called twice for NAPI case
+ */
+static inline bool netpoll_rx(struct sk_buff *skb)
 {
-	struct netpoll_info *npinfo = skb->dev->npinfo;
-	unsigned long flags;
-	int ret = 0;
-
-	if (!npinfo || !npinfo->rx_np)
-		return 0;
-
-	spin_lock_irqsave(&npinfo->rx_lock, flags);
-	if (__netpoll_rx(skb))
-		ret = 1;
-	spin_unlock_irqrestore(&npinfo->rx_lock, flags);
+	if (unlikely(skb->dev->npinfo))
+		return __netpoll_rx(skb);
 
-	return ret;
-}
-
-static inline int netpoll_receive_skb(struct sk_buff *skb)
-{
-	if (!list_empty(&skb->dev->napi_list))
-		return netpoll_rx(skb);
-	return 0;
+	return false;
 }
 
 static inline void *netpoll_poll_lock(struct napi_struct *napi)
--- a/net/core/dev.c	2007-11-03 11:05:33.000000000 -0700
+++ b/net/core/dev.c	2007-11-03 11:08:36.000000000 -0700
@@ -2020,8 +2020,7 @@ int netif_receive_skb(struct sk_buff *sk
 	int ret = NET_RX_DROP;
 	__be16 type;
 
-	/* if we've gotten here through NAPI, check netpoll */
-	if (netpoll_receive_skb(skb))
+	if (netpoll_rx(skb))
 		return NET_RX_DROP;
 
 	if (!skb->tstamp.tv64)
--- a/net/core/netpoll.c	2007-11-03 11:08:23.000000000 -0700
+++ b/net/core/netpoll.c	2007-11-03 11:08:36.000000000 -0700
@@ -458,16 +458,23 @@ static void arp_reply(struct sk_buff *sk
 	netpoll_send_skb(np, send_skb);
 }
 
-int __netpoll_rx(struct sk_buff *skb)
+bool __netpoll_rx(struct sk_buff *skb)
 {
 	int proto, len, ulen;
 	struct iphdr *iph;
 	struct udphdr *uh;
 	struct netpoll_info *npi = skb->dev->npinfo;
-	struct netpoll *np = npi->rx_np;
+	struct netpoll *np;
+	unsigned long flags;
+
+	if (!npi)
+		return false;
 
+	spin_lock_irqsave(&npi->rx_lock, flags);
+	np = npi->rx_np;
 	if (!np)
 		goto out;
+
 	if (skb->dev->type != ARPHRD_ETHER)
 		goto out;
 
@@ -528,20 +535,22 @@ int __netpoll_rx(struct sk_buff *skb)
 	np->rx_hook(np, ntohs(uh->source),
 		    (char *)(uh+1),
 		    ulen - sizeof(struct udphdr));
+	spin_unlock_irqrestore(&npi->rx_lock, flags);
 
 	kfree_skb(skb);
-	return 1;
+	return true;
 
 out:
+	spin_unlock_irqrestore(&npi->rx_lock, flags);
 	/* If packet received while already in poll then just
 	 * silently drop.
 	 */
 	if (atomic_read(&trapped)) {
 		kfree_skb(skb);
-		return 1;
+		return true;
 	}
 
-	return 0;
+	return false;
 }
 
 void netpoll_print_options(const char *name, struct netpoll *np)

-- 
Stephen Hemminger <shemminger@linux-foundation.org>

[PATCH 11/11] netpoll: rx use RCU

[Stephen Hemminger]

[-- Attachment #1: netpoll-rcu.patch --]
[-- Type: text/plain, Size: 3049 bytes --]

Get rid of rx_lock and use Read-Copy-Update to make sure
that netpoll info and rx handle are not used after free.

Signed-off-by: Stephen Hemminger <shemminger@linux-foundation.org>


--- a/include/linux/netpoll.h	2007-11-03 11:08:36.000000000 -0700
+++ b/include/linux/netpoll.h	2007-11-03 11:15:11.000000000 -0700
@@ -23,7 +23,6 @@ struct netpoll {
 
 struct netpoll_info {
 	atomic_t refcnt;
-	spinlock_t rx_lock;
 	struct netpoll *rx_np; /* netpoll that registered an rx_hook */
 	struct sk_buff_head arp_tx; /* list of arp requests to reply to */
 	struct sk_buff_head txq;
--- a/net/core/netpoll.c	2007-11-03 11:08:36.000000000 -0700
+++ b/net/core/netpoll.c	2007-11-03 11:15:29.000000000 -0700
@@ -463,15 +463,15 @@ bool __netpoll_rx(struct sk_buff *skb)
 	int proto, len, ulen;
 	struct iphdr *iph;
 	struct udphdr *uh;
-	struct netpoll_info *npi = skb->dev->npinfo;
+	struct netpoll_info *npi;
 	struct netpoll *np;
-	unsigned long flags;
 
+	rcu_read_lock();
+	npi = rcu_dereference(skb->dev->npinfo);
 	if (!npi)
-		return false;
+		goto out;
 
-	spin_lock_irqsave(&npi->rx_lock, flags);
-	np = npi->rx_np;
+	np = rcu_dereference(npi->rx_np);
 	if (!np)
 		goto out;
 
@@ -535,13 +535,13 @@ bool __netpoll_rx(struct sk_buff *skb)
 	np->rx_hook(np, ntohs(uh->source),
 		    (char *)(uh+1),
 		    ulen - sizeof(struct udphdr));
-	spin_unlock_irqrestore(&npi->rx_lock, flags);
+	rcu_read_unlock();
 
 	kfree_skb(skb);
 	return true;
 
 out:
-	spin_unlock_irqrestore(&npi->rx_lock, flags);
+	rcu_read_unlock();
 	/* If packet received while already in poll then just
 	 * silently drop.
 	 */
@@ -678,7 +678,6 @@ int netpoll_setup(struct netpoll *np, st
 
 		npinfo->rx_np = NULL;
 
-		spin_lock_init(&npinfo->rx_lock);
 		skb_queue_head_init(&npinfo->arp_tx);
 		skb_queue_head_init(&npinfo->txq);
 		INIT_DELAYED_WORK(&npinfo->tx_work, queue_process);
@@ -755,11 +754,8 @@ int netpoll_setup(struct netpoll *np, st
 		       HIPQUAD(np->local_ip));
 	}
 
-	if (np->rx_hook) {
-		spin_lock_irqsave(&npinfo->rx_lock, flags);
-		npinfo->rx_np = np;
-		spin_unlock_irqrestore(&npinfo->rx_lock, flags);
-	}
+	if (np->rx_hook)
+		rcu_assign_pointer(npinfo->rx_np, np);
 
 	/* fill up the skb queue */
 	refill_skbs();
@@ -794,21 +790,21 @@ void netpoll_cleanup(struct netpoll *np)
 	if (np->dev) {
 		npinfo = np->dev->npinfo;
 		if (npinfo) {
-			if (npinfo->rx_np == np) {
-				spin_lock_irqsave(&npinfo->rx_lock, flags);
-				npinfo->rx_np = NULL;
-				spin_unlock_irqrestore(&npinfo->rx_lock, flags);
-			}
+			if (npinfo->rx_np == np)
+				rcu_assign_pointer(npinfo->rx_np, NULL);
 
 			if (atomic_dec_and_test(&npinfo->refcnt)) {
+
 				skb_queue_purge(&npinfo->arp_tx);
 				skb_queue_purge(&npinfo->txq);
 				cancel_rearming_delayed_work(&npinfo->tx_work);
 
 				/* clean after last, unfinished work */
 				__skb_queue_purge(&npinfo->txq);
+
+				rcu_assign_pointer(np->dev->npinfo, NULL);
+				synchronize_net();
 				kfree(npinfo);
-				np->dev->npinfo = NULL;
 			}
 		}
 

-- 
Stephen Hemminger <shemminger@linux-foundation.org>

Re: [PATCH 01/11] netpoll: use skb_queue_purge

[David Miller]

From: Stephen Hemminger <shemminger@linux-foundation.org>
Date: Sat, 03 Nov 2007 11:43:15 -0700

> Use standard route for flushing queue.
> 
> Signed-off-by: Stephen Hemminger <shemminger@linux-foundation.org>

Applied to net-2.6.25, thanks!

Re: [PATCH 02/11] netpoll: netpoll_poll cleanup

[David Miller]

From: Stephen Hemminger <shemminger@linux-foundation.org>
Date: Sat, 03 Nov 2007 11:43:16 -0700

> Restructure code slightly to improve readability:
>   * dereference device once
>   * change obvious while() loop
>   * let poll_napi() handle null list itself
> 
> Signed-off-by: Stephen Hemminger <shemminger@linux-foundation.org>

Applied to net-2.6.25, but I made some coding style fixups, one of
which is a huge pet peeve of mine.

When declaring local variables for a function I always list the
longest lines down gradually to the shortest lines. It is nicer to the
eye and naturally it means that all the complicated structure
assignments and dereferences sit at the top and the simpler iterators
and counters like 'i' end up at the bottom making local variable lists
that much easier to read and search when learning how a function works.

You explicitly changed this one I had set up:

>  {
> -	struct netpoll_info *npinfo = np->dev->npinfo;
> -	struct napi_struct *napi;
>  	int budget = 16;
> +	struct napi_struct *napi;

And I thus reverted it back to the correct order:

	struct napi_struct *napi;
	int budget = 16;

I also got rid of the mid-parens spaces in:

> +		while ( (skb = skb_dequeue(&npi->arp_tx)) )

Thanks.

Re: [PATCH 03/11] netpoll: no need to store local_mac

[David Miller]

From: Stephen Hemminger <shemminger@linux-foundation.org>
Date: Sat, 03 Nov 2007 11:43:17 -0700

> The local_mac is managed by the network device, no need to
> keep a spare copy and all the management problems that could
> cause.
> 
> Signed-off-by: Stephen Hemminger <shemminger@linux-foundation.org>

Applied to net-2.6.25, thanks!

Re: [PATCH 04/11] netpoll: alternative implementation of dropping

[David Miller]

From: Stephen Hemminger <shemminger@linux-foundation.org>
Date: Sat, 03 Nov 2007 11:43:18 -0700

> Signed-off-by: Stephen Hemminger <shemminger@linux-foundation.org>

Applied to net-2.6.25, but I gave the changelog a more descriptive
headline:

	[NETPOLL]: Kill NETPOLL_RX_DROP, set but never tested.

Thanks.

Re: [PATCH 05/11] netpoll: dont need rx_flags

[David Miller]

From: Stephen Hemminger <shemminger@linux-foundation.org>
Date: Sat, 03 Nov 2007 11:43:19 -0700

> The rx_flags variable is redundant. Turning rx on/off is done
> via setting the rx_np pointer.
> 
> Signed-off-by: Stephen Hemminger <shemminger@linux-foundation.org>

Not all of these transformations are equivalent, and as a result
you're adding a bug.

> @@ -51,12 +50,11 @@ static inline int netpoll_rx(struct sk_b
>  	unsigned long flags;
>  	int ret = 0;
>  
> -	if (!npinfo || (!npinfo->rx_np && !npinfo->rx_flags))
> +	if (!npinfo || !npinfo->rx_np)
>  		return 0;
>  
>  	spin_lock_irqsave(&npinfo->rx_lock, flags);
> -	/* check rx_flags again with the lock held */
> -	if (npinfo->rx_flags && __netpoll_rx(skb))
> +	if (__netpoll_rx(skb))
>  		ret = 1;
>  	spin_unlock_irqrestore(&npinfo->rx_lock, flags);

If you're using ->rx_np == NULL as your new guard, you have
to duplicate that test inside of holding the lock.

->rx_np goes to NULL under the lock, but in the previous code if
the rx_flags is set we know that ->rx_np is also NULL, that's
why it was OK to only recheck ->rx_flags in the lock and not
->rx_np as well.

Therefore we have to replace the test on ->rx_flags with ->rx_np in
all spots to retain correct semantics.

I've made this correction to your patch and applied it to net-2.6.25

Thanks.

Re: [PATCH 06/11] netpoll: remove dev_name for npinfo

[David Miller]

From: Stephen Hemminger <shemminger@linux-foundation.org>
Date: Sat, 03 Nov 2007 11:43:20 -0700

> The device name was only in npinfo for netconsole target
> configuration, so move it to netconsole.  Netconsole only
> needs the value during config, so no need to do all
> the device name tracking etc.. 
> 
> Make functions for common code for instantiation and 
> start up.
> 
> Signed-off-by: Stephen Hemminger <shemminger@linux-foundation.org>

Sigh...

	return -EUNTESTED;

In netpoll_parse_options() np->dev isn't setup yet, so if you had
tested this patch you would have gotten an immediate OOPS.

That's why it needs the npinfo->dev_name in the first place, because
it has to be parsed in order to even know what device to attach to.
The np->dev usually isn't setup until netpoll_setup() is called.

I appreciate all the work you are doing trying to clean up this beast
but you have to start testing this stuff and audit the transformations
you are making, instead of always relying on me or someone else to do
it for you.

That's why I let these particular patches sit in my inbox for two
weeks, I knew half of them would have bugs and most if not all of them
were totally untested, so I dreaded reviewing them.

I might have to toss some of the rest of these netconsole things
if there are dependencies on this bogus change.

Re: [PATCH 07/11] netpoll: get rid of name parameter

[David Miller]

From: Stephen Hemminger <shemminger@linux-foundation.org>
Date: Sat, 03 Nov 2007 11:43:21 -0700

> The name was being stored and used only for error messages.
> The same effect can be had by just passing it in where needed during
> config.
> 
> Signed-off-by: Stephen Hemminger <shemminger@linux-foundation.org>

The whole world of netpoll is not netconsole even though it just
happens to be that way currently.  Otherwise we can just eliminate
the modularity completely and merge all of netconsole into
net/core/netpoll.c but of course we won't do that.

If another user of netpoll comes by we'll just have to add this thing
back again.

I'm dropping this patch.

Re: [PATCH 08/11] netpoll: NETPOLL_TRAP configuration change

[David Miller]

From: Stephen Hemminger <shemminger@linux-foundation.org>
Date: Sat, 03 Nov 2007 11:43:22 -0700

> --- a/drivers/net/Kconfig	2007-11-03 11:30:57.000000000 -0700
> +++ b/drivers/net/Kconfig	2007-11-03 11:30:59.000000000 -0700
> @@ -3118,8 +3118,8 @@ config NETCONSOLE_DYNAMIC
>  config NETPOLL
>  	def_bool NETCONSOLE
>  
> +# If needed use select to enable
>  config NETPOLL_TRAP
> -	bool "Netpoll traffic trapping"
>  	default n
>  	depends on NETPOLL

I'm not applying this.

This change means that allmodconfig doesn't pick up this symbol
anymore and therefore it's way to difficult to make sure these
code segments still get build tested.

Re: [PATCH 09/11] netpoll: ethernet devices only

[David Miller]

From: Stephen Hemminger <shemminger@linux-foundation.org>
Date: Sat, 03 Nov 2007 11:43:23 -0700

> Netpoll only works on Ethernet devices, so check during setup
> rather than just failing silently later.
> 
> Signed-off-by: Stephen Hemminger <shemminger@linux-foundation.org>
> 
> --- a/net/core/netpoll.c	2007-11-03 11:05:33.000000000 -0700
> +++ b/net/core/netpoll.c	2007-11-03 11:08:23.000000000 -0700
> @@ -653,6 +653,12 @@ int netpoll_setup(struct netpoll *np, st
>  	unsigned long flags;
>  	int err;
>  
> +	if (ndev->type != ARPHRD_ETHER) {
> +		printk(KERN_ERR "netpoll: %s is not an ethernet device\n",
> +		       ndev->name);
> +		return -EINVAL;
> +	}
> +
>  	np->dev = ndev;

This patch depends upon the broken np->dev_name one, and I also would
rather fix the limitations in netpoll.

Re: [PATCH 10/11] netpoll: rx optimization

[David Miller]

From: Stephen Hemminger <shemminger@linux-foundation.org>
Date: Sat, 03 Nov 2007 11:43:24 -0700

> This patch makes netpoll work for non-NAPI devices that call
> netif_receive_skb. Devices are allowed to call netif_receive_skb
> if they are receiving packets in softirq (ie in tasklet).
> One side effect of this is that received packets will be looked
> at twice for the non-NAPI case, but this is harmless.
> 
> Move the locking out of the inline hook and into the internal
> function.
> 
> Signed-off-by: Stephen Hemminger <shemminger@linux-foundation.org>

This one looks OK but it no longer applied, please respin.

Thanks.

Re: [PATCH 11/11] netpoll: rx use RCU

[David Miller]

From: Stephen Hemminger <shemminger@linux-foundation.org>
Date: Sat, 03 Nov 2007 11:43:25 -0700

> Get rid of rx_lock and use Read-Copy-Update to make sure
> that netpoll info and rx handle are not used after free.
> 
> Signed-off-by: Stephen Hemminger <shemminger@linux-foundation.org>

Please also respin this one too, thanks.

Re: [PATCH 06/11] netpoll: remove dev_name for npinfo

[Stephen Hemminger]

On Mon, 19 Nov 2007 19:47:50 -0800 (PST)
David Miller <davem@davemloft.net> wrote:

> From: Stephen Hemminger <shemminger@linux-foundation.org>
> Date: Sat, 03 Nov 2007 11:43:20 -0700
> 
> > The device name was only in npinfo for netconsole target
> > configuration, so move it to netconsole.  Netconsole only
> > needs the value during config, so no need to do all
> > the device name tracking etc.. 
> > 
> > Make functions for common code for instantiation and 
> > start up.
> > 
> > Signed-off-by: Stephen Hemminger <shemminger@linux-foundation.org>
> 
> Sigh...
> 
> 	return -EUNTESTED;
> 
> In netpoll_parse_options() np->dev isn't setup yet, so if you had
> tested this patch you would have gotten an immediate OOPS.

Sorry, I missed the boot up case, I was testing with modprobe
and dyanmic reconfig stuff.  Places that store the name (and then
get confused by renames) are one of the lingering mis-features of
the whole network device infrastructure. 


-- 
Stephen Hemminger <shemminger@linux-foundation.org>

Re: [PATCH 09/11] netpoll: ethernet devices only

[Stephen Hemminger]

On Mon, 19 Nov 2007 19:55:15 -0800 (PST)
David Miller <davem@davemloft.net> wrote:

> From: Stephen Hemminger <shemminger@linux-foundation.org>
> Date: Sat, 03 Nov 2007 11:43:23 -0700
> 
> > Netpoll only works on Ethernet devices, so check during setup
> > rather than just failing silently later.
> > 
> > Signed-off-by: Stephen Hemminger <shemminger@linux-foundation.org>
> > 
> > --- a/net/core/netpoll.c	2007-11-03 11:05:33.000000000 -0700
> > +++ b/net/core/netpoll.c	2007-11-03 11:08:23.000000000 -0700
> > @@ -653,6 +653,12 @@ int netpoll_setup(struct netpoll *np, st
> >  	unsigned long flags;
> >  	int err;
> >  
> > +	if (ndev->type != ARPHRD_ETHER) {
> > +		printk(KERN_ERR "netpoll: %s is not an ethernet device\n",
> > +		       ndev->name);
> > +		return -EINVAL;
> > +	}
> > +
> >  	np->dev = ndev;
> 
> This patch depends upon the broken np->dev_name one, and I also would
> rather fix the limitations in netpoll.

Existing netpoll depends on Ethernet and IPV4. Doing the right thing
might be hard because it would mean going through more inside irq.

Probably the right thing to do would be to build a header through the
normal path and cache it some how.

-- 
Stephen Hemminger <shemminger@linux-foundation.org>