Messages in this thread | | | From | Ivan Babrou <> | Date | Mon, 23 Oct 2023 16:22:35 -0700 | Subject | Re: wait_for_unix_gc can cause CPU overload for well behaved programs |
| |
On Fri, Oct 20, 2023 at 6:23 PM Hillf Danton <hdanton@sina.com> wrote: > > On Fri, 20 Oct 2023 10:25:25 -0700 Ivan Babrou <ivan@cloudflare.com> > > > > This could solve wait_for_unix_gc spinning, but it wouldn't affect > > unix_gc itself, from what I understand. There would always be one > > socket writer or destroyer punished by running the gc still. > > See what you want. The innocents are rescued by kicking a worker off. > Only for thoughts. > > --- x/net/unix/garbage.c > +++ y/net/unix/garbage.c > @@ -86,7 +86,6 @@ > /* Internal data structures and random procedures: */ > > static LIST_HEAD(gc_candidates); > -static DECLARE_WAIT_QUEUE_HEAD(unix_gc_wait); > > static void scan_inflight(struct sock *x, void (*func)(struct unix_sock *), > struct sk_buff_head *hitlist) > @@ -185,24 +184,25 @@ static void inc_inflight_move_tail(struc > list_move_tail(&u->link, &gc_candidates); > } > > -static bool gc_in_progress; > +static void __unix_gc(struct work_struct *w); > +static DECLARE_WORK(unix_gc_work, __unix_gc); > + > #define UNIX_INFLIGHT_TRIGGER_GC 16000 > > 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); > + * kick a garbage collect right now. > + * > + * todo s/wait_for_unix_gc/kick_unix_gc/ > + */ > + if (READ_ONCE(unix_tot_inflight) > UNIX_INFLIGHT_TRIGGER_GC /2) > + queue_work(system_unbound_wq, &unix_gc_work); > } > > -/* The external entry point: unix_gc() */ > -void unix_gc(void) > +static DEFINE_MUTEX(unix_gc_mutex); > + > +static void __unix_gc(struct work_struct *w) > { > struct sk_buff *next_skb, *skb; > struct unix_sock *u; > @@ -211,15 +211,10 @@ void unix_gc(void) > struct list_head cursor; > LIST_HEAD(not_cycle_list); > > + if (!mutex_trylock(&unix_gc_mutex)) > + return; > 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,12 @@ 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); > - > - wake_up(&unix_gc_wait); > - > - out: > spin_unlock(&unix_gc_lock); > + mutex_unlock(&unix_gc_mutex); > +} > + > +/* The external entry point: unix_gc() */ > +void unix_gc(void) > +{ > + __unix_gc(NULL); > } > --
This one results in less overall load than Kuniyuki's proposed patch with my repro:
* https://lore.kernel.org/netdev/20231020220511.45854-1-kuniyu@amazon.com/
My guess is that's because my repro is the one that is getting penalized there.
There's still a lot work done in unix_release_sock here, where GC runs as long as you have any fds inflight:
* https://elixir.bootlin.com/linux/v6.1/source/net/unix/af_unix.c#L670
Perhaps it can be improved.
| |