netdev - Re: [PATCH 1/2] net: Allow no-cache copy from user on transmit

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite for Android: free password hash cracker in your pocket
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <AANLkTikFSRr649_P8V8J7Xj_KY8hSEw23RBAyyV2wUQr@mail.gmail.com>
Date:	Wed, 9 Mar 2011 14:04:14 +0100
From:	Michał Mirosław <mirqus@...il.com>
To:	Tom Herbert <therbert@...gle.com>
Cc:	davem@...emloft.net, netdev@...r.kernel.org
Subject: Re: [PATCH 1/2] net: Allow no-cache copy from user on transmit

2011/3/9 Tom Herbert <therbert@...gle.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, the
> device must also be doing TX csum offload to enable.  It seems
> reasonable to set this when the device does not copy or
> otherwise touch the data.
>
> This patch was tested using 200 instances of netperf TCP_RR with
> 1400 byte request and one byte reply.  Platform is 16 core AMD.
>
> No-cache copy disabled:
>   672703 tps, 97.13% client utilization
>   50/90/99% latency 244.31 484.205 1028.41
>
> No-cache copy enabled:
>   702113 tps, 96.16% client utilization,
>   50/90/99% latency 238.56 467.56 956.955
>
> This seems to provide a nice little 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.  I don't
> yet see a downside to using this.
>
> Signed-off-by: Tom Herbert <therbert@...gle.com>
> ---
>  include/linux/ethtool.h   |    4 ++++
>  include/linux/netdevice.h |    4 +++-
>  include/net/sock.h        |    5 +++++
>  net/core/ethtool.c        |   11 ++++++++++-
>  4 files changed, 22 insertions(+), 2 deletions(-)
>
> diff --git a/include/linux/ethtool.h b/include/linux/ethtool.h
> index aac3e2e..2cf3cc9 100644
> --- a/include/linux/ethtool.h
> +++ b/include/linux/ethtool.h
> @@ -833,6 +833,10 @@ struct ethtool_ops {
>  #define ETHTOOL_GFEATURES      0x0000003a /* Get device offload settings */
>  #define ETHTOOL_SFEATURES      0x0000003b /* Change device offload settings */
>
> +#define ETHTOOL_GNCCOPY                0x0000003c /* Get no-cache-copy enable
> +                                           * (ethtool_value). */
> +#define ETHTOOL_SNCCOPY                0x0000003d /* Set no-cache-copy enable
> +                                           * (ethtool_value). */

Please don't add new get/set ops for netdev features - G/SFEATURES
handle them all.

Proper way is to modify NETIF_F_ETHTOOL_BITS, netdev_fix_features()
and, if needed for now,
legacy get/set ops (they will go away, eventually).

>  /* compatibility with older code */
>  #define SPARC_ETH_GSET         ETHTOOL_GSET
>  #define SPARC_ETH_SSET         ETHTOOL_SSET
> diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
> index 71563e7..1b72f29 100644
> --- a/include/linux/netdevice.h
> +++ b/include/linux/netdevice.h
> @@ -978,6 +978,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
> @@ -1020,7 +1021,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..55b1385 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/ethtool.c b/net/core/ethtool.c
> index c1a71bb..a8b2638 100644
> --- a/net/core/ethtool.c
> +++ b/net/core/ethtool.c
> @@ -328,6 +328,7 @@ static const char netdev_features_strings[ETHTOOL_DEV_FEATURE_WORDS * 32][ETH_GS
>        /* NETIF_F_NETNS_LOCAL */     "netns-local",
>        /* NETIF_F_GRO */             "rx-gro",
>        /* NETIF_F_LRO */             "rx-lro",
> +       /* NETIF_F_NOCACHE_COPY */    "tx-nocache-copy",
>
>        /* NETIF_F_TSO */             "tx-tcp-segmentation",
>        /* NETIF_F_UFO */             "tx-udp-fragmentation",

You can't put the strings in the middle of this array. Positions in
the array are dependent on flag bit position.

[...]

Best Regards,
Michał Mirosław
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html