All of lore.kernel.org
 help / color / mirror / Atom feed
* [RFC PATCH] clocksource: skip check while watchdog hung up or unstable
@ 2021-08-11  9:55 brookxu
  2021-08-11 12:44 ` Thomas Gleixner
  2021-08-11 13:00 ` kernel test robot
  0 siblings, 2 replies; 8+ messages in thread
From: brookxu @ 2021-08-11  9:55 UTC (permalink / raw)
  To: john.stultz, tglx, sboyd; +Cc: linux-kernel

From: Chunguang Xu <brookxu@tencent.com>

After patch 1f45f1f3 (clocksource: Make clocksource validation work
for all clocksources), md_nsec may be 0 in some scenarios, such as
the watchdog is delayed for a long time or the watchdog has a
time-warp.

We found a problem when testing nvme disks with fio, when multiple
queue interrupts of a disk were mapped to a single CPU. IO interrupt
processing will cause the watchdog to be delayed for a long time
(155 seconds), the system reports TSC unstable and switches the clock
to hpet. It seems that this scenario cannot be handled by optimizing
softirq. Therefore, when md_nsec returns 0, the machine or watchdog
should be in unstable state,the verification result not unreliable.
Is it possible for us to skip the current check at this time?
1. If the watchdog is delayed because the system is busy, and the
   clocksource is switched to hpet due to a wrong judgment, the
   performance degradation may directly cause the machine to be
   unavailable and cause more problems.
2. If watchdog has time-warp, we should not rely on hpet to directly
   mark TSC as unstable.

Later we register watchdog to other CPU, if other CPU is not busy, we
can also check the stability of TSC.

Signed-off-by: Chunguang Xu <brookxu@tencent.com>
---
 kernel/time/clocksource.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/kernel/time/clocksource.c b/kernel/time/clocksource.c
index b89c76e1c02c..9b9014d67f1d 100644
--- a/kernel/time/clocksource.c
+++ b/kernel/time/clocksource.c
@@ -399,6 +399,13 @@ static void clocksource_watchdog(struct timer_list *unused)
 		cs->cs_last = csnow;
 		cs->wd_last = wdnow;
 
+		if (!wd_nsec) {
+			pr_warn("timekeeping watchdog on CPU%d seems hung up or unstable:");
+			pr_warn("'%s' wd_now: %llx wd_last: %llx mask: %llx\n",
+				watchdog->name, wdnow, wdlast, watchdog->mask);
+			continue;
+		}
+
 		if (atomic_read(&watchdog_reset_pending))
 			continue;
 
-- 
2.30.0


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

* Re: [RFC PATCH] clocksource: skip check while watchdog hung up or unstable
  2021-08-11  9:55 [RFC PATCH] clocksource: skip check while watchdog hung up or unstable brookxu
@ 2021-08-11 12:44 ` Thomas Gleixner
  2021-08-11 13:18   ` brookxu
  2021-08-11 13:00 ` kernel test robot
  1 sibling, 1 reply; 8+ messages in thread
From: Thomas Gleixner @ 2021-08-11 12:44 UTC (permalink / raw)
  To: brookxu, john.stultz, sboyd; +Cc: linux-kernel

On Wed, Aug 11 2021 at 17:55, brookxu wrote:
> From: Chunguang Xu <brookxu@tencent.com>
>
> After patch 1f45f1f3 (clocksource: Make clocksource validation work
> for all clocksources), md_nsec may be 0 in some scenarios, such as
> the watchdog is delayed for a long time or the watchdog has a
> time-warp.

Maybe 0? There is exactly one single possibility for it to be zero:

  cs->wd_last == wdnow, i.e. delta = 0 -> wd_nsec = 0

So how does that condition solve any long delay or wrap around of the
watchdog? It's more than unlikely to hit exactly this case where the
readout is identical to the previous readout unless the watchdog stopped
counting.

> We found a problem when testing nvme disks with fio, when multiple
> queue interrupts of a disk were mapped to a single CPU. IO interrupt
> processing will cause the watchdog to be delayed for a long time
> (155 seconds), the system reports TSC unstable and switches the clock

If you hold off the softirq from running for 155 seconds then the TSC
watchdog is the least of your problems.

Thanks,

        tglx

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

* Re: [RFC PATCH] clocksource: skip check while watchdog hung up or unstable
  2021-08-11  9:55 [RFC PATCH] clocksource: skip check while watchdog hung up or unstable brookxu
  2021-08-11 12:44 ` Thomas Gleixner
