linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH] use 64bit timer for hpet
@ 2021-07-02  8:13 zhaoyan.liao
  2021-07-02 15:57 ` kernel test robot
  2021-07-07 10:04 ` Thomas Gleixner
  0 siblings, 2 replies; 8+ messages in thread
From: zhaoyan.liao @ 2021-07-02  8:13 UTC (permalink / raw)
  To: tglx, mingo, hpa, dwmw
  Cc: linux-kernel, kernel-janitors, songmuchun, likunkun,
	guancheng.rjk, zhaoyan.liao

From: "zhaoyan.liao" <zhaoyan.liao@linux.alibaba.com>

The kernel judges whether the tsc clock is accurate in the
clocksource_watchdog background thread function. The hpet clock source
is 32-bit, but tsc is 64-bit. Therefore, when the system is busy and the
clocksource_watchdog cannot be scheduled in time, the hpet clock may
overflow and cause the system to misjudge tsc as unreliable.
In this case, we recommend that the kernel adopts the 64-bit hpet clock
by default to keep the width of the two clock sources the same to reduce
misjudgment. Some CPU models may not support 64-bit hpet, but according
to the description of the CPU's register manual, it does not affect our
reading action.

Signed-off-by: zhaoyan.liao <zhaoyan.liao@linux.alibaba.com>
---
 arch/x86/kernel/hpet.c | 103 ++++++++++++++++++++++++++++---------------------
 1 file changed, 60 insertions(+), 43 deletions(-)

diff --git a/arch/x86/kernel/hpet.c b/arch/x86/kernel/hpet.c
index 7a50f0b..463e8dc 100644
--- a/arch/x86/kernel/hpet.c
+++ b/arch/x86/kernel/hpet.c
@@ -38,7 +38,7 @@ struct hpet_base {
 	struct hpet_channel		*channels;
 };
 
-#define HPET_MASK			CLOCKSOURCE_MASK(32)
+#define HPET_MASK			CLOCKSOURCE_MASK(64)
 
 #define HPET_MIN_CYCLES			128
 #define HPET_MIN_PROG_DELTA		(HPET_MIN_CYCLES + (HPET_MIN_CYCLES >> 1))
@@ -82,6 +82,16 @@ static inline void hpet_writel(unsigned int d, unsigned int a)
 	writel(d, hpet_virt_address + a);
 }
 
