From: Ivan Babrou <ivan@cloudflare.com>
To: Hillf Danton <hdanton@sina.com>
Cc: Linux Kernel Network Developers <netdev@vger.kernel.org>,
kernel-team <kernel-team@cloudflare.com>,
Eric Dumazet <edumazet@google.com>,
Paolo Abeni <pabeni@redhat.com>,
Kuniyuki Iwashima <kuniyu@amazon.com>,
linux-kernel <linux-kernel@vger.kernel.org>
Subject: Re: wait_for_unix_gc can cause CPU overload for well behaved programs
Date: Mon, 23 Oct 2023 16:22:35 -0700 [thread overview]
Message-ID: <CABWYdi0j4yXWV6-Pr=2q7S6SQSZR7O6F61BLRdU=gDxvuQ3e1w@mail.gmail.com> (raw)
In-Reply-To: <20231021012322.1799-1-hdanton@sina.com>
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.
next prev parent reply other threads:[~2023-10-23 23:24 UTC|newest]
Thread overview: 9+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-10-19 22:35 wait_for_unix_gc can cause CPU overload for well behaved programs Ivan Babrou
[not found] ` <20231020104728.2060-1-hdanton@sina.com>
2023-10-20 17:25 ` Ivan Babrou
[not found] ` <20231021012322.1799-1-hdanton@sina.com>
2023-10-23 23:22 ` Ivan Babrou [this message]
2023-10-23 23:45 ` Kuniyuki Iwashima
2023-11-17 23:38 ` Ivan Babrou
2023-11-20 19:29 ` Kuniyuki Iwashima
2023-11-20 22:30 ` Ivan Babrou
2023-11-20 23:53 ` Kuniyuki Iwashima
2023-10-20 22:05 ` Kuniyuki Iwashima
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to='CABWYdi0j4yXWV6-Pr=2q7S6SQSZR7O6F61BLRdU=gDxvuQ3e1w@mail.gmail.com' \
--to=ivan@cloudflare.com \
--cc=edumazet@google.com \
--cc=hdanton@sina.com \
--cc=kernel-team@cloudflare.com \
--cc=kuniyu@amazon.com \
--cc=linux-kernel@vger.kernel.org \
--cc=netdev@vger.kernel.org \
--cc=pabeni@redhat.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).