netdev.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v3] net: Allow no-cache copy from user on transmit
@ 2011-03-31  5:04 Tom Herbert
  2011-03-31  8:52 ` Michał Mirosław
  2011-04-02  3:49 ` David Miller
  0 siblings, 2 replies; 12+ messages in thread
From: Tom Herbert @ 2011-03-31  5:04 UTC (permalink / raw)
  To: davem, netdev

This patch uses __copy_from_user_nocache (from skb_copy_to_page)
on transmit to bypass data cache for a performance improvement.
This functionality is configurable per device using ethtool.

Presumably, this feature would only be useful when the driver does
not touch the data.  The feature is turned on by default if a device
indicates that it does some form of checksum offload; it is off by
default for devices that do no checksum offload or indicate no checksum
is necessary.  For the former case copy-checksum is probably done
anyway, in the latter case the device is likely loopback in which case
the no cache copy is probably not beneficial.

This patch was tested using 200 instances of netperf TCP_RR with
1400 byte request and one byte reply.  Platform is 16 core AMD x86.

No-cache copy disabled:
   672703 tps, 97.13% utilization
   50/90/99% latency:244.31 484.205 1028.41

No-cache copy enabled:
   702113 tps, 96.16% utilization,
   50/90/99% latency 238.56 467.56 956.955

Using 14000 byte request and response sizes demonstrate the
effects more dramatically:

No-cache copy disabled:
   79571 tps, 34.34 %utlization
   50/90/95% latency 1584.46 2319.59 5001.76

No-cache copy enabled:
   83856 tps, 34.81% utilization
   50/90/95% latency 2508.42 2622.62 2735.88

Note especially the effect on latency tail (95th percentile).

This seems to provide a nice performance improvement and is
consistent in the tests I ran.  Presumably, this would provide
the greatest benfits in the presence of an application workload
stressing the cache and a lot of transmit data happening.

Signed-off-by: Tom Herbert <therbert@google.com>
---
 drivers/net/bonding/bond_main.c |    2 +-
 include/linux/netdevice.h       |    6 ++++--
 include/net/sock.h              |    5 +++++
 net/core/dev.c                  |   11 +++++++++++
 net/core/ethtool.c              |    2 +-
 5 files changed, 22 insertions(+), 4 deletions(-)

diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
index 1a6e9eb..59b6ce9 100644
--- a/drivers/net/bonding/bond_main.c
+++ b/drivers/net/bonding/bond_main.c
@@ -1407,7 +1407,7 @@ static int bond_compute_features(struct bonding *bond)
 	int i;
 
 	features &= ~(NETIF_F_ALL_CSUM | BOND_VLAN_FEATURES);
-	features |=  NETIF_F_GSO_MASK | NETIF_F_NO_CSUM;
+	features |=  NETIF_F_GSO_MASK | NETIF_F_NO_CSUM | NETIF_F_NOCACHE_COPY;
 
 	if (!bond->first_slave)
 		goto done;
diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
index 5eeb2cd..06327b5 100644
--- a/include/linux/netdevice.h
+++ b/include/linux/netdevice.h
@@ -1066,6 +1066,7 @@ struct net_device {
 #define NETIF_F_NTUPLE		(1 << 27) /* N-tuple filters supported */
 #define NETIF_F_RXHASH		(1 << 28) /* Receive hashing offload */
 #define NETIF_F_RXCSUM		(1 << 29) /* Receive checksumming offload */
+#define NETIF_F_NOCACHE_COPY	(1 << 30) /* Use no-cache copyfromuser */
 
 	/* Segmentation offload features */
 #define NETIF_F_GSO_SHIFT	16
@@ -1081,7 +1082,7 @@ struct net_device {
 	/* = all defined minus driver/device-class-related */
 #define NETIF_F_NEVER_CHANGE	(NETIF_F_HIGHDMA | NETIF_F_VLAN_CHALLENGED | \
 				  NETIF_F_LLTX | NETIF_F_NETNS_LOCAL)
-#define NETIF_F_ETHTOOL_BITS	(0x3f3fffff & ~NETIF_F_NEVER_CHANGE)
+#define NETIF_F_ETHTOOL_BITS	(0x7f3fffff & ~NETIF_F_NEVER_CHANGE)
 
 	/* List of features with software fallbacks. */
 #define NETIF_F_GSO_SOFTWARE	(NETIF_F_TSO | NETIF_F_TSO_ECN | \
@@ -1108,7 +1109,8 @@ struct net_device {
 				 NETIF_F_FRAGLIST)
 
 	/* changeable features with no special hardware requirements */
-#define NETIF_F_SOFT_FEATURES	(NETIF_F_GSO | NETIF_F_GRO)
+#define NETIF_F_SOFT_FEATURES	(NETIF_F_GSO | NETIF_F_GRO |	\
+				 NETIF_F_NOCACHE_COPY)
 
 	/* Interface index. Unique device identifier	*/
 	int			ifindex;