+inline unsigned long hpet_readq(unsigned int a)
+{
+	return readq(hpet_virt_address + a);
+}
+
+static inline void hpet_writeq(unsigned long d, unsigned int a)
+{
+	writeq(d, hpet_virt_address + a);
+}
+
 static inline void hpet_set_mapping(void)
 {
 	hpet_virt_address = ioremap(hpet_address, HPET_MMAP_SIZE);
@@ -253,8 +263,7 @@ static void hpet_stop_counter(void)
 
 static void hpet_reset_counter(void)
 {
-	hpet_writel(0, HPET_COUNTER);
-	hpet_writel(0, HPET_COUNTER + 4);
+	hpet_writeq(0, HPET_COUNTER);
 }
 
 static void hpet_start_counter(void)
@@ -295,19 +304,20 @@ static void hpet_enable_legacy_int(void)
 static int hpet_clkevt_set_state_periodic(struct clock_event_device *evt)
 {
 	unsigned int channel = clockevent_to_channel(evt)->num;
-	unsigned int cfg, cmp, now;
+	unsigned int cfg;
+	unsigned long cmp, now;
 	uint64_t delta;
 
 	hpet_stop_counter();
 	delta = ((uint64_t)(NSEC_PER_SEC / HZ)) * evt->mult;
 	delta >>= evt->shift;
-	now = hpet_readl(HPET_COUNTER);
-	cmp = now + (unsigned int)delta;
+	now = hpet_readq(HPET_COUNTER);
+	cmp = now + delta;
 	cfg = hpet_readl(HPET_Tn_CFG(channel));
-	cfg |= HPET_TN_ENABLE | HPET_TN_PERIODIC | HPET_TN_SETVAL |
-	       HPET_TN_32BIT;
+	cfg &= ~HPET_TN_32BIT;
+	cfg |= HPET_TN_ENABLE | HPET_TN_PERIODIC | HPET_TN_SETVAL;
 	hpet_writel(cfg, HPET_Tn_CFG(channel));
-	hpet_writel(cmp, HPET_Tn_CMP(channel));
+	hpet_writeq(cmp, HPET_Tn_CMP(channel));
 	udelay(1);
 	/*
 	 * HPET on AMD 81xx needs a second write (with HPET_TN_SETVAL
@@ -316,7 +326,7 @@ static int hpet_clkevt_set_state_periodic(struct clock_event_device *evt)
 	 * (See AMD-8111 HyperTransport I/O Hub Data Sheet,
 	 * Publication # 24674)
 	 */
-	hpet_writel((unsigned int)delta, HPET_Tn_CMP(channel));
+	hpet_writeq(delta, HPET_Tn_CMP(channel));
 	hpet_start_counter();
 	hpet_print_config();
 
@@ -329,8 +339,8 @@ static int hpet_clkevt_set_state_oneshot(struct clock_event_device *evt)
 	unsigned int cfg;
 
 	cfg = hpet_readl(HPET_Tn_CFG(channel));
-	cfg &= ~HPET_TN_PERIODIC;
-	cfg |= HPET_TN_ENABLE | HPET_TN_32BIT;
+	cfg &= ~(HPET_TN_PERIODIC|HPET_TN_32BIT);
+	cfg |= HPET_TN_ENABLE;
 	hpet_writel(cfg, HPET_Tn_CFG(channel));
 
 	return 0;
@@ -342,7 +352,7 @@ static int hpet_clkevt_set_state_shutdown(struct clock_event_device *evt)
 	unsigned int cfg;
 
 	cfg = hpet_readl(HPET_Tn_CFG(channel));
-	cfg &= ~HPET_TN_ENABLE;
+	cfg &= ~(HPET_TN_PERIODIC|HPET_TN_32BIT);
 	hpet_writel(cfg, HPET_Tn_CFG(channel));
 
 	return 0;
@@ -359,12 +369,12 @@ static int hpet_clkevt_legacy_resume(struct clock_event_device *evt)
 hpet_clkevt_set_next_event(unsigned long delta, struct clock_event_device *evt)
 {
 	unsigned int channel = clockevent_to_channel(evt)->num;
-	u32 cnt;
-	s32 res;
+	u64 cnt;
+	s64 res;
 
-	cnt = hpet_readl(HPET_COUNTER);
-	cnt += (u32) delta;
-	hpet_writel(cnt, HPET_Tn_CMP(channel));
+	cnt = hpet_readq(HPET_COUNTER);
+	cnt += (u64) delta;
+	hpet_writeq(cnt, HPET_Tn_CMP(channel));
 
 	/*
 	 * HPETs are a complete disaster. The compare register is
@@ -388,7 +398,7 @@ static int hpet_clkevt_legacy_resume(struct clock_event_device *evt)
 	 * the event. The minimum programming delta for the generic
 	 * clockevents code is set to 1.5 * HPET_MIN_CYCLES.
 	 */
-	res = (s32)(cnt - hpet_readl(HPET_COUNTER));
+	res = (s64)(cnt - hpet_readq(HPET_COUNTER));
 
 	return res < HPET_MIN_CYCLES ? -ETIME : 0;
 }
@@ -671,9 +681,10 @@ static inline void hpet_select_clockevents(void) { }
 union hpet_lock {
 	struct {
 		arch_spinlock_t lock;
-		u32 value;
+		u32 reserved;
+		u64 value;
 	};
-	u64 lockval;
+	u64 lockval1, lockval2;
 };
 
 static union hpet_lock hpet __cacheline_aligned = {
@@ -685,25 +696,28 @@ static u64 read_hpet(struct clocksource *cs)
 	unsigned long flags;
 	union hpet_lock old, new;
 
-	BUILD_BUG_ON(sizeof(union hpet_lock) != 8);
+	BUILD_BUG_ON(sizeof(union hpet_lock) != 16);
 
 	/*
 	 * Read HPET directly if in NMI.
 	 */
 	if (in_nmi())
-		return (u64)hpet_readl(HPET_COUNTER);
+		return (u64)hpet_readq(HPET_COUNTER);
 
 	/*
 	 * Read the current state of the lock and HPET value atomically.
 	 */
-	old.lockval = READ_ONCE(hpet.lockval);
+	local_irq_save(flags);
+	old.lockval1 = READ_ONCE(hpet.lockval1);
+	old.lockval2 = READ_ONCE(hpet.lockval2);
+	local_irq_restore(flags);
 
 	if (arch_spin_is_locked(&old.lock))
 		goto contended;
 
 	local_irq_save(flags);
 	if (arch_spin_trylock(&hpet.lock)) {
-		new.value = hpet_readl(HPET_COUNTER);
+		new.value = hpet_readq(HPET_COUNTER);
 		/*
 		 * Use WRITE_ONCE() to prevent store tearing.
 		 */
@@ -729,7 +743,10 @@ static u64 read_hpet(struct clocksource *cs)
 	 */
 	do {
 		cpu_relax();
-		new.lockval = READ_ONCE(hpet.lockval);
+		local_irq_save(flags);
+		new.lockval1 = READ_ONCE(hpet.lockval1);
+		new.lockval2 = READ_ONCE(hpet.lockval2);
+		local_irq_restore(flags);
 	} while ((new.value == old.value) && arch_spin_is_locked(&new.lock));
 
 	return (u64)new.value;
@@ -740,7 +757,7 @@ static u64 read_hpet(struct clocksource *cs)
  */
 static u64 read_hpet(struct clocksource *cs)
 {
-	return (u64)hpet_readl(HPET_COUNTER);
+	return hpet_readq(HPET_COUNTER);
 }
 #endif
 
@@ -787,7 +804,7 @@ static bool __init hpet_counting(void)
 
 	hpet_restart_counter();
 
-	t1 = hpet_readl(HPET_COUNTER);
+	t1 = hpet_readq(HPET_COUNTER);
 	start = rdtsc();
 
 	/*
@@ -797,7 +814,7 @@ static bool __init hpet_counting(void)
 	 * 1 GHz == 200us
 	 */
 	do {
-		if (t1 != hpet_readl(HPET_COUNTER))
+		if (t1 != hpet_readq(HPET_COUNTER))
 			return true;
 		now = rdtsc();
 	} while ((now - start) < 200000UL);
@@ -881,11 +898,11 @@ int __init hpet_enable(void)
 		irq = (cfg & Tn_INT_ROUTE_CNF_MASK) >> Tn_INT_ROUTE_CNF_SHIFT;
 		hc->irq = irq;
 
-		cfg &= ~(HPET_TN_ENABLE | HPET_TN_LEVEL | HPET_TN_FSB);
+		cfg &= ~(HPET_TN_ENABLE | HPET_TN_32BIT | HPET_TN_LEVEL | HPET_TN_FSB);
 		hpet_writel(cfg, HPET_Tn_CFG(i));
 
 		cfg &= ~(HPET_TN_PERIODIC | HPET_TN_PERIODIC_CAP
-			 | HPET_TN_64BIT_CAP | HPET_TN_32BIT | HPET_TN_ROUTE
+			 | HPET_TN_64BIT_CAP | HPET_TN_ROUTE
 			 | HPET_TN_FSB | HPET_TN_FSB_CAP);
 		if (cfg)
 			pr_warn("Channel #%u config: Unknown bits %#x\n", i, cfg);
@@ -1028,9 +1045,9 @@ void hpet_disable(void)
 static int hpet_prev_update_sec;
 static struct rtc_time hpet_alarm_time;
 static unsigned long hpet_pie_count;
-static u32 hpet_t1_cmp;
-static u32 hpet_default_delta;
-static u32 hpet_pie_delta;
+static u64 hpet_t1_cmp;
+static u64 hpet_default_delta;
+static u64 hpet_pie_delta;
 static unsigned long hpet_pie_limit;
 
 static rtc_irq_handler irq_handler;
@@ -1081,8 +1098,8 @@ void hpet_unregister_irq_handler(rtc_irq_handler handler)
  */
 int hpet_rtc_timer_init(void)
 {
-	unsigned int cfg, cnt, delta;
-	unsigned long flags;
+	unsigned int cfg;
+	unsigned long cnt, delta, flags;
 
 	if (!is_hpet_enabled())
 		return 0;
@@ -1103,13 +1120,13 @@ int hpet_rtc_timer_init(void)
 
 	local_irq_save(flags);
 
-	cnt = delta + hpet_readl(HPET_COUNTER);
-	hpet_writel(cnt, HPET_T1_CMP);
+	cnt = delta + hpet_readq(HPET_COUNTER);
+	hpet_writeq(cnt, HPET_T1_CMP);
 	hpet_t1_cmp = cnt;
 
 	cfg = hpet_readl(HPET_T1_CFG);
-	cfg &= ~HPET_TN_PERIODIC;
-	cfg |= HPET_TN_ENABLE | HPET_TN_32BIT;
+	cfg &= ~(HPET_TN_PERIODIC|HPET_TN_32BIT);
+	cfg |= HPET_TN_ENABLE;
 	hpet_writel(cfg, HPET_T1_CFG);
 
 	local_irq_restore(flags);
@@ -1207,7 +1224,7 @@ int hpet_rtc_dropped_irq(void)
 
 static void hpet_rtc_timer_reinit(void)
 {
-	unsigned int delta;
+	unsigned long delta;
 	int lost_ints = -1;
 
 	if (unlikely(!hpet_rtc_flags))
@@ -1224,9 +1241,9 @@ static void hpet_rtc_timer_reinit(void)
 	 */
 	do {
 		hpet_t1_cmp += delta;
-		hpet_writel(hpet_t1_cmp, HPET_T1_CMP);
+		hpet_writeq(hpet_t1_cmp, HPET_T1_CMP);
 		lost_ints++;
-	} while (!hpet_cnt_ahead(hpet_t1_cmp, hpet_readl(HPET_COUNTER)));
+	} while (!hpet_cnt_ahead(hpet_t1_cmp, hpet_readq(HPET_COUNTER)));
 
 	if (lost_ints) {
 		if (hpet_rtc_flags & RTC_PIE)
-- 
1.8.3.1


^ permalink raw reply related	[flat|nested] 8+ messages in thread

* Re: [PATCH] use 64bit timer for hpet
  2021-07-02  8:13 [PATCH] use 64bit timer for hpet zhaoyan.liao
@ 2021-07-02 15:57 ` kernel test robot
  2021-07-07 10:04 ` Thomas Gleixner
  1 sibling, 0 replies; 8+ messages in thread
From: kernel test robot @ 2021-07-02 15:57 UTC (permalink / raw)
  To: zhaoyan.liao, tglx, mingo, hpa, dwmw
  Cc: kbuild-all, linux-kernel, kernel-janitors, songmuchun, likunkun,
	guancheng.rjk, zhaoyan.liao

[-- Attachment #1: Type: text/plain, Size: 2287 bytes --]

Hi "zhaoyan.liao",

Thank you for the patch! Yet something to improve:

[auto build test ERROR on tip/x86/core]
[also build test ERROR on v5.13 next-20210701]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/0day-ci/linux/commits/zhaoyan-liao/use-64bit-timer-for-hpet/20210702-161515
base:   https://git.kernel.org/pub/scm/linux/kernel/git/tip/tip.git 2c88d45edbb89029c1190bb3b136d2602f057c98
config: i386-randconfig-c021-20210628 (attached as .config)
compiler: gcc-9 (Debian 9.3.0-22) 9.3.0
reproduce (this is a W=1 build):
        # https://github.com/0day-ci/linux/commit/905ed014f601086571e608c13b14319c2f56a95c
        git remote add linux-review https://github.com/0day-ci/linux
        git fetch --no-tags linux-review zhaoyan-liao/use-64bit-timer-for-hpet/20210702-161515
        git checkout 905ed014f601086571e608c13b14319c2f56a95c
        # save the attached .config to linux build tree
        make W=1 ARCH=i386 

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

All errors (new ones prefixed by >>):

   arch/x86/kernel/hpet.c: In function 'hpet_readq':
>> arch/x86/kernel/hpet.c:88:9: error: implicit declaration of function 'readq'; did you mean 'readl'? [-Werror=implicit-function-declaration]
      88 |  return readq(hpet_virt_address + a);
         |         ^~~~~
         |         readl
   arch/x86/kernel/hpet.c: In function 'hpet_writeq':
>> arch/x86/kernel/hpet.c:93:2: error: implicit declaration of function 'writeq'; did you mean 'writel'? [-Werror=implicit-function-declaration]
      93 |  writeq(d, hpet_virt_address + a);
         |  ^~~~~~
         |  writel
   cc1: some warnings being treated as errors


vim +88 arch/x86/kernel/hpet.c

    85	
    86	inline unsigned long hpet_readq(unsigned int a)
    87	{
  > 88		return readq(hpet_virt_address + a);
    89	}
    90	
    91	static inline void hpet_writeq(unsigned long d, unsigned int a)
    92	{
  > 93		writeq(d, hpet_virt_address + a);
    94	}
    95	

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 45040 bytes --]

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] use 64bit timer for hpet
  2021-07-02  8:13 [PATCH] use 64bit timer for hpet zhaoyan.liao
  2021-07-02 15:57 ` kernel test robot
@ 2021-07-07 10:04 ` Thomas Gleixner
  2021-07-08  3:11   ` Linux
  1 sibling, 1 reply; 8+ messages in thread
From: Thomas Gleixner @ 2021-07-07 10:04 UTC (permalink / raw)
  To: zhaoyan.liao, mingo, hpa, dwmw
  Cc: linux-kernel, kernel-janitors, songmuchun, likunkun,
	guancheng.rjk, zhaoyan.liao

Liao,

On Fri, Jul 02 2021 at 16:13, zhaoyan liao wrote:
> The kernel judges whether the tsc clock is accurate in the
> clocksource_watchdog background thread function. The hpet clock source
> is 32-bit, but tsc is 64-bit. Therefore, when the system is busy and the
> clocksource_watchdog cannot be scheduled in time, the hpet clock may
> overflow and cause the system to misjudge tsc as unreliable.

Seriously? The wrap-around time for 32bit HPET @24MHz is ~3 minutes.

> In this case, we recommend that the kernel adopts the 64-bit hpet clock
> by default to keep the width of the two clock sources the same to reduce
> misjudgment. Some CPU models may not support 64-bit hpet, but according
> to the description of the CPU's register manual, it does not affect our
> reading action.

So much for the theory.

> -#define HPET_MASK			CLOCKSOURCE_MASK(32)
> +#define HPET_MASK			CLOCKSOURCE_MASK(64)

How is that valid for a 32bit HPET? This breaks the clocksource.
 
> +inline unsigned long hpet_readq(unsigned int a)
> +{
> +	return readq(hpet_virt_address + a);

Breaks 32bit build immediately.

Aside of that the reason why the kernel does not support 64bit HPET is
that there are HPETs which advertise 64bit support, but the
implementation is buggy.

IOW, while this works for your hardware this breaks quite some parts of
the universe. Not really a good approach.

Thanks,

        tglx

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] use 64bit timer for hpet
  2021-07-07 10:04 ` Thomas Gleixner
@ 2021-07-08  3:11   ` Linux
  2021-07-08 11:17     ` Thomas Gleixner
  0 siblings, 1 reply; 8+ messages in thread
From: Linux @ 2021-07-08  3:11 UTC (permalink / raw)
  To: Thomas Gleixner
  Cc: mingo, hpa, dwmw, linux-kernel, kernel-janitors, songmuchun,
	likunkun, guancheng.rjk, duanxiongchun, wenan.mao

Gleixner,

> Seriously? The wrap-around time for 32bit HPET @24MHz is ~3 minutes.

In some cases, our system will be very busy, and the timeout of 3 minutes 
is not an exaggeration. Then, the system considers that the tsc clock is 
inaccurate and switches the tsc clock to the hpet clock, which brings 
greater performance overhead.

> Aside of that the reason why the kernel does not support 64bit HPET is
> that there are HPETs which advertise 64bit support, but the
> implementation is buggy.

Can you tell me what is the buggy with the 64-bit hpet clock? In my opinion, 
it is unreasonable to use a lower-bit width clock to calibrate a higher-bit width
 clock, and the hardware already supports the higher-bit width.


> 2021年7月7日 下午6:04,Thomas Gleixner <tglx@linutronix.de> 写道:
> 
> Liao,
> 
> On Fri, Jul 02 2021 at 16:13, zhaoyan liao wrote:
>> The kernel judges whether the tsc clock is accurate in the
>> clocksource_watchdog background thread function. The hpet clock source
>> is 32-bit, but tsc is 64-bit. Therefore, when the system is busy and the
>> clocksource_watchdog cannot be scheduled in time, the hpet clock may
>> overflow and cause the system to misjudge tsc as unreliable.
> 
> Seriously? The wrap-around time for 32bit HPET @24MHz is ~3 minutes.
> 
>> In this case, we recommend that the kernel adopts the 64-bit hpet clock
>> by default to keep the width of the two clock sources the same to reduce
>> misjudgment. Some CPU models may not support 64-bit hpet, but according
>> to the description of the CPU's register manual, it does not affect our
>> reading action.
> 
> So much for the theory.
> 
>> -#define HPET_MASK			CLOCKSOURCE_MASK(32)
>> +#define HPET_MASK			CLOCKSOURCE_MASK(64)
> 
> How is that valid for a 32bit HPET? This breaks the clocksource.
> 
>> +inline unsigned long hpet_readq(unsigned int a)
>> +{
>> +	return readq(hpet_virt_address + a);
> 
> Breaks 32bit build immediately.
> 
> Aside of that the reason why the kernel does not support 64bit HPET is
> that there are HPETs which advertise 64bit support, but the
> implementation is buggy.
> 
> IOW, while this works for your hardware this breaks quite some parts of
> the universe. Not really a good approach.
> 
> Thanks,
> 
>        tglx


^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] use 64bit timer for hpet
  2021-07-08  3:11   ` Linux
@ 2021-07-08 11:17     ` Thomas Gleixner
  2021-07-12  4:52       ` Linux
  0 siblings, 1 reply; 8+ messages in thread
From: Thomas Gleixner @ 2021-07-08 11:17 UTC (permalink / raw)
  To: Linux
  Cc: mingo, hpa, dwmw, linux-kernel, kernel-janitors, songmuchun,
	likunkun, guancheng.rjk, duanxiongchun, wenan.mao

Liao!

On Thu, Jul 08 2021 at 11:11, Linux wrote:
>> 2021年7月7日 下午6:04,Thomas Gleixner <tglx@linutronix.de> 写道:
>> Seriously? The wrap-around time for 32bit HPET @24MHz is ~3 minutes.
>
> In some cases, our system will be very busy, and the timeout of 3 minutes 
> is not an exaggeration. Then, the system considers that the tsc clock is 
> inaccurate and switches the tsc clock to the hpet clock, which brings 
> greater performance overhead.

Sorry, keeping the softirq from running for 3 minutes is simply out of
spec. If the sysadmin decides to do so, then he can keep the pieces.

>> Aside of that the reason why the kernel does not support 64bit HPET is
>> that there are HPETs which advertise 64bit support, but the
>> implementation is buggy.
>
> Can you tell me what is the buggy with the 64-bit hpet clock?

I forgot the details, but when I tried moving HPET to 64bit it did not
work on one of my machines due to an erratum and other people reported
similar issues on different CPUs/chipsets.

TBH, I'm not interested at all to chase down these buggy implementations
and have yet another pile of quirks.

> In my opinion, it is unreasonable to use a lower-bit width clock to
> calibrate a higher-bit width clock, and the hardware already supports
> the higher-bit width.

There is nothing unreasonable with that, really:

   1) This is not about calibration. It's a sanity check to catch
      broken TSC implementations.

      Aside of that it _IS_ very reasonable for calibration. We even
      calibrate TSC via the PIT if we can't get the frequency from
      the firmware.

   2) Expecting that the softirq runs within 3 minutes is very
      reasonable.

   3) On modern machines this is usually not longer necessary. If you
      are confident that the TSC on your system is stable then you
      can disable the watchdog via the kernel command line.

      There is also effort underway to come up with reasonable
      conditions to avoid the watchdog on those CPUs in the first place.

   4) For any system which actually has to use HPET the 64bit HPET is
      overhead. HPET access is slow enough already.

   5) 32bit HPET has to be supported as well and just claiming that a
      64bit access on 32bit HPET does not matter is just wishful
      thinking. Aside of breaking 32bit kernels along the way which
      is just a NONO.
      
#4 and #5 were the main reason why I gave up on it - aside of the
discovery that there are broken implementations out there.

So no, there is really no compelling reason to support 64bit HPETs.

Thanks,

        tglx
---
P.S: Please trim your replies.

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] use 64bit timer for hpet
  2021-07-08 11:17     ` Thomas Gleixner
@ 2021-07-12  4:52       ` Linux
  2021-07-12  7:25         ` Thomas Gleixner
  0 siblings, 1 reply; 8+ messages in thread
From: Linux @ 2021-07-12  4:52 UTC (permalink / raw)
  To: Thomas Gleixner
  Cc: mingo, hpa, dwmw, linux-kernel, kernel-janitors, songmuchun,
	likunkun, guancheng.rjk, duanxiongchun, wenan.mao

Gleixner,

> Sorry, keeping the softirq from running for 3 minutes is simply out of
> spec. If the sysadmin decides to do so, then he can keep the pieces.

It is because the kernel thread is busy that the clocksource_watchdog 
thread is not scheduled, not softirq.

>   4) For any system which actually has to use HPET the 64bit HPET is
>      overhead. HPET access is slow enough already.
I agree with your opinion. If it is unreasonable to use a 64-bit HPET timer, 
is there any other more reasonable method to avoid misjudgment of the 
tsc clock?
I will also try to switch to other methods.
Thanks
           Zhaoyan Liao



> 2021年7月8日 下午7:17,Thomas Gleixner <tglx@linutronix.de> 写道:
> 
> Liao!
> 
> On Thu, Jul 08 2021 at 11:11, Linux wrote:
>>> 2021年7月7日 下午6:04,Thomas Gleixner <tglx@linutronix.de> 写道:
>>> Seriously? The wrap-around time for 32bit HPET @24MHz is ~3 minutes.
>> 
>> In some cases, our system will be very busy, and the timeout of 3 minutes 
>> is not an exaggeration. Then, the system considers that the tsc clock is 
>> inaccurate and switches the tsc clock to the hpet clock, which brings 
>> greater performance overhead.
> 
> Sorry, keeping the softirq from running for 3 minutes is simply out of
> spec. If the sysadmin decides to do so, then he can keep the pieces.
> 
>>> Aside of that the reason why the kernel does not support 64bit HPET is
>>> that there are HPETs which advertise 64bit support, but the
>>> implementation is buggy.
>> 
>> Can you tell me what is the buggy with the 64-bit hpet clock?
> 
> I forgot the details, but when I tried moving HPET to 64bit it did not
> work on one of my machines due to an erratum and other people reported
> similar issues on different CPUs/chipsets.
> 
> TBH, I'm not interested at all to chase down these buggy implementations
> and have yet another pile of quirks.
> 
>> In my opinion, it is unreasonable to use a lower-bit width clock to
>> calibrate a higher-bit width clock, and the hardware already supports
>> the higher-bit width.
> 
> There is nothing unreasonable with that, really:
> 
>   1) This is not about calibration. It's a sanity check to catch
>      broken TSC implementations.
> 
>      Aside of that it _IS_ very reasonable for calibration. We even
>      calibrate TSC via the PIT if we can't get the frequency from
>      the firmware.
> 
>   2) Expecting that the softirq runs within 3 minutes is very
>      reasonable.
> 
>   3) On modern machines this is usually not longer necessary. If you
>      are confident that the TSC on your system is stable then you
>      can disable the watchdog via the kernel command line.
> 
>      There is also effort underway to come up with reasonable
>      conditions to avoid the watchdog on those CPUs in the first place.
> 
>   4) For any system which actually has to use HPET the 64bit HPET is
>      overhead. HPET access is slow enough already.
> 
>   5) 32bit HPET has to be supported as well and just claiming that a
>      64bit access on 32bit HPET does not matter is just wishful
>      thinking. Aside of breaking 32bit kernels along the way which
>      is just a NONO.
> 
> #4 and #5 were the main reason why I gave up on it - aside of the
> discovery that there are broken implementations out there.
> 
> So no, there is really no compelling reason to support 64bit HPETs.
> 
> Thanks,
> 
>        tglx
> ---
> P.S: Please trim your replies.