@ 2021-08-11 13:00 ` kernel test robot
  1 sibling, 0 replies; 8+ messages in thread
From: kernel test robot @ 2021-08-11 13:00 UTC (permalink / raw)
  To: kbuild-all

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

Hi brookxu,

[FYI, it's a private test report for your RFC patch.]
[auto build test WARNING on tip/timers/core]
[also build test WARNING on v5.14-rc5 next-20210811]
[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/brookxu/clocksource-skip-check-while-watchdog-hung-up-or-unstable/20210811-175707
base:   https://git.kernel.org/pub/scm/linux/kernel/git/tip/tip.git 1e7f7fbcd40c69d23e3fe641ead9f3dc128fa8aa
config: x86_64-randconfig-c001-20210810 (attached as .config)
compiler: clang version 14.0.0 (https://github.com/llvm/llvm-project d39ebdae674c8efc84ebe8dc32716ec353220530)
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # https://github.com/0day-ci/linux/commit/085c7bc9e8da6deabad14b2093bd8409cb77b3be
        git remote add linux-review https://github.com/0day-ci/linux
        git fetch --no-tags linux-review brookxu/clocksource-skip-check-while-watchdog-hung-up-or-unstable/20210811-175707
        git checkout 085c7bc9e8da6deabad14b2093bd8409cb77b3be
        # save the attached .config to linux build tree
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross ARCH=x86_64 

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

All warnings (new ones prefixed by >>):

>> kernel/time/clocksource.c:403:41: warning: more '%' conversions than data arguments [-Wformat-insufficient-args]
                           pr_warn("timekeeping watchdog on CPU%d seems hung up or unstable:");
                                                               ~^
   include/linux/printk.h:400:29: note: expanded from macro 'pr_warn'
           printk(KERN_WARNING pr_fmt(fmt), ##__VA_ARGS__)
                                      ^~~
   kernel/time/clocksource.c:8:41: note: expanded from macro 'pr_fmt'
   #define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
                                           ^~~
   1 warning generated.


vim +403 kernel/time/clocksource.c

   352	
   353	static void clocksource_watchdog(struct timer_list *unused)
   354	{
   355		u64 csnow, wdnow, cslast, wdlast, delta;
   356		int next_cpu, reset_pending;
   357		int64_t wd_nsec, cs_nsec;
   358		struct clocksource *cs;
   359		u32 md;
   360	
   361		spin_lock(&watchdog_lock);
   362		if (!watchdog_running)
   363			goto out;
   364	
   365		reset_pending = atomic_read(&watchdog_reset_pending);
   366	
   367		list_for_each_entry(cs, &watchdog_list, wd_list) {
   368	
   369			/* Clocksource already marked unstable? */
   370			if (cs->flags & CLOCK_SOURCE_UNSTABLE) {
   371				if (finished_booting)
   372					schedule_work(&watchdog_work);
   373				continue;
   374			}
   375	
   376			if (!cs_watchdog_read(cs, &csnow, &wdnow)) {
   377				/* Clock readout unreliable, so give it up. */
   378				__clocksource_unstable(cs);
   379				continue;
   380			}
   381	
   382			/* Clocksource initialized ? */
   383			if (!(cs->flags & CLOCK_SOURCE_WATCHDOG) ||
   384			    atomic_read(&watchdog_reset_pending)) {
   385				cs->flags |= CLOCK_SOURCE_WATCHDOG;
   386				cs->wd_last = wdnow;
   387				cs->cs_last = csnow;
   388				continue;
   389			}
   390	
   391			delta = clocksource_delta(wdnow, cs->wd_last, watchdog->mask);
   392			wd_nsec = clocksource_cyc2ns(delta, watchdog->mult,
   393						     watchdog->shift);
   394	
   395			delta = clocksource_delta(csnow, cs->cs_last, cs->mask);
   396			cs_nsec = clocksource_cyc2ns(delta, cs->mult, cs->shift);
   397			wdlast = cs->wd_last; /* save these in case we print them */
   398			cslast = cs->cs_last;
   399			cs->cs_last = csnow;
   400			cs->wd_last = wdnow;
   401	
   402			if (!wd_nsec) {
 > 403				pr_warn("timekeeping watchdog on CPU%d seems hung up or unstable:");
   404				pr_warn("'%s' wd_now: %llx wd_last: %llx mask: %llx\n",
   405					watchdog->name, wdnow, wdlast, watchdog->mask);
   406				continue;
   407			}
   408	
   409			if (atomic_read(&watchdog_reset_pending))
   410				continue;
   411	
   412			/* Check the deviation from the watchdog clocksource. */
   413			md = cs->uncertainty_margin + watchdog->uncertainty_margin;
   414			if (abs(cs_nsec - wd_nsec) > md) {
   415				pr_warn("timekeeping watchdog on CPU%d: Marking clocksource '%s' as unstable because the skew is too large:\n",
   416					smp_processor_id(), cs->name);
   417				pr_warn("                      '%s' wd_nsec: %lld wd_now: %llx wd_last: %llx mask: %llx\n",
   418					watchdog->name, wd_nsec, wdnow, wdlast, watchdog->mask);
   419				pr_warn("                      '%s' cs_nsec: %lld cs_now: %llx cs_last: %llx mask: %llx\n",
   420					cs->name, cs_nsec, csnow, cslast, cs->mask);
   421				if (curr_clocksource == cs)
   422					pr_warn("                      '%s' is current clocksource.\n", cs->name);
   423				else if (curr_clocksource)
   424					pr_warn("                      '%s' (not '%s') is current clocksource.\n", curr_clocksource->name, cs->name);
   425				else
   426					pr_warn("                      No current clocksource.\n");
   427				__clocksource_unstable(cs);
   428				continue;
   429			}
   430	
   431			if (cs == curr_clocksource && cs->tick_stable)
   432				cs->tick_stable(cs);
   433	
   434			if (!(cs->flags & CLOCK_SOURCE_VALID_FOR_HRES) &&
   435			    (cs->flags & CLOCK_SOURCE_IS_CONTINUOUS) &&
   436			    (watchdog->flags & CLOCK_SOURCE_IS_CONTINUOUS)) {
   437				/* Mark it valid for high-res. */
   438				cs->flags |= CLOCK_SOURCE_VALID_FOR_HRES;
   439	
   440				/*
   441				 * clocksource_done_booting() will sort it if
   442				 * finished_booting is not set yet.
   443				 */
   444				if (!finished_booting)
   445					continue;
   446	
   447				/*
   448				 * If this is not the current clocksource let
   449				 * the watchdog thread reselect it. Due to the
   450				 * change to high res this clocksource might
   451				 * be preferred now. If it is the current
   452				 * clocksource let the tick code know about
   453				 * that change.
   454				 */
   455				if (cs != curr_clocksource) {
   456					cs->flags |= CLOCK_SOURCE_RESELECT;
   457					schedule_work(&watchdog_work);
   458				} else {
   459					tick_clock_notify();
   460				}
   461			}
   462		}
   463	
   464		/*
   465		 * We only clear the watchdog_reset_pending, when we did a
   466		 * full cycle through all clocksources.
   467		 */
   468		if (reset_pending)
   469			atomic_dec(&watchdog_reset_pending);
   470	
   471		/*
   472		 * Cycle through CPUs to check if the CPUs stay synchronized
   473		 * to each other.
   474		 */
   475		next_cpu = cpumask_next(raw_smp_processor_id(), cpu_online_mask);
   476		if (next_cpu >= nr_cpu_ids)
   477			next_cpu = cpumask_first(cpu_online_mask);
   478	
   479		/*
   480		 * Arm timer if not already pending: could race with concurrent
   481		 * pair clocksource_stop_watchdog() clocksource_start_watchdog().
   482		 */
   483		if (!timer_pending(&watchdog_timer)) {
   484			watchdog_timer.expires += WATCHDOG_INTERVAL;
   485			add_timer_on(&watchdog_timer, next_cpu);
   486		}
   487	out:
   488		spin_unlock(&watchdog_lock);
   489	}
   490	

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

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

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

* Re: [RFC PATCH] clocksource: skip check while watchdog hung up or unstable
  2021-08-11 12:44 ` Thomas Gleixner
