Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
MIME-Version: 1.0
In-Reply-To: <87sfphpwvy.ffs@tglx>
References: <20220504223148.644228-1-swboyd@chromium.org> <87sfphpwvy.ffs@tglx>
From:   Stephen Boyd <swboyd@chromium.org>
User-Agent: alot/0.10
Date:   Wed, 11 May 2022 13:02:09 -0700
Message-ID: <CAE-0n51KTiQjVqJgFe3S9qCiTM+2jdqyVZ1trNO1KanbQJccyA@mail.gmail.com>
Subject: Re: [PATCH] timers: Provide a better debugobjects hint for delayed works
To:     John Stultz <john.stultz@linaro.org>,
        Thomas Gleixner <tglx@linutronix.de>
Cc:     linux-kernel@vger.kernel.org, patches@lists.linux.dev,
        Tejun Heo <tj@kernel.org>,
        Lai Jiangshan <jiangshanlai@gmail.com>,
        Guenter Roeck <groeck@chromium.org>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

Quoting Thomas Gleixner (2022-05-10 02:20:01)
> On Wed, May 04 2022 at 15:31, Stephen Boyd wrote:
> > ---
> > I have an alternative approach which is to treat delayed works with a
> > different debug_obj_descr structure but it basically boils down to
> > another version of timer debugobjects in the workqueue code. The idea is
> > to make the delayed work active once the timer is queued and then
> > convert it over from a delayed work descriptor to a work descriptor once
> > the timer runs delayed_work_timer_fn() or when we pull it off to flush
> > out.
>
> Nah.

:)

>
> >  #include <linux/uaccess.h>
> >  #include <asm/unistd.h>
> > @@ -617,7 +618,17 @@ static const struct debug_obj_descr timer_debug_descr;
> >
> >  static void *timer_debug_hint(void *addr)
> >  {
> > -     return ((struct timer_list *) addr)->function;
> > +     struct timer_list *timer = addr;
> > +
> > +     if (timer->function == delayed_work_timer_fn) {
> > +             struct delayed_work *dwork;
> > +
> > +             dwork = container_of(timer, struct delayed_work, timer);
> > +
> > +             return dwork->work.func;
> > +     }
>
> The same issue exists for kthread_delayed_work_timer_fn.
>
> So maybe something like the uncompiled/untested below.

Cool. Looks good to me. One problem below.

>
> Thanks,
>
>         tglx
> ---
> --- a/kernel/time/timer.c
> +++ b/kernel/time/timer.c
> @@ -638,9 +638,35 @@ static void internal_add_timer(struct ti
>
>  static const struct debug_obj_descr timer_debug_descr;
>
> +struct timer_hint {
> +       void    (*function)(struct timer_list *);
> +       long    offset;
> +};
> +
> +#define TIMER_HINT(fn, container, timr, hintfn)                        \
> +       {                                                       \
> +               .function = fn,                                 \
> +               .offset   = offsetof(container, hintfn) -       \
> +                           offsetof(container, timr)   \
> +       }
> +
> +static const struct timer_hint timer_hints[] = {
> +       TIMER_HINT(delayed_work_timer_fn,
> +                  struct delayed_work, timer, work.func),
> +       TIMER_HINT(kthread_delayed_work_timer_fn,
> +                  struct kthread_delayed_work, timer, work.func),
> +};
> +
>  static void *timer_debug_hint(void *addr)
>  {
> -       return ((struct timer_list *) addr)->function;
> +       struct timer_list *timer = addr;
> +       int i;
> +
> +       for (i = 0; i < ARRAY_SIZE(timer_hints); i++) {
> +               if (timer_hints[i].function == timer->function)
> +                       return addr + timer_hints[i].offset;

This locates the correct address of the function pointer 'work.func' but
it needs to be dereferenced to return the function's address instead of
the pointer to the function. We don't really care about the function
signature so we could cast it to a void function pointer and deref:

                      void (**fn)(void) = addr + timer_hints[i].offset;

		      return *fn;

I'll send this version of the patch.