[PATCH] net: remove useless prefetch() call

[PATCH] net: remove useless prefetch() call

[Eric Dumazet]

There is no point to use prefetch() call here.
start_xmit() is a function like others...

Signed-off-by: Eric Dumazet <dada1@cosmosbay.com>

diff --git a/net/core/dev.c b/net/core/dev.c
index c013031..c97e27d 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -1670,7 +1670,6 @@ int dev_hard_start_xmit(struct sk_buff *skb, struct net_device *dev,
 	const struct net_device_ops *ops = dev->netdev_ops;
 	int rc;
 
-	prefetch(&dev->netdev_ops->ndo_start_xmit);
 	if (likely(!skb->next)) {
 		if (!list_empty(&ptype_all))
 			dev_queue_xmit_nit(skb, dev);

Re: [PATCH] net: remove useless prefetch() call

[David Miller]

From: Eric Dumazet <dada1@cosmosbay.com>
Date: Fri, 20 Mar 2009 09:32:53 +0100

> There is no point to use prefetch() call here.
> start_xmit() is a function like others...
> 
> Signed-off-by: Eric Dumazet <dada1@cosmosbay.com>

Yes but the operation pointer might not be in the CPU
cache at this time?

And if it's not we can get it into the cpu whilst we do
other processing, such as the dev_queue_xmit_nit() stuff.

[NET] net: reorder struct net_device_ops

[Eric Dumazet]

David Miller a écrit :
> From: Eric Dumazet <dada1@cosmosbay.com>
> Date: Fri, 20 Mar 2009 09:32:53 +0100
> 
>> There is no point to use prefetch() call here.
>> start_xmit() is a function like others...
>>
>> Signed-off-by: Eric Dumazet <dada1@cosmosbay.com>
> 
> Yes but the operation pointer might not be in the CPU
> cache at this time?
> 
> And if it's not we can get it into the cpu whilst we do
> other processing, such as the dev_queue_xmit_nit() stuff.

This slow down fast path, but we can find a compromise.

I saw a strange effect on oprofile because of this prefetch()
on a situation we call xxx.xxx times per second dev_hard_start_xmit()
(So this ought to be in CPU cache already)

prefetch() is *free* only if the address computation is fast too :)

Thank you

[NET] net: reorder struct net_device_ops

Moving ndo_start_xmit() field at first position in
struct net_device_ops reduce the assembly needed to compute
the prefetch() address.

There seems to be an issue here on some cpus as spotted by oprofile
in dev_hard_start_xmit()
(prefetch() has a dependancy on previous add instruction)

	mov    %eax,-0x14(%ebp) /* store ops */
	add    $0x10,%eax       /* compute &ops->ndo_start_xmit */
	prefetcht0 (%eax)       /* stall here */

After patch, no add instruction is needed anymore.

Signed-off-by: Eric Dumazet <dada1@cosmosbay.com>

diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
index be3ebd7..e507c6e 100644
--- a/include/linux/netdevice.h
+++ b/include/linux/netdevice.h
@@ -547,14 +547,14 @@ struct netdev_queue {
  */
 #define HAVE_NET_DEVICE_OPS
 struct net_device_ops {
-	int			(*ndo_init)(struct net_device *dev);
-	void			(*ndo_uninit)(struct net_device *dev);
-	int			(*ndo_open)(struct net_device *dev);
-	int			(*ndo_stop)(struct net_device *dev);
 	int			(*ndo_start_xmit) (struct sk_buff *skb,
 						   struct net_device *dev);
 	u16			(*ndo_select_queue)(struct net_device *dev,
 						    struct sk_buff *skb);
+	int			(*ndo_init)(struct net_device *dev);
+	void			(*ndo_uninit)(struct net_device *dev);
+	int			(*ndo_open)(struct net_device *dev);
+	int			(*ndo_stop)(struct net_device *dev);
 #define HAVE_CHANGE_RX_FLAGS
 	void			(*ndo_change_rx_flags)(struct net_device *dev,
 						       int flags);
diff --git a/net/core/dev.c b/net/core/dev.c
index c013031..2e5ebd0 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -1670,7 +1670,7 @@ int dev_hard_start_xmit(struct sk_buff *skb, struct net_device *dev,
 	const struct net_device_ops *ops = dev->netdev_ops;
 	int rc;
 
-	prefetch(&dev->netdev_ops->ndo_start_xmit);
+	prefetch(&ops->ndo_start_xmit);
 	if (likely(!skb->next)) {
 		if (!list_empty(&ptype_all))
 			dev_queue_xmit_nit(skb, dev);

[PATCH] net: remove useless prefetch() call

[Eric Dumazet]

Eric Dumazet a écrit :
> David Miller a écrit :
>> From: Eric Dumazet <dada1@cosmosbay.com>
>> Date: Fri, 20 Mar 2009 09:32:53 +0100
>>
>>> There is no point to use prefetch() call here.
>>> start_xmit() is a function like others...
>>>
>>> Signed-off-by: Eric Dumazet <dada1@cosmosbay.com>
>> Yes but the operation pointer might not be in the CPU
>> cache at this time?
>>
>> And if it's not we can get it into the cpu whilst we do
>> other processing, such as the dev_queue_xmit_nit() stuff.
> 
> This slow down fast path, but we can find a compromise.
> 

Hmm.. it seems that ndo_select_queue is accessed right before
ndo_start_xmit - by dev_pick_tx()) - and they share same cache line,
so operation pointer is in CPU cache.

So first patch is OK, what about this updated Changelog ?

Thank you

[PATCH] net: remove useless prefetch() call

There is no gain using prefetch() in dev_hard_start_xmit(), since
we already had to read ops->ndo_select_queue pointer in dev_pick_tx(),
and both pointers are probably located in the same cache line.

This prefetch call slows down fast path because of a stall in address
computation.

diff --git a/net/core/dev.c b/net/core/dev.c
index c013031..c97e27d 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -1670,7 +1670,6 @@ int dev_hard_start_xmit(struct sk_buff *skb, struct net_device *dev,
 	const struct net_device_ops *ops = dev->netdev_ops;
 	int rc;
 
-	prefetch(&dev->netdev_ops->ndo_start_xmit);
 	if (likely(!skb->next)) {
 		if (!list_empty(&ptype_all))
 			dev_queue_xmit_nit(skb, dev);

Re: [PATCH] net: remove useless prefetch() call

[David Miller]

From: Eric Dumazet <dada1@cosmosbay.com>
Date: Fri, 20 Mar 2009 11:07:33 +0100

> So first patch is OK, what about this updated Changelog ?
> 
> [PATCH] net: remove useless prefetch() call
> 
> There is no gain using prefetch() in dev_hard_start_xmit(), since
> we already had to read ops->ndo_select_queue pointer in dev_pick_tx(),
> and both pointers are probably located in the same cache line.
> 
> This prefetch call slows down fast path because of a stall in address
> computation.

Applied, thanks.

Re: [NET] net: reorder struct net_device_ops

[Bjørn Mork]

Eric Dumazet <dada1@cosmosbay.com> writes:

> [NET] net: reorder struct net_device_ops
>
> Moving ndo_start_xmit() field at first position in
> struct net_device_ops reduce the assembly needed to compute
> the prefetch() address.

You can't do this while the compatibility code is still there.

from net/core/dev.c:register_netdevice() :

		/* This works only because net_device_ops and the
		   compatiablity structure are the same. */
		dev->netdev_ops = (void *) &(dev->init);


Bjørn

Re: [NET] net: reorder struct net_device_ops

[Eric Dumazet]

Bjørn Mork a écrit :
> Eric Dumazet <dada1@cosmosbay.com> writes:
> 
>> [NET] net: reorder struct net_device_ops
>>
>> Moving ndo_start_xmit() field at first position in
>> struct net_device_ops reduce the assembly needed to compute
>> the prefetch() address.
> 
> You can't do this while the compatibility code is still there.
> 
> from net/core/dev.c:register_netdevice() :
> 
> 		/* This works only because net_device_ops and the
> 		   compatiablity structure are the same. */
> 		dev->netdev_ops = (void *) &(dev->init);
> 
> 
> Bjørn

Nice catch !

Yes, very true, but I discarded this patch anyway, I wont correct this.

Thank you