diff --git a/include/net/sock.h b/include/net/sock.h
index da0534d..74ce586 100644
--- a/include/net/sock.h
+++ b/include/net/sock.h
@@ -1401,6 +1401,11 @@ static inline int skb_copy_to_page(struct sock *sk, char __user *from,
 		if (err)
 			return err;
 		skb->csum = csum_block_add(skb->csum, csum, skb->len);
+	} else if (sk->sk_route_caps & NETIF_F_NOCACHE_COPY) {
+		if (!access_ok(VERIFY_READ, from, copy) ||
+		    __copy_from_user_nocache(page_address(page) + off,
+						from, copy))
+			return -EFAULT;
 	} else if (copy_from_user(page_address(page) + off, from, copy))
 		return -EFAULT;
 
diff --git a/net/core/dev.c b/net/core/dev.c
index 0b88eba..454abfd 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -5444,6 +5444,13 @@ int register_netdevice(struct net_device *dev)
 		dev->features &= ~NETIF_F_GSO;
 	}
 
+	/* Turn no cache copy off if HW isn't doing checksum */
+	if (!(dev->features & NETIF_F_ALL_CSUM) ||
+	    (dev->features & NETIF_F_NO_CSUM)) {
+		dev->wanted_features &= ~NETIF_F_NOCACHE_COPY;
+		dev->features &= ~NETIF_F_NOCACHE_COPY;
+	}
+
 	/* Enable GRO and NETIF_F_HIGHDMA for vlans by default,
 	 * vlan_dev_init() will do the dev->features check, so these features
 	 * are enabled only if supported by underlying device.
@@ -6201,6 +6208,10 @@ u32 netdev_increment_features(u32 all, u32 one, u32 mask)
 		}
 	}
 
+	/* If device can't no cache copy, don't do for all */
+	if (!(one & NETIF_F_NOCACHE_COPY))
+		all &= ~NETIF_F_NOCACHE_COPY;
+
 	one |= NETIF_F_ALL_CSUM;
 
 	one |= all & NETIF_F_ONE_FOR_ALL;
diff --git a/net/core/ethtool.c b/net/core/ethtool.c
index c1a71bb..40b6fe0 100644
--- a/net/core/ethtool.c
+++ b/net/core/ethtool.c
@@ -344,7 +344,7 @@ static const char netdev_features_strings[ETHTOOL_DEV_FEATURE_WORDS * 32][ETH_GS
 	/* NETIF_F_NTUPLE */          "rx-ntuple-filter",
 	/* NETIF_F_RXHASH */          "rx-hashing",
 	/* NETIF_F_RXCSUM */          "rx-checksum",
-	"",
+	/* NETIF_F_NOCACHE_COPY */    "tx-nocache-copy"
 	"",
 };
 
-- 
1.7.3.1


^ permalink raw reply related	[flat|nested] 12+ messages in thread

* Re: [PATCH v3] net: Allow no-cache copy from user on transmit
  2011-03-31  5:04 [PATCH v3] net: Allow no-cache copy from user on transmit Tom Herbert
