linux-kernel - Re: [PATCH 2/2] locking: Apply contention tracepoints in the slow path

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

Message-ID: <CAM9d7cjUR6shddKM2h9uFXgQf+0F504fnJmKRSfc3+PG3TmEyg@mail.gmail.com>
Date:   Fri, 18 Mar 2022 14:55:27 -0700
From:   Namhyung Kim <namhyung@...nel.org>
To:     Peter Zijlstra <peterz@...radead.org>
Cc:     Boqun Feng <boqun.feng@...il.com>, Ingo Molnar <mingo@...nel.org>,
        Will Deacon <will@...nel.org>,
        Waiman Long <longman@...hat.com>,
        LKML <linux-kernel@...r.kernel.org>,
        Thomas Gleixner <tglx@...utronix.de>,
        Steven Rostedt <rostedt@...dmis.org>,
        Byungchul Park <byungchul.park@....com>,
        "Paul E. McKenney" <paulmck@...nel.org>,
        Mathieu Desnoyers <mathieu.desnoyers@...icios.com>,
        Arnd Bergmann <arnd@...db.de>,
        Radoslaw Burny <rburny@...gle.com>,
        linux-arch <linux-arch@...r.kernel.org>,
        bpf <bpf@...r.kernel.org>
Subject: Re: [PATCH 2/2] locking: Apply contention tracepoints in the slow path

Hello,

On Fri, Mar 18, 2022 at 9:43 AM Peter Zijlstra <peterz@...radead.org> wrote:
>
> On Fri, Mar 18, 2022 at 08:55:32PM +0800, Boqun Feng wrote:
> > On Wed, Mar 16, 2022 at 03:45:48PM -0700, Namhyung Kim wrote:
> > [...]
> > > @@ -209,6 +210,7 @@ static inline int __sched __down_common(struct semaphore *sem, long state,
> > >                                                             long timeout)
> > >  {
> > >     struct semaphore_waiter waiter;
> > > +   bool tracing = false;
> > >
> > >     list_add_tail(&waiter.list, &sem->wait_list);
> > >     waiter.task = current;
> > > @@ -220,18 +222,28 @@ static inline int __sched __down_common(struct semaphore *sem, long state,
> > >             if (unlikely(timeout <= 0))
> > >                     goto timed_out;
> > >             __set_current_state(state);
> > > +           if (!tracing) {
> > > +                   trace_contention_begin(sem, 0);
> >
> > This looks a littl ugly ;-/ Maybe we can rename __down_common() to
> > ___down_common() and implement __down_common() as:
> >
> >       static inline int __sched __down_common(...)
> >       {
> >               int ret;
> >               trace_contention_begin(sem, 0);
> >               ret = ___down_common(...);
> >               trace_contention_end(sem, ret);
> >               return ret;
> >       }
> >
> > Thoughts?
>
> Yeah, that works, except I think he wants a few extra
> __set_current_state()'s like so:

Not anymore, I decided not to because of noise in the task state.

Also I'm considering two tracepoints for the return path to reduce
the buffer size as Mathieu suggested.  Normally it'd return with 0
so we can ignore it in the contention_end.  For non-zero cases,
we can add a new tracepoint to save the return value.

Thanks,
Namhyung

>
> diff --git a/kernel/locking/semaphore.c b/kernel/locking/semaphore.c
> index 9ee381e4d2a4..e2049a7e0ea4 100644
> --- a/kernel/locking/semaphore.c
> +++ b/kernel/locking/semaphore.c
> @@ -205,8 +205,7 @@ struct semaphore_waiter {
>   * constant, and thus optimised away by the compiler.  Likewise the
>   * 'timeout' parameter for the cases without timeouts.
>   */
> -static inline int __sched __down_common(struct semaphore *sem, long state,
> -                                                               long timeout)
> +static __always_inline int ___down_common(struct semaphore *sem, long state, long timeout)
>  {
>         struct semaphore_waiter waiter;
>
> @@ -227,15 +226,28 @@ static inline int __sched __down_common(struct semaphore *sem, long state,
>                         return 0;
>         }
>
> - timed_out:
> +timed_out:
>         list_del(&waiter.list);
>         return -ETIME;
>
> - interrupted:
> +interrupted:
>         list_del(&waiter.list);
>         return -EINTR;
>  }
>
> +static __always_inline int __down_common(struct semaphore *sem, long state, long timeout)
> +{
> +       int ret;
> +
> +       __set_current_state(state);
> +       trace_contention_begin(sem, 0);
> +       ret = ___down_common(sem, state, timeout);
> +       __set_current_state(TASK_RUNNING);
> +       trace_contention_end(sem, ret);
> +
> +       return ret;
> +}
> +
>  static noinline void __sched __down(struct semaphore *sem)
>  {
>         __down_common(sem, TASK_UNINTERRUPTIBLE, MAX_SCHEDULE_TIMEOUT);