Messages in this thread Patch in this message | | | Date | Wed, 10 Nov 2021 14:53:27 +0100 | From | Heiner Kallweit <> | Subject | Re: [PATCH] clocksource: Improve cs_watchdog_read() |
| |
On 10.11.2021 13:48, Paul E. McKenney wrote: > On Tue, Nov 09, 2021 at 09:55:08PM +0100, Heiner Kallweit wrote: >> If max_cswd_read_retries is set to 0 or 1 then the current warning >> behavior doesn't seem to make too much sense to me. >> If set to 0, then we'd warn with each watchdog run. >> If set to 1, then we'd warn at the first retry, even though the commit >> description of db3a34e17433 states that one retry is expected behavior. >> If printing a message at all in this case, then it should be debug >> level. > > The behavior for max_cswd_read_retries==1 is exactly what you want when > you are checking to see whether or not your system would retry at all > for the duration of a given run. > > The behavior for max_cswd_read_retries==0 is exactly what you want when > you are testing the ability to print that message on a system that will > not do a retry in a reasonable period of time. > > Or am I missing something here? > For me this mixes production warning and debug features. To support your debug use cases, maybe use something like this?
if (nretries > 1) pr_warn() else if (nretries >= max_cswd_read_retries) pr_debug()
>> Whilst being at it, move declaration of wd_end and wd_delta into the >> loop and remove not needed braces. > > I am OK with moving those two variables into the "for" loop. > > I am personally OK removing the braces, but if I remember correctly, > my upstream maintainer asked that I add them due to the statement being > split across two lines. > > Thanx, Paul > >> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com> >> --- >> kernel/time/clocksource.c | 6 +++--- >> 1 file changed, 3 insertions(+), 3 deletions(-) >> >> diff --git a/kernel/time/clocksource.c b/kernel/time/clocksource.c >> index f29d1a524..8c0be9c02 100644 >> --- a/kernel/time/clocksource.c >> +++ b/kernel/time/clocksource.c >> @@ -208,10 +208,11 @@ module_param(verify_n_cpus, int, 0644); >> static bool cs_watchdog_read(struct clocksource *cs, u64 *csnow, u64 *wdnow) >> { >> unsigned int nretries; >> - u64 wd_end, wd_delta; >> int64_t wd_delay; >> >> for (nretries = 0; nretries <= max_cswd_read_retries; nretries++) { >> + u64 wd_end, wd_delta; >> + >> local_irq_disable(); >> *wdnow = watchdog->read(watchdog); >> *csnow = cs->read(cs); >> @@ -222,10 +223,9 @@ static bool cs_watchdog_read(struct clocksource *cs, u64 *csnow, u64 *wdnow) >> wd_delay = clocksource_cyc2ns(wd_delta, watchdog->mult, >> watchdog->shift); >> if (wd_delay <= WATCHDOG_MAX_SKEW) { >> - if (nretries > 1 || nretries >= max_cswd_read_retries) { >> + if (nretries > 1) >> pr_warn("timekeeping watchdog on CPU%d: %s retried %d times before success\n", >> smp_processor_id(), watchdog->name, nretries); >> - } >> return true; >> } >> } >> -- >> 2.33.1 >>
| |