netdev - Re: [PATCH net-next 2/2] udp: track the forward memory release threshold in an hot cacheline

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: <48dc93489465e75a0f37c4b02f4711598cb1ed4d.camel@redhat.com>
Date:   Wed, 19 Oct 2022 18:58:33 +0200
From:   Paolo Abeni <pabeni@...hat.com>
To:     Kuniyuki Iwashima <kuniyu@...zon.com>
Cc:     davem@...emloft.net, dsahern@...nel.org, edumazet@...gle.com,
        kuba@...nel.org, mathew.j.martineau@...ux.intel.com,
        matthieu.baerts@...sares.net, mptcp@...ts.linux.dev,
        netdev@...r.kernel.org
Subject: Re: [PATCH net-next 2/2] udp: track the forward memory release
 threshold in an hot cacheline

On Wed, 2022-10-19 at 09:33 -0700, Kuniyuki Iwashima wrote:
> From:   Paolo Abeni <pabeni@...hat.com>
> Date:   Wed, 19 Oct 2022 12:02:01 +0200
> > When the receiver process and the BH runs on different cores,
> > udp_rmem_release() experience a cache miss while accessing sk_rcvbuf,
> > as the latter shares the same cacheline with sk_forward_alloc, written
> > by the BH.
> > 
> > With this patch, UDP tracks the rcvbuf value and its update via custom
> > SOL_SOCKET socket options, and copies the forward memory threshold value
> > used by udp_rmem_release() in a different cacheline, already accessed by
> > the above function and uncontended.
> > 
> > Overall the above give a 10% peek throughput increase under UDP flood.
> > 
> > Signed-off-by: Paolo Abeni <pabeni@...hat.com>
> > ---
> >  include/linux/udp.h |  3 +++
> >  net/ipv4/udp.c      | 22 +++++++++++++++++++---
> >  net/ipv6/udp.c      |  8 ++++++--
> >  3 files changed, 28 insertions(+), 5 deletions(-)
> > 
> > diff --git a/include/linux/udp.h b/include/linux/udp.h
> > index e96da4157d04..5cdba00a904a 100644
> > --- a/include/linux/udp.h
> > +++ b/include/linux/udp.h
> > @@ -87,6 +87,9 @@ struct udp_sock {
> >  
> >  	/* This field is dirtied by udp_recvmsg() */
> >  	int		forward_deficit;
> > +
> > +	/* This fields follows rcvbuf value, and is touched by udp_recvmsg */
> > +	int		forward_threshold;
> >  };
> >  
> >  #define UDP_MAX_SEGMENTS	(1 << 6UL)
> > diff --git a/net/ipv4/udp.c b/net/ipv4/udp.c
> > index 8126f67d18b3..915f573587fa 100644
> > --- a/net/ipv4/udp.c
> > +++ b/net/ipv4/udp.c
> > @@ -1448,7 +1448,7 @@ static void udp_rmem_release(struct sock *sk, int size, int partial,
> >  	if (likely(partial)) {
> >  		up->forward_deficit += size;
> >  		size = up->forward_deficit;
> > -		if (size < (sk->sk_rcvbuf >> 2) &&
> > +		if (size < READ_ONCE(up->forward_threshold) &&
> >  		    !skb_queue_empty(&up->reader_queue))
> >  			return;
> >  	} else {
> > @@ -1622,8 +1622,12 @@ static void udp_destruct_sock(struct sock *sk)
> >  
> >  int udp_init_sock(struct sock *sk)
> >  {
> > -	skb_queue_head_init(&udp_sk(sk)->reader_queue);
> > +	struct udp_sock *up = udp_sk(sk);
> > +
> > +	skb_queue_head_init(&up->reader_queue);
> > +	up->forward_threshold = sk->sk_rcvbuf >> 2;
> >  	sk->sk_destruct = udp_destruct_sock;
> > +	set_bit(SOCK_CUSTOM_SOCKOPT, &sk->sk_socket->flags);
> >  	return 0;
> >  }
> >  
> > @@ -2671,6 +2675,18 @@ int udp_lib_setsockopt(struct sock *sk, int level, int optname,
> >  	int err = 0;
> >  	int is_udplite = IS_UDPLITE(sk);
> >  
> > +	if (level == SOL_SOCKET) {
> > +		err = sk_setsockopt(sk, level, optname, optval, optlen);
> > +
> > +		if (optname == SO_RCVBUF || optname == SO_RCVBUFFORCE) {
> > +			sockopt_lock_sock(sk);
> 
> Can we drop this lock by adding READ_ONCE() to sk->sk_rcvbuf below ?

I think we can't. If there are racing thread updating rcvbuf, we could
end-up with mismatching value in forward_threshold. Not a likely
scenario, but still... This is control path, acquiring the lock once
more should not be a problem.

> > +			/* paired with READ_ONCE in udp_rmem_release() */
> > +			WRITE_ONCE(up->forward_threshold, sk->sk_rcvbuf >> 2);
> > +			sockopt_release_sock(sk);
> > +		}
> > +		return err;
> > +	}
> > +
> >  	if (optlen < sizeof(int))
> >  		return -EINVAL;
> >  
> > @@ -2784,7 +2800,7 @@ EXPORT_SYMBOL(udp_lib_setsockopt);
> >  int udp_setsockopt(struct sock *sk, int level, int optname, sockptr_t optval,
> >  		   unsigned int optlen)
> >  {
> > -	if (level == SOL_UDP  ||  level == SOL_UDPLITE)
> > +	if (level == SOL_UDP  ||  level == SOL_UDPLITE || level == SOL_SOCKET)
> >  		return udp_lib_setsockopt(sk, level, optname,
> >  					  optval, optlen,
> >  					  udp_push_pending_frames);
> > diff --git a/net/ipv6/udp.c b/net/ipv6/udp.c
> > index 8d09f0ea5b8c..1ed20bcfd7a0 100644
> > --- a/net/ipv6/udp.c
> > +++ b/net/ipv6/udp.c
> > @@ -64,8 +64,12 @@ static void udpv6_destruct_sock(struct sock *sk)
> >  
> >  int udpv6_init_sock(struct sock *sk)
> >  {
> > -	skb_queue_head_init(&udp_sk(sk)->reader_queue);
> > +	struct udp_sock *up = udp_sk(sk);
> > +
> > +	skb_queue_head_init(&up->reader_queue);
> > +	up->forward_threshold = sk->sk_rcvbuf >> 2;
> >  	sk->sk_destruct = udpv6_destruct_sock;
> > +	set_bit(SOCK_CUSTOM_SOCKOPT, &sk->sk_socket->flags);
> >  	return 0;
> >  }
> 
> It's time to factorise this part like udp_destruct_common() ?

I guess it makes sense. Possibly 'udp_lib_destruct()' just to follow
others helper style?

> 
Thanks,

Paolo