Messages in this thread Patch in this message | | | From | Kuniyuki Iwashima <> | Subject | Re: wait_for_unix_gc can cause CPU overload for well behaved programs | Date | Fri, 20 Oct 2023 15:05:11 -0700 |
| |
From: Ivan Babrou <ivan@cloudflare.com> Date: Thu, 19 Oct 2023 15:35:01 -0700 > Hello, > > We have observed this issue twice (2019 and 2023): a well behaved > service that doesn't pass any file descriptors around starts to spend > a ton of CPU time in wait_for_unix_gc. > > The cause of this is that the unix send path unconditionally calls > wait_for_unix_gc, which is a global garbage collection. If any > misbehaved program exists on a system, it can force extra work for > well behaved programs. > > This behavior is not new: 9915672d4127 ("af_unix: limit > unix_tot_inflight") is from 2010. > > I managed to come up with a repro for this behavior: > > * https://gist.github.com/bobrik/82e5722261920c9f23d9402b88a0bb27 > > It also includes a flamegraph illustrating the issue. It's all in one > program for convenience, but in reality the offender not picking up > SCM_RIGHTS messages and the suffering program just minding its own > business are separate. > > It is also non-trivial to find the offender when this happens as it > can be completely idle while wrecking havoc for the rest of the > system. > > I don't think it's fair to penalize every unix_stream_sendmsg like > this. The 16k threshold also doesn't feel very flexible, surely > computers are bigger these days and can handle more.
Probably we could do the gc async and enforce the penalty only on the offender by checking user->unix_inflight.
compile test only:
---8<--- diff --git a/include/net/af_unix.h b/include/net/af_unix.h index 824c258143a3..a119f37953cc 100644 --- a/include/net/af_unix.h +++ b/include/net/af_unix.h @@ -12,8 +12,9 @@ void unix_inflight(struct user_struct *user, struct file *fp); 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 unix_gc_start(void); +void unix_gc_stop(void); struct sock *unix_get_socket(struct file *filp); struct sock *unix_peer_get(struct sock *sk); diff --git a/net/unix/af_unix.c b/net/unix/af_unix.c index 3e8a04a13668..56db096b13f1 100644 --- a/net/unix/af_unix.c +++ b/net/unix/af_unix.c @@ -681,7 +681,7 @@ static void unix_release_sock(struct sock *sk, int embrion) */ if (READ_ONCE(unix_tot_inflight)) - unix_gc(); /* Garbage collect fds */ + unix_gc_start(); /* Garbage collect fds */ } static void init_peercred(struct sock *sk) @@ -3683,6 +3683,7 @@ static int __init af_unix_init(void) static void __exit af_unix_exit(void) { + unix_gc_stop(); sock_unregister(PF_UNIX); proto_unregister(&unix_dgram_proto); proto_unregister(&unix_stream_proto); diff --git a/net/unix/garbage.c b/net/unix/garbage.c index 2405f0f9af31..fb24d62fe34a 100644 --- a/net/unix/garbage.c +++ b/net/unix/garbage.c @@ -185,24 +185,26 @@ static void inc_inflight_move_tail(struct unix_sock *u) list_move_tail(&u->link, &gc_candidates); } -static bool gc_in_progress; #define UNIX_INFLIGHT_TRIGGER_GC 16000 +static void unix_gc(struct work_struct *work); +static DECLARE_WORK(unix_gc_work, unix_gc); + void wait_for_unix_gc(void) { - /* If number of inflight sockets is insane, - * force a garbage collect right now. - * Paired with the WRITE_ONCE() in unix_inflight(), - * unix_notinflight() and gc_in_progress(). - */ - if (READ_ONCE(unix_tot_inflight) > UNIX_INFLIGHT_TRIGGER_GC && - !READ_ONCE(gc_in_progress)) - unix_gc(); - wait_event(unix_gc_wait, gc_in_progress == false); + struct user_struct *user = get_uid(current_user()); + + if (READ_ONCE(unix_tot_inflight) > UNIX_INFLIGHT_TRIGGER_GC) + schedule_work(&unix_gc_work); + + if (!READ_ONCE(user->unix_inflight)) + return; + + flush_work(&unix_gc_work); } /* The external entry point: unix_gc() */ -void unix_gc(void) +static void unix_gc(struct work_struct *work) { struct sk_buff *next_skb, *skb; struct unix_sock *u; @@ -213,13 +215,6 @@ void unix_gc(void) spin_lock(&unix_gc_lock); - /* Avoid a recursive GC. */ - if (gc_in_progress) - goto out; - - /* Paired with READ_ONCE() in wait_for_unix_gc(). */ - WRITE_ONCE(gc_in_progress, true); - /* First, select candidates for garbage collection. Only * in-flight sockets are considered, and from those only ones * which don't have any external reference. @@ -325,11 +320,15 @@ void unix_gc(void) /* All candidates should have been detached by now. */ BUG_ON(!list_empty(&gc_candidates)); - /* Paired with READ_ONCE() in wait_for_unix_gc(). */ - WRITE_ONCE(gc_in_progress, false); + spin_unlock(&unix_gc_lock); +} - wake_up(&unix_gc_wait); +void unix_gc_start(void) +{ + schedule_work(&unix_gc_work); +} - out: - spin_unlock(&unix_gc_lock); +void __exit unix_gc_stop(void) +{ + flush_work(&unix_gc_work); } ---8<---
| |