^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] use 64bit timer for hpet
  2021-07-12  4:52       ` Linux
@ 2021-07-12  7:25         ` Thomas Gleixner
  2021-07-13  1:43           ` Linux
  0 siblings, 1 reply; 8+ messages in thread
From: Thomas Gleixner @ 2021-07-12  7:25 UTC (permalink / raw)
  To: Linux
  Cc: mingo, hpa, dwmw, linux-kernel, kernel-janitors, songmuchun,
	likunkun, guancheng.rjk, duanxiongchun, wenan.mao

Liao,

On Mon, Jul 12 2021 at 12:52, Linux wrote:
>> Sorry, keeping the softirq from running for 3 minutes is simply out of
>> spec. If the sysadmin decides to do so, then he can keep the pieces.
>
> It is because the kernel thread is busy that the clocksource_watchdog 
> thread is not scheduled, not softirq.

Which thread?

The clocksource watchdog runs from a timer_list timer callback in
softirq context. Even if the softirq is switched to the softirq thread
then still my argument of starving that for 3 minutes still stands.

This is _not_ a kernel problem. Overcommitment is a admin problem.

Thanks,

        tglx

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] use 64bit timer for hpet
  2021-07-12  7:25         ` Thomas Gleixner
@ 2021-07-13  1:43           ` Linux
  0 siblings, 0 replies; 8+ messages in thread
From: Linux @ 2021-07-13  1:43 UTC (permalink / raw)
  To: Thomas Gleixner
  Cc: mingo, hpa, dwmw, linux-kernel, kernel-janitors, songmuchun,
	likunkun, guancheng.rjk, duanxiongchun, wenan.mao

OK, Thank you for your patience. the last question:
> I forgot the details, but when I tried moving HPET to 64bit it did not
> work on one of my machines due to an erratum and other people reported
> similar issues on different CPUs/chipsets.
> 
> TBH, I'm not interested at all to chase down these buggy implementations
> and have yet another pile of quirks.

Can you tell me the erranum or issue link at that time? This is very important
 to us.

Thank you very much.


> 2021年7月12日 下午3:25,Thomas Gleixner <tglx@linutronix.de> 写道:
> 
> Liao,
> 
> On Mon, Jul 12 2021 at 12:52, Linux wrote:
>>> Sorry, keeping the softirq from running for 3 minutes is simply out of
>>> spec. If the sysadmin decides to do so, then he can keep the pieces.
>> 
>> It is because the kernel thread is busy that the clocksource_watchdog 
>> thread is not scheduled, not softirq.
> 
> Which thread?
> 
> The clocksource watchdog runs from a timer_list timer callback in
> softirq context. Even if the softirq is switched to the softirq thread
> then still my argument of starving that for 3 minutes still stands.
> 
> This is _not_ a kernel problem. Overcommitment is a admin problem.
> 
> Thanks,
> 
>        tglx


^ permalink raw reply	[flat|nested] 8+ messages in thread

end of thread, other threads:[~2021-07-13  1:43 UTC | newest]

Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-07-02  8:13 [PATCH] use 64bit timer for hpet zhaoyan.liao
2021-07-02 15:57 ` kernel test robot
2021-07-07 10:04 ` Thomas Gleixner
2021-07-08  3:11   ` Linux
2021-07-08 11:17     ` Thomas Gleixner
2021-07-12  4:52       ` Linux
2021-07-12  7:25         ` Thomas Gleixner
2021-07-13  1:43           ` Linux

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).