@ 2021-08-11 13:18   ` brookxu
  2021-08-11 14:01     ` Thomas Gleixner
  0 siblings, 1 reply; 8+ messages in thread
From: brookxu @ 2021-08-11 13:18 UTC (permalink / raw)
  To: Thomas Gleixner, john.stultz, sboyd; +Cc: linux-kernel


Thanks for your time.

Thomas Gleixner wrote on 2021/8/11 8:44 下午:
> On Wed, Aug 11 2021 at 17:55, brookxu wrote:
>> From: Chunguang Xu <brookxu@tencent.com>
>>
>> After patch 1f45f1f3 (clocksource: Make clocksource validation work
>> for all clocksources), md_nsec may be 0 in some scenarios, such as
>> the watchdog is delayed for a long time or the watchdog has a
>> time-warp.
> 
> Maybe 0? There is exactly one single possibility for it to be zero:
> 
>   cs->wd_last == wdnow, i.e. delta = 0 -> wd_nsec = 0
> 
> So how does that condition solve any long delay or wrap around of the
> watchdog? It's more than unlikely to hit exactly this case where the
> readout is identical to the previous readout unless the watchdog stopped
> counting.

Maybe I missed something. Like this example, when watchdog run ,hpet have
wrap around:

'hpet' wd_now: d76e5a69 wd_last: f929eb3c mask: ffffffff

