linux-kernel - Re: wait_for_unix_gc can cause CPU overload for well behaved programs

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-next>] [day] [month] [year] [list]

Message-ID: <CABWYdi0j4yXWV6-Pr=2q7S6SQSZR7O6F61BLRdU=gDxvuQ3e1w@mail.gmail.com>
Date:   Mon, 23 Oct 2023 16:22:35 -0700
From:   Ivan Babrou <ivan@...udflare.com>
To:     Hillf Danton <hdanton@...a.com>
Cc:     Linux Kernel Network Developers <netdev@...r.kernel.org>,
        kernel-team <kernel-team@...udflare.com>,
        Eric Dumazet <edumazet@...gle.com>,
        Paolo Abeni <pabeni@...hat.com>,
        Kuniyuki Iwashima <kuniyu@...zon.com>,
        linux-kernel <linux-kernel@...r.kernel.org>
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@...a.com> wrote:
>
> On Fri, 20 Oct 2023 10:25:25 -0700 Ivan Babrou <ivan@...udflare.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.