Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Subject: Re: [PATCH] acpi_pm: Reduce PMTMR counter read contention
To:     zhenzhong.duan@oracle.com, Thomas Gleixner <tglx@linutronix.de>
Cc:     linux-kernel@vger.kernel.org, x86@kernel.org,
        Daniel Lezcano <daniel.lezcano@linaro.org>,
        Waiman Long <longman@redhat.com>,
        Srinivas Eeda <srinivas.eeda@oracle.com>
References: <1548141807-25825-1-git-send-email-zhenzhong.duan@oracle.com>
 <alpine.DEB.2.21.1901300903350.1950@nanos.tec.linutronix.de>
 <019e583c-7bcb-c234-200c-fcdb6c49fbb0@oracle.com>
 <alpine.DEB.2.21.1901311525360.8200@nanos.tec.linutronix.de>
 <853e8cf6-aba9-0200-8e39-e362848399ba@oracle.com>
From:   Kin Cho <kin.cho@oracle.com>
Message-ID: <b307e679-8a12-1eff-595d-2c39e6c6fb6a@oracle.com>
Date:   Fri, 1 Feb 2019 23:55:55 -0800
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101
 Thunderbird/60.4.0
MIME-Version: 1.0
In-Reply-To: <853e8cf6-aba9-0200-8e39-e362848399ba@oracle.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 8bit
Content-Language: en-US
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

Zhenzhong,
The machine is running test this weekend, I'll try your simple fix next 
week.

All,
We're not aware of a specific customer need for acpi_pm/PMTMR,
but if we must keep acpi_pm/PMTMR in the kernel, let's fix it so it
actually works, even on machine like ours.
On our hardware currently it's broken both during clocksource selection
and as a permanent clocksource.

Thanks,

-kin

On 2/1/19 6:52 PM, Zhenzhong Duan wrote:
> On 2019/1/31 22:26, Thomas Gleixner wrote:
>> On Thu, 31 Jan 2019, Zhenzhong Duan wrote:
>>
>>> On 2019/1/30 16:06, Thomas Gleixner wrote:
>>>> On Tue, 22 Jan 2019, Zhenzhong Duan wrote:
>>>>
>>>>> On a large system with many CPUs, using PMTMR as the clock source can
>>>>> have a significant impact on the overall system performance because
>>>>> of the following reasons:
>>>>>    1) There is a single PMTMR counter shared by all the CPUs.
>>>>>    2) PMTMR counter reading is a very slow operation.
>>>>>
>>>>> Using PMTMR as the default clock source may happen when, for example,
>>>>> the TSC clock calibration exceeds the allowable tolerance and HPET
>>>>> disabled by nohpet on kernel command line. Sometimes the performance
>>>>
>>>> The question is why would anyone disable HPET on a larger machine 
>>>> when the
>>>> TSC is wreckaged?
>>>
>>> There may be broken hardware where TSC is wreckaged.
>>
>> I know that.
>>
>>>> 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. 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;
>  }
>
>
> Thanks
> Zhenzhong