We can calculate the number of elapsed cycles:
cycles = wd_now - wd_last = 0xde446f2d

clocksource_delta() uses the MSB to determine an invalid inteval and returns
0, but for 0xde446f2d, this judgment should be wrong.


>> We found a problem when testing nvme disks with fio, when multiple
>> queue interrupts of a disk were mapped to a single CPU. IO interrupt
>> processing will cause the watchdog to be delayed for a long time
>> (155 seconds), the system reports TSC unstable and switches the clock
> 
> If you hold off the softirq from running for 155 seconds then the TSC
> watchdog is the least of your problems.

To be precise, we are processing interrupts in handle_edge_irq() for a long
time. Since the interrupts of multiple hardware queues are mapped to a single
CPU, multiple cores are continuously issuing IO, and then a single core is
processing IO. Perhaps the test case can be optimized, but shouldn't this lead
to switching clocks in principle?

> Thanks,
> 
>         tglx
> 

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

* Re: [RFC PATCH] clocksource: skip check while watchdog hung up or unstable
  2021-08-11 13:18   ` brookxu
@ 2021-08-11 14:01     ` Thomas Gleixner
  2021-08-11 15:26       ` brookxu
  0 siblings, 1 reply; 8+ messages in thread
From: Thomas Gleixner @ 2021-08-11 14:01 UTC (permalink / raw)
  To: brookxu, john.stultz, sboyd; +Cc: linux-kernel

On Wed, Aug 11 2021 at 21:18, brookxu wrote:
> Thomas Gleixner wrote on 2021/8/11 8:44 下午:
>> On Wed, Aug 11 2021 at 17:55, brookxu wrote:
>>> From: Chunguang Xu <brookxu@tencent.com>
>>>
>>> After patch 1f45f1f3 (clocksource: Make clocksource validation work
>>> for all clocksources), md_nsec may be 0 in some scenarios, such as
>>> the watchdog is delayed for a long time or the watchdog has a
>>> time-warp.
>> 
>> Maybe 0? There is exactly one single possibility for it to be zero:
>> 
>>   cs->wd_last == wdnow, i.e. delta = 0 -> wd_nsec = 0
>> 
>> So how does that condition solve any long delay or wrap around of the
>> watchdog? It's more than unlikely to hit exactly this case where the
>> readout is identical to the previous readout unless the watchdog stopped
>> counting.
>
> Maybe I missed something. Like this example, when watchdog run ,hpet have
> wrap around:
>
> 'hpet' wd_now: d76e5a69 wd_last: f929eb3c mask: ffffffff
>
> We can calculate the number of elapsed cycles:
> cycles = wd_now - wd_last = 0xde446f2d
>
> clocksource_delta() uses the MSB to determine an invalid inteval and returns
> 0, but for 0xde446f2d, this judgment should be wrong.

You're right. I forgot about the MSB check which is enabled on x86.

>>> We found a problem when testing nvme disks with fio, when multiple
>>> queue interrupts of a disk were mapped to a single CPU. IO interrupt
>>> processing will cause the watchdog to be delayed for a long time
>>> (155 seconds), the system reports TSC unstable and switches the clock
>> 
>> If you hold off the softirq from running for 155 seconds then the TSC
>> watchdog is the least of your problems.
>
> To be precise, we are processing interrupts in handle_edge_irq() for a long
> time. Since the interrupts of multiple hardware queues are mapped to a single
> CPU, multiple cores are continuously issuing IO, and then a single core is
> processing IO. Perhaps the test case can be optimized, but shouldn't this lead
> to switching clocks in principle?

The clocksource watchdog failure is only _ONE_ consequence. Processing
hard interrupts for 155 seconds straight will trigger lockup detectors
of all sorts if you have them enabled.

So just papering over the clocksource watchdog does not solve anything,
really. Next week you have to add similar hacks to the lockup detectors,
RCU and whatever.

Thanks,

        tglx

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

* Re: [RFC PATCH] clocksource: skip check while watchdog hung up or unstable
  2021-08-11 14:01     ` Thomas Gleixner