@ 2011-03-31  8:52 ` Michał Mirosław
  2011-04-01  4:05   ` Tom Herbert
  2011-04-02  3:49 ` David Miller
  1 sibling, 1 reply; 12+ messages in thread
From: Michał Mirosław @ 2011-03-31  8:52 UTC (permalink / raw)
  To: Tom Herbert; +Cc: davem, netdev

2011/3/31 Tom Herbert <therbert@google.com>:
> This patch uses __copy_from_user_nocache (from skb_copy_to_page)
> on transmit to bypass data cache for a performance improvement.
> This functionality is configurable per device using ethtool.
[...]
> diff --git a/include/net/sock.h b/include/net/sock.h
> index da0534d..74ce586 100644
> --- a/include/net/sock.h
> +++ b/include/net/sock.h
> @@ -1401,6 +1401,11 @@ static inline int skb_copy_to_page(struct sock *sk, char __user *from,
>                if (err)
>                        return err;
>                skb->csum = csum_block_add(skb->csum, csum, skb->len);
> +       } else if (sk->sk_route_caps & NETIF_F_NOCACHE_COPY) {
> +               if (!access_ok(VERIFY_READ, from, copy) ||
> +                   __copy_from_user_nocache(page_address(page) + off,
> +                                               from, copy))
> +                       return -EFAULT;
>        } else if (copy_from_user(page_address(page) + off, from, copy))
>                return -EFAULT;
>

BTW, the checks should take protocol into account. If, for example,
device has NOCACHE_COPY and CSUM_IPV4, but not CSUM_IPV6, then it will
take a hit for every IPv6 packet that needs checksumming in software.
Maybe this can be taken into account when sk_route_caps are set?

[...]
> diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
> index 1a6e9eb..59b6ce9 100644
> --- a/drivers/net/bonding/bond_main.c
> +++ b/drivers/net/bonding/bond_main.c
> @@ -1407,7 +1407,7 @@ static int bond_compute_features(struct bonding *bond)
>        int i;
>
>        features &= ~(NETIF_F_ALL_CSUM | BOND_VLAN_FEATURES);
> -       features |=  NETIF_F_GSO_MASK | NETIF_F_NO_CSUM;
> +       features |=  NETIF_F_GSO_MASK | NETIF_F_NO_CSUM | NETIF_F_NOCACHE_COPY;
>
>        if (!bond->first_slave)
>                goto done;
[...]
> diff --git a/net/core/dev.c b/net/core/dev.c
> index 0b88eba..454abfd 100644
> --- a/net/core/dev.c
> +++ b/net/core/dev.c
> @@ -5444,6 +5444,13 @@ int register_netdevice(struct net_device *dev)
>                dev->features &= ~NETIF_F_GSO;
>        }
>
> +       /* Turn no cache copy off if HW isn't doing checksum */
> +       if (!(dev->features & NETIF_F_ALL_CSUM) ||
> +           (dev->features & NETIF_F_NO_CSUM)) {
> +               dev->wanted_features &= ~NETIF_F_NOCACHE_COPY;
> +               dev->features &= ~NETIF_F_NOCACHE_COPY;
> +       }
> +
>        /* Enable GRO and NETIF_F_HIGHDMA for vlans by default,
>         * vlan_dev_init() will do the dev->features check, so these features
>         * are enabled only if supported by underlying device.

Why NO_CSUM is disabling NOCACHE_COPY? You combine those features in
bonding code anyway.

You didn't add netdev_fix_features() checks, so NOCACHE_COPY might be
enabled via ethtool after device is registered even if driver doesn't
support checksum offloads.

Best Regards,
Michał Mirosław

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH v3] net: Allow no-cache copy from user on transmit
  2011-03-31  8:52 ` Michał Mirosław
@ 2011-04-01  4:05   ` Tom Herbert
  0 siblings, 0 replies; 12+ messages in thread
From: Tom Herbert @ 2011-04-01  4:05 UTC (permalink / raw)
  To: Michał Mirosław; +Cc: davem, netdev

> BTW, the checks should take protocol into account. If, for example,
> device has NOCACHE_COPY and CSUM_IPV4, but not CSUM_IPV6, then it will
> take a hit for every IPv6 packet that needs checksumming in software.

If the HW does not do checksum for a protocol, then the checksum-copy
path is taken for packets so there should be no hit.  The logic of the
default setting is that if HW does not checksum offload, then there is
likely no benefit to turning on no cache copy; all the packets would
go through copy checksum.

> Maybe this can be taken into account when sk_route_caps are set?
>
> [...]
>> diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
>> index 1a6e9eb..59b6ce9 100644
>> --- a/drivers/net/bonding/bond_main.c
>> +++ b/drivers/net/bonding/bond_main.c
>> @@ -1407,7 +1407,7 @@ static int bond_compute_features(struct bonding *bond)
>>        int i;
>>
>>        features &= ~(NETIF_F_ALL_CSUM | BOND_VLAN_FEATURES);
>> -       features |=  NETIF_F_GSO_MASK | NETIF_F_NO_CSUM;
>> +       features |=  NETIF_F_GSO_MASK | NETIF_F_NO_CSUM | NETIF_F_NOCACHE_COPY;
>>
>>        if (!bond->first_slave)
>>                goto done;
> [...]
>> diff --git a/net/core/dev.c b/net/core/dev.c
>> index 0b88eba..454abfd 100644
>> --- a/net/core/dev.c
>> +++ b/net/core/dev.c
>> @@ -5444,6 +5444,13 @@ int register_netdevice(struct net_device *dev)
>>                dev->features &= ~NETIF_F_GSO;
>>        }
>>
>> +       /* Turn no cache copy off if HW isn't doing checksum */
>> +       if (!(dev->features & NETIF_F_ALL_CSUM) ||
>> +           (dev->features & NETIF_F_NO_CSUM)) {
>> +               dev->wanted_features &= ~NETIF_F_NOCACHE_COPY;
>> +               dev->features &= ~NETIF_F_NOCACHE_COPY;
>> +       }
>> +
>>        /* Enable GRO and NETIF_F_HIGHDMA for vlans by default,
>>         * vlan_dev_init() will do the dev->features check, so these features
>>         * are enabled only if supported by underlying device.
>
> Why NO_CSUM is disabling NOCACHE_COPY? You combine those features in
> bonding code anyway.
>
This is likely loopback in which case no cache copy probably does not
help.  For bonding, no cache copy is only enabled when all slaves are
doing it.

> You didn't add netdev_fix_features() checks, so NOCACHE_COPY might be
> enabled via ethtool after device is registered even if driver doesn't
> support checksum offloads.
>
I don't see a reason to strictly enforce this.  It's not guaranteed
that if a driver doesn't support csum offload then no cache copy is
bad (again it's probably noop anyway because of copy-csum path).
Turning it on for loopback is a benefit if sender and receiver are in
different cache domains is good.

I do need to send another patch.  The skb_add_data function can also
use this.  I will create skb_do_copy_data_nocache and
skb_add_data_nocache to make the use of this explicit in each protocol
(TCP being the first supported).

Tom

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH v3] net: Allow no-cache copy from user on transmit
  2011-03-31  5:04 [PATCH v3] net: Allow no-cache copy from user on transmit Tom Herbert
  2011-03-31  8:52 ` Michał Mirosław
