netdev - Re: [PATCH net-next] udp: use sk_protocol instead of pcflag to detect udplite sockets

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]

Message-ID: <1490966709.8750.7.camel@edumazet-glaptop3.roam.corp.google.com>
Date:   Fri, 31 Mar 2017 06:25:09 -0700
From:   Eric Dumazet <eric.dumazet@...il.com>
To:     Paolo Abeni <pabeni@...hat.com>
Cc:     netdev@...r.kernel.org, "David S. Miller" <davem@...emloft.net>,
        Eric Dumazet <edumazet@...gle.com>
Subject: Re: [PATCH net-next] udp: use sk_protocol instead of pcflag to
 detect udplite sockets

On Fri, 2017-03-31 at 11:47 +0200, Paolo Abeni wrote:
> In the udp_sock struct, the 'forward_deficit' and 'pcflag' fields
> share the same cacheline. While the first is dirtied by
> udp_recvmsg, the latter is read, possibly several times, by the
> bottom half processing to discriminate between udp and udplite
> sockets.
> 
> With this patch, sk->sk_protocol is used to check is the socket is
> really an udplite one, avoiding some cache misses per
> packet and improving the performance under udp_flood with
> small packet up to 10%.
> 
> Signed-off-by: Paolo Abeni <pabeni@...hat.com>
> ---
>  include/linux/udp.h | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/include/linux/udp.h b/include/linux/udp.h
> index c0f5308..6cb4061 100644
> --- a/include/linux/udp.h
> +++ b/include/linux/udp.h
> @@ -115,6 +115,6 @@ static inline bool udp_get_no_check6_rx(struct sock *sk)
>  #define udp_portaddr_for_each_entry_rcu(__sk, list) \
>  	hlist_for_each_entry_rcu(__sk, list, __sk_common.skc_portaddr_node)
>  
> -#define IS_UDPLITE(__sk) (udp_sk(__sk)->pcflag)
> +#define IS_UDPLITE(__sk) (__sk->sk_protocol == IPPROTO_UDPLITE)
>  
>  #endif	/* _LINUX_UDP_H */



I am pretty sure we agreed in the past that forward_deficit would need
to be placed on a cache line of its own. Somehow we manage to not
implement this properly.

What about other fields like encap_rcv, encap_destroy, gro_receive,
gro_complete. They really should have the same false sharing issue.

Proper fix is :

 include/linux/udp.h |    6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/include/linux/udp.h b/include/linux/udp.h
index c0f530809d1f3db7323e51a52224eb49d8f97da0..bdfd11823c12f6c08dc6b2adb247b8e0a7f2e5d5 100644
--- a/include/linux/udp.h
+++ b/include/linux/udp.h
@@ -80,8 +80,10 @@ struct udp_sock {
 						struct sk_buff *skb,
 						int nhoff);
 
-	/* This field is dirtied by udp_recvmsg() */
-	int		forward_deficit;
+	/* This field is dirtied by udp_recvmsg().
+	 * Make sure it wont share a cache line with prior fields.
+	 */
+	int		forward_deficit ____cacheline_aligned_in_smp;
 };
 
 static inline struct udp_sock *udp_sk(const struct sock *sk)