[PATCH 0/4] network device interface cleanups

[PATCH 0/4] network device interface cleanups

[Stephen Hemminger]

This set of patches makes softnet_data local to dev.c and
does some code cleanups, no API changes.

-- 

[PATCH 1/4] net: make dev_kfree_skb_irq not inline

[Stephen Hemminger]

[-- Attachment #1: kfree_skb_any.patch --]
[-- Type: text/plain, Size: 1690 bytes --]

Move the dev_kfree_skb_irq function from netdevice.h to dev.c
for a couple of reasons. Primarily, I want to make softnet_data
local to dev.c; also this function is called 300+ places already.

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

--- linux-2.6.20-rc1.orig/include/linux/netdevice.h
+++ linux-2.6.20-rc1/include/linux/netdevice.h
@@ -676,20 +676,7 @@ static inline int netif_running(const st
 /* Use this variant when it is known for sure that it
  * is executing from interrupt context.
  */
-static inline void dev_kfree_skb_irq(struct sk_buff *skb)
-{
-	if (atomic_dec_and_test(&skb->users)) {
-		struct softnet_data *sd;
-		unsigned long flags;
-
-		local_irq_save(flags);
-		sd = &__get_cpu_var(softnet_data);
-		skb->next = sd->completion_queue;
-		sd->completion_queue = skb;
-		raise_softirq_irqoff(NET_TX_SOFTIRQ);
-		local_irq_restore(flags);
-	}
-}
+extern void dev_kfree_skb_irq(struct sk_buff *skb);
 
 /* Use this variant in places where it could be invoked
  * either from interrupt or non-interrupt context.
--- linux-2.6.20-rc1.orig/net/core/dev.c
+++ linux-2.6.20-rc1/net/core/dev.c
@@ -1141,6 +1141,21 @@ void dev_kfree_skb_any(struct sk_buff *s
 }
 EXPORT_SYMBOL(dev_kfree_skb_any);
 
+void dev_kfree_skb_irq(struct sk_buff *skb)
+{
+	if (atomic_dec_and_test(&skb->users)) {
+		struct softnet_data *sd;
+		unsigned long flags;
+
+		local_irq_save(flags);
+		sd = &__get_cpu_var(softnet_data);
+		skb->next = sd->completion_queue;
+		sd->completion_queue = skb;
+		raise_softirq_irqoff(NET_TX_SOFTIRQ);
+		local_irq_restore(flags);
+	}
+}
+EXPORT_SYMBOL(dev_kfree_skb_irq);
 
 /* Hot-plugging. */
 void netif_device_detach(struct net_device *dev)

-- 

Re: [PATCH 1/4] net: make dev_kfree_skb_irq not inline

[Christoph Hellwig]

On Thu, Dec 14, 2006 at 12:48:15PM -0800, Stephen Hemminger wrote:
> Move the dev_kfree_skb_irq function from netdevice.h to dev.c
> for a couple of reasons. Primarily, I want to make softnet_data
> local to dev.c; also this function is called 300+ places already.
> 
> Signed-off-by: Stephen Hemminger <shemminger@osdl.org>
> 
> --- linux-2.6.20-rc1.orig/include/linux/netdevice.h
> +++ linux-2.6.20-rc1/include/linux/netdevice.h
> @@ -676,20 +676,7 @@ static inline int netif_running(const st
>  /* Use this variant when it is known for sure that it
>   * is executing from interrupt context.
>   */
> -static inline void dev_kfree_skb_irq(struct sk_buff *skb)
> -{
> -	if (atomic_dec_and_test(&skb->users)) {
> -		struct softnet_data *sd;
> -		unsigned long flags;
> -
> -		local_irq_save(flags);
> -		sd = &__get_cpu_var(softnet_data);
> -		skb->next = sd->completion_queue;
> -		sd->completion_queue = skb;
> -		raise_softirq_irqoff(NET_TX_SOFTIRQ);
> -		local_irq_restore(flags);
> -	}
> -}
> +extern void dev_kfree_skb_irq(struct sk_buff *skb);

Maybe you should only move the slowpath out of line ala:

static inline void dev_kfree_skb_irq(struct sk_buff *skb)
{
	if (atomic_dec_and_test(&skb->users)) 
		__dev_kfree_skb_irq(skb);
}

Re: [PATCH 1/4] net: make dev_kfree_skb_irq not inline

[Stephen Hemminger]

On Thu, 14 Dec 2006 22:30:09 +0000
Christoph Hellwig <hch@infradead.org> wrote:

> On Thu, Dec 14, 2006 at 12:48:15PM -0800, Stephen Hemminger wrote:
> > Move the dev_kfree_skb_irq function from netdevice.h to dev.c
> > for a couple of reasons. Primarily, I want to make softnet_data
> > local to dev.c; also this function is called 300+ places already.
> > 
> > Signed-off-by: Stephen Hemminger <shemminger@osdl.org>
> > 
> > --- linux-2.6.20-rc1.orig/include/linux/netdevice.h
> > +++ linux-2.6.20-rc1/include/linux/netdevice.h
> > @@ -676,20 +676,7 @@ static inline int netif_running(const st
> >  /* Use this variant when it is known for sure that it
> >   * is executing from interrupt context.
> >   */
> > -static inline void dev_kfree_skb_irq(struct sk_buff *skb)
> > -{
> > -	if (atomic_dec_and_test(&skb->users)) {
> > -		struct softnet_data *sd;
> > -		unsigned long flags;
> > -
> > -		local_irq_save(flags);
> > -		sd = &__get_cpu_var(softnet_data);
> > -		skb->next = sd->completion_queue;
> > -		sd->completion_queue = skb;
> > -		raise_softirq_irqoff(NET_TX_SOFTIRQ);
> > -		local_irq_restore(flags);
> > -	}
> > -}
> > +extern void dev_kfree_skb_irq(struct sk_buff *skb);
> 
> Maybe you should only move the slowpath out of line ala:
> 
> static inline void dev_kfree_skb_irq(struct sk_buff *skb)
> {
> 	if (atomic_dec_and_test(&skb->users)) 
> 		__dev_kfree_skb_irq(skb);
> }
> 

We could but for routing or other cases where buffer isn't cloned
it goes through the test.

-- 
Stephen Hemminger <shemminger@osdl.org>

Re: [PATCH 1/4] net: make dev_kfree_skb_irq not inline

[David Miller]

From: Christoph Hellwig <hch@infradead.org>
Date: Thu, 14 Dec 2006 22:30:09 +0000

> Maybe you should only move the slowpath out of line ala:
> 
> static inline void dev_kfree_skb_irq(struct sk_buff *skb)
> {
> 	if (atomic_dec_and_test(&skb->users)) 
> 		__dev_kfree_skb_irq(skb);
> }

The atomic operation all by itself is either a function
call or a 6-7 instruction sequence, so the inlining doesn't
make sense even in this case.

[PATCH 2/4] net: uninline netif_rx_reschedule

[Stephen Hemminger]

[-- Attachment #1: netif_rx_reschedule.patch --]
[-- Type: text/plain, Size: 1638 bytes --]

Move netif_rx_reschedule out of line, so that softnet_data can be
made local.

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


--- linux-2.6.20-rc1.orig/include/linux/netdevice.h
+++ linux-2.6.20-rc1/include/linux/netdevice.h
@@ -851,21 +851,7 @@ static inline void netif_rx_schedule(str
 /* Try to reschedule poll. Called by dev->poll() after netif_rx_complete().
  * Do not inline this?
  */
-static inline int netif_rx_reschedule(struct net_device *dev, int undo)
-{
-	if (netif_rx_schedule_prep(dev)) {
-		unsigned long flags;
-
-		dev->quota += undo;
-
-		local_irq_save(flags);
-		list_add_tail(&dev->poll_list, &__get_cpu_var(softnet_data).poll_list);
-		__raise_softirq_irqoff(NET_RX_SOFTIRQ);
-		local_irq_restore(flags);
-		return 1;
-	}
-	return 0;
-}
+extern int netif_rx_reschedule(struct net_device *dev, int undo);
 
 /* Remove interface from poll list: it must be in the poll list
  * on current cpu. This primitive is called by dev->poll(), when
--- linux-2.6.20-rc1.orig/net/core/dev.c
+++ linux-2.6.20-rc1/net/core/dev.c
@@ -1132,6 +1132,23 @@ void __netif_rx_schedule(struct net_devi
 }
 EXPORT_SYMBOL(__netif_rx_schedule);
 
+int netif_rx_reschedule(struct net_device *dev, int undo)
+{
+	if (netif_rx_schedule_prep(dev)) {
+		unsigned long flags;
+
+		dev->quota += undo;
+
+		local_irq_save(flags);
+		list_add_tail(&dev->poll_list, &__get_cpu_var(softnet_data).poll_list);
+		__raise_softirq_irqoff(NET_RX_SOFTIRQ);
+		local_irq_restore(flags);
+		return 1;
+	}
+	return 0;
+}
+EXPORT_SYMBOL(netif_rx_reschedule);
+
 void dev_kfree_skb_any(struct sk_buff *skb)
 {
 	if (in_irq() || irqs_disabled())

-- 

[PATCH 3/4] net: move softnet_data

[Stephen Hemminger]

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

Make softnet_data local to dev.c.  

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


--- linux-2.6.20-rc1.orig/include/linux/netdevice.h
+++ linux-2.6.20-rc1/include/linux/netdevice.h
@@ -600,6 +600,9 @@ extern int		dev_restart(struct net_devic
 #ifdef CONFIG_NETPOLL_TRAP
 extern int		netpoll_trap(void);
 #endif
+#ifdef CONFIG_NETPOLL
+extern void		netpoll_do_completion(void);
+#endif
 
 typedef int gifconf_func_t(struct net_device * dev, char __user * bufptr, int len);
 extern int		register_gifconf(unsigned int family, gifconf_func_t * gifconf);
@@ -608,26 +611,6 @@ static inline int unregister_gifconf(uns
 	return register_gifconf(family, NULL);
 }
 
-/*
- * Incoming packets are placed on per-cpu queues so that
- * no locking is needed.
- */
-
-struct softnet_data
-{
-	struct net_device	*output_queue;
-	struct sk_buff_head	input_pkt_queue;
-	struct list_head	poll_list;
-	struct sk_buff		*completion_queue;
-
-	struct net_device	backlog_dev;	/* Sorry. 8) */
-#ifdef CONFIG_NET_DMA
-	struct dma_chan		*net_dma;
-#endif
-};
-
-DECLARE_PER_CPU(struct softnet_data,softnet_data);
-
 #define HAVE_NETIF_QUEUE
 
 extern void __netif_schedule(struct net_device *dev);
--- linux-2.6.20-rc1.orig/net/core/dev.c
+++ linux-2.6.20-rc1/net/core/dev.c
@@ -203,10 +203,23 @@ static inline struct hlist_head *dev_ind
 static RAW_NOTIFIER_HEAD(netdev_chain);
 
 /*
- *	Device drivers call our routines to queue packets here. We empty the
- *	queue in the local softnet handler.
+ * Incoming packets are placed on per-cpu queues so that
+ * no locking is needed.
  */
-DEFINE_PER_CPU(struct softnet_data, softnet_data) = { NULL };
+struct softnet_data
+{
+	struct net_device	*output_queue;
+	struct sk_buff_head	input_pkt_queue;
+	struct list_head	poll_list;
+	struct sk_buff		*completion_queue;
+
+	struct net_device	backlog_dev;	/* Sorry. 8) */
+#ifdef CONFIG_NET_DMA
+	struct dma_chan		*net_dma;
+#endif
+};
+
+static DEFINE_PER_CPU(struct softnet_data, softnet_data);
 
 #ifdef CONFIG_SYSFS
 extern int netdev_sysfs_init(void);
@@ -1673,6 +1686,34 @@ static inline struct net_device *skb_bon
 	return dev;
 }
 
+#ifdef CONFIG_NETPOLL
+void netpoll_do_completion(void)
+{
+	unsigned long flags;
+	struct softnet_data *sd = &get_cpu_var(softnet_data);
+
+	if (sd->completion_queue) {
+		struct sk_buff *clist;
+
+		local_irq_save(flags);
+		clist = sd->completion_queue;
+		sd->completion_queue = NULL;
+		local_irq_restore(flags);
+
+		while (clist != NULL) {
+			struct sk_buff *skb = clist;
+			clist = clist->next;
+			if (skb->destructor)
+				dev_kfree_skb_any(skb); /* put this one back */
+			else
+				__kfree_skb(skb);
+		}
+	}
+
+	put_cpu_var(softnet_data);
+}
+#endif
+
 static void net_tx_action(struct softirq_action *h)
 {
 	struct softnet_data *sd = &__get_cpu_var(softnet_data);
--- linux-2.6.20-rc1.orig/net/core/netpoll.c
+++ linux-2.6.20-rc1/net/core/netpoll.c
@@ -47,7 +47,6 @@ static atomic_t trapped;
 		(MAX_UDP_CHUNK + sizeof(struct udphdr) + \
 				sizeof(struct iphdr) + sizeof(struct ethhdr))
 
-static void zap_completion_queue(void);
 static void arp_reply(struct sk_buff *skb);
 
 static void queue_process(struct work_struct *work)
@@ -162,7 +161,7 @@ void netpoll_poll(struct netpoll *np)
 
 	service_arp_queue(np->dev->npinfo);
 
-	zap_completion_queue();
+	netpoll_do_completion();
 }
 
 static void refill_skbs(void)
@@ -181,7 +180,7 @@ static void refill_skbs(void)
 	spin_unlock_irqrestore(&skb_pool.lock, flags);
 }
 
-static void zap_completion_queue(void)
+static void netpoll_do_completion(void)
 {
 	unsigned long flags;
 	struct softnet_data *sd = &get_cpu_var(softnet_data);
@@ -212,7 +211,7 @@ static struct sk_buff *find_skb(struct n
 	int count = 0;
 	struct sk_buff *skb;
 
-	zap_completion_queue();
+	netpoll_do_completion();
 	refill_skbs();
 repeat:
 

-- 

Re: [PATCH 3/4] net: move softnet_data

[David Miller]

From: Stephen Hemminger <shemminger@osdl.org>
Date: Thu, 14 Dec 2006 12:48:17 -0800

> Make softnet_data local to dev.c.  
> 
> Signed-off-by: Stephen Hemminger <shemminger@osdl.org>

Slight mistake here, I think:

> +#ifdef CONFIG_NETPOLL
> +extern void		netpoll_do_completion(void);
> +#endif

Ok.

> --- linux-2.6.20-rc1.orig/net/core/dev.c
> +++ linux-2.6.20-rc1/net/core/dev.c
 ...
> @@ -1673,6 +1686,34 @@ static inline struct net_device *skb_bon
>  	return dev;
>  }
>  
> +#ifdef CONFIG_NETPOLL
> +void netpoll_do_completion(void)
> +{

Ok.

> --- linux-2.6.20-rc1.orig/net/core/netpoll.c
> +++ linux-2.6.20-rc1/net/core/netpoll.c
  ...
> @@ -181,7 +180,7 @@ static void refill_skbs(void)
>  	spin_unlock_irqrestore(&skb_pool.lock, flags);
>  }
>  
> -static void zap_completion_queue(void)
> +static void netpoll_do_completion(void)
>  {
>  	unsigned long flags;
>  	struct softnet_data *sd = &get_cpu_var(softnet_data);
> @@ -212,7 +211,7 @@ static struct sk_buff *find_skb(struct n
>  	int count = 0;
>  	struct sk_buff *skb;
>  
> -	zap_completion_queue();
> +	netpoll_do_completion();
>  	refill_skbs();
>  repeat:

-ENOTESTED, I think this makes netpoll_do_completion() call itself,
ie. the local static version here, not the one you are exporting from
net/core/dev.c

Because of the extern you added, I can't see how the compiler
didn't complain loudly about this mismatch.

I had to see this, so I compiled it myself, and it doesn't even
build:

  CC      net/core/netpoll.o
net/core/netpoll.c:184: error: static declaration of ^[$,1rx^[(Bnetpoll_do_completion^[$,1ry^[(B follows non-static declaration
include/linux/netdevice.h:604: error: previous declaration of ^[$,1rx^[(Bnetpoll_do_completion^[$,1ry^[(B was here
net/core/netpoll.c: In function ^[$,1rx^[(Bnetpoll_do_completion^[$,1ry^[(B:
net/core/netpoll.c:186: error: ^[$,1rx^[(Bper_cpu__softnet_data^[$,1ry^[(B undeclared (first use in this function)
net/core/netpoll.c:186: error: (Each undeclared identifier is reported only once
net/core/netpoll.c:186: error: for each function it appears in.)
net/core/netpoll.c:188: error: dereferencing pointer to incomplete type
net/core/netpoll.c:192: error: dereferencing pointer to incomplete type
net/core/netpoll.c:193: error: dereferencing pointer to incomplete type

Stephen... come on :-)

[PATCH 4/4] net: rearrange functions in netdevice.h

[Stephen Hemminger]

[-- Attachment #1: netdevice-reorg.patch --]
[-- Type: text/plain, Size: 2256 bytes --]

Use existing inline functions rather than having multiple
copies of same code.

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


--- linux-2.6.20-rc1.orig/include/linux/netdevice.h
+++ linux-2.6.20-rc1/include/linux/netdevice.h
@@ -615,9 +615,14 @@ static inline int unregister_gifconf(uns
 
 extern void __netif_schedule(struct net_device *dev);
 
+static inline int netif_queue_stopped(const struct net_device *dev)
+{
+	return test_bit(__LINK_STATE_XOFF, &dev->state);
+}
+
 static inline void netif_schedule(struct net_device *dev)
 {
-	if (!test_bit(__LINK_STATE_XOFF, &dev->state))
+	if (!netif_queue_stopped(dev))
 		__netif_schedule(dev);
 }
 
@@ -645,11 +650,6 @@ static inline void netif_stop_queue(stru
 	set_bit(__LINK_STATE_XOFF, &dev->state);
 }
 
-static inline int netif_queue_stopped(const struct net_device *dev)
-{
-	return test_bit(__LINK_STATE_XOFF, &dev->state);
-}
-
 static inline int netif_running(const struct net_device *dev)
 {
 	return test_bit(__LINK_STATE_START, &dev->state);
@@ -841,15 +841,20 @@ extern int netif_rx_reschedule(struct ne
  * it completes the work. The device cannot be out of poll list at this
  * moment, it is BUG().
  */
-static inline void netif_rx_complete(struct net_device *dev)
+static inline void __netif_rx_complete(struct net_device *dev)
 {
-	unsigned long flags;
-
-	local_irq_save(flags);
 	BUG_ON(!test_bit(__LINK_STATE_RX_SCHED, &dev->state));
 	list_del(&dev->poll_list);
 	smp_mb__before_clear_bit();
 	clear_bit(__LINK_STATE_RX_SCHED, &dev->state);
+}
+
+static inline void netif_rx_complete(struct net_device *dev)
+{
+	unsigned long flags;
+
+	local_irq_save(flags);
+	__netif_rx_complete(dev);
 	local_irq_restore(flags);
 }
 
@@ -865,17 +870,6 @@ static inline void netif_poll_enable(str
 	clear_bit(__LINK_STATE_RX_SCHED, &dev->state);
 }
 
-/* same as netif_rx_complete, except that local_irq_save(flags)
- * has already been issued
- */
-static inline void __netif_rx_complete(struct net_device *dev)
-{
-	BUG_ON(!test_bit(__LINK_STATE_RX_SCHED, &dev->state));
-	list_del(&dev->poll_list);
-	smp_mb__before_clear_bit();
-	clear_bit(__LINK_STATE_RX_SCHED, &dev->state);
-}
-
 static inline void netif_tx_lock(struct net_device *dev)
 {
 	spin_lock(&dev->_xmit_lock);

--