@ 2011-04-02  3:49 ` David Miller
  2011-04-02  3:52   ` David Miller
  1 sibling, 1 reply; 12+ messages in thread
From: David Miller @ 2011-04-02  3:49 UTC (permalink / raw)
  To: therbert; +Cc: netdev

From: Tom Herbert <therbert@google.com>
Date: Wed, 30 Mar 2011 22:04:32 -0700 (PDT)

> This patch uses __copy_from_user_nocache (from skb_copy_to_page)
> on transmit to bypass data cache for a performance improvement.
> This functionality is configurable per device using ethtool.
...
> Signed-off-by: Tom Herbert <therbert@google.com>

It's early in net-next-2.6, so I've applied this now and we can
add any fixups and tweaks as needed.

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH v3] net: Allow no-cache copy from user on transmit
  2011-04-02  3:49 ` David Miller
@ 2011-04-02  3:52   ` David Miller
  0 siblings, 0 replies; 12+ messages in thread
From: David Miller @ 2011-04-02  3:52 UTC (permalink / raw)
  To: therbert; +Cc: netdev

From: David Miller <davem@davemloft.net>
Date: Fri, 01 Apr 2011 20:49:22 -0700 (PDT)

> From: Tom Herbert <therbert@google.com>
> Date: Wed, 30 Mar 2011 22:04:32 -0700 (PDT)
> 
>> This patch uses __copy_from_user_nocache (from skb_copy_to_page)
>> on transmit to bypass data cache for a performance improvement.
>> This functionality is configurable per device using ethtool.
> ...
>> Signed-off-by: Tom Herbert <therbert@google.com>
> 
> It's early in net-next-2.6, so I've applied this now and we can
> add any fixups and tweaks as needed.

Nevermind, you have some work to do in the generic kernel before I
can add this, because nocache copies are not a universally available
feature, for example on sparc64:

In file included from include/linux/tcp.h:210,
                 from include/linux/ipv6.h:221,
                 from include/net/ip_vs.h:26,
                 from kernel/sysctl_binary.c:6:
include/net/sock.h: In function ‘skb_copy_to_page’:
include/net/sock.h:1406: error: implicit declaration of function ‘__copy_from_user_nocache’
make[1]: *** [kernel/sysctl_binary.o] Error 1
make: *** [kernel] Error 2
make: *** Waiting for unfinished jobs....
In file included from include/linux/if_pppox.h:170,
                 from fs/compat_ioctl.c:38:
include/net/sock.h: In function ‘skb_copy_to_page’:
include/net/sock.h:1406: error: implicit declaration of function ‘__copy_from_user_nocache’
In file included from include/linux/highmem.h:7,
                 from include/linux/pagemap.h:10,
                 from include/linux/blkdev.h:12,
                 from fs/compat_ioctl.c:48:
