From mboxrd@z Thu Jan 1 00:00:00 1970 From: Willem de Bruijn Subject: Re: [RFC PATCH v2 01/10] udp: implement complete book-keeping for encap_needed Date: Mon, 22 Oct 2018 12:06:35 -0400 Message-ID: References: Mime-Version: 1.0 Content-Type: text/plain; charset="UTF-8" Cc: Network Development , Willem de Bruijn , steffen.klassert@secunet.com To: Paolo Abeni Return-path: Received: from mail-ed1-f65.google.com ([209.85.208.65]:41335 "EHLO mail-ed1-f65.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1728306AbeJWA0V (ORCPT ); Mon, 22 Oct 2018 20:26:21 -0400 Received: by mail-ed1-f65.google.com with SMTP id x31-v6so38467327edd.8 for ; Mon, 22 Oct 2018 09:07:12 -0700 (PDT) In-Reply-To: Sender: netdev-owner@vger.kernel.org List-ID: On Fri, Oct 19, 2018 at 10:30 AM Paolo Abeni wrote: > > The *encap_needed static keys are enabled by UDP tunnels > and several UDP encapsulations type, but they are never > turned off. This can cause unneeded overall performance > degradation for systems where such features are used > transiently. > > This patch introduces complete book-keeping for such keys, > decreasing the usage at socket destruction time, if needed, > and avoiding that the same socket could increase the key > usage multiple times. > > Signed-off-by: Paolo Abeni > --- > include/linux/udp.h | 7 ++++++- > include/net/udp_tunnel.h | 6 ++++++ > net/ipv4/udp.c | 18 ++++++++++++------ > net/ipv6/udp.c | 14 +++++++++----- > 4 files changed, 33 insertions(+), 12 deletions(-) > > diff --git a/include/linux/udp.h b/include/linux/udp.h > index 320d49d85484..a4dafff407fb 100644 > --- a/include/linux/udp.h > +++ b/include/linux/udp.h > @@ -49,7 +49,12 @@ struct udp_sock { > unsigned int corkflag; /* Cork is required */ > __u8 encap_type; /* Is this an Encapsulation socket? */ > unsigned char no_check6_tx:1,/* Send zero UDP6 checksums on TX? */ > - no_check6_rx:1;/* Allow zero UDP6 checksums on RX? */ > + no_check6_rx:1,/* Allow zero UDP6 checksums on RX? */ > + encap_enabled:1; /* This socket enabled encap > + * processing; UDP tunnels and > + * different encapsulation layer set > + * this > + */ > /* > * Following member retains the information to create a UDP header > * when the socket is uncorked. > diff --git a/include/net/udp_tunnel.h b/include/net/udp_tunnel.h > index fe680ab6b15a..3fbe56430e3b 100644 > --- a/include/net/udp_tunnel.h > +++ b/include/net/udp_tunnel.h > @@ -165,6 +165,12 @@ static inline int udp_tunnel_handle_offloads(struct sk_buff *skb, bool udp_csum) > > static inline void udp_tunnel_encap_enable(struct socket *sock) > { > + struct udp_sock *up = udp_sk(sock->sk); > + > + if (up->encap_enabled) > + return; > + > + up->encap_enabled = 1; > #if IS_ENABLED(CONFIG_IPV6) > if (sock->sk->sk_family == PF_INET6) > ipv6_stub->udpv6_encap_enable(); > diff --git a/net/ipv4/udp.c b/net/ipv4/udp.c > index cf8252d05a01..9fcb5374e166 100644 > --- a/net/ipv4/udp.c > +++ b/net/ipv4/udp.c > @@ -2382,11 +2382,15 @@ void udp_destroy_sock(struct sock *sk) > bool slow = lock_sock_fast(sk); > udp_flush_pending_frames(sk); > unlock_sock_fast(sk, slow); > - if (static_branch_unlikely(&udp_encap_needed_key) && up->encap_type) { > - void (*encap_destroy)(struct sock *sk); > - encap_destroy = READ_ONCE(up->encap_destroy); > - if (encap_destroy) > - encap_destroy(sk); > + if (static_branch_unlikely(&udp_encap_needed_key)) { > + if (up->encap_type) { > + void (*encap_destroy)(struct sock *sk); > + encap_destroy = READ_ONCE(up->encap_destroy); > + if (encap_destroy) > + encap_destroy(sk); > + } > + if (up->encap_enabled) > + static_branch_disable(&udp_encap_needed_key); > } > } > > @@ -2431,7 +2435,9 @@ int udp_lib_setsockopt(struct sock *sk, int level, int optname, > /* FALLTHROUGH */ > case UDP_ENCAP_L2TPINUDP: > up->encap_type = val; > - udp_encap_enable(); > + if (!up->encap_enabled) > + udp_encap_enable(); > + up->encap_enabled = 1; nit: no need for the branch: udp_encap_enable already has a branch and is static inline. Perhaps it makes sense to convert that to take the udp_sock and handle the state change within, to avoid having to open code at multiple locations.