More prefetch fall-out

More prefetch fall-out

[Linus Torvalds]

So it turns out that the architectures that don't define their
prefetch() macros in <asm/processor.h> (and instead depend on
linux/prefetch.h to give them the default ones) are currently broken.

Fine, that's really trivial to fix. And, in fact, I'm now testing on
x86 by having x86 put its prefetch infrastructure in <asm/prefetch.h>
and thus making sure that you *have* to include <linux/prefetch.h> to
get it (rather than get it accidentally just because everything ends
up including <asm/processor.h>).

However, one of the breakages is <linux/skbuff.h> that does its own
list-walking functions ("skb_queue_walk*"), and they do prefetching.
So now I have the option to either just add <linux/prefetch.h> to that
file, or remove the prefetches. And you're the one that said that the
prefetches in the networking code were annoying.

So should the skb queues use prefetching? I have a hard time judging.
Are those lists usually long? Is the loop usually large enough that
there is any point to prefetching the next entry?

                         Linus

Re: More prefetch fall-out

[Eric Dumazet]

Le dimanche 22 mai 2011 à 10:49 -0700, Linus Torvalds a écrit :

> However, one of the breakages is <linux/skbuff.h> that does its own
> list-walking functions ("skb_queue_walk*"), and they do prefetching.
> So now I have the option to either just add <linux/prefetch.h> to that
> file, or remove the prefetches. And you're the one that said that the
> prefetches in the networking code were annoying.
> 
> So should the skb queues use prefetching? I have a hard time judging.
> Are those lists usually long? Is the loop usually large enough that
> there is any point to prefetching the next entry?
> 

Taking a look at various skb_queue_walk* call sites, I think prefetches
are not a clear win, I would just remove them all from skbuff.h

BTW, the skb_queue_walk_safe(), skb_queue_walk_from_safe(),
skb_queue_reverse_walk_safe, skb_queue_reverse_walk_from_safe() dont
have prefetch() calls.

Re: More prefetch fall-out

[Linus Torvalds]

On Sun, May 22, 2011 at 11:23 AM, Eric Dumazet <eric.dumazet@gmail.com> wrote:
>
> Taking a look at various skb_queue_walk* call sites, I think prefetches
> are not a clear win, I would just remove them all from skbuff.h

Hmm.

I did that, and it turns out that we also have:

 - *lots* of network drivers that use "prefetch" (without including prefetch.h)
 - net/netlabel/netlabel_addrlist.h doing the same.

and fixing that up is annoying "grunt work".

So for now, I think I'll just do the "add the <linux/prefetch.h> to
<linux/skbuff.h>" thing, and leave networking alone. That makes
everything compile ("allyesconfig") even with my "you *have* to
include <linux/prefetch.h>" test-patch.

                                 Linus

Re: More prefetch fall-out

[Eric Dumazet]

Le dimanche 22 mai 2011 à 11:38 -0700, Linus Torvalds a écrit :
> On Sun, May 22, 2011 at 11:23 AM, Eric Dumazet <eric.dumazet@gmail.com> wrote:
> >
> > Taking a look at various skb_queue_walk* call sites, I think prefetches
> > are not a clear win, I would just remove them all from skbuff.h
> 
> Hmm.
> 
> I did that, and it turns out that we also have:
> 
>  - *lots* of network drivers that use "prefetch" (without including prefetch.h)
>  - net/netlabel/netlabel_addrlist.h doing the same.
> 
> and fixing that up is annoying "grunt work".
> 
> So for now, I think I'll just do the "add the <linux/prefetch.h> to
> <linux/skbuff.h>" thing, and leave networking alone. That makes
> everything compile ("allyesconfig") even with my "you *have* to
> include <linux/prefetch.h>" test-patch.
> 

Or include <linux/prefetch.h> from linux/skbuff.h but remove the
prefetch() calls...

BTW this reminds me Andi Kleen prior patch

http://linux.derkeiler.com/Mailing-Lists/Kernel/2010-09/msg03096.html

Re: More prefetch fall-out

[Paul Gortmaker]

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

