Re: About CPU hot-plug stress test failed in cpufreq driver

["Rafael J. Wysocki" <rafael@kernel.org>]

On Fri, Nov 22, 2019 at 6:15 AM Anson Huang <anson.huang@nxp.com> wrote:
>
> Hi, Rafael
>         Theoretically, yes, the CPU being offline will run the irq work list to make sure the irq work pending on it will be clear, but the fact is NOT,

So this looks like a problem with irq_work_sync() working not as expected.

>         both ondemand and schedutil governor can reproduce this issue if running stress CPU hotplug test.
>         I tried add a "int cpu" in irq work structure to record CPU number which has irq work pending, when issue happen, I can see the irq work is pending at CPU #3 which is already offline, this is why issue happen, but I don't know how it happens...
>
> diff --git a/include/linux/irq_work.h b/include/linux/irq_work.h
> index b11fcdf..f8da06f9 100644
> --- a/include/linux/irq_work.h
> +++ b/include/linux/irq_work.h
> @@ -25,6 +25,7 @@ struct irq_work {
>         unsigned long flags;
>         struct llist_node llnode;
>         void (*func)(struct irq_work *);
> +       int cpu;
>  };
>
>  static inline
> diff --git a/kernel/irq_work.c b/kernel/irq_work.c
> index d42acaf..2e893d5 100644
> --- a/kernel/irq_work.c
> +++ b/kernel/irq_work.c
> @@ -10,6 +10,7 @@
>  #include <linux/kernel.h>
>  #include <linux/export.h>
>  #include <linux/irq_work.h>
> +#include <linux/jiffies.h>
>  #include <linux/percpu.h>
>  #include <linux/hardirq.h>
>  #include <linux/irqflags.h>
> @@ -78,6 +79,7 @@ bool irq_work_queue(struct irq_work *work)
>         if (!irq_work_claim(work))
>                 return false;
>
> +       work->cpu = smp_processor_id();
>         /* Queue the entry and raise the IPI if needed. */
>         preempt_disable();
>         __irq_work_queue_local(work);
> @@ -105,6 +107,7 @@ bool irq_work_queue_on(struct irq_work *work, int cpu)
>         /* Only queue if not already pending */
>         if (!irq_work_claim(work))
>                 return false;
> +       work->cpu = cpu;
>
>         preempt_disable();
>         if (cpu != smp_processor_id()) {
> @@ -161,6 +164,7 @@ static void irq_work_run_list(struct llist_head *list)
>                  */
>                 flags = work->flags & ~IRQ_WORK_PENDING;
>                 xchg(&work->flags, flags);
> +               work->cpu = -1;
>
>                 work->func(work);
>                 /*
> @@ -197,9 +201,13 @@ void irq_work_tick(void)
>   */
>  void irq_work_sync(struct irq_work *work)
>  {
> +       unsigned long timeout = jiffies + msecs_to_jiffies(500);
>         lockdep_assert_irqs_enabled();

Can you please add something like

pr_info("%s: CPU %d\n", __func__, work->cpu);

here re-run the test and collect a log again?

I need to know if irq_work_sync() runs during CPU offline as expected.

>
> -       while (work->flags & IRQ_WORK_BUSY)
> +       while (work->flags & IRQ_WORK_BUSY) {
> +               if (time_after(jiffies, timeout))
> +                       pr_warn("irq_work_sync 500ms timeout, work cpu %d\n", work->cpu);
>                 cpu_relax();
> +       }
>  }
>  EXPORT_SYMBOL_GPL(irq_work_sync);