include/linux/uaccess.h: At top level:
include/linux/uaccess.h:49: error: conflicting types for ‘__copy_from_user_nocache’
include/net/sock.h:1406: note: previous implicit declaration of ‘__copy_from_user_nocache’ was here

^ permalink raw reply	[flat|nested] 12+ messages in thread

* [PATCH v3] net: Allow no-cache copy from user on transmit
@ 2011-04-04  4:56 Tom Herbert
  2011-04-04  5:03 ` David Miller
  0 siblings, 1 reply; 12+ messages in thread
From: Tom Herbert @ 2011-04-04  4:56 UTC (permalink / raw)
  To: davem, netdev

This patch uses __copy_from_user_nocache on transmit to bypass data
cache for a performance improvement.  skb_add_data_nocache and
skb_copy_to_page_nocache can be called by sendmsg functions to use
this feature, initial support is in tcp_sendmsg.  This functionality is
configurable per device using ethtool.

Presumably, this feature would only be useful when the driver does
not touch the data.  The feature is turned on by default if a device
indicates that it does some form of checksum offload; it is off by
default for devices that do no checksum offload or indicate no checksum
is necessary.  For the former case copy-checksum is probably done
anyway, in the latter case the device is likely loopback in which case
the no cache copy is probably not beneficial.

This patch was tested using 200 instances of netperf TCP_RR with
1400 byte request and one byte reply.  Platform is 16 core AMD x86.

No-cache copy disabled:
   672703 tps, 97.13% utilization
   50/90/99% latency:244.31 484.205 1028.41

No-cache copy enabled:
   702113 tps, 96.16% utilization,
   50/90/99% latency 238.56 467.56 956.955

Using 14000 byte request and response sizes demonstrate the
effects more dramatically:

No-cache copy disabled:
   79571 tps, 34.34 %utlization
   50/90/95% latency 1584.46 2319.59 5001.76

No-cache copy enabled:
   83856 tps, 34.81% utilization
   50/90/95% latency 2508.42 2622.62 2735.88

Note especially the effect on latency tail (95th percentile).

This seems to provide a nice performance improvement and is
consistent in the tests I ran.  Presumably, this would provide
the greatest benfits in the presence of an application workload
stressing the cache and a lot of transmit data happening.

Signed-off-by: Tom Herbert <therbert@google.com>
---
 drivers/net/bonding/bond_main.c |    2 +-
 include/linux/netdevice.h       |    3 +-
 include/net/sock.h              |   55 +++++++++++++++++++++++++++++++++++++++
 net/core/dev.c                  |   15 ++++++++++
 net/core/ethtool.c              |    2 +-
 net/ipv4/tcp.c                  |    7 +++--
 6 files changed, 78 insertions(+), 6 deletions(-)

diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
index 16d6fe9..b51e021 100644
--- a/drivers/net/bonding/bond_main.c
+++ b/drivers/net/bonding/bond_main.c
@@ -1407,7 +1407,7 @@ static int bond_compute_features(struct bonding *bond)
 	int i;
 
 	features &= ~(NETIF_F_ALL_CSUM | BOND_VLAN_FEATURES);
-	features |=  NETIF_F_GSO_MASK | NETIF_F_NO_CSUM;
+	features |=  NETIF_F_GSO_MASK | NETIF_F_NO_CSUM | NETIF_F_NOCACHE_COPY;
 
 	if (!bond->first_slave)
 		goto done;
diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
index 423a544..1828119 100644
--- a/include/linux/netdevice.h
+++ b/include/linux/netdevice.h
@@ -1066,6 +1066,7 @@ struct net_device {
 #define NETIF_F_NTUPLE		(1 << 27) /* N-tuple filters supported */
 #define NETIF_F_RXHASH		(1 << 28) /* Receive hashing offload */
 #define NETIF_F_RXCSUM		(1 << 29) /* Receive checksumming offload */
+#define NETIF_F_NOCACHE_COPY	(1 << 30) /* Use no-cache copyfromuser */
 
 	/* Segmentation offload features */
 #define NETIF_F_GSO_SHIFT	16
@@ -1081,7 +1082,7 @@ struct net_device {
 	/* = all defined minus driver/device-class-related */
 #define NETIF_F_NEVER_CHANGE	(NETIF_F_HIGHDMA | NETIF_F_VLAN_CHALLENGED | \
 				  NETIF_F_LLTX | NETIF_F_NETNS_LOCAL)
-#define NETIF_F_ETHTOOL_BITS	(0x3f3fffff & ~NETIF_F_NEVER_CHANGE)
+#define NETIF_F_ETHTOOL_BITS	(0x7f3fffff & ~NETIF_F_NEVER_CHANGE)
 
 	/* List of features with software fallbacks. */
 #define NETIF_F_GSO_SOFTWARE	(NETIF_F_TSO | NETIF_F_TSO_ECN | \
diff --git a/include/net/sock.h b/include/net/sock.h
index da0534d..91c81f5 100644
--- a/include/net/sock.h
+++ b/include/net/sock.h
@@ -52,6 +52,7 @@
 #include <linux/mm.h>
 #include <linux/security.h>
 #include <linux/slab.h>
+#include <linux/uaccess.h>
 
 #include <linux/filter.h>
 #include <linux/rculist_nulls.h>
@@ -1389,6 +1390,60 @@ static inline void sk_nocaps_add(struct sock *sk, int flags)
 	sk->sk_route_caps &= ~flags;
 }
 
+static inline int skb_do_copy_data_nocache(struct sock *sk, struct sk_buff *skb,
+					   char __user *from, char *to,
+					   int copy)
+{
+	if (skb->ip_summed == CHECKSUM_NONE) {
+		int err = 0;
+		__wsum csum = csum_and_copy_from_user(from, to, copy, 0, &err);
+		if (err)
+			return err;
+		skb->csum = csum_block_add(skb->csum, csum, skb->len);
+#ifdef ARCH_HAS_NOCACHE_UACCESS
+	} else if (sk->sk_route_caps & NETIF_F_NOCACHE_COPY) {
+		if (!access_ok(VERIFY_READ, from, copy) ||
+		    __copy_from_user_nocache(to, from, copy))
+			return -EFAULT;
+#endif
+	} else if (copy_from_user(to, from, copy))
+		return -EFAULT;
+
+	return 0;
+}
+
+static inline int skb_add_data_nocache(struct sock *sk, struct sk_buff *skb,
+				       char __user *from, int copy)
+{
+	int err;
+
+	err = skb_do_copy_data_nocache(sk, skb, from, skb_put(skb, copy), copy);
+	if (err)
+		__skb_trim(skb, skb->len);
+
+	return err;
+}
+
+static inline int skb_copy_to_page_nocache(struct sock *sk, char __user *from,
+					   struct sk_buff *skb,
+					   struct page *page,
+					   int off, int copy)
+{
+	int err;
+
+	err = skb_do_copy_data_nocache(sk, skb, from,
+				       page_address(page) + off, copy);
+	if (err)
+		return err;
+
+	skb->len	     += copy;
+	skb->data_len	     += copy;
+	skb->truesize	     += copy;
+	sk->sk_wmem_queued   += copy;
+	sk_mem_charge(sk, copy);
+	return 0;
+}
+
 static inline int skb_copy_to_page(struct sock *sk, char __user *from,
 				   struct sk_buff *skb, struct page *page,
 				   int off, int copy)
diff --git a/net/core/dev.c b/net/core/dev.c
index 02f5637..4c58a90 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -5425,6 +5425,17 @@ int register_netdevice(struct net_device *dev)
 		dev->features &= ~NETIF_F_GSO;
 	}
 
+#ifdef ARCH_HAS_NOCACHE_UACCESS
+	dev->hw_features |= NETIF_F_NOCACHE_COPY;
+
+	/* Turn on no cache copy off if HW is doing checksum */
+	if ((dev->features & NETIF_F_ALL_CSUM) &&
+	    !(dev->features & NETIF_F_NO_CSUM)) {
+		dev->wanted_features |= NETIF_F_NOCACHE_COPY;
+		dev->features |= NETIF_F_NOCACHE_COPY;
+	}
+#endif
+
 	/* Enable GRO and NETIF_F_HIGHDMA for vlans by default,
 	 * vlan_dev_init() will do the dev->features check, so these features
 	 * are enabled only if supported by underlying device.
@@ -6182,6 +6193,10 @@ u32 netdev_increment_features(u32 all, u32 one, u32 mask)
 		}
 	}
 
+	/* If device can't no cache copy, don't do for all */
+	if (!(one & NETIF_F_NOCACHE_COPY))
+		all &= ~NETIF_F_NOCACHE_COPY;
+
 	one |= NETIF_F_ALL_CSUM;
 
 	one |= all & NETIF_F_ONE_FOR_ALL;
diff --git a/net/core/ethtool.c b/net/core/ethtool.c
index 439e4b0..719670a 100644
--- a/net/core/ethtool.c
+++ b/net/core/ethtool.c
@@ -359,7 +359,7 @@ static const char netdev_features_strings[ETHTOOL_DEV_FEATURE_WORDS * 32][ETH_GS
 	/* NETIF_F_NTUPLE */          "rx-ntuple-filter",
 	/* NETIF_F_RXHASH */          "rx-hashing",
 	/* NETIF_F_RXCSUM */          "rx-checksum",
-	"",
+	/* NETIF_F_NOCACHE_COPY */    "tx-nocache-copy"
 	"",
 };
 
diff --git a/net/ipv4/tcp.c b/net/ipv4/tcp.c
index b22d450..054a59d 100644
--- a/net/ipv4/tcp.c
+++ b/net/ipv4/tcp.c
@@ -999,7 +999,8 @@ new_segment:
 				/* We have some space in skb head. Superb! */
 				if (copy > skb_tailroom(skb))
 					copy = skb_tailroom(skb);
-				if ((err = skb_add_data(skb, from, copy)) != 0)
+				err = skb_add_data_nocache(sk, skb, from, copy);
+				if (err)
 					goto do_fault;
 			} else {
 				int merge = 0;
@@ -1042,8 +1043,8 @@ new_segment:
 
 				/* Time to copy data. We are close to
 				 * the end! */
-				err = skb_copy_to_page(sk, from, skb, page,
-						       off, copy);
+				err = skb_copy_to_page_nocache(sk, from, skb,
+							       page, off, copy);
 				if (err) {
 					/* If this page was new, give it to the
 					 * socket so it does not get leaked.
-- 
1.7.3.1


^ permalink raw reply related	[flat|nested] 12+ messages in thread

* Re: [PATCH v3] net: Allow no-cache copy from user on transmit
  2011-04-04  4:56 Tom Herbert
@ 2011-04-04  5:03 ` David Miller
  2011-04-04  5:23   ` David Miller
  0 siblings, 1 reply; 12+ messages in thread
From: David Miller @ 2011-04-04  5:03 UTC (permalink / raw)
  To: therbert; +Cc: netdev

From: Tom Herbert <therbert@google.com>
Date: Sun, 3 Apr 2011 21:56:17 -0700 (PDT)

> This patch uses __copy_from_user_nocache on transmit to bypass data
> cache for a performance improvement.  skb_add_data_nocache and
> skb_copy_to_page_nocache can be called by sendmsg functions to use
> this feature, initial support is in tcp_sendmsg.  This functionality is
> configurable per device using ethtool.
 ...
> Signed-off-by: Tom Herbert <therbert@google.com>

Applied, thanks Tom.

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH v3] net: Allow no-cache copy from user on transmit
  2011-04-04  5:03 ` David Miller
