lkml.org 
[lkml]   [2019]   [Feb]   [17]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
SubjectRe: [PATCH] acpi_pm: Reduce PMTMR counter read contention
From
Date
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

\
 
 \ /
  Last update: 2019-02-18 04:49    [W:0.091 / U:2.508 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site