@ 2021-08-11 15:26       ` brookxu
  2021-08-12 10:53         ` Thomas Gleixner
  0 siblings, 1 reply; 8+ messages in thread
From: brookxu @ 2021-08-11 15:26 UTC (permalink / raw)
  To: Thomas Gleixner, john.stultz, sboyd; +Cc: linux-kernel

Thanks for your time.

Thomas Gleixner wrote on 2021/8/11 22:01:
> On Wed, Aug 11 2021 at 21:18, brookxu wrote:
>> Thomas Gleixner wrote on 2021/8/11 8:44 下午:
>>> On Wed, Aug 11 2021 at 17:55, brookxu wrote:
>>>> From: Chunguang Xu <brookxu@tencent.com>
>>>>
>>>> After patch 1f45f1f3 (clocksource: Make clocksource validation work
>>>> for all clocksources), md_nsec may be 0 in some scenarios, such as
>>>> the watchdog is delayed for a long time or the watchdog has a
>>>> time-warp.
>>>
>>> Maybe 0? There is exactly one single possibility for it to be zero:
>>>
>>>   cs->wd_last == wdnow, i.e. delta = 0 -> wd_nsec = 0
>>>
>>> So how does that condition solve any long delay or wrap around of the
>>> watchdog? It's more than unlikely to hit exactly this case where the
>>> readout is identical to the previous readout unless the watchdog stopped
>>> counting.
>>
>> Maybe I missed something. Like this example, when watchdog run ,hpet have
>> wrap around:
>>
>> 'hpet' wd_now: d76e5a69 wd_last: f929eb3c mask: ffffffff
>>
>> We can calculate the number of elapsed cycles:
>> cycles = wd_now - wd_last = 0xde446f2d
>>
>> clocksource_delta() uses the MSB to determine an invalid inteval and returns
>> 0, but for 0xde446f2d, this judgment should be wrong.
> 
> You're right. I forgot about the MSB check which is enabled on x86.
> 
>>>> We found a problem when testing nvme disks with fio, when multiple
>>>> queue interrupts of a disk were mapped to a single CPU. IO interrupt
>>>> processing will cause the watchdog to be delayed for a long time
>>>> (155 seconds), the system reports TSC unstable and switches the clock
>>>
>>> If you hold off the softirq from running for 155 seconds then the TSC
>>> watchdog is the least of your problems.
>>
>> To be precise, we are processing interrupts in handle_edge_irq() for a long
>> time. Since the interrupts of multiple hardware queues are mapped to a single
>> CPU, multiple cores are continuously issuing IO, and then a single core is
>> processing IO. Perhaps the test case can be optimized, but shouldn't this lead
>> to switching clocks in principle?
> 
> The clocksource watchdog failure is only _ONE_ consequence. Processing
> hard interrupts for 155 seconds straight will trigger lockup detectors
> of all sorts if you have them enabled.
> 
> So just papering over the clocksource watchdog does not solve anything,
> really. Next week you have to add similar hacks to the lockup detectors,
> RCU and whatever.

