[clocksource,4/5] clocksource: Provide a module parameter to fuzz per-CPU clock checking
diff mbox series

Message ID 20210202170635.24839-4-paulmck@kernel.org
State In Next
Commit db058f20e5ac1f7f4d3e3caf04a18b85e45724fb
Headers show
Series
  • [clocksource,1/5] clocksource: Provide module parameters to inject delays in watchdog
Related show

Commit Message

Paul E. McKenney Feb. 2, 2021, 5:06 p.m. UTC
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(-)

Comments

Randy Dunlap Feb. 2, 2021, 7:51 p.m. UTC | #1
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...

> +			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

> +
>  	clocksource.max_read_retries= [KNL]
>  			Number of clocksource_watchdog() retries due to
>  			external delays before the clock will be marked
Paul E. McKenney Feb. 3, 2021, 12:50 a.m. UTC | #2
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
>
Randy Dunlap Feb. 3, 2021, 1:31 a.m. UTC | #3
On 2/2/21 4:50 PM, Paul E. McKenney wrote:
> 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?

Much better, thanks.

> 	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

I know that in "writing," "four" should be written out as you have it,
but IMO using "4" here would be much better.  FWIW.

> 			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


thanks!
Paul E. McKenney Feb. 3, 2021, 1:40 a.m. UTC | #4
On Tue, Feb 02, 2021 at 05:31:55PM -0800, Randy Dunlap wrote:
> On 2/2/21 4:50 PM, Paul E. McKenney wrote:
> > 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?
> 
> Much better, thanks.
> 
> > 	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
> 
> I know that in "writing," "four" should be written out as you have it,
> but IMO using "4" here would be much better.  FWIW.

As long as it is "four" and not "fore"!  Updated as requested.  ;-)

						Thanx, Paul

> > 			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
> 
> 
> thanks!
> -- 
> ~Randy
>

Patch
diff mbox series

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,
+			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.
+
 	clocksource.max_read_retries= [KNL]
 			Number of clocksource_watchdog() retries due to
 			external delays before the clock will be marked
diff --git a/kernel/time/clocksource.c b/kernel/time/clocksource.c
index 23bcefe..67cf41c 100644
--- a/kernel/time/clocksource.c
+++ b/kernel/time/clocksource.c
@@ -190,6 +190,8 @@  static int inject_delay_freq;
 module_param(inject_delay_freq, int, 0644);
 static int inject_delay_run = 1;
 module_param(inject_delay_run, int, 0644);
+static int inject_delay_shift_percpu = -1;
+module_param(inject_delay_shift_percpu, int, 0644);
 static int max_read_retries = 3;
 module_param(max_read_retries, int, 0644);
 
@@ -219,8 +221,14 @@  static cpumask_t cpus_behind;
 static void clocksource_verify_one_cpu(void *csin)
 {
 	struct clocksource *cs = (struct clocksource *)csin;
+	s64 delta = 0;
+	int sign;
 
-	__this_cpu_write(csnow_mid, cs->read(cs));
+	if (inject_delay_shift_percpu >= 0) {
+		sign = ((smp_processor_id() >> inject_delay_shift_percpu) & 0x1) * 2 - 1;
+		delta = sign * NSEC_PER_SEC;
+	}
+	__this_cpu_write(csnow_mid, cs->read(cs) + delta);
 }
 
 static void clocksource_verify_percpu_wq(struct work_struct *unused)