@ 2011-04-04  5:23   ` David Miller
  2011-04-04 15:10     ` Tom Herbert
  0 siblings, 1 reply; 12+ messages in thread
From: David Miller @ 2011-04-04  5:23 UTC (permalink / raw)
  To: therbert; +Cc: netdev

From: David Miller <davem@davemloft.net>
Date: Sun, 03 Apr 2011 22:03:05 -0700 (PDT)

> From: Tom Herbert <therbert@google.com>
> Date: Sun, 3 Apr 2011 21:56:17 -0700 (PDT)
> 
>> This patch uses __copy_from_user_nocache on transmit to bypass data
>> cache for a performance improvement.  skb_add_data_nocache and
>> skb_copy_to_page_nocache can be called by sendmsg functions to use
>> this feature, initial support is in tcp_sendmsg.  This functionality is
>> configurable per device using ethtool.
>  ...
>> Signed-off-by: Tom Herbert <therbert@google.com>
> 
> Applied, thanks Tom.

Actually, I'm sorry, I have to kick this back to you again Tom.

The original problem is that "linux/uaccess.h" has not been included
in the spot where you try to invoke the nocache copies.

linux/uaccess.h, when ARCH_HAS_NOCACHE_UACCESS is defined, provides
dummy routines.

So it's not correct to use ARCH_HAS_NOCACHE_UACCESS to conditionalize
things in the networking, just make sure linux/uaccess.h is included
at the call sites.