Yeah, we have observed soft lockup and RCU stall, but these behaviors are
expected because the current CPU scheduling is disabled. However, marking
TSC unstable is inconsistent with the actual situation. The worst problem
is that after the clocksource switched to hpet, the abnormal time will be
greatly prolonged due to the degradation of performance. We have not found
that soft lockup and RCU stall will affect the machine for a long time in
this test. Aside from these, as the watchdog is scheduled periodically, when
wd_nsec is 0, it means that something maybe abnormal, do we readlly still
need to continue to verify TSC? and how to ensure the correctness of the
results?

> Thanks,
> 
>         tglx
> 

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

* Re: [RFC PATCH] clocksource: skip check while watchdog hung up or unstable
  2021-08-11 15:26       ` brookxu
@ 2021-08-12 10:53         ` Thomas Gleixner
  2021-08-13  0:54           ` brookxu
  0 siblings, 1 reply; 8+ messages in thread
From: Thomas Gleixner @ 2021-08-12 10:53 UTC (permalink / raw)
  To: brookxu, john.stultz, sboyd; +Cc: linux-kernel

On Wed, Aug 11 2021 at 23:26, brookxu wrote:
> Thomas Gleixner wrote on 2021/8/11 22:01:
>>> To be precise, we are processing interrupts in handle_edge_irq() for a long
>>> time. Since the interrupts of multiple hardware queues are mapped to a single
>>> CPU, multiple cores are continuously issuing IO, and then a single core is
>>> processing IO. Perhaps the test case can be optimized, but shouldn't this lead
>>> to switching clocks in principle?
>> 
>> The clocksource watchdog failure is only _ONE_ consequence. Processing
>> hard interrupts for 155 seconds straight will trigger lockup detectors
>> of all sorts if you have them enabled.
>> 
>> So just papering over the clocksource watchdog does not solve anything,
>> really. Next week you have to add similar hacks to the lockup detectors,
>> RCU and whatever.
>
> Yeah, we have observed soft lockup and RCU stall, but these behaviors are
> expected because the current CPU scheduling is disabled. However, marking
> TSC unstable is inconsistent with the actual situation. The worst problem
> is that after the clocksource switched to hpet, the abnormal time will be
> greatly prolonged due to the degradation of performance. We have not found
> that soft lockup and RCU stall will affect the machine for a long time in
> this test. Aside from these, as the watchdog is scheduled periodically, when
> wd_nsec is 0, it means that something maybe abnormal, do we readlly still
> need to continue to verify TSC? and how to ensure the correctness of the
> results?

Sorry no. While softlockups and RCU stalls might have no long term
effect in the first place, this argumentation vs. the clocksource
watchdog is just a strawman. You're abusing the system in a way which
causes it to malfunction so you have to live with the consequences.

Aside of that this 'workaround' is just duct taping a particular part of
the problem. What guarantees that after the interrupt storm subsided the
clocksource delta of the watchdog becomes 0 (negative)?

Absolutely nothing. The delta can be positive, but then the watchdog and
the TSC are not in sync anymore which will disable the TSC as well.

A 24MHz HPET has a wraparound time of ~178s which means during:

  89s < tdelta < 178s

your hack papers over the problem. Any interrupt storm time outside of
that window results in fail.

Now run the same test on a machine with a 14MHz HPET and you get

 153s < tdelta < 306s

so your 155s interrupt storm barely fits. And what are you doing with
your next test which runs only 80 seconds?

Not to talk about the fact that you wreckage detection of a watchdog
clocksource going stale.

So no, we are not adding hacks to support abuse.

What we really want to do is to add detection for interrupt storms of
this sort and shut those interrupts down for good.

Thanks,

        tglx
---
Patient: "Doctor, it hurts when I hammer on my toe."
Doctor:  "Don't do that then!"



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

* Re: [RFC PATCH] clocksource: skip check while watchdog hung up or unstable
  2021-08-12 10:53         ` Thomas Gleixner