On Sun, May 22, 2011 at 2:38 PM, Linus Torvalds
<torvalds@linux-foundation.org> wrote:
> On Sun, May 22, 2011 at 11:23 AM, Eric Dumazet <eric.dumazet@gmail.com> wrote:
>>
>> Taking a look at various skb_queue_walk* call sites, I think prefetches
>> are not a clear win, I would just remove them all from skbuff.h
>
> Hmm.
>
> I did that, and it turns out that we also have:
>
>  - *lots* of network drivers that use "prefetch" (without including prefetch.h)
>  - net/netlabel/netlabel_addrlist.h doing the same.
>
> and fixing that up is annoying "grunt work".

I just did a cheezy script (embedded in the attached commit) that should
help make the grunt work less annoying by doing a mechanical 1st pass
on fixing the drivers/net ones.   Script is tested -- compile isn't (I'm not
currently somewhere that I can do a yesconfig/compile without growing
old waiting for it).  Feel free to use the script and scrap the commit, or
whatever makes things easiest for people.

Paul.

>
> So for now, I think I'll just do the "add the <linux/prefetch.h> to
> <linux/skbuff.h>" thing, and leave networking alone. That makes
> everything compile ("allyesconfig") even with my "you *have* to
> include <linux/prefetch.h>" test-patch.
>
>                                 Linus

