Re: [PATCH clocksource 4/5] clocksource: Provide a module parameter to fuzz per-CPU clock checking

From: "Paul E. McKenney" <paulmck@kernel.org>
To: Randy Dunlap <rdunlap@infradead.org>
Cc: linux-kernel@vger.kernel.org, kernel-team@fb.com,
	john.stultz@linaro.org, tglx@linutronix.de, sboyd@kernel.org,
	corbet@lwn.net, Mark.Rutland@arm.com, maz@kernel.org,
	ak@linux.intel.com, clm@fb.com
Subject: Re: [PATCH clocksource 4/5] clocksource: Provide a module parameter to fuzz per-CPU clock checking
Date: Tue, 2 Feb 2021 16:50:20 -0800	[thread overview]
Message-ID: <20210203005020.GI2743@paulmck-ThinkPad-P72> (raw)
In-Reply-To: <5badf6e2-4600-4fe9-6b45-d0de94ad718b@infradead.org>

On Tue, Feb 02, 2021 at 11:51:02AM -0800, Randy Dunlap wrote:
> On 2/2/21 9:06 AM, paulmck@kernel.org wrote:
> > From: "Paul E. McKenney" <paulmck@kernel.org>
> > 
> > Code that checks for clock desynchronization must itself be tested, so
> > this commit creates a new clocksource.inject_delay_shift_percpu= kernel
> > boot parameter that adds or subtracts a large value from the check read,
> > using the specified bit of the CPU ID to determine whether to add or
> > to subtract.
> > 
> > Cc: John Stultz <john.stultz@linaro.org>
> > Cc: Thomas Gleixner <tglx@linutronix.de>
> > Cc: Stephen Boyd <sboyd@kernel.org>
> > Cc: Jonathan Corbet <corbet@lwn.net>
> > Cc: Mark Rutland <Mark.Rutland@arm.com>
> > Cc: Marc Zyngier <maz@kernel.org>
> > Cc: Andi Kleen <ak@linux.intel.com>
> > Reported-by: Chris Mason <clm@fb.com>
> > Signed-off-by: Paul E. McKenney <paulmck@kernel.org>
> > ---
> >  Documentation/admin-guide/kernel-parameters.txt |  9 +++++++++
> >  kernel/time/clocksource.c                       | 10 +++++++++-
> >  2 files changed, 18 insertions(+), 1 deletion(-)
> > 
> > diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt
> > index 9965266..f561e94 100644
> > --- a/Documentation/admin-guide/kernel-parameters.txt
> > +++ b/Documentation/admin-guide/kernel-parameters.txt
> > @@ -593,6 +593,15 @@
> >  			times the value specified for inject_delay_freq
> >  			of consecutive non-delays.
> >  
> > +	clocksource.inject_delay_shift_percpu= [KNL]
> > +			Shift count to obtain bit from CPU number to
> > +			determine whether to shift the time of the per-CPU
> > +			clock under test ahead or behind.  For example,
> 
> It's a good think that you give an example -- it helps a little bit.
> That sentence above needs to be rewritten...

That is a bit obscure, now that you mention it.

> > +			setting this to the value four will result in
> > +			alternating groups of 16 CPUs shifting ahead and
> > +			the rest of the CPUs shifting behind.  The default
> > +			value of -1 disable this type of error injection.
> 
> 			            disables

Good eyes!

So how about like this?

	clocksource.inject_delay_shift_percpu= [KNL]
			Clocksource delay injection partitions the CPUs
			into two sets, one whose clocks are moved ahead
			and the other whose clocks are moved behind.
			This kernel parameter selects the CPU-number
			bit that determines which of these two sets the
			corresponding CPU is placed into.  For example,
			setting this parameter to the value four will
			result in the first set containing alternating
			groups of 16 CPUs whose clocks are moved ahead,
			while the second set will contain the rest of
			the CPUs, whose clocks are moved behind.

			The default value of -1 disables this type of
			error injection.

							Thanx, Paul

> > +
> >  	clocksource.max_read_retries= [KNL]
> >  			Number of clocksource_watchdog() retries due to
> >  			external delays before the clock will be marked
> 
> 
> -- 
> ~Randy
> 