Thanks.

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH v3] net: Allow no-cache copy from user on transmit
  2011-04-04  5:23   ` David Miller
@ 2011-04-04 15:10     ` Tom Herbert
  2011-04-04 16:52       ` David Miller
  0 siblings, 1 reply; 12+ messages in thread
From: Tom Herbert @ 2011-04-04 15:10 UTC (permalink / raw)
  To: David Miller; +Cc: netdev

> Actually, I'm sorry, I have to kick this back to you again Tom.
>
I don't think I see the problem.  Does this still fail sparc compilation?

> The original problem is that "linux/uaccess.h" has not been included
> in the spot where you try to invoke the nocache copies.
>
So I added include of linux/uaccess.h to net/sock.h.

> linux/uaccess.h, when ARCH_HAS_NOCACHE_UACCESS is defined, provides
> dummy routines.
>
I assume you mean "is not defined"?

> So it's not correct to use ARCH_HAS_NOCACHE_UACCESS to conditionalize
> things in the networking, just make sure linux/uaccess.h is included
> at the call sites.
>
Why isn't this correct?  Shouldn't it be okay if linux/uaccess.h
(asm/uaccess.h) is always included where  ARCH_HAS_NOCACHE_UACCESS is
used?

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH v3] net: Allow no-cache copy from user on transmit
  2011-04-04 15:10     ` Tom Herbert
