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] [day] [month] [year] [list]
Date:   Wed, 19 Oct 2022 10:09:40 -0700
From:   Kuniyuki Iwashima <kuniyu@...zon.com>
To:     <pabeni@...hat.com>
CC:     <davem@...emloft.net>, <dsahern@...nel.org>, <edumazet@...gle.com>,
        <kuba@...nel.org>, <kuniyu@...zon.com>,
        <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

From:   Paolo Abeni <pabeni@...hat.com>
Date:   Wed, 19 Oct 2022 18:58:33 +0200
> 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.

I see.
Thank you!


> > > +			/* 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?

Ah, I should have named it so :)


> 
> > 
> Thanks,
> 
> Paolo