[<prev] [next>] [thread-next>] [day] [month] [year] [list]
Message-ID: <Y6UrQd2VIyakhV/U@feng-clx>
Date: Fri, 23 Dec 2022 12:14:57 +0800
From: Feng Tang <feng.tang@...el.com>
To: Waiman Long <longman@...hat.com>
CC: <paulmck@...nel.org>, John Stultz <jstultz@...gle.com>,
Thomas Gleixner <tglx@...utronix.de>,
Stephen Boyd <sboyd@...nel.org>, <x86@...nel.org>,
Peter Zijlstra <peterz@...radead.org>,
<linux-kernel@...r.kernel.org>, "Tim Chen" <tim.c.chen@...el.com>
Subject: Re: [RFC PATCH] clocksource: Suspend the watchdog temporarily when
high read lantency detected
On Thu, Dec 22, 2022 at 10:49:23PM -0500, Waiman Long wrote:
> On 12/22/22 22:37, Paul E. McKenney wrote:
> > > > commit dfbf67806c4c7f2bdd79cdefe86a2bea6e7afcab
> > > > Author: Paul E. McKenney<paulmck@...nel.org>
> > > > Date: Thu Dec 22 13:21:47 2022 -0800
> > > >
> > > > clocksource: Permit limited-duration clocksource watchdogging
> > > > Some systems want regular clocksource checking, but their workloads
> > > > cannot tolerate the overhead of full-time clocksource checking.
> > > > Therefore, add a CLOCKSOURCE_WATCHDOG_DURATION Kconfig option and a
> > > > clocksource.watchdog_duration kernel-boot parameter that limits the
> > > > clocksource watchdog to the specified number of minutes past boot.
> > > > Values of zero disable checking, and a value of -1 restores the
> > > > traditional behavior of always checking.
> > > > This does change behavior compared to older kernels, but recent kernels
> > > > disable the clocksource watchdog completely in the common case where the
> > > > TSC is judged to be trustworthy. This change in behavior is therefore
> > > > not a real issue.
> > > Yes, this changes the general semantics. Last year, I've posted a
> > > patch to limit the watchdog to run for 10 minutes, and at that time
> > > Thomas mentioned one of his machine may show tsc issue after running
> > > for one day depending on work load [1].
> > >
> > > As the intention is to validate HPET/PMTIMER, which are not as
> > > delicate as TSC, maybe we can add a per-clocksource verify-period
> > > field, and only set it for HPET/PMTIMER?
> > >
> > > [1].https://lore.kernel.org/lkml/875z286xtk.fsf@nanos.tec.linutronix.de/
> > Got it.
> >
> > The workloads I am closest to are OK with the clocksource watchdog
> > running indefinitely, but thus far the skew is visible very early.
> > But broken hardware can do whatever it wants whenever it wants. I could
> > meet Thomas's request by making the default be indefinite, and allowing
> > whoever cares to make it finite. Or maybe the fact that the TSC is not
> > marked unstable makes a difference.
> >
> > Thoughts?
>
> Sorry for the late reply.
>
> Maybe the default should be an auto mode where if TSC is marked stable and
> don't need to verify, we can run watchdog for HPET and PMTMR for 10 mins.
> Otherwise, run it indefinitely to not change existing behavior.
Yes, sounds reasonable to me.
btw, what I suggested in last mail is some code (untested) like this:
---
diff --git a/arch/x86/kernel/hpet.c b/arch/x86/kernel/hpet.c
index c8eb1ac5125a..db20aac5d14d 100644
--- a/arch/x86/kernel/hpet.c
+++ b/arch/x86/kernel/hpet.c
@@ -862,6 +862,7 @@ static struct clocksource clocksource_hpet = {
.mask = HPET_MASK,
.flags = CLOCK_SOURCE_IS_CONTINUOUS,
.resume = hpet_resume_counter,
+ .wd_limited = true,
};
/*
diff --git a/include/linux/clocksource.h b/include/linux/clocksource.h
index 1d42d4b17327..2b6278f69516 100644
--- a/include/linux/clocksource.h
+++ b/include/linux/clocksource.h
@@ -125,6 +125,8 @@ struct clocksource {
struct list_head wd_list;
u64 cs_last;
u64 wd_last;
+ bool wd_limited;
+ u64 wd_iters;
#endif
struct module *owner;
};
diff --git a/kernel/time/clocksource.c b/kernel/time/clocksource.c
index 777a5eba68fd..eb2d9adf06b0 100644
--- a/kernel/time/clocksource.c
+++ b/kernel/time/clocksource.c
@@ -425,6 +425,8 @@ static void clocksource_watchdog(struct timer_list *unused)
cs->flags |= CLOCK_SOURCE_WATCHDOG;
cs->wd_last = wdnow;
cs->cs_last = csnow;
+ if (cs->wd_limited)
+ cs->wd_iters = 1200;
continue;
}
@@ -492,6 +494,9 @@ static void clocksource_watchdog(struct timer_list *unused)
tick_clock_notify();
}
}
+
+ if (cs->wd_limited && !(cs->wd_iters--))
+ list_del_init(&cs->wd_list);
}
/*
Thanks,
Feng
> Given such a default, I don't think we need your second patch to determine
> if both HPET and PMTMR needs to be checked.
>
> Cheers,
> Longman
Powered by blists - more mailing lists