Message ID | 20221104054053.431922658@goodmis.org (mailing list archive) |
---|---|
Headers | show |
Series | timers: Use timer_shutdown*() before freeing timers | expand |
On Thu, Nov 3, 2022 at 10:48 PM Steven Rostedt <rostedt@goodmis.org> wrote: > > Ideally, I would have the first patch go into this rc cycle, which is mostly > non functional as it will allow the other patches to come in via the respective > subsystems in the next merge window. Ack. I also wonder if we could do the completely trivially correct conversions immediately. I'm talking about the scripted ones where it's currently a "del_timer_sync()", and the very next action is freeing whatever data structure the timer is in (possibly with something like free_irq() in between - my point is that there's an unconditional free that is very clear and unambiguous), so that there is absolutely no question about whether they should use "timer_shutdown_sync()" or not. IOW, things like patches 03, 17 and 31, and at least parts others in this series. This series clearly has several much more complex cases that need actual real code review, and I think it would help to have the completely unambiguous cases out of the way, just to get rid of noise. So I'd take that first patch, and a scripted set of "this cannot change any semantics" patches early. Linus
On Fri, Nov 04, 2022 at 01:40:53AM -0400, Steven Rostedt wrote: > > Back in April, I posted an RFC patch set to help mitigate a common issue > where a timer gets armed just before it is freed, and when the timer > goes off, it crashes in the timer code without any evidence of who the > culprit was. I got side tracked and never finished up on that patch set. > Since this type of crash is still our #1 crash we are seeing in the field, > it has become a priority again to finish it. > > This is v3 of that patch set. Thomas Gleixner posted an untested version > that makes timer->function NULL as the flag that it is shutdown. I took that > code, tested it (fixed it up), added more comments, and changed the > name to timer_shutdown_sync(). I also converted it to use WARN_ON_ONCE() > instead of just WARN_ON() as Linus asked for. > Unfortunately the renaming caused some symbol conflicts. Global definition: timer_shutdown File Line 0 time.c 93 static inline void timer_shutdown(struct clock_event_device *evt) 1 arm_arch_timer.c 690 static __always_inline int timer_shutdown(const int access, 2 timer-fttmr010.c 105 int (*timer_shutdown)(struct clock_event_device *evt); 3 timer-sp804.c 158 static inline void timer_shutdown(struct clock_event_device *evt) 4 timer.h 239 static inline int timer_shutdown(struct timer_list *timer) Guenter
On Fri, 4 Nov 2022 12:22:32 -0700 Guenter Roeck <linux@roeck-us.net> wrote: > Unfortunately the renaming caused some symbol conflicts. > > Global definition: timer_shutdown > > File Line > 0 time.c 93 static inline void timer_shutdown(struct clock_event_device *evt) > 1 arm_arch_timer.c 690 static __always_inline int timer_shutdown(const int access, > 2 timer-fttmr010.c 105 int (*timer_shutdown)(struct clock_event_device *evt); > 3 timer-sp804.c 158 static inline void timer_shutdown(struct clock_event_device *evt) > 4 timer.h 239 static inline int timer_shutdown(struct timer_list *timer) $ git grep '\btimer_shutdown' arch/arm/mach-spear/time.c:static inline void timer_shutdown(struct clock_event_device *evt) arch/arm/mach-spear/time.c: timer_shutdown(evt); arch/arm/mach-spear/time.c: timer_shutdown(evt); arch/arm/mach-spear/time.c: timer_shutdown(evt); drivers/clocksource/arm_arch_timer.c:static __always_inline int timer_shutdown(const int access, drivers/clocksource/arm_arch_timer.c: return timer_shutdown(ARCH_TIMER_VIRT_ACCESS, clk); drivers/clocksource/arm_arch_timer.c: return timer_shutdown(ARCH_TIMER_PHYS_ACCESS, clk); drivers/clocksource/arm_arch_timer.c: return timer_shutdown(ARCH_TIMER_MEM_VIRT_ACCESS, clk); drivers/clocksource/arm_arch_timer.c: return timer_shutdown(ARCH_TIMER_MEM_PHYS_ACCESS, clk); drivers/clocksource/timer-fttmr010.c: int (*timer_shutdown)(struct clock_event_device *evt); drivers/clocksource/timer-fttmr010.c: fttmr010->timer_shutdown(evt); drivers/clocksource/timer-fttmr010.c: fttmr010->timer_shutdown(evt); drivers/clocksource/timer-fttmr010.c: fttmr010->timer_shutdown(evt); drivers/clocksource/timer-fttmr010.c: fttmr010->timer_shutdown = ast2600_timer_shutdown; drivers/clocksource/timer-fttmr010.c: fttmr010->timer_shutdown = fttmr010_timer_shutdown; drivers/clocksource/timer-fttmr010.c: fttmr010->clkevt.set_state_shutdown = fttmr010->timer_shutdown; drivers/clocksource/timer-fttmr010.c: fttmr010->clkevt.tick_resume = fttmr010->timer_shutdown; drivers/clocksource/timer-sp804.c:static inline void timer_shutdown(struct clock_event_device *evt) drivers/clocksource/timer-sp804.c: timer_shutdown(evt); drivers/clocksource/timer-sp804.c: timer_shutdown(evt); Honestly, I think these need to be renamed, as "timer_shutdown()" should be specific to the timer code, and not individual timers. I'll start making a patch set that starts by renaming these timers, then adds the timer_shutdown() API, and finished with the trivial updates, and that will be a real "PATCH" (non RFC). Linus, should I also add any patches that has already been acked by the respective maintainer? -- Steve
On Fri, Nov 4, 2022 at 12:42 PM Steven Rostedt <rostedt@goodmis.org> wrote: > > Linus, should I also add any patches that has already been acked by the > respective maintainer? No, I'd prefer to keep only the ones that are 100% unambiguously not changing any semantics. Linus
On Fri, 4 Nov 2022 15:42:09 -0400 Steven Rostedt <rostedt@goodmis.org> wrote: > $ git grep '\btimer_shutdown' > arch/arm/mach-spear/time.c:static inline void timer_shutdown(struct clock_event_device *evt) > arch/arm/mach-spear/time.c: timer_shutdown(evt); > arch/arm/mach-spear/time.c: timer_shutdown(evt); > arch/arm/mach-spear/time.c: timer_shutdown(evt); > drivers/clocksource/arm_arch_timer.c:static __always_inline int timer_shutdown(const int access, > drivers/clocksource/arm_arch_timer.c: return timer_shutdown(ARCH_TIMER_VIRT_ACCESS, clk); > drivers/clocksource/arm_arch_timer.c: return timer_shutdown(ARCH_TIMER_PHYS_ACCESS, clk); > drivers/clocksource/arm_arch_timer.c: return timer_shutdown(ARCH_TIMER_MEM_VIRT_ACCESS, clk); > drivers/clocksource/arm_arch_timer.c: return timer_shutdown(ARCH_TIMER_MEM_PHYS_ACCESS, clk); > drivers/clocksource/timer-fttmr010.c: int (*timer_shutdown)(struct clock_event_device *evt); > drivers/clocksource/timer-fttmr010.c: fttmr010->timer_shutdown(evt); > drivers/clocksource/timer-fttmr010.c: fttmr010->timer_shutdown(evt); > drivers/clocksource/timer-fttmr010.c: fttmr010->timer_shutdown(evt); > drivers/clocksource/timer-fttmr010.c: fttmr010->timer_shutdown = ast2600_timer_shutdown; > drivers/clocksource/timer-fttmr010.c: fttmr010->timer_shutdown = fttmr010_timer_shutdown; > drivers/clocksource/timer-fttmr010.c: fttmr010->clkevt.set_state_shutdown = fttmr010->timer_shutdown; > drivers/clocksource/timer-fttmr010.c: fttmr010->clkevt.tick_resume = fttmr010->timer_shutdown; I won't touch structure fields though. -- Steve > drivers/clocksource/timer-sp804.c:static inline void timer_shutdown(struct clock_event_device *evt) > drivers/clocksource/timer-sp804.c: timer_shutdown(evt); > drivers/clocksource/timer-sp804.c: timer_shutdown(evt);
On Fri, Nov 04, 2022 at 03:42:09PM -0400, Steven Rostedt wrote: > On Fri, 4 Nov 2022 12:22:32 -0700 > Guenter Roeck <linux@roeck-us.net> wrote: > > > Unfortunately the renaming caused some symbol conflicts. > > > > Global definition: timer_shutdown > > > > File Line > > 0 time.c 93 static inline void timer_shutdown(struct clock_event_device *evt) > > 1 arm_arch_timer.c 690 static __always_inline int timer_shutdown(const int access, > > 2 timer-fttmr010.c 105 int (*timer_shutdown)(struct clock_event_device *evt); > > 3 timer-sp804.c 158 static inline void timer_shutdown(struct clock_event_device *evt) > > 4 timer.h 239 static inline int timer_shutdown(struct timer_list *timer) > > $ git grep '\btimer_shutdown' > arch/arm/mach-spear/time.c:static inline void timer_shutdown(struct clock_event_device *evt) > arch/arm/mach-spear/time.c: timer_shutdown(evt); > arch/arm/mach-spear/time.c: timer_shutdown(evt); > arch/arm/mach-spear/time.c: timer_shutdown(evt); > drivers/clocksource/arm_arch_timer.c:static __always_inline int timer_shutdown(const int access, > drivers/clocksource/arm_arch_timer.c: return timer_shutdown(ARCH_TIMER_VIRT_ACCESS, clk); > drivers/clocksource/arm_arch_timer.c: return timer_shutdown(ARCH_TIMER_PHYS_ACCESS, clk); > drivers/clocksource/arm_arch_timer.c: return timer_shutdown(ARCH_TIMER_MEM_VIRT_ACCESS, clk); > drivers/clocksource/arm_arch_timer.c: return timer_shutdown(ARCH_TIMER_MEM_PHYS_ACCESS, clk); > drivers/clocksource/timer-fttmr010.c: int (*timer_shutdown)(struct clock_event_device *evt); > drivers/clocksource/timer-fttmr010.c: fttmr010->timer_shutdown(evt); > drivers/clocksource/timer-fttmr010.c: fttmr010->timer_shutdown(evt); > drivers/clocksource/timer-fttmr010.c: fttmr010->timer_shutdown(evt); > drivers/clocksource/timer-fttmr010.c: fttmr010->timer_shutdown = ast2600_timer_shutdown; > drivers/clocksource/timer-fttmr010.c: fttmr010->timer_shutdown = fttmr010_timer_shutdown; > drivers/clocksource/timer-fttmr010.c: fttmr010->clkevt.set_state_shutdown = fttmr010->timer_shutdown; > drivers/clocksource/timer-fttmr010.c: fttmr010->clkevt.tick_resume = fttmr010->timer_shutdown; > drivers/clocksource/timer-sp804.c:static inline void timer_shutdown(struct clock_event_device *evt) > drivers/clocksource/timer-sp804.c: timer_shutdown(evt); > drivers/clocksource/timer-sp804.c: timer_shutdown(evt); > > Honestly, I think these need to be renamed, as "timer_shutdown()" > should be specific to the timer code, and not individual timers. Yes, that is what I did locally. I am repeating my test now with that change made. Guenter
On Fri, Nov 04, 2022 at 04:38:34PM -0400, Steven Rostedt wrote: > On Fri, 4 Nov 2022 15:42:09 -0400 > Steven Rostedt <rostedt@goodmis.org> wrote: > [ ... ] > > > drivers/clocksource/timer-fttmr010.c: fttmr010->timer_shutdown(evt); > > drivers/clocksource/timer-fttmr010.c: fttmr010->timer_shutdown(evt); > > drivers/clocksource/timer-fttmr010.c: fttmr010->timer_shutdown(evt); > > drivers/clocksource/timer-fttmr010.c: fttmr010->timer_shutdown = ast2600_timer_shutdown; > > drivers/clocksource/timer-fttmr010.c: fttmr010->timer_shutdown = fttmr010_timer_shutdown; > > drivers/clocksource/timer-fttmr010.c: fttmr010->clkevt.set_state_shutdown = fttmr010->timer_shutdown; > > drivers/clocksource/timer-fttmr010.c: fttmr010->clkevt.tick_resume = fttmr010->timer_shutdown; > > I won't touch structure fields though. > Agreed, same here. Guenter
On Fri, Nov 04, 2022 at 01:40:53AM -0400, Steven Rostedt wrote: > > Back in April, I posted an RFC patch set to help mitigate a common issue > where a timer gets armed just before it is freed, and when the timer > goes off, it crashes in the timer code without any evidence of who the > culprit was. I got side tracked and never finished up on that patch set. > Since this type of crash is still our #1 crash we are seeing in the field, > it has become a priority again to finish it. > After applying the patches attached below, everything compiles for me, and there are no crashes. There are still various warnings, most in networking. I know I need to apply some patch(es) to fix the networking warnings, but I didn't entirely understand what exactly to apply, so I didn't try. Complete logs are at https://kerneltests.org/builders, on the bottom half of the page (qemu tests, in the 'testing' column). Guenter --- Warnings: ODEBUG: free active (active state 0) object type: timer_list hint: tcp_write_timer+0x0/0x1d0 from tcp_close -> __sk_destruct -> tcp_write_timer ODEBUG: free active (active state 0) object type: timer_list hint: tcp_keepalive_timer+0x0/0x4c0 from tcp_close -> __sk_destruct -> tcp_keepalive_timer -> __del_timer_sync ODEBUG: free active (active state 0) object type: timer_list hint: blk_rq_timed_out_timer+0x0/0x40 blk_free_queue_rcu -> blk_free_queue_rcu -> blk_rq_timed_out_timer --- Changes applied on top of patch set to fix build errors: diff --git a/arch/arm/mach-spear/time.c b/arch/arm/mach-spear/time.c index e979e2197f8e..5371c824786d 100644 --- a/arch/arm/mach-spear/time.c +++ b/arch/arm/mach-spear/time.c @@ -90,7 +90,7 @@ static void __init spear_clocksource_init(void) 200, 16, clocksource_mmio_readw_up); } -static inline void timer_shutdown(struct clock_event_device *evt) +static inline void spear_timer_shutdown(struct clock_event_device *evt) { u16 val = readw(gpt_base + CR(CLKEVT)); @@ -101,7 +101,7 @@ static inline void timer_shutdown(struct clock_event_device *evt) static int spear_shutdown(struct clock_event_device *evt) { - timer_shutdown(evt); + spear_timer_shutdown(evt); return 0; } @@ -111,7 +111,7 @@ static int spear_set_oneshot(struct clock_event_device *evt) u16 val; /* stop the timer */ - timer_shutdown(evt); + spear_timer_shutdown(evt); val = readw(gpt_base + CR(CLKEVT)); val |= CTRL_ONE_SHOT; @@ -126,7 +126,7 @@ static int spear_set_periodic(struct clock_event_device *evt) u16 val; /* stop the timer */ - timer_shutdown(evt); + spear_timer_shutdown(evt); period = clk_get_rate(gpt_clk) / HZ; period >>= CTRL_PRESCALER16; diff --git a/drivers/clocksource/arm_arch_timer.c b/drivers/clocksource/arm_arch_timer.c index a7ff77550e17..9c3420a0d19d 100644 --- a/drivers/clocksource/arm_arch_timer.c +++ b/drivers/clocksource/arm_arch_timer.c @@ -687,8 +687,8 @@ static irqreturn_t arch_timer_handler_virt_mem(int irq, void *dev_id) return timer_handler(ARCH_TIMER_MEM_VIRT_ACCESS, evt); } -static __always_inline int timer_shutdown(const int access, - struct clock_event_device *clk) +static __always_inline int arch_timer_shutdown(const int access, + struct clock_event_device *clk) { unsigned long ctrl; @@ -701,22 +701,22 @@ static __always_inline int timer_shutdown(const int access, static int arch_timer_shutdown_virt(struct clock_event_device *clk) { - return timer_shutdown(ARCH_TIMER_VIRT_ACCESS, clk); + return arch_timer_shutdown(ARCH_TIMER_VIRT_ACCESS, clk); } static int arch_timer_shutdown_phys(struct clock_event_device *clk) { - return timer_shutdown(ARCH_TIMER_PHYS_ACCESS, clk); + return arch_timer_shutdown(ARCH_TIMER_PHYS_ACCESS, clk); } static int arch_timer_shutdown_virt_mem(struct clock_event_device *clk) { - return timer_shutdown(ARCH_TIMER_MEM_VIRT_ACCESS, clk); + return arch_timer_shutdown(ARCH_TIMER_MEM_VIRT_ACCESS, clk); } static int arch_timer_shutdown_phys_mem(struct clock_event_device *clk) { - return timer_shutdown(ARCH_TIMER_MEM_PHYS_ACCESS, clk); + return arch_timer_shutdown(ARCH_TIMER_MEM_PHYS_ACCESS, clk); } static __always_inline void set_next_event(const int access, unsigned long evt, diff --git a/drivers/clocksource/timer-sp804.c b/drivers/clocksource/timer-sp804.c index e6a87f4af2b5..a3c38e1343f0 100644 --- a/drivers/clocksource/timer-sp804.c +++ b/drivers/clocksource/timer-sp804.c @@ -155,14 +155,14 @@ static irqreturn_t sp804_timer_interrupt(int irq, void *dev_id) return IRQ_HANDLED; } -static inline void timer_shutdown(struct clock_event_device *evt) +static inline void sp804_timer_shutdown(struct clock_event_device *evt) { writel(0, common_clkevt->ctrl); } static int sp804_shutdown(struct clock_event_device *evt) { - timer_shutdown(evt); + sp804_timer_shutdown(evt); return 0; } @@ -171,7 +171,7 @@ static int sp804_set_periodic(struct clock_event_device *evt) unsigned long ctrl = TIMER_CTRL_32BIT | TIMER_CTRL_IE | TIMER_CTRL_PERIODIC | TIMER_CTRL_ENABLE; - timer_shutdown(evt); + sp804_timer_shutdown(evt); writel(common_clkevt->reload, common_clkevt->load); writel(ctrl, common_clkevt->ctrl); return 0;