Re: [RFC][PATCH v2 20/31] timers: usb: Use del_timer_shutdown() before freeing timer
From: Steven Rostedt
Date: Fri Oct 28 2022 - 14:01:22 EST
On Thu, 27 Oct 2022 22:23:06 -0700
Guenter Roeck <linux@xxxxxxxxxxxx> wrote:
> > index bbab424b0d55..397f263ab7da 100644
> > --- a/drivers/usb/core/hub.c
> > +++ b/drivers/usb/core/hub.c
> > @@ -1261,6 +1261,9 @@ static void hub_activate(struct usb_hub *hub, enum hub_activation_type type)
> >
> > /* Don't do a long sleep inside a workqueue routine */
> > if (type == HUB_INIT2) {
> > + /* Timers must be shutdown before they are re-initialized */
> > + if (hub->init_work.work.func)
> > + del_timer_shutdown(&hub->init_work.timer);
> > INIT_DELAYED_WORK(&hub->init_work, hub_init_func3);
>
> A similar call to INIT_DELAYED_WORK() around line 1085 needs the same change.
>
> It would be great if that can somehow be hidden in INIT_DELAYED_WORK().
I've decided to treat INIT_DELAYED_WORK() like it was before. It only
checks from the time the timer is added to the time it is removed without
needing a shutdown call. That's because there's no API in the workqueue
code that allows for us to require a shutdown on the INIT_DELAYED_WORK's
timer.
Guenter,
Can you remove all the extra patches that touched the timer.h and timer.c
code, and replace the last patch with this, and then try again?
-- Steve
include/linux/timer.h | 38 +++++++++++++++++++++++++++--
include/linux/workqueue.h | 4 ++--
kernel/time/timer.c | 50 ++++++++++++++++++++++++++++++++++-----
kernel/workqueue.c | 12 ++++++++++
4 files changed, 94 insertions(+), 10 deletions(-)
diff --git a/include/linux/timer.h b/include/linux/timer.h
index 45392b0ac2e1..27e3a8676ff8 100644
--- a/include/linux/timer.h
+++ b/include/linux/timer.h
@@ -8,6 +8,12 @@
#include <linux/debugobjects.h>
#include <linux/stringify.h>
+enum timer_debug_state {
+ TIMER_DEBUG_DISABLED,
+ TIMER_DEBUG_ENABLED,
+ TIMER_DEBUG_WORK,
+};
+
struct timer_list {
/*
* All fields that change during normal runtime grouped to the
@@ -18,6 +24,9 @@ struct timer_list {
void (*function)(struct timer_list *);
u32 flags;
+#ifdef CONFIG_DEBUG_OBJECTS_TIMERS
+ enum timer_debug_state enabled;
+#endif
#ifdef CONFIG_LOCKDEP
struct lockdep_map lockdep_map;
#endif
@@ -128,6 +137,31 @@ static inline void init_timer_on_stack_key(struct timer_list *timer,
init_timer_on_stack_key((_timer), (_fn), (_flags), NULL, NULL)
#endif
+#ifdef CONFIG_DEBUG_OBJECTS_TIMERS
+#define __init_timer_debug(_timer, _fn, _flags) \
+ do { \
+ (_timer)->enabled = TIMER_DEBUG_DISABLED; \
+ __init_timer((_timer), (_fn), (_flags)); \
+ } while (0)
+#define __init_timer_work(_timer, _fn, _flags) \
+ do { \
+ (_timer)->enabled = TIMER_DEBUG_WORK; \
+ __init_timer((_timer), (_fn), (_flags)); \
+ } while (0)
+#define __init_timer_work_on_stack(_timer, _fn, _flags) \
+ do { \
+ (_timer)->enabled = TIMER_DEBUG_WORK; \
+ __init_timer_on_stack((_timer), (_fn), (_flags)); \
+ } while (0)
+#else
+#define __init_timer_debug(_timer, _fn, _flags) \
+ __init_timer((_timer), (_fn), (_flags))
+#define __init_timer_work(_timer, _fn, _flags) \
+ __init_timer((_timer), (_fn), (_flags))
+#define __init_timer_work_on_stack(_timer, _fn, _flags) \
+ __init_timer_on_stack((_timer), (_fn), (_flags))
+#endif
+
/**
* timer_setup - prepare a timer for first use
* @timer: the timer in question
@@ -139,7 +173,7 @@ static inline void init_timer_on_stack_key(struct timer_list *timer,
* be used and must be balanced with a call to destroy_timer_on_stack().
*/
#define timer_setup(timer, callback, flags) \
- __init_timer((timer), (callback), (flags))
+ __init_timer_debug((timer), (callback), (flags))
#define timer_setup_on_stack(timer, callback, flags) \
__init_timer_on_stack((timer), (callback), (flags))
@@ -243,7 +277,7 @@ static inline int del_timer_shutdown(struct timer_list *timer)
return __del_timer_sync(timer, true);
}
-#define del_singleshot_timer_sync(t) del_timer_sync(t)
+#define del_singleshot_timer_sync(t) del_timer_shutdown(t)
extern void init_timers(void);
struct hrtimer;
diff --git a/include/linux/workqueue.h b/include/linux/workqueue.h
index a0143dd24430..290c96429ce1 100644
--- a/include/linux/workqueue.h
+++ b/include/linux/workqueue.h
@@ -250,7 +250,7 @@ static inline unsigned int work_static(struct work_struct *work) { return 0; }
#define __INIT_DELAYED_WORK(_work, _func, _tflags) \
do { \
INIT_WORK(&(_work)->work, (_func)); \
- __init_timer(&(_work)->timer, \
+ __init_timer_work(&(_work)->timer, \
delayed_work_timer_fn, \
(_tflags) | TIMER_IRQSAFE); \
} while (0)
@@ -258,7 +258,7 @@ static inline unsigned int work_static(struct work_struct *work) { return 0; }
#define __INIT_DELAYED_WORK_ONSTACK(_work, _func, _tflags) \
do { \
INIT_WORK_ONSTACK(&(_work)->work, (_func)); \
- __init_timer_on_stack(&(_work)->timer, \
+ __init_timer_work_on_stack(&(_work)->timer, \
delayed_work_timer_fn, \
(_tflags) | TIMER_IRQSAFE); \
} while (0)
diff --git a/kernel/time/timer.c b/kernel/time/timer.c
index 5179ac2335a0..9a921843cc4f 100644
--- a/kernel/time/timer.c
+++ b/kernel/time/timer.c
@@ -691,7 +691,11 @@ static bool timer_fixup_init(void *addr, enum debug_obj_state state)
switch (state) {
case ODEBUG_STATE_ACTIVE:
- del_timer_sync(timer);
+ if (timer->enabled != TIMER_DEBUG_WORK)
+ timer->enabled = TIMER_DEBUG_ENABLED;
+ del_timer_shutdown(timer);
+ if (timer->enabled != TIMER_DEBUG_WORK)
+ timer->enabled = TIMER_DEBUG_DISABLED;
debug_object_init(timer, &timer_debug_descr);
return true;
default:
@@ -737,8 +741,10 @@ static bool timer_fixup_free(void *addr, enum debug_obj_state state)
switch (state) {
case ODEBUG_STATE_ACTIVE:
- del_timer_sync(timer);
+ del_timer_shutdown(timer);
debug_object_free(timer, &timer_debug_descr);
+ if (timer->enabled != TIMER_DEBUG_WORK)
+ timer->enabled = TIMER_DEBUG_DISABLED;
return true;
default:
return false;
@@ -774,16 +780,36 @@ static const struct debug_obj_descr timer_debug_descr = {
static inline void debug_timer_init(struct timer_list *timer)
{
+ if (timer->enabled == TIMER_DEBUG_ENABLED)
+ return;
+
debug_object_init(timer, &timer_debug_descr);
}
static inline void debug_timer_activate(struct timer_list *timer)
{
+ if (timer->enabled == TIMER_DEBUG_ENABLED)
+ return;
+
+ if (timer->enabled == TIMER_DEBUG_DISABLED)
+ timer->enabled = TIMER_DEBUG_ENABLED;
+
debug_object_activate(timer, &timer_debug_descr);
}
-static inline void debug_timer_deactivate(struct timer_list *timer)
+static inline void debug_timer_deactivate(struct timer_list *timer, bool free)
{
+ switch (timer->enabled) {
+ case TIMER_DEBUG_DISABLED:
+ return;
+ case TIMER_DEBUG_ENABLED:
+ if (!free)
+ return;
+ timer->enabled = TIMER_DEBUG_DISABLED;
+ break;
+ case TIMER_DEBUG_WORK:
+ break;
+ }
debug_object_deactivate(timer, &timer_debug_descr);
}
@@ -813,6 +839,14 @@ void destroy_timer_on_stack(struct timer_list *timer)
}
EXPORT_SYMBOL_GPL(destroy_timer_on_stack);
+static struct timer_base *lock_timer_base(struct timer_list *timer,
+ unsigned long *flags);
+
+void __timer_reinit_debug_objects(struct timer_list *timer)
+{
+ return;
+}
+
#else
static inline void debug_timer_init(struct timer_list *timer) { }
static inline void debug_timer_activate(struct timer_list *timer) { }
@@ -828,7 +862,7 @@ static inline void debug_init(struct timer_list *timer)
static inline void debug_deactivate(struct timer_list *timer)
{
- debug_timer_deactivate(timer);
+ debug_timer_deactivate(timer, false);
trace_timer_cancel(timer);
}
@@ -1251,8 +1285,10 @@ int __del_timer(struct timer_list *timer, bool free)
if (timer_pending(timer)) {
base = lock_timer_base(timer, &flags);
ret = detach_if_pending(timer, base, true);
- if (free && ret)
+ if (free) {
timer->function = NULL;
+ debug_timer_deactivate(timer, true);
+ }
raw_spin_unlock_irqrestore(&base->lock, flags);
}
@@ -1272,8 +1308,10 @@ static int __try_to_del_timer_sync(struct timer_list *timer, bool free)
if (base->running_timer != timer)
ret = detach_if_pending(timer, base, true);
- if (free)
+ if (free) {
timer->function = NULL;
+ debug_timer_deactivate(timer, true);
+ }
raw_spin_unlock_irqrestore(&base->lock, flags);
diff --git a/kernel/workqueue.c b/kernel/workqueue.c
index 47a7124bbea4..9a48213fc4e4 100644
--- a/kernel/workqueue.c
+++ b/kernel/workqueue.c
@@ -1225,6 +1225,16 @@ static void pwq_dec_nr_in_flight(struct pool_workqueue *pwq, unsigned long work_
put_pwq(pwq);
}
+static void deactivate_timer(struct work_struct *work, bool is_dwork)
+{
+ struct delayed_work *dwork;
+
+ if (!is_dwork)
+ return;
+
+ dwork = to_delayed_work(work);
+}
+
/**
* try_to_grab_pending - steal work item from worklist and disable irq
* @work: work item to steal
@@ -3148,6 +3158,8 @@ static bool __cancel_work_timer(struct work_struct *work, bool is_dwork)
}
} while (unlikely(ret < 0));
+ deactivate_timer(work, is_dwork);
+
/* tell other tasks trying to grab @work to back off */
mark_work_canceling(work);
local_irq_restore(flags);
--
2.35.1