Message-ID: <CANn89iJS0Lab6iSM0=7+WxAb5dr0TpRDjy+5F_+pVJH1UsTw3A@mail.gmail.com>
Date:   Sat, 1 Oct 2022 15:50:18 -0700
From:   Eric Dumazet <edumazet@...gle.com>
To:     "Jason A. Donenfeld" <Jason@...c4.com>
Cc:     Eric Dumazet <eric.dumazet@...il.com>,
        "David S . Miller" <davem@...emloft.net>,
        Jakub Kicinski <kuba@...nel.org>,
        Paolo Abeni <pabeni@...hat.com>,
        netdev <netdev@...r.kernel.org>,
        Christophe Leroy <christophe.leroy@...roup.eu>,
        Willy Tarreau <w@....eu>
Subject: Re: [PATCH net-next] once: add DO_ONCE_SLOW() for sleepable contexts

On Sat, Oct 1, 2022 at 3:44 PM Jason A. Donenfeld <Jason@...c4.com> wrote:
>
> On Sat, Oct 01, 2022 at 01:51:02PM -0700, Eric Dumazet wrote:
> > From: Eric Dumazet <edumazet@...gle.com>
> >
> > Christophe Leroy reported a ~80ms latency spike
> > happening at first TCP connect() time.
> >
> > This is because __inet_hash_connect() uses get_random_once()
> > to populate a perturbation table which became quite big
> > after commit 4c2c8f03a5ab ("tcp: increase source port perturb table to 2^16")
> >
> > get_random_once() uses DO_ONCE(), which block hard irqs for the duration
> > of the operation.
> >
> > This patch adds DO_ONCE_SLOW() which uses a mutex instead of a spinlock
> > for operations where we prefer to stay in process context.
> >
> > Then __inet_hash_connect() can use get_random_slow_once()
> > to populate its perturbation table.
> >
> > Fixes: 4c2c8f03a5ab ("tcp: increase source port perturb table to 2^16")
> > Fixes: 190cc82489f4 ("tcp: change source port randomizarion at connect() time")
> > Reported-by: Christophe Leroy <christophe.leroy@...roup.eu>
> > Link: https://lore.kernel.org/netdev/CANn89iLAEYBaoYajy0Y9UmGFff5GPxDUoG-ErVB2jDdRNQ5Tug@mail.gmail.com/T/#t
> > Signed-off-by: Eric Dumazet <edumazet@...gle.com>
> > Cc: Willy Tarreau <w@....eu>
> > ---
> >  include/linux/once.h       | 28 ++++++++++++++++++++++++++++
> >  lib/once.c                 | 30 ++++++++++++++++++++++++++++++
> >  net/ipv4/inet_hashtables.c |  4 ++--
> >  3 files changed, 60 insertions(+), 2 deletions(-)
> >
> > diff --git a/include/linux/once.h b/include/linux/once.h
> > index b14d8b309d52b198bb144689fe67d9ed235c2b3e..176ab75b42df740a738d04d8480821a0b3b65ba9 100644
> > --- a/include/linux/once.h
> > +++ b/include/linux/once.h
> > @@ -5,10 +5,18 @@
> >  #include <linux/types.h>
> >  #include <linux/jump_label.h>
> >
> > +/* Helpers used from arbitrary contexts.
> > + * Hard irqs are blocked, be cautious.
> > + */
> >  bool __do_once_start(bool *done, unsigned long *flags);
> >  void __do_once_done(bool *done, struct static_key_true *once_key,
> >                   unsigned long *flags, struct module *mod);
> >
> > +/* Variant for process contexts only. */
> > +bool __do_once_slow_start(bool *done);
> > +void __do_once_slow_done(bool *done, struct static_key_true *once_key,
> > +                      struct module *mod);
> > +
> >  /* Call a function exactly once. The idea of DO_ONCE() is to perform
> >   * a function call such as initialization of random seeds, etc, only
> >   * once, where DO_ONCE() can live in the fast-path. After @func has
> > @@ -52,7 +60,27 @@ void __do_once_done(bool *done, struct static_key_true *once_key,
> >               ___ret;                                                      \
> >       })
> >
> > +/* Variant of DO_ONCE() for process/sleepable contexts. */
> > +#define DO_ONCE_SLOW(func, ...)                                                   \
> > +     ({                                                                   \
> > +             bool ___ret = false;                                         \
> > +             static bool __section(".data.once") ___done = false;         \
> > +             static DEFINE_STATIC_KEY_TRUE(___once_key);                  \
> > +             if (static_branch_unlikely(&___once_key)) {                  \
> > +                     ___ret = __do_once_slow_start(&___done);             \
> > +                     if (unlikely(___ret)) {                              \
> > +                             func(__VA_ARGS__);                           \
> > +                             __do_once_slow_done(&___done, &___once_key,  \
> > +                                                 THIS_MODULE);            \
> > +                     }                                                    \
> > +             }                                                            \
> > +             ___ret;                                                      \
> > +     })
> > +
>
> Hmm, I dunno about this macro-choice explosion here. The whole thing
> with DO_ONCE() is that the static branch makes it zero cost most of the
> time while being somewhat expensive the rest of the time, but who cares,
> because "the rest" is just once.
>
> So instead, why not just branch on whether or not we can sleep here, if
> that can be worked out dynamically? If not, and if you really do need
> two sets of macros and functions, at least you can call the new one
> something other than "slow"? Maybe something about being _SLEEPABLE()
> instead?

No idea what you mean. I do not want to over engineer code that yet have to be
adopted by other callers. If you think you want to spend week end time on this,
feel free to take over at this point.

>
> Also, the __do_once_slow_done() function misses a really nice
> optimization, which is that the static branch can be changed
> synchronously instead of having to allocate and fire off that workqueue,
> since by definition we're in sleepable context here.
>

This was deliberate. We already spent a lot of time in the called function,
better just return to the caller as fast as possible.

This really does not matter, the work queue is fired once by definition.

Powered by blists - more mailing lists