[-- Attachment #2: 0001-drivers-net-add-prefetch-header-for-prefetch-users.patch --]
[-- Type: text/x-patch, Size: 12121 bytes --]

From e7e542046f621ecc32e4e4f3c3cd3082571bf38f Mon Sep 17 00:00:00 2001
From: Paul Gortmaker <paul.gortmaker@windriver.com>
Date: Sun, 22 May 2011 16:47:17 -0400
Subject: [PATCH] drivers/net: add prefetch header for prefetch users

After discovering that wide use of prefetch on modern CPUs
could be a net loss instead of a win, net drivers which were
relying on the implicit inclusion of prefetch.h via the list
headers showed up in the resulting cleanup fallout.  Give
them an explicit include via the following $0.02 script.

 =========================================
 #!/bin/bash
 MANUAL=""
 for i in `git grep -l 'prefetch(.*)' .` ; do
 	grep -q '<linux/prefetch.h>' $i
 	if [ $? = 0 ] ; then
 		continue
 	fi

 	(	echo '?^#include <linux/?a'
 		echo '#include <linux/prefetch.h>'
 		echo .
 		echo w
 		echo q
 	) | ed -s $i > /dev/null 2>&1
 	if [ $? != 0 ]; then
 		echo $i needs manual fixup
 		MANUAL="$i $MANUAL"
 	fi
 done
 echo ------------------- 8\<----------------------
 echo vi $MANUAL
 =========================================

Signed-off-by: Paul <paul.gortmaker@windriver.com>

diff --git a/drivers/net/benet/be_main.c b/drivers/net/benet/be_main.c
index 4b5e0ed..a485f7f 100644
--- a/drivers/net/benet/be_main.c
+++ b/drivers/net/benet/be_main.c
@@ -15,6 +15,7 @@
  * Costa Mesa, CA 92626
  */
 
+#include <linux/prefetch.h>
 #include "be.h"
 #include "be_cmds.h"
 #include <asm/div64.h>
diff --git a/drivers/net/bna/bnad.c b/drivers/net/bna/bnad.c
index e588511..7d25a97 100644
--- a/drivers/net/bna/bnad.c
+++ b/drivers/net/bna/bnad.c
@@ -23,6 +23,7 @@
 #include <linux/if_vlan.h>
 #include <linux/if_ether.h>
 #include <linux/ip.h>
+#include <linux/prefetch.h>
 
 #include "bnad.h"
 #include "bna.h"
diff --git a/drivers/net/bnx2x/bnx2x_cmn.c b/drivers/net/bnx2x/bnx2x_cmn.c
index ca2bbc0..64d01e7 100644
--- a/drivers/net/bnx2x/bnx2x_cmn.c
+++ b/drivers/net/bnx2x/bnx2x_cmn.c
@@ -21,6 +21,7 @@
 #include <net/ipv6.h>
 #include <net/ip6_checksum.h>
 #include <linux/firmware.h>
+#include <linux/prefetch.h>
 #include "bnx2x_cmn.h"
 
 #include "bnx2x_init.h"
diff --git a/drivers/net/chelsio/sge.c b/drivers/net/chelsio/sge.c
index b948ea7..58380d2 100644
--- a/drivers/net/chelsio/sge.c
+++ b/drivers/net/chelsio/sge.c
@@ -54,6 +54,7 @@
 #include <linux/in.h>
 #include <linux/if_arp.h>
 #include <linux/slab.h>
+#include <linux/prefetch.h>
 
 #include "cpl5_cmd.h"
 #include "sge.h"
diff --git a/drivers/net/cnic.c b/drivers/net/cnic.c
index cde59b4..11a92af 100644
--- a/drivers/net/cnic.c
+++ b/drivers/net/cnic.c
@@ -27,6 +27,7 @@
 #include <linux/delay.h>
 #include <linux/ethtool.h>
 #include <linux/if_vlan.h>
+#include <linux/prefetch.h>
 #if defined(CONFIG_VLAN_8021Q) || defined(CONFIG_VLAN_8021Q_MODULE)
 #define BCM_VLAN 1
 #endif
diff --git a/drivers/net/cxgb3/sge.c b/drivers/net/cxgb3/sge.c
index cba1401..3f562ba 100644
--- a/drivers/net/cxgb3/sge.c
+++ b/drivers/net/cxgb3/sge.c
@@ -37,6 +37,7 @@
 #include <linux/tcp.h>
 #include <linux/dma-mapping.h>
 #include <linux/slab.h>
+#include <linux/prefetch.h>
 #include <net/arp.h>
 #include "common.h"
 #include "regs.h"
diff --git a/drivers/net/cxgb4/sge.c b/drivers/net/cxgb4/sge.c
index 75a4b0f..56adf44 100644
--- a/drivers/net/cxgb4/sge.c
+++ b/drivers/net/cxgb4/sge.c
@@ -39,6 +39,7 @@
 #include <linux/ip.h>
 #include <linux/dma-mapping.h>
 #include <linux/jiffies.h>
+#include <linux/prefetch.h>
 #include <net/ipv6.h>
 #include <net/tcp.h>
 #include "cxgb4.h"
diff --git a/drivers/net/cxgb4vf/sge.c b/drivers/net/cxgb4vf/sge.c
index 5182960..5fd75fd 100644
--- a/drivers/net/cxgb4vf/sge.c
+++ b/drivers/net/cxgb4vf/sge.c
@@ -41,6 +41,7 @@
 #include <net/ipv6.h>
 #include <net/tcp.h>
 #include <linux/dma-mapping.h>
+#include <linux/prefetch.h>
 
 #include "t4vf_common.h"
 #include "t4vf_defs.h"
diff --git a/drivers/net/e1000/e1000_main.c b/drivers/net/e1000/e1000_main.c
index c18cb8e..76e8af0 100644
--- a/drivers/net/e1000/e1000_main.c
+++ b/drivers/net/e1000/e1000_main.c
@@ -29,6 +29,7 @@
 #include "e1000.h"
 #include <net/ip6_checksum.h>
 #include <linux/io.h>
+#include <linux/prefetch.h>
 
 /* Intel Media SOC GbE MDIO physical base address */
 static unsigned long ce4100_gbe_mdio_base_phy;
diff --git a/drivers/net/e1000e/netdev.c b/drivers/net/e1000e/netdev.c
index 0939040..d960056 100644
--- a/drivers/net/e1000e/netdev.c
+++ b/drivers/net/e1000e/netdev.c
@@ -49,6 +49,7 @@
 #include <linux/pm_qos_params.h>
 #include <linux/pm_runtime.h>
 #include <linux/aer.h>
+#include <linux/prefetch.h>
 
 #include "e1000.h"
 
diff --git a/drivers/net/ehea/ehea_qmr.h b/drivers/net/ehea/ehea_qmr.h
index 3810473..fddff8e 100644
--- a/drivers/net/ehea/ehea_qmr.h
+++ b/drivers/net/ehea/ehea_qmr.h
@@ -29,6 +29,7 @@
 #ifndef __EHEA_QMR_H__
 #define __EHEA_QMR_H__
 
+#include <linux/prefetch.h>
 #include "ehea.h"
 #include "ehea_hw.h"
 
diff --git a/drivers/net/enic/enic_main.c b/drivers/net/enic/enic_main.c
index 3d99b0f..2f433fb 100644
--- a/drivers/net/enic/enic_main.c
+++ b/drivers/net/enic/enic_main.c
@@ -35,6 +35,7 @@
 #include <linux/ipv6.h>
 #include <linux/tcp.h>
 #include <linux/rtnetlink.h>
+#include <linux/prefetch.h>
 #include <net/ip6_checksum.h>
 
 #include "cq_enet_desc.h"
diff --git a/drivers/net/forcedeth.c b/drivers/net/forcedeth.c
index d09e8b0..537b695 100644
--- a/drivers/net/forcedeth.c
+++ b/drivers/net/forcedeth.c
@@ -64,6 +64,7 @@
 #include <linux/dma-mapping.h>
 #include <linux/slab.h>
 #include <linux/uaccess.h>
+#include <linux/prefetch.h>
 #include  <linux/io.h>
 
 #include <asm/irq.h>
diff --git a/drivers/net/igb/igb_main.c b/drivers/net/igb/igb_main.c
index ce7838e..0ae9352 100644
--- a/drivers/net/igb/igb_main.c
+++ b/drivers/net/igb/igb_main.c
@@ -47,6 +47,7 @@
 #include <linux/aer.h>
 #ifdef CONFIG_IGB_DCA
 #include <linux/dca.h>
+#include <linux/prefetch.h>
 #endif
 #include "igb.h"
 
diff --git a/drivers/net/igbvf/netdev.c b/drivers/net/igbvf/netdev.c
index 1d04ca6..1c77fb3 100644
--- a/drivers/net/igbvf/netdev.c
+++ b/drivers/net/igbvf/netdev.c
@@ -41,6 +41,7 @@
 #include <linux/mii.h>
 #include <linux/ethtool.h>
 #include <linux/if_vlan.h>
+#include <linux/prefetch.h>
 
 #include "igbvf.h"
 
diff --git a/drivers/net/ixgb/ixgb_main.c b/drivers/net/ixgb/ixgb_main.c
index 0f681ac..6a130eb 100644
--- a/drivers/net/ixgb/ixgb_main.c
+++ b/drivers/net/ixgb/ixgb_main.c
@@ -28,6 +28,7 @@
 
 #define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
 
+#include <linux/prefetch.h>
 #include "ixgb.h"
 
 char ixgb_driver_name[] = "ixgb";
diff --git a/drivers/net/ixgbe/ixgbe_main.c b/drivers/net/ixgbe/ixgbe_main.c
index fa01b0b..08e8e25 100644
--- a/drivers/net/ixgbe/ixgbe_main.c
+++ b/drivers/net/ixgbe/ixgbe_main.c
@@ -41,6 +41,7 @@
 #include <net/ip6_checksum.h>
 #include <linux/ethtool.h>
 #include <linux/if_vlan.h>
+#include <linux/prefetch.h>
 #include <scsi/fc/fc_fcoe.h>
 
 #include "ixgbe.h"
diff --git a/drivers/net/ixgbevf/ixgbevf_main.c b/drivers/net/ixgbevf/ixgbevf_main.c
index d7ab202..28d3cb2 100644
--- a/drivers/net/ixgbevf/ixgbevf_main.c
+++ b/drivers/net/ixgbevf/ixgbevf_main.c
@@ -44,6 +44,7 @@
 #include <net/ip6_checksum.h>
 #include <linux/ethtool.h>
 #include <linux/if_vlan.h>
+#include <linux/prefetch.h>
 
 #include "ixgbevf.h"
 
diff --git a/drivers/net/myri10ge/myri10ge.c b/drivers/net/myri10ge/myri10ge.c
index b1358f7..bf84849 100644
--- a/drivers/net/myri10ge/myri10ge.c
+++ b/drivers/net/myri10ge/myri10ge.c
@@ -65,6 +65,7 @@
 #include <linux/io.h>
 #include <linux/log2.h>
 #include <linux/slab.h>
+#include <linux/prefetch.h>
 #include <net/checksum.h>
 #include <net/ip.h>
 #include <net/tcp.h>
diff --git a/drivers/net/pasemi_mac.c b/drivers/net/pasemi_mac.c
index 828e97c..9ec112c 100644
--- a/drivers/net/pasemi_mac.c
+++ b/drivers/net/pasemi_mac.c
@@ -35,6 +35,7 @@
 #include <linux/tcp.h>
 #include <net/checksum.h>
 #include <linux/inet_lro.h>
+#include <linux/prefetch.h>
 
 #include <asm/irq.h>
 #include <asm/firmware.h>
diff --git a/drivers/net/pch_gbe/pch_gbe_main.c b/drivers/net/pch_gbe/pch_gbe_main.c
index c2476fd..eac3c5c 100644
--- a/drivers/net/pch_gbe/pch_gbe_main.c
+++ b/drivers/net/pch_gbe/pch_gbe_main.c
@@ -20,6 +20,7 @@
 
 #include "pch_gbe.h"
 #include "pch_gbe_api.h"
+#include <linux/prefetch.h>
 
 #define DRV_VERSION     "1.00"
 const char pch_driver_version[] = DRV_VERSION;
diff --git a/drivers/net/qla3xxx.c b/drivers/net/qla3xxx.c
index d495a68..771bb61 100644
--- a/drivers/net/qla3xxx.c
+++ b/drivers/net/qla3xxx.c
@@ -35,6 +35,7 @@
 #include <linux/if_vlan.h>
 #include <linux/delay.h>
 #include <linux/mm.h>
+#include <linux/prefetch.h>
 
 #include "qla3xxx.h"
 
diff --git a/drivers/net/qlge/qlge_main.c b/drivers/net/qlge/qlge_main.c
index 6c9d124..930ae45 100644
--- a/drivers/net/qlge/qlge_main.c
+++ b/drivers/net/qlge/qlge_main.c
@@ -38,6 +38,7 @@
 #include <linux/delay.h>
 #include <linux/mm.h>
 #include <linux/vmalloc.h>
+#include <linux/prefetch.h>
 #include <net/ip6_checksum.h>
 
 #include "qlge.h"
diff --git a/drivers/net/r8169.c b/drivers/net/r8169.c
index 04f4e60..ef1ce2e 100644
--- a/drivers/net/r8169.c
+++ b/drivers/net/r8169.c
@@ -26,6 +26,7 @@
 #include <linux/pm_runtime.h>
 #include <linux/firmware.h>
 #include <linux/pci-aspm.h>
+#include <linux/prefetch.h>
 
 #include <asm/system.h>
 #include <asm/io.h>
diff --git a/drivers/net/s2io.c b/drivers/net/s2io.c
index 89cfee7..a9a5f5e 100644
--- a/drivers/net/s2io.c
+++ b/drivers/net/s2io.c
@@ -78,6 +78,7 @@
 #include <linux/uaccess.h>
 #include <linux/io.h>
 #include <linux/slab.h>
+#include <linux/prefetch.h>
 #include <net/tcp.h>
 
 #include <asm/system.h>
diff --git a/drivers/net/sb1250-mac.c b/drivers/net/sb1250-mac.c
index d96d2f7..68d5042 100644
--- a/drivers/net/sb1250-mac.c
+++ b/drivers/net/sb1250-mac.c
@@ -43,6 +43,7 @@
 #include <linux/mii.h>
 #include <linux/phy.h>
 #include <linux/platform_device.h>
+#include <linux/prefetch.h>
 
 #include <asm/cache.h>
 #include <asm/io.h>
diff --git a/drivers/net/sfc/rx.c b/drivers/net/sfc/rx.c
index b7dc891..62e4364 100644
--- a/drivers/net/sfc/rx.c
+++ b/drivers/net/sfc/rx.c
@@ -14,6 +14,7 @@
 #include <linux/ip.h>
 #include <linux/tcp.h>
 #include <linux/udp.h>
+#include <linux/prefetch.h>
 #include <net/ip.h>
 #include <net/checksum.h>
 #include "net_driver.h"
diff --git a/drivers/net/skge.c b/drivers/net/skge.c
index 52a48cb..f4be5c7 100644
--- a/drivers/net/skge.c
+++ b/drivers/net/skge.c
@@ -44,6 +44,7 @@
 #include <linux/mii.h>
 #include <linux/slab.h>
 #include <linux/dmi.h>
+#include <linux/prefetch.h>
 #include <asm/irq.h>
 
 #include "skge.h"
diff --git a/drivers/net/stmmac/stmmac_main.c b/drivers/net/stmmac/stmmac_main.c
index e15c4a0..e25e44a 100644
--- a/drivers/net/stmmac/stmmac_main.c
+++ b/drivers/net/stmmac/stmmac_main.c
@@ -45,6 +45,7 @@
 #include <linux/if_vlan.h>
 #include <linux/dma-mapping.h>
 #include <linux/slab.h>
+#include <linux/prefetch.h>
 #include "stmmac.h"
 
 #define STMMAC_RESOURCE_NAME	"stmmaceth"
diff --git a/drivers/net/tc35815.c b/drivers/net/tc35815.c
index 7ca51ce..4a55a16 100644
--- a/drivers/net/tc35815.c
+++ b/drivers/net/tc35815.c
@@ -47,6 +47,7 @@ static const char *version = "tc35815.c:v" DRV_VERSION "\n";
 #include <linux/phy.h>
 #include <linux/workqueue.h>
 #include <linux/platform_device.h>
+#include <linux/prefetch.h>
 #include <asm/io.h>
 #include <asm/byteorder.h>
 
diff --git a/drivers/net/vxge/vxge-main.c b/drivers/net/vxge/vxge-main.c
index fc837cf..8ab870a 100644
--- a/drivers/net/vxge/vxge-main.c
+++ b/drivers/net/vxge/vxge-main.c
@@ -52,6 +52,7 @@
 #include <linux/etherdevice.h>
 #include <linux/firmware.h>
 #include <linux/net_tstamp.h>
+#include <linux/prefetch.h>
 #include "vxge-main.h"
 #include "vxge-reg.h"
 
diff --git a/drivers/net/vxge/vxge-traffic.c b/drivers/net/vxge/vxge-traffic.c
index 2638b8d..f935170 100644
--- a/drivers/net/vxge/vxge-traffic.c
+++ b/drivers/net/vxge/vxge-traffic.c
@@ -12,6 +12,7 @@
  * Copyright(c) 2002-2010 Exar Corp.
  ******************************************************************************/
 #include <linux/etherdevice.h>
+#include <linux/prefetch.h>
 
 #include "vxge-traffic.h"
 #include "vxge-config.h"
-- 
1.7.4.5

Re: More prefetch fall-out

[David Miller]

From: Paul Gortmaker <paul.gortmaker@gmail.com>
Date: Sun, 22 May 2011 17:02:08 -0400

> I just did a cheezy script (embedded in the attached commit) that should
> help make the grunt work less annoying by doing a mechanical 1st pass
> on fixing the drivers/net ones.   Script is tested -- compile isn't (I'm not
> currently somewhere that I can do a yesconfig/compile without growing
> old waiting for it).  Feel free to use the script and scrap the commit, or
> whatever makes things easiest for people.

Looks good, thanks Paul.

I'll try to get it so we can remove the linux/prefetch.h thing
from skbuff.h

Re: More prefetch fall-out

[David Miller]

From: Linus Torvalds <torvalds@linux-foundation.org>
Date: Sun, 22 May 2011 10:49:11 -0700

> However, one of the breakages is <linux/skbuff.h> that does its own
> list-walking functions ("skb_queue_walk*"), and they do prefetching.
> So now I have the option to either just add <linux/prefetch.h> to that
> file, or remove the prefetches. And you're the one that said that the
> prefetches in the networking code were annoying.
> 
> So should the skb queues use prefetching? I have a hard time judging.
> Are those lists usually long? Is the loop usually large enough that
> there is any point to prefetching the next entry?

Please just remove the prefetches.

Those are modelled after list.h as I intend to eventually convert
SKB list handling to "struct list_head" but we're not there yet.

Therefore if we kill prefetches from list.h we should kill it from
these things in skbuff.h too.

Thanks!