@ 2011-04-04 16:52       ` David Miller
  2011-04-04 19:44         ` Tom Herbert
  0 siblings, 1 reply; 12+ messages in thread
From: David Miller @ 2011-04-04 16:52 UTC (permalink / raw)
  To: therbert; +Cc: netdev

From: Tom Herbert <therbert@google.com>
Date: Mon, 4 Apr 2011 08:10:08 -0700

>> So it's not correct to use ARCH_HAS_NOCACHE_UACCESS to conditionalize
>> things in the networking, just make sure linux/uaccess.h is included
>> at the call sites.
>>
> Why isn't this correct?  Shouldn't it be okay if linux/uaccess.h
> (asm/uaccess.h) is always included where  ARCH_HAS_NOCACHE_UACCESS is
> used?

I'm simply saying to get rid of the ARCH_HAS_NOCACHE_UACCESS ifdefs
you're adding to the networking code, since linux/uaccess.h makes sure
that a nop version of the nocache routines are available always.

If you ifdef the networking bits unnecessarily, those code paths
won't get build tested in the majority of my test builds, which are
on sparc64.  So I want to avoid the conditionalized compilation if
at all possible.

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH v3] net: Allow no-cache copy from user on transmit
  2011-04-04 16:52       ` David Miller
@ 2011-04-04 19:44         ` Tom Herbert
  2011-04-04 20:05           ` David Miller
  0 siblings, 1 reply; 12+ messages in thread
From: Tom Herbert @ 2011-04-04 19:44 UTC (permalink / raw)
  To: David Miller; +Cc: netdev

> I'm simply saying to get rid of the ARCH_HAS_NOCACHE_UACCESS ifdefs
> you're adding to the networking code, since linux/uaccess.h makes sure
> that a nop version of the nocache routines are available always.
>
> If you ifdef the networking bits unnecessarily, those code paths
> won't get build tested in the majority of my test builds, which are
> on sparc64.  So I want to avoid the conditionalized compilation if
> at all possible.
>

I'll take them out, but that will result in one needless conditional
in the transmit path for architectures that don't support the no cache
copy, which seems to be everything except x86!?

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH v3] net: Allow no-cache copy from user on transmit
  2011-04-04 19:44         ` Tom Herbert
@ 2011-04-04 20:05           ` David Miller
  0 siblings, 0 replies; 12+ messages in thread
From: David Miller @ 2011-04-04 20:05 UTC (permalink / raw)
  To: therbert; +Cc: netdev

From: Tom Herbert <therbert@google.com>
Date: Mon, 4 Apr 2011 12:44:08 -0700

> I'll take them out, but that will result in one needless conditional
> in the transmit path for architectures that don't support the no cache
> copy, which seems to be everything except x86!?

I think this is a small price to pay for the build regression
exposure, and perheps it's also a bit of an incentive for more
architectures to support nocache copies :-)

^ permalink raw reply	[flat|nested] 12+ messages in thread

end of thread, other threads:[~2011-04-04 20:06 UTC | newest]

Thread overview: 12+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2011-03-31  5:04 [PATCH v3] net: Allow no-cache copy from user on transmit Tom Herbert
2011-03-31  8:52 ` Michał Mirosław
2011-04-01  4:05   ` Tom Herbert
2011-04-02  3:49 ` David Miller
2011-04-02  3:52   ` David Miller
  -- strict thread matches above, loose matches on Subject: below --
2011-04-04  4:56 Tom Herbert
2011-04-04  5:03 ` David Miller
2011-04-04  5:23   ` David Miller
2011-04-04 15:10     ` Tom Herbert
2011-04-04 16:52       ` David Miller
2011-04-04 19:44         ` Tom Herbert
2011-04-04 20:05           ` David Miller

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).