With debugobjects enabled the timer hint for freeing of active timers
embedded inside delayed works is always the same, i.e. the hint is
delayed_work_timer_fn(), even though the function the delayed work is
going to run can be wildly different depending on what work was
scheduled. Enabling workqueue debugobjects doesn't help either because
the delayed work isn't considered active until it is actually queued to
run on a workqueue. That's because if the work is freed while the timer
is pending the work isn't considered active to debugobjects so we don't
get any information about freeing an active work.
Provide better information here by special casing delayed works in the
timer debugobjects hint logic so that the work function is returned
instead of the timer function delayed_work_timer_fn(). This will help us
understand what delayed work was pending that got freed, leading to
faster bug resolutions.
Cc: Tejun Heo <[email protected]>
Cc: Lai Jiangshan <[email protected]>
Cc: Guenter Roeck <[email protected]>
Signed-off-by: Stephen Boyd <[email protected]>
---
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.
kernel/time/timer.c | 13 ++++++++++++-
1 file changed, 12 insertions(+), 1 deletion(-)
diff --git a/kernel/time/timer.c b/kernel/time/timer.c
index 9dd2a39cb3b0..7b3c1019835c 100644
--- a/kernel/time/timer.c
+++ b/kernel/time/timer.c
@@ -44,6 +44,7 @@
#include <linux/slab.h>
#include <linux/compat.h>
#include <linux/random.h>
+#include <linux/workqueue.h>
#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;
+ }
+
+ return timer->function;
}
static bool timer_is_static_object(void *addr)
base-commit: 672c0c5173427e6b3e2a9bbb7be51ceeec78093a
--
https://chromeos.dev
On Wed, May 04 2022 at 15:31, Stephen Boyd wrote:
> Provide better information here by special casing delayed works in the
> timer debugobjects hint logic so that the work function is returned
> instead of the timer function delayed_work_timer_fn(). This will help us
> understand what delayed work was pending that got freed, leading to
> faster bug resolutions.
Makes sense.
> ---
> 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.
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;
+ }
+ return timer->function;
}
static bool timer_is_static_object(void *addr)
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.
On Wed, May 11 2022 at 13:02, Stephen Boyd wrote:
> Quoting Thomas Gleixner (2022-05-10 02:20:01)
>> 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;
That's why I said: "So maybe something like the uncompiled/untested
below."
I was pretty sure that I missed some nasty detail.
> I'll send this version of the patch.
Appreciated.
Thanks,
tglx