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

[Anson Huang <anson.huang@nxp.com>]

Hi, Rafael
	Looks like adding pr_info() in irq_work_sync() makes issue can NOT be reproduced, any possibility of race happen there and the pr_info eliminate the race condition? I will continue run the test with the pr_info to see if any luck to reproduce it.

Anson

> 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);