netdev - Re: [PATCH net-next] net: use indirect call wrappers for skb_copy_datagram

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: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Tue, 24 Mar 2020 21:22:16 -0700
From:   Eric Dumazet <eric.dumazet@...il.com>
To:     Eric Dumazet <edumazet@...gle.com>,
        "David S . Miller" <davem@...emloft.net>
Cc:     netdev <netdev@...r.kernel.org>,
        Eric Dumazet <eric.dumazet@...il.com>,
        Sagi Grimberg <sagi@...htbitslabs.com>
Subject: Re: [PATCH net-next] net: use indirect call wrappers for
 skb_copy_datagram_iter()



On 3/24/20 7:23 PM, Eric Dumazet wrote:
> TCP recvmsg() calls skb_copy_datagram_iter(), which
> calls an indirect function (cb pointing to simple_copy_to_iter())
> for every MSS (fragment) present in the skb.
> 
> CONFIG_RETPOLINE=y forces a very expensive operation
> that we can avoid thanks to indirect call wrappers.
> 
> This patch gives a 13% increase of performance on
> a single flow, if the bottleneck is the thread reading
> the TCP socket.
> 
> Signed-off-by: Eric Dumazet <edumazet@...gle.com>


BTW, the expensive indirect call came with :

So we could also add a Fixes: tag eventually

Fixes: 950fcaecd5cc ("datagram: consolidate datagram copy to iter helpers")

commit 950fcaecd5cc6c014bb96506fd0652a501c85276
Author: Sagi Grimberg <sagi@...htbitslabs.com>
Date:   Mon Dec 3 17:52:08 2018 -0800

    datagram: consolidate datagram copy to iter helpers
    
    skb_copy_datagram_iter and skb_copy_and_csum_datagram are essentialy
    the same but with a couple of differences: The first is the copy
    operation used which either a simple copy or a csum_and_copy, and the
    second are the behavior on the "short copy" path where simply copy
    needs to return the number of bytes successfully copied while csum_and_copy
    needs to fault immediately as the checksum is partial.
    
    Introduce __skb_datagram_iter that additionally accepts:
    1. copy operation function pointer
    2. private data that goes with the copy operation
    3. fault_short flag to indicate the action on short copy
    
    Suggested-by: David S. Miller <davem@...emloft.net>
    Acked-by: David S. Miller <davem@...emloft.net>
    Signed-off-by: Sagi Grimberg <sagi@...htbitslabs.com>
    Signed-off-by: Christoph Hellwig <hch@....de>

> ---
>  net/core/datagram.c | 14 +++++++++++---
>  1 file changed, 11 insertions(+), 3 deletions(-)
> 
> diff --git a/net/core/datagram.c b/net/core/datagram.c
> index 4213081c6ed3d4fda69501641a8c76e041f26b42..639745d4f3b94a248da9a685f45158410a85bec7 100644
> --- a/net/core/datagram.c
> +++ b/net/core/datagram.c
> @@ -51,6 +51,7 @@
>  #include <linux/slab.h>
>  #include <linux/pagemap.h>
>  #include <linux/uio.h>
> +#include <linux/indirect_call_wrapper.h>
>  
>  #include <net/protocol.h>
>  #include <linux/skbuff.h>
> @@ -403,6 +404,11 @@ int skb_kill_datagram(struct sock *sk, struct sk_buff *skb, unsigned int flags)
>  }
>  EXPORT_SYMBOL(skb_kill_datagram);
>  
> +INDIRECT_CALLABLE_DECLARE(static size_t simple_copy_to_iter(const void *addr,
> +						size_t bytes,
> +						void *data __always_unused,
> +						struct iov_iter *i));
> +
>  static int __skb_datagram_iter(const struct sk_buff *skb, int offset,
>  			       struct iov_iter *to, int len, bool fault_short,
>  			       size_t (*cb)(const void *, size_t, void *,
> @@ -416,7 +422,8 @@ static int __skb_datagram_iter(const struct sk_buff *skb, int offset,
>  	if (copy > 0) {
>  		if (copy > len)
>  			copy = len;
> -		n = cb(skb->data + offset, copy, data, to);
> +		n = INDIRECT_CALL_1(cb, simple_copy_to_iter,
> +				    skb->data + offset, copy, data, to);
>  		offset += n;
>  		if (n != copy)
>  			goto short_copy;
> @@ -438,8 +445,9 @@ static int __skb_datagram_iter(const struct sk_buff *skb, int offset,
>  
>  			if (copy > len)
>  				copy = len;
> -			n = cb(vaddr + skb_frag_off(frag) + offset - start,
> -			       copy, data, to);
> +			n = INDIRECT_CALL_1(cb, simple_copy_to_iter,
> +					vaddr + skb_frag_off(frag) + offset - start,
> +					copy, data, to);
>  			kunmap(page);
>  			offset += n;
>  			if (n != copy)
>