Re: [PATCH v2 net-next 4/4] af_unix: Try to run GC async.

[Kuniyuki Iwashima <kuniyu@amazon.com>]

From: Paolo Abeni <pabeni@redhat.com>
Date: Mon, 27 Nov 2023 10:59:03 +0100
> On Wed, 2023-11-22 at 17:47 -0800, Kuniyuki Iwashima wrote:
> > If more than 16000 inflight AF_UNIX sockets exist and the garbage
> > collector is not running, unix_(dgram|stream)_sendmsg() call unix_gc().
> > Also, they wait for unix_gc() to complete.
> > 
> > In unix_gc(), all inflight AF_UNIX sockets are traversed at least once,
> > and more if they are the GC candidate.  Thus, sendmsg() significantly
> > slows down with too many inflight AF_UNIX sockets.
> > 
> > However, if a process sends data with no AF_UNIX FD, the sendmsg() call
> > does not need to wait for GC.  After this change, only the process that
> > meets the condition below will be blocked under such a situation.
> > 
> >   1) cmsg contains AF_UNIX socket
> >   2) more than 32 AF_UNIX sent by the same user are still inflight
> > 
> > Note that even a sendmsg() call that does not meet the condition but has
> > AF_UNIX FD will be blocked later in unix_scm_to_skb() by the spinlock,
> > but we allow that as a bonus for sane users.
> > 
> > The results below are the time spent in unix_dgram_sendmsg() sending 1
> > byte of data with no FD 4096 times on a host where 32K inflight AF_UNIX
> > sockets exist.
> > 
> > Without series: the sane sendmsg() needs to wait gc unreasonably.
> > 
> >   $ sudo /usr/share/bcc/tools/funclatency -p 11165 unix_dgram_sendmsg
> >   Tracing 1 functions for "unix_dgram_sendmsg"... Hit Ctrl-C to end.
> >   ^C
> >        nsecs               : count     distribution
> >   [...]
> >       524288 -> 1048575    : 0        |                                        |
> >      1048576 -> 2097151    : 3881     |****************************************|
> >      2097152 -> 4194303    : 214      |**                                      |
> >      4194304 -> 8388607    : 1        |                                        |
> > 
> >   avg = 1825567 nsecs, total: 7477526027 nsecs, count: 4096
> > 
> > With series: the sane sendmsg() can finish much faster.
> > 
> >   $ sudo /usr/share/bcc/tools/funclatency -p 8702  unix_dgram_sendmsg
> >   Tracing 1 functions for "unix_dgram_sendmsg"... Hit Ctrl-C to end.
> >   ^C
> >        nsecs               : count     distribution
> >   [...]
> >          128 -> 255        : 0        |                                        |
> >          256 -> 511        : 4092     |****************************************|
> >          512 -> 1023       : 2        |                                        |
> >         1024 -> 2047       : 0        |                                        |
> >         2048 -> 4095       : 0        |                                        |
> >         4096 -> 8191       : 1        |                                        |
> >         8192 -> 16383      : 1        |                                        |
> > 
> >   avg = 410 nsecs, total: 1680510 nsecs, count: 4096
> > 
> > Signed-off-by: Kuniyuki Iwashima <kuniyu@amazon.com>
> > ---
> >  include/net/af_unix.h |  2 +-
> >  include/net/scm.h     |  1 +
> >  net/core/scm.c        |  5 +++++
> >  net/unix/af_unix.c    |  6 ++++--
> >  net/unix/garbage.c    | 10 ++++++++--
> >  5 files changed, 19 insertions(+), 5 deletions(-)
> > 
> > diff --git a/include/net/af_unix.h b/include/net/af_unix.h
> > index c628d30ceb19..f8e654d418e6 100644
> > --- a/include/net/af_unix.h
> > +++ b/include/net/af_unix.h
> > @@ -13,7 +13,7 @@ void unix_notinflight(struct user_struct *user, struct file *fp);
> >  void unix_destruct_scm(struct sk_buff *skb);
> >  void io_uring_destruct_scm(struct sk_buff *skb);
> >  void unix_gc(void);
> > -void wait_for_unix_gc(void);
> > +void wait_for_unix_gc(struct scm_fp_list *fpl);
> >  struct unix_sock *unix_get_socket(struct file *filp);
> >  struct sock *unix_peer_get(struct sock *sk);
> >  
> > diff --git a/include/net/scm.h b/include/net/scm.h
> > index e8c76b4be2fe..1ff6a2855064 100644
> > --- a/include/net/scm.h
> > +++ b/include/net/scm.h
> > @@ -24,6 +24,7 @@ struct scm_creds {
> >  
> >  struct scm_fp_list {
> >  	short			count;
> > +	short			count_unix;
> >  	short			max;
> >  	struct user_struct	*user;
> >  	struct file		*fp[SCM_MAX_FD];
> > diff --git a/net/core/scm.c b/net/core/scm.c
> > index 880027ecf516..c1aae77d120b 100644
> > --- a/net/core/scm.c
> > +++ b/net/core/scm.c
> > @@ -35,6 +35,7 @@
> >  #include <net/compat.h>
> >  #include <net/scm.h>
> >  #include <net/cls_cgroup.h>
> > +#include <net/af_unix.h>
> >  
> >  
> >  /*
> > @@ -105,6 +106,10 @@ static int scm_fp_copy(struct cmsghdr *cmsg, struct scm_fp_list **fplp)
> >  			return -EBADF;
> >  		*fpp++ = file;
> >  		fpl->count++;
> > +#if IS_ENABLED(CONFIG_UNIX)
> > +		if (unix_get_socket(file))
> > +			fpl->count_unix++;
> > +#endif
> >  	}
> >  
> >  	if (!fpl->user)
> > diff --git a/net/unix/af_unix.c b/net/unix/af_unix.c
> > index 1e6f5aaf1cc9..bbad3959751d 100644
> > --- a/net/unix/af_unix.c
> > +++ b/net/unix/af_unix.c
> > @@ -1925,11 +1925,12 @@ static int unix_dgram_sendmsg(struct socket *sock, struct msghdr *msg,
> >  	long timeo;
> >  	int err;
> >  
> > -	wait_for_unix_gc();
> >  	err = scm_send(sock, msg, &scm, false);
> >  	if (err < 0)
> >  		return err;
> >  
> > +	wait_for_unix_gc(scm.fp);
> > +
> >  	err = -EOPNOTSUPP;
> >  	if (msg->msg_flags&MSG_OOB)
> >  		goto out;
> > @@ -2201,11 +2202,12 @@ static int unix_stream_sendmsg(struct socket *sock, struct msghdr *msg,
> >  	bool fds_sent = false;
> >  	int data_len;
> >  
> > -	wait_for_unix_gc();
> >  	err = scm_send(sock, msg, &scm, false);
> >  	if (err < 0)
> >  		return err;
> >  
> > +	wait_for_unix_gc(scm.fp);
> > +
> >  	err = -EOPNOTSUPP;
> >  	if (msg->msg_flags & MSG_OOB) {
> >  #if IS_ENABLED(CONFIG_AF_UNIX_OOB)
> > diff --git a/net/unix/garbage.c b/net/unix/garbage.c
> > index 8bc93a7e745f..73091d6b7fc4 100644
> > --- a/net/unix/garbage.c
> > +++ b/net/unix/garbage.c
> > @@ -184,8 +184,9 @@ static void inc_inflight_move_tail(struct unix_sock *u)
> >  }
> >  
> >  #define UNIX_INFLIGHT_TRIGGER_GC 16000
> > +#define UNIX_INFLIGHT_SANE_USER 32
> 
> I don't have any relevant usage stats for unix sockets, but out of
> sheer ignorance on my side '32' looks a bit low. Why/how did you pick
> such value?

My take was that the peer should receive the fds in timely manner so that
no one will be punished, but I admit 32 is small enough, which can be
reached by a single SCM_RIGHTS (SCM_MAX_FD == 253) cmsg.  So, probably we
can bump it up to 1024 or 2048 (> (4 or 8) * SCM_MAX_FD).


> > -void wait_for_unix_gc(void)
> > +void wait_for_unix_gc(struct scm_fp_list *fpl)
> >  {
> >  	/* If number of inflight sockets is insane, kick a
> >  	 * garbage collect right now.
> > @@ -195,7 +196,12 @@ void wait_for_unix_gc(void)
> >  	if (READ_ONCE(unix_tot_inflight) > UNIX_INFLIGHT_TRIGGER_GC)
> >  		queue_work(system_unbound_wq, &unix_gc_work);
> >  
> > -	flush_work(&unix_gc_work);
> > +	/* Penalise users who want to send AF_UNIX sockets
> > +	 * but whose sockets have not been received yet.
> > +	 */
> > +	if (fpl && fpl->count_unix &&
> > +	    READ_ONCE(fpl->user->unix_inflight) > UNIX_INFLIGHT_SANE_USER)
> > +		flush_work(&unix_gc_work);
> 
> flush_work() will be called even when 'unix_tot_inflight' is (much)
> less then 'UNIX_INFLIGHT_TRIGGER_GC'. Could that cause some regressions
> for workload with moderated numbers of fd in flights, where the GC was
> never triggered before this series?

Ah exactly, I'll add work_pending() in v3.

	if (!fpl || !fpl->count_unix)
		return

	if (work_pending(&unix_gc_work) &&
	    READ_ONCE(fpl->user->unix_inflight) > UNIX_INFLIGHT_SANE_USER)
		flush_work(&unix_gc_work)

Thanks!