@ 2021-08-13  0:54           ` brookxu
  0 siblings, 0 replies; 8+ messages in thread
From: brookxu @ 2021-08-13  0:54 UTC (permalink / raw)
  To: Thomas Gleixner, john.stultz, sboyd; +Cc: linux-kernel



Thomas Gleixner wrote on 2021/8/12 6:53 下午:
> On Wed, Aug 11 2021 at 23:26, brookxu wrote:
>> Thomas Gleixner wrote on 2021/8/11 22:01:
>>>> To be precise, we are processing interrupts in handle_edge_irq() for a long
>>>> time. Since the interrupts of multiple hardware queues are mapped to a single
>>>> CPU, multiple cores are continuously issuing IO, and then a single core is
>>>> processing IO. Perhaps the test case can be optimized, but shouldn't this lead
>>>> to switching clocks in principle?
>>>
>>> The clocksource watchdog failure is only _ONE_ consequence. Processing
>>> hard interrupts for 155 seconds straight will trigger lockup detectors
>>> of all sorts if you have them enabled.
>>>
>>> So just papering over the clocksource watchdog does not solve anything,
>>> really. Next week you have to add similar hacks to the lockup detectors,
>>> RCU and whatever.
>>
>> Yeah, we have observed soft lockup and RCU stall, but these behaviors are
>> expected because the current CPU scheduling is disabled. However, marking
>> TSC unstable is inconsistent with the actual situation. The worst problem
>> is that after the clocksource switched to hpet, the abnormal time will be
>> greatly prolonged due to the degradation of performance. We have not found
>> that soft lockup and RCU stall will affect the machine for a long time in
>> this test. Aside from these, as the watchdog is scheduled periodically, when
>> wd_nsec is 0, it means that something maybe abnormal, do we readlly still
>> need to continue to verify TSC? and how to ensure the correctness of the
>> results?
> 
> Sorry no. While softlockups and RCU stalls might have no long term
> effect in the first place, this argumentation vs. the clocksource
> watchdog is just a strawman. You're abusing the system in a way which
> causes it to malfunction so you have to live with the consequences.
> 
> Aside of that this 'workaround' is just duct taping a particular part of
> the problem. What guarantees that after the interrupt storm subsided the
> clocksource delta of the watchdog becomes 0 (negative)?
> 
> Absolutely nothing. The delta can be positive, but then the watchdog and
> the TSC are not in sync anymore which will disable the TSC as well.
> 
> A 24MHz HPET has a wraparound time of ~178s which means during:
> 
>   89s < tdelta < 178s
> 
> your hack papers over the problem. Any interrupt storm time outside of
> that window results in fail.
> 
> Now run the same test on a machine with a 14MHz HPET and you get
> 
>  153s < tdelta < 306s
> 
> so your 155s interrupt storm barely fits. And what are you doing with
> your next test which runs only 80 seconds?
> 
> Not to talk about the fact that you wreckage detection of a watchdog
> clocksource going stale.
> 
> So no, we are not adding hacks to support abuse.
> 
> What we really want to do is to add detection for interrupt storms of
> this sort and shut those interrupts down for good.

ok, thanks for your suggestion.

> Thanks,
> 
>         tglx
> ---
> Patient: "Doctor, it hurts when I hammer on my toe."
> Doctor:  "Don't do that then!"
> 
> 

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

end of thread, other threads:[~2021-08-13  0:57 UTC | newest]

Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-08-11  9:55 [RFC PATCH] clocksource: skip check while watchdog hung up or unstable brookxu
2021-08-11 12:44 ` Thomas Gleixner
2021-08-11 13:18   ` brookxu
2021-08-11 14:01     ` Thomas Gleixner
2021-08-11 15:26       ` brookxu
2021-08-12 10:53         ` Thomas Gleixner
2021-08-13  0:54           ` brookxu
2021-08-11 13:00 ` kernel test robot

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.