Re: [PATCH] acpi_pm: Reduce PMTMR counter read contention

[Zhenzhong Duan]

On 2019/2/11 5:08, Thomas Gleixner wrote:
> On Sat, 2 Feb 2019, Zhenzhong Duan wrote:
> > On 2019/1/31 22:26, Thomas Gleixner wrote:
>
> > > > > I'm not against the change per se, but I really want to understand
> > > > > why we need all the complexity for something which should never be
> > > > > used in a real world deployment.
> > > > Hmm, it's a strong word of "never be used". Customers may happen to
> > > > use nohpet(sanity test?) and report bug to us. Sometimes they does
> > > > report a bug that reproduce with their customed config. There may
> > > > also be BIOS setting HPET disabled.
> > >
> > > And because the customer MAY do completely nonsensical things (and there
> > > are a lot more than the HPET) the kernel has to handle all of them?
> >
> > Ok, then. I don't have more suggestion to convince you.
>
> You give up too fast :)

Ah, because I thought of a simple fix.
> The point is, that we really want proper justifications for changes like
> this. Some 'may, could and more handwaving' simply does not cut it.
>
> So if you can just describe a realistic scenario, which does not involve
> thoughtless flipping of BIOS options, then this becomes way more palatable.

I indeed don't see a realistic scenario in a product env needing to use 
nohpet. My only justification is now that we have nohpet as kernel 
parameter, we should fix the softlockup in large machines for enterprise 
use.
> > I just think of a simple fix as below. I think it will work for both hpet
> > and pmtmr. We will test it when the env is available.
>
> > --- a/kernel/time/timekeeping.c
> > +++ b/kernel/time/timekeeping.c
> > @@ -1353,6 +1353,7 @@ static int change_clocksource(void *data)
> >
> >          write_seqcount_end(&tk_core.seq);
> >          raw_spin_unlock_irqrestore(&timekeeper_lock, flags);
> > +       tick_clock_notify();
> >
> >          return 0;
> >   }
> > @@ -1371,7 +1372,6 @@ int timekeeping_notify(struct clocksource *clock)
> >          if (tk->tkr_mono.clock == clock)
> >                  return 0;
> >          stop_machine(change_clocksource, clock, NULL);
> > -       tick_clock_notify();
> >          return tk->tkr_mono.clock == clock ? 0 : -1;
> >   }
>
> This won't resolve the concurrency issues of HPET or PMTIMER in any
> way.

Just got chance to test and Kin confirmed it fix the softlockup of 
PMTMR(with nohpet) and HPET(without nohpet, revert previous hpet commit) 
at bootup stage.

My understandig is, at bootup stage tick device is firstly initialized 
in periodic mode and then switch to one-shot mode. In periodic mode 
clock event interrupt is triggered every 1ms(HZ=1000), contention in 
HPET or PMTIMER exceeds 1ms and delayed the clock interrupt. Then CPUs 
continue to process interrupt one by one without a break, 
tick_clock_notify() have no chance to be called and we never switch to 
one-shot mode.

In one-shot mode, the contention is still there but next event is always 
set with a future value. We may missed some ticks, but the timer code is 
smart enough to pick up those missed ticks.

By moving tick_clock_notify() in stop_machine, kernel changes to 
one-shot mode early before the contention accumulate and lockup system.

> Instead it breaks the careful orchestrated mechanism of clocksource
> change.

Sorry, I didn't get a idea how it breaks, tick_clock_notify() is a 
simple function setting bitmask in percpu variable. Could you explain a bit?

Thanks
Zhenzhong