Re: [PATCH] clocksource: Improve cs_watchdog_read()

[Heiner Kallweit]

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@xxxxxxxxx>
>> ---
>>  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
>>