[PATCH smp,csd] Throw an error if a CSD lock is stuck for too long

[PATCH smp,csd] Throw an error if a CSD lock is stuck for too long

[Paul E. McKenney]

The CSD lock seems to get stuck in 2 "modes". When it gets stuck
temporarily, it usually gets released in a few seconds, and sometimes
up to one or two minutes.

If the CSD lock stays stuck for more than several minutes, it never
seems to get unstuck, and gradually more and more things in the system
end up also getting stuck.

In the latter case, we should just give up, so the system can dump out
a little more information about what went wrong, and, with panic_on_oops
and a kdump kernel loaded, dump a whole bunch more information about
what might have gone wrong.

Question: should this have its own panic_on_ipistall switch in
/proc/sys/kernel, or maybe piggyback on panic_on_oops in a different
way than via BUG_ON?

Signed-off-by: Rik van Riel <riel@surriel.com>
Signed-off-by: Paul E. McKenney <paulmck@kernel.org>

diff --git a/kernel/smp.c b/kernel/smp.c
index 8455a53465af..059f1f53fc6b 100644
--- a/kernel/smp.c
+++ b/kernel/smp.c
@@ -230,6 +230,7 @@ static bool csd_lock_wait_toolong(struct __call_single_data *csd, u64 ts0, u64 *
 	}
 
 	ts2 = sched_clock();
+	/* How long since we last checked for a stuck CSD lock.*/
 	ts_delta = ts2 - *ts1;
 	if (likely(ts_delta <= csd_lock_timeout_ns || csd_lock_timeout_ns == 0))
 		return false;
@@ -243,9 +244,17 @@ static bool csd_lock_wait_toolong(struct __call_single_data *csd, u64 ts0, u64 *
 	else
 		cpux = cpu;
 	cpu_cur_csd = smp_load_acquire(&per_cpu(cur_csd, cpux)); /* Before func and info. */
+	/* How long since this CSD lock was stuck. */
+	ts_delta = ts2 - ts0;
 	pr_alert("csd: %s non-responsive CSD lock (#%d) on CPU#%d, waiting %llu ns for CPU#%02d %pS(%ps).\n",
-		 firsttime ? "Detected" : "Continued", *bug_id, raw_smp_processor_id(), ts2 - ts0,
+		 firsttime ? "Detected" : "Continued", *bug_id, raw_smp_processor_id(), ts_delta,
 		 cpu, csd->func, csd->info);
+	/*
+	 * If the CSD lock is still stuck after 5 minutes, it is unlikely
+	 * to become unstuck. Use a signed comparison to avoid triggering
+	 * on underflows when the TSC is out of sync between sockets.
+	 */
+	BUG_ON((s64)ts_delta > 300000000000LL);
 	if (cpu_cur_csd && csd != cpu_cur_csd) {
 		pr_alert("\tcsd: CSD lock (#%d) handling prior %pS(%ps) request.\n",
 			 *bug_id, READ_ONCE(per_cpu(cur_csd_func, cpux)),

Re: [PATCH smp,csd] Throw an error if a CSD lock is stuck for too long

[Imran Khan]

Hello Paul,

On 6/10/2023 3:48 am, Paul E. McKenney wrote:
> The CSD lock seems to get stuck in 2 "modes". When it gets stuck
> temporarily, it usually gets released in a few seconds, and sometimes
> up to one or two minutes.
> 
> If the CSD lock stays stuck for more than several minutes, it never
> seems to get unstuck, and gradually more and more things in the system
> end up also getting stuck.
> 
> In the latter case, we should just give up, so the system can dump out
> a little more information about what went wrong, and, with panic_on_oops
> and a kdump kernel loaded, dump a whole bunch more information about
> what might have gone wrong.
> 
> Question: should this have its own panic_on_ipistall switch in
> /proc/sys/kernel, or maybe piggyback on panic_on_oops in a different
> way than via BUG_ON?
> 
panic_on_ipistall (set to 1 by default) looks better option to me. For systems
where such delay is acceptable and system can eventually get back to sane state,
this option (set to 0 after boot) would prevent crashing the system for
apparently benign CSD hangs of long duration.

> Signed-off-by: Rik van Riel <riel@surriel.com>
> Signed-off-by: Paul E. McKenney <paulmck@kernel.org>
> 
> diff --git a/kernel/smp.c b/kernel/smp.c
> index 8455a53465af..059f1f53fc6b 100644
> --- a/kernel/smp.c
> +++ b/kernel/smp.c
> @@ -230,6 +230,7 @@ static bool csd_lock_wait_toolong(struct __call_single_data *csd, u64 ts0, u64 *
>  	}
>  
>  	ts2 = sched_clock();
> +	/* How long since we last checked for a stuck CSD lock.*/
>  	ts_delta = ts2 - *ts1;
>  	if (likely(ts_delta <= csd_lock_timeout_ns || csd_lock_timeout_ns == 0))
>  		return false;
> @@ -243,9 +244,17 @@ static bool csd_lock_wait_toolong(struct __call_single_data *csd, u64 ts0, u64 *
>  	else
>  		cpux = cpu;
>  	cpu_cur_csd = smp_load_acquire(&per_cpu(cur_csd, cpux)); /* Before func and info. */
> +	/* How long since this CSD lock was stuck. */
> +	ts_delta = ts2 - ts0;
>  	pr_alert("csd: %s non-responsive CSD lock (#%d) on CPU#%d, waiting %llu ns for CPU#%02d %pS(%ps).\n",
> -		 firsttime ? "Detected" : "Continued", *bug_id, raw_smp_processor_id(), ts2 - ts0,
> +		 firsttime ? "Detected" : "Continued", *bug_id, raw_smp_processor_id(), ts_delta,
>  		 cpu, csd->func, csd->info);
> +	/*
> +	 * If the CSD lock is still stuck after 5 minutes, it is unlikely
> +	 * to become unstuck. Use a signed comparison to avoid triggering
> +	 * on underflows when the TSC is out of sync between sockets.
> +	 */
> +	BUG_ON((s64)ts_delta > 300000000000LL);
Can we make this a module_param (default value 5 mins), so that if needed it can
be tweaked to a bigger/smaller value?
>  	if (cpu_cur_csd && csd != cpu_cur_csd) {
>  		pr_alert("\tcsd: CSD lock (#%d) handling prior %pS(%ps) request.\n",
>  			 *bug_id, READ_ONCE(per_cpu(cur_csd_func, cpux)),

Thanks,
Imran

Re: [PATCH smp,csd] Throw an error if a CSD lock is stuck for too long

[Paul E. McKenney]

On Fri, Oct 06, 2023 at 10:32:07AM +1100, Imran Khan wrote:
> Hello Paul,
> 
> On 6/10/2023 3:48 am, Paul E. McKenney wrote:
> > The CSD lock seems to get stuck in 2 "modes". When it gets stuck
> > temporarily, it usually gets released in a few seconds, and sometimes
> > up to one or two minutes.
> > 
> > If the CSD lock stays stuck for more than several minutes, it never
> > seems to get unstuck, and gradually more and more things in the system
> > end up also getting stuck.
> > 
> > In the latter case, we should just give up, so the system can dump out
> > a little more information about what went wrong, and, with panic_on_oops
> > and a kdump kernel loaded, dump a whole bunch more information about
> > what might have gone wrong.
> > 
> > Question: should this have its own panic_on_ipistall switch in
> > /proc/sys/kernel, or maybe piggyback on panic_on_oops in a different
> > way than via BUG_ON?
> > 
> panic_on_ipistall (set to 1 by default) looks better option to me. For systems
> where such delay is acceptable and system can eventually get back to sane state,
> this option (set to 0 after boot) would prevent crashing the system for
> apparently benign CSD hangs of long duration.

Good point!  How about like the following?

							Thanx, Paul

------------------------------------------------------------------------

commit 6bcf3786291b86f13b3e13d51e998737a8009ec3
Author: Rik van Riel <riel@surriel.com>
Date:   Mon Aug 21 16:04:09 2023 -0400

    smp,csd: Throw an error if a CSD lock is stuck for too long
    
    The CSD lock seems to get stuck in 2 "modes". When it gets stuck
    temporarily, it usually gets released in a few seconds, and sometimes
    up to one or two minutes.
    
    If the CSD lock stays stuck for more than several minutes, it never
    seems to get unstuck, and gradually more and more things in the system
    end up also getting stuck.
    
    In the latter case, we should just give up, so the system can dump out
    a little more information about what went wrong, and, with panic_on_oops
    and a kdump kernel loaded, dump a whole bunch more information about what
    might have gone wrong.  In addition, there is an smp.panic_on_ipistall
    kernel boot parameter that by default retains the old behavior, but when
    set enables the panic after the CSD lock has been stuck for more than
    five minutes.
    
    [ paulmck: Apply Imran Khan feedback. ]
    
    Link: https://lore.kernel.org/lkml/bc7cc8b0-f587-4451-8bcd-0daae627bcc7@paulmck-laptop/
    Signed-off-by: Rik van Riel <riel@surriel.com>
    Signed-off-by: Paul E. McKenney <paulmck@kernel.org>
    Cc: Peter Zijlstra <peterz@infradead.org>
    Cc: Valentin Schneider <vschneid@redhat.com>
    Cc: Juergen Gross <jgross@suse.com>
    Cc: Jonathan Corbet <corbet@lwn.net>
    Cc: Randy Dunlap <rdunlap@infradead.org>

diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt
index 0a1731a0f0ef..592935267ce2 100644
--- a/Documentation/admin-guide/kernel-parameters.txt
+++ b/Documentation/admin-guide/kernel-parameters.txt
@@ -5858,6 +5858,11 @@
 			This feature may be more efficiently disabled
 			using the csdlock_debug- kernel parameter.
 
+	smp.panic_on_ipistall= [KNL]
+			If a csd_lock_timeout extends for more than
+			five minutes, panic the system.  By default, let
+			CSD-lock acquisition take as long as they take.
+
 	smsc-ircc2.nopnp	[HW] Don't use PNP to discover SMC devices
 	smsc-ircc2.ircc_cfg=	[HW] Device configuration I/O port
 	smsc-ircc2.ircc_sir=	[HW] SIR base I/O port
diff --git a/kernel/smp.c b/kernel/smp.c
index 8455a53465af..b6a0773a7015 100644
--- a/kernel/smp.c
+++ b/kernel/smp.c
@@ -170,6 +170,8 @@ static DEFINE_PER_CPU(void *, cur_csd_info);
 
 static ulong csd_lock_timeout = 5000;  /* CSD lock timeout in milliseconds. */
 module_param(csd_lock_timeout, ulong, 0444);
+static bool panic_on_ipistall;
+module_param(panic_on_ipistall, bool, 0444);
 
 static atomic_t csd_bug_count = ATOMIC_INIT(0);
 
@@ -230,6 +232,7 @@ static bool csd_lock_wait_toolong(struct __call_single_data *csd, u64 ts0, u64 *
 	}
 
 	ts2 = sched_clock();
+	/* How long since we last checked for a stuck CSD lock.*/
 	ts_delta = ts2 - *ts1;
 	if (likely(ts_delta <= csd_lock_timeout_ns || csd_lock_timeout_ns == 0))
 		return false;
@@ -243,9 +246,17 @@ static bool csd_lock_wait_toolong(struct __call_single_data *csd, u64 ts0, u64 *
 	else
 		cpux = cpu;
 	cpu_cur_csd = smp_load_acquire(&per_cpu(cur_csd, cpux)); /* Before func and info. */
+	/* How long since this CSD lock was stuck. */
+	ts_delta = ts2 - ts0;
 	pr_alert("csd: %s non-responsive CSD lock (#%d) on CPU#%d, waiting %llu ns for CPU#%02d %pS(%ps).\n",
-		 firsttime ? "Detected" : "Continued", *bug_id, raw_smp_processor_id(), ts2 - ts0,
+		 firsttime ? "Detected" : "Continued", *bug_id, raw_smp_processor_id(), ts_delta,
 		 cpu, csd->func, csd->info);
+	/*
+	 * If the CSD lock is still stuck after 5 minutes, it is unlikely
+	 * to become unstuck. Use a signed comparison to avoid triggering
+	 * on underflows when the TSC is out of sync between sockets.
+	 */
+	BUG_ON(panic_on_ipistall && (s64)ts_delta > 300000000000LL);
 	if (cpu_cur_csd && csd != cpu_cur_csd) {
 		pr_alert("\tcsd: CSD lock (#%d) handling prior %pS(%ps) request.\n",
 			 *bug_id, READ_ONCE(per_cpu(cur_csd_func, cpux)),

Re: [PATCH smp,csd] Throw an error if a CSD lock is stuck for too long

[Imran Khan]

Hello Paul,

On 10/10/2023 3:39 am, Paul E. McKenney wrote:
> On Fri, Oct 06, 2023 at 10:32:07AM +1100, Imran Khan wrote:
>> Hello Paul,
>>
>> On 6/10/2023 3:48 am, Paul E. McKenney wrote:
>>> The CSD lock seems to get stuck in 2 "modes". When it gets stuck
>>> temporarily, it usually gets released in a few seconds, and sometimes
>>> up to one or two minutes.
>>>
>>> If the CSD lock stays stuck for more than several minutes, it never
>>> seems to get unstuck, and gradually more and more things in the system
>>> end up also getting stuck.
>>>
>>> In the latter case, we should just give up, so the system can dump out
>>> a little more information about what went wrong, and, with panic_on_oops
>>> and a kdump kernel loaded, dump a whole bunch more information about
>>> what might have gone wrong.
>>>
>>> Question: should this have its own panic_on_ipistall switch in
>>> /proc/sys/kernel, or maybe piggyback on panic_on_oops in a different
>>> way than via BUG_ON?
>>>
>> panic_on_ipistall (set to 1 by default) looks better option to me. For systems
>> where such delay is acceptable and system can eventually get back to sane state,
>> this option (set to 0 after boot) would prevent crashing the system for
>> apparently benign CSD hangs of long duration.
> 
> Good point!  How about like the following?
> 

Yes, this looks good.
Just realized that keeping panic_on_ipistall set by default(as suggested earlier
by me) would not follow convention of other similar switches like
hard/softlockup_panic etc. which are 0 by deafault.
So default value of 0 looks better choice for panic_on_ipistall as well.

> 							Thanx, Paul
> 
> ------------------------------------------------------------------------
> 
> commit 6bcf3786291b86f13b3e13d51e998737a8009ec3
> Author: Rik van Riel <riel@surriel.com>
> Date:   Mon Aug 21 16:04:09 2023 -0400
> 
>     smp,csd: Throw an error if a CSD lock is stuck for too long
>     
>     The CSD lock seems to get stuck in 2 "modes". When it gets stuck
>     temporarily, it usually gets released in a few seconds, and sometimes
>     up to one or two minutes.
>     
>     If the CSD lock stays stuck for more than several minutes, it never
>     seems to get unstuck, and gradually more and more things in the system
>     end up also getting stuck.
>     
>     In the latter case, we should just give up, so the system can dump out
>     a little more information about what went wrong, and, with panic_on_oops
>     and a kdump kernel loaded, dump a whole bunch more information about what
>     might have gone wrong.  In addition, there is an smp.panic_on_ipistall
>     kernel boot parameter that by default retains the old behavior, but when
>     set enables the panic after the CSD lock has been stuck for more than
>     five minutes.
>     
>     [ paulmck: Apply Imran Khan feedback. ]
>     
>     Link: https://urldefense.com/v3/__https://lore.kernel.org/lkml/bc7cc8b0-f587-4451-8bcd-0daae627bcc7@paulmck-laptop/__;!!ACWV5N9M2RV99hQ!PDFpjgGTCPjxqCyusua5IZWkvdWEMf51igFDc-yb9cVK9PYr7FpEE1oGpWp09YK4lc15C2taMdcuEOqyH8k$ 
>     Signed-off-by: Rik van Riel <riel@surriel.com>
>     Signed-off-by: Paul E. McKenney <paulmck@kernel.org>
>     Cc: Peter Zijlstra <peterz@infradead.org>
>     Cc: Valentin Schneider <vschneid@redhat.com>
>     Cc: Juergen Gross <jgross@suse.com>
>     Cc: Jonathan Corbet <corbet@lwn.net>
>     Cc: Randy Dunlap <rdunlap@infradead.org>
>
Reviewed-by: Imran Khan <imran.f.khan@oracle.com>

> diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt
> index 0a1731a0f0ef..592935267ce2 100644
> --- a/Documentation/admin-guide/kernel-parameters.txt
> +++ b/Documentation/admin-guide/kernel-parameters.txt
> @@ -5858,6 +5858,11 @@
>  			This feature may be more efficiently disabled
>  			using the csdlock_debug- kernel parameter.
>  
> +	smp.panic_on_ipistall= [KNL]
> +			If a csd_lock_timeout extends for more than
> +			five minutes, panic the system.  By default, let
> +			CSD-lock acquisition take as long as they take.
> +
>  	smsc-ircc2.nopnp	[HW] Don't use PNP to discover SMC devices
>  	smsc-ircc2.ircc_cfg=	[HW] Device configuration I/O port
>  	smsc-ircc2.ircc_sir=	[HW] SIR base I/O port
> diff --git a/kernel/smp.c b/kernel/smp.c
> index 8455a53465af..b6a0773a7015 100644
> --- a/kernel/smp.c
> +++ b/kernel/smp.c
> @@ -170,6 +170,8 @@ static DEFINE_PER_CPU(void *, cur_csd_info);
>  
>  static ulong csd_lock_timeout = 5000;  /* CSD lock timeout in milliseconds. */
>  module_param(csd_lock_timeout, ulong, 0444);
> +static bool panic_on_ipistall;
> +module_param(panic_on_ipistall, bool, 0444);
>  
>  static atomic_t csd_bug_count = ATOMIC_INIT(0);
>  
> @@ -230,6 +232,7 @@ static bool csd_lock_wait_toolong(struct __call_single_data *csd, u64 ts0, u64 *
>  	}
>  
>  	ts2 = sched_clock();
> +	/* How long since we last checked for a stuck CSD lock.*/
>  	ts_delta = ts2 - *ts1;
>  	if (likely(ts_delta <= csd_lock_timeout_ns || csd_lock_timeout_ns == 0))
>  		return false;
> @@ -243,9 +246,17 @@ static bool csd_lock_wait_toolong(struct __call_single_data *csd, u64 ts0, u64 *
>  	else
>  		cpux = cpu;
>  	cpu_cur_csd = smp_load_acquire(&per_cpu(cur_csd, cpux)); /* Before func and info. */
> +	/* How long since this CSD lock was stuck. */
> +	ts_delta = ts2 - ts0;
>  	pr_alert("csd: %s non-responsive CSD lock (#%d) on CPU#%d, waiting %llu ns for CPU#%02d %pS(%ps).\n",
> -		 firsttime ? "Detected" : "Continued", *bug_id, raw_smp_processor_id(), ts2 - ts0,
> +		 firsttime ? "Detected" : "Continued", *bug_id, raw_smp_processor_id(), ts_delta,
>  		 cpu, csd->func, csd->info);
> +	/*
> +	 * If the CSD lock is still stuck after 5 minutes, it is unlikely
> +	 * to become unstuck. Use a signed comparison to avoid triggering
> +	 * on underflows when the TSC is out of sync between sockets.
> +	 */
> +	BUG_ON(panic_on_ipistall && (s64)ts_delta > 300000000000LL);
>  	if (cpu_cur_csd && csd != cpu_cur_csd) {
>  		pr_alert("\tcsd: CSD lock (#%d) handling prior %pS(%ps) request.\n",
>  			 *bug_id, READ_ONCE(per_cpu(cur_csd_func, cpux)),

Re: [PATCH smp,csd] Throw an error if a CSD lock is stuck for too long

[Paul E. McKenney]

On Tue, Oct 10, 2023 at 03:58:43PM +1100, Imran Khan wrote:
> Hello Paul,
> 
> On 10/10/2023 3:39 am, Paul E. McKenney wrote:
> > On Fri, Oct 06, 2023 at 10:32:07AM +1100, Imran Khan wrote:
> >> Hello Paul,
> >>
> >> On 6/10/2023 3:48 am, Paul E. McKenney wrote:
> >>> The CSD lock seems to get stuck in 2 "modes". When it gets stuck
> >>> temporarily, it usually gets released in a few seconds, and sometimes
> >>> up to one or two minutes.
> >>>
> >>> If the CSD lock stays stuck for more than several minutes, it never
> >>> seems to get unstuck, and gradually more and more things in the system
> >>> end up also getting stuck.
> >>>
> >>> In the latter case, we should just give up, so the system can dump out
> >>> a little more information about what went wrong, and, with panic_on_oops
> >>> and a kdump kernel loaded, dump a whole bunch more information about
> >>> what might have gone wrong.
> >>>
> >>> Question: should this have its own panic_on_ipistall switch in
> >>> /proc/sys/kernel, or maybe piggyback on panic_on_oops in a different
> >>> way than via BUG_ON?
> >>>
> >> panic_on_ipistall (set to 1 by default) looks better option to me. For systems
> >> where such delay is acceptable and system can eventually get back to sane state,
> >> this option (set to 0 after boot) would prevent crashing the system for
> >> apparently benign CSD hangs of long duration.
> > 
> > Good point!  How about like the following?
> > 
> 
> Yes, this looks good.
> Just realized that keeping panic_on_ipistall set by default(as suggested earlier
> by me) would not follow convention of other similar switches like
> hard/softlockup_panic etc. which are 0 by deafault.
> So default value of 0 looks better choice for panic_on_ipistall as well.

Plus if a new option is set by default and causes problems, people
get (understandably) annoyed.  ;-)

> > ------------------------------------------------------------------------
> > 
> > commit 6bcf3786291b86f13b3e13d51e998737a8009ec3
> > Author: Rik van Riel <riel@surriel.com>
> > Date:   Mon Aug 21 16:04:09 2023 -0400
> > 
> >     smp,csd: Throw an error if a CSD lock is stuck for too long
> >     
> >     The CSD lock seems to get stuck in 2 "modes". When it gets stuck
> >     temporarily, it usually gets released in a few seconds, and sometimes
> >     up to one or two minutes.
> >     
> >     If the CSD lock stays stuck for more than several minutes, it never
> >     seems to get unstuck, and gradually more and more things in the system
> >     end up also getting stuck.
> >     
> >     In the latter case, we should just give up, so the system can dump out
> >     a little more information about what went wrong, and, with panic_on_oops
> >     and a kdump kernel loaded, dump a whole bunch more information about what
> >     might have gone wrong.  In addition, there is an smp.panic_on_ipistall
> >     kernel boot parameter that by default retains the old behavior, but when
> >     set enables the panic after the CSD lock has been stuck for more than
> >     five minutes.
> >     
> >     [ paulmck: Apply Imran Khan feedback. ]
> >     
> >     Link: https://urldefense.com/v3/__https://lore.kernel.org/lkml/bc7cc8b0-f587-4451-8bcd-0daae627bcc7@paulmck-laptop/__;!!ACWV5N9M2RV99hQ!PDFpjgGTCPjxqCyusua5IZWkvdWEMf51igFDc-yb9cVK9PYr7FpEE1oGpWp09YK4lc15C2taMdcuEOqyH8k$ 
> >     Signed-off-by: Rik van Riel <riel@surriel.com>
> >     Signed-off-by: Paul E. McKenney <paulmck@kernel.org>
> >     Cc: Peter Zijlstra <peterz@infradead.org>
> >     Cc: Valentin Schneider <vschneid@redhat.com>
> >     Cc: Juergen Gross <jgross@suse.com>
> >     Cc: Jonathan Corbet <corbet@lwn.net>
> >     Cc: Randy Dunlap <rdunlap@infradead.org>
> >
> Reviewed-by: Imran Khan <imran.f.khan@oracle.com>

Thank you, and I will apply this on my next rebase.

							Thanx, Paul

> > diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt
> > index 0a1731a0f0ef..592935267ce2 100644
> > --- a/Documentation/admin-guide/kernel-parameters.txt
> > +++ b/Documentation/admin-guide/kernel-parameters.txt
> > @@ -5858,6 +5858,11 @@
> >  			This feature may be more efficiently disabled
> >  			using the csdlock_debug- kernel parameter.
> >  
> > +	smp.panic_on_ipistall= [KNL]
> > +			If a csd_lock_timeout extends for more than
> > +			five minutes, panic the system.  By default, let
> > +			CSD-lock acquisition take as long as they take.
> > +
> >  	smsc-ircc2.nopnp	[HW] Don't use PNP to discover SMC devices
> >  	smsc-ircc2.ircc_cfg=	[HW] Device configuration I/O port
> >  	smsc-ircc2.ircc_sir=	[HW] SIR base I/O port
> > diff --git a/kernel/smp.c b/kernel/smp.c
> > index 8455a53465af..b6a0773a7015 100644
> > --- a/kernel/smp.c
> > +++ b/kernel/smp.c
> > @@ -170,6 +170,8 @@ static DEFINE_PER_CPU(void *, cur_csd_info);
> >  
> >  static ulong csd_lock_timeout = 5000;  /* CSD lock timeout in milliseconds. */
> >  module_param(csd_lock_timeout, ulong, 0444);
> > +static bool panic_on_ipistall;
> > +module_param(panic_on_ipistall, bool, 0444);
> >  
> >  static atomic_t csd_bug_count = ATOMIC_INIT(0);
> >  
> > @@ -230,6 +232,7 @@ static bool csd_lock_wait_toolong(struct __call_single_data *csd, u64 ts0, u64 *
> >  	}
> >  
> >  	ts2 = sched_clock();
> > +	/* How long since we last checked for a stuck CSD lock.*/
> >  	ts_delta = ts2 - *ts1;
> >  	if (likely(ts_delta <= csd_lock_timeout_ns || csd_lock_timeout_ns == 0))
> >  		return false;
> > @@ -243,9 +246,17 @@ static bool csd_lock_wait_toolong(struct __call_single_data *csd, u64 ts0, u64 *
> >  	else
> >  		cpux = cpu;
> >  	cpu_cur_csd = smp_load_acquire(&per_cpu(cur_csd, cpux)); /* Before func and info. */
> > +	/* How long since this CSD lock was stuck. */
> > +	ts_delta = ts2 - ts0;
> >  	pr_alert("csd: %s non-responsive CSD lock (#%d) on CPU#%d, waiting %llu ns for CPU#%02d %pS(%ps).\n",
> > -		 firsttime ? "Detected" : "Continued", *bug_id, raw_smp_processor_id(), ts2 - ts0,
> > +		 firsttime ? "Detected" : "Continued", *bug_id, raw_smp_processor_id(), ts_delta,
> >  		 cpu, csd->func, csd->info);
> > +	/*
> > +	 * If the CSD lock is still stuck after 5 minutes, it is unlikely
> > +	 * to become unstuck. Use a signed comparison to avoid triggering
> > +	 * on underflows when the TSC is out of sync between sockets.
> > +	 */
> > +	BUG_ON(panic_on_ipistall && (s64)ts_delta > 300000000000LL);
> >  	if (cpu_cur_csd && csd != cpu_cur_csd) {
> >  		pr_alert("\tcsd: CSD lock (#%d) handling prior %pS(%ps) request.\n",
> >  			 *bug_id, READ_ONCE(per_cpu(cur_csd_func, cpux)),

Re: [PATCH smp,csd] Throw an error if a CSD lock is stuck for too long

[Leonardo Bras]

On Mon, Oct 09, 2023 at 09:39:38AM -0700, Paul E. McKenney wrote:
> On Fri, Oct 06, 2023 at 10:32:07AM +1100, Imran Khan wrote:
> > Hello Paul,
> > 
> > On 6/10/2023 3:48 am, Paul E. McKenney wrote:
> > > The CSD lock seems to get stuck in 2 "modes". When it gets stuck
> > > temporarily, it usually gets released in a few seconds, and sometimes
> > > up to one or two minutes.
> > > 
> > > If the CSD lock stays stuck for more than several minutes, it never
> > > seems to get unstuck, and gradually more and more things in the system
> > > end up also getting stuck.
> > > 
> > > In the latter case, we should just give up, so the system can dump out
> > > a little more information about what went wrong, and, with panic_on_oops
> > > and a kdump kernel loaded, dump a whole bunch more information about
> > > what might have gone wrong.
> > > 
> > > Question: should this have its own panic_on_ipistall switch in
> > > /proc/sys/kernel, or maybe piggyback on panic_on_oops in a different
> > > way than via BUG_ON?
> > > 
> > panic_on_ipistall (set to 1 by default) looks better option to me. For systems
> > where such delay is acceptable and system can eventually get back to sane state,
> > this option (set to 0 after boot) would prevent crashing the system for
> > apparently benign CSD hangs of long duration.
> 
> Good point!  How about like the following?
> 
> 							Thanx, Paul
> 
> ------------------------------------------------------------------------
> 
> commit 6bcf3786291b86f13b3e13d51e998737a8009ec3
> Author: Rik van Riel <riel@surriel.com>
> Date:   Mon Aug 21 16:04:09 2023 -0400
> 
>     smp,csd: Throw an error if a CSD lock is stuck for too long
>     
>     The CSD lock seems to get stuck in 2 "modes". When it gets stuck
>     temporarily, it usually gets released in a few seconds, and sometimes
>     up to one or two minutes.
>     
>     If the CSD lock stays stuck for more than several minutes, it never
>     seems to get unstuck, and gradually more and more things in the system
>     end up also getting stuck.
>     
>     In the latter case, we should just give up, so the system can dump out
>     a little more information about what went wrong, and, with panic_on_oops
>     and a kdump kernel loaded, dump a whole bunch more information about what
>     might have gone wrong.  In addition, there is an smp.panic_on_ipistall
>     kernel boot parameter that by default retains the old behavior, but when
>     set enables the panic after the CSD lock has been stuck for more than
>     five minutes.
>     
>     [ paulmck: Apply Imran Khan feedback. ]
>     
>     Link: https://lore.kernel.org/lkml/bc7cc8b0-f587-4451-8bcd-0daae627bcc7@paulmck-laptop/
>     Signed-off-by: Rik van Riel <riel@surriel.com>
>     Signed-off-by: Paul E. McKenney <paulmck@kernel.org>
>     Cc: Peter Zijlstra <peterz@infradead.org>
>     Cc: Valentin Schneider <vschneid@redhat.com>
>     Cc: Juergen Gross <jgross@suse.com>
>     Cc: Jonathan Corbet <corbet@lwn.net>
>     Cc: Randy Dunlap <rdunlap@infradead.org>
> 
> diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt
> index 0a1731a0f0ef..592935267ce2 100644
> --- a/Documentation/admin-guide/kernel-parameters.txt
> +++ b/Documentation/admin-guide/kernel-parameters.txt
> @@ -5858,6 +5858,11 @@
>  			This feature may be more efficiently disabled
>  			using the csdlock_debug- kernel parameter.
>  
> +	smp.panic_on_ipistall= [KNL]
> +			If a csd_lock_timeout extends for more than
> +			five minutes, panic the system.  By default, let
> +			CSD-lock acquisition take as long as they take.
> +

It could be interesting to have it as an s64 parameter (in {mili,}seconds) 
instead of bool, this way the user could pick the time to wait before the 
panic happens. 0 or -1 could mean disabled.

What do you think?

Other than that,
Reviewed-by: Leonardo Bras <leobras@redhat.com>


>  	smsc-ircc2.nopnp	[HW] Don't use PNP to discover SMC devices
>  	smsc-ircc2.ircc_cfg=	[HW] Device configuration I/O port
>  	smsc-ircc2.ircc_sir=	[HW] SIR base I/O port
> diff --git a/kernel/smp.c b/kernel/smp.c
> index 8455a53465af..b6a0773a7015 100644
> --- a/kernel/smp.c
> +++ b/kernel/smp.c
> @@ -170,6 +170,8 @@ static DEFINE_PER_CPU(void *, cur_csd_info);
>  
>  static ulong csd_lock_timeout = 5000;  /* CSD lock timeout in milliseconds. */
>  module_param(csd_lock_timeout, ulong, 0444);
> +static bool panic_on_ipistall;
> +module_param(panic_on_ipistall, bool, 0444);
>  
>  static atomic_t csd_bug_count = ATOMIC_INIT(0);
>  
> @@ -230,6 +232,7 @@ static bool csd_lock_wait_toolong(struct __call_single_data *csd, u64 ts0, u64 *
>  	}
>  
>  	ts2 = sched_clock();
> +	/* How long since we last checked for a stuck CSD lock.*/
>  	ts_delta = ts2 - *ts1;
>  	if (likely(ts_delta <= csd_lock_timeout_ns || csd_lock_timeout_ns == 0))
>  		return false;
> @@ -243,9 +246,17 @@ static bool csd_lock_wait_toolong(struct __call_single_data *csd, u64 ts0, u64 *
>  	else
>  		cpux = cpu;
>  	cpu_cur_csd = smp_load_acquire(&per_cpu(cur_csd, cpux)); /* Before func and info. */
> +	/* How long since this CSD lock was stuck. */
> +	ts_delta = ts2 - ts0;
>  	pr_alert("csd: %s non-responsive CSD lock (#%d) on CPU#%d, waiting %llu ns for CPU#%02d %pS(%ps).\n",
> -		 firsttime ? "Detected" : "Continued", *bug_id, raw_smp_processor_id(), ts2 - ts0,
> +		 firsttime ? "Detected" : "Continued", *bug_id, raw_smp_processor_id(), ts_delta,
>  		 cpu, csd->func, csd->info);
> +	/*
> +	 * If the CSD lock is still stuck after 5 minutes, it is unlikely
> +	 * to become unstuck. Use a signed comparison to avoid triggering
> +	 * on underflows when the TSC is out of sync between sockets.
> +	 */
> +	BUG_ON(panic_on_ipistall && (s64)ts_delta > 300000000000LL);
>  	if (cpu_cur_csd && csd != cpu_cur_csd) {
>  		pr_alert("\tcsd: CSD lock (#%d) handling prior %pS(%ps) request.\n",
>  			 *bug_id, READ_ONCE(per_cpu(cur_csd_func, cpux)),
> 

Re: [PATCH smp,csd] Throw an error if a CSD lock is stuck for too long

[Paul E. McKenney]

On Fri, Oct 13, 2023 at 12:26:22PM -0300, Leonardo Bras wrote:
> On Mon, Oct 09, 2023 at 09:39:38AM -0700, Paul E. McKenney wrote:
> > On Fri, Oct 06, 2023 at 10:32:07AM +1100, Imran Khan wrote:
> > > Hello Paul,
> > > 
> > > On 6/10/2023 3:48 am, Paul E. McKenney wrote:
> > > > The CSD lock seems to get stuck in 2 "modes". When it gets stuck
> > > > temporarily, it usually gets released in a few seconds, and sometimes
> > > > up to one or two minutes.
> > > > 
> > > > If the CSD lock stays stuck for more than several minutes, it never
> > > > seems to get unstuck, and gradually more and more things in the system
> > > > end up also getting stuck.
> > > > 
> > > > In the latter case, we should just give up, so the system can dump out
> > > > a little more information about what went wrong, and, with panic_on_oops
> > > > and a kdump kernel loaded, dump a whole bunch more information about
> > > > what might have gone wrong.
> > > > 
> > > > Question: should this have its own panic_on_ipistall switch in
> > > > /proc/sys/kernel, or maybe piggyback on panic_on_oops in a different
> > > > way than via BUG_ON?
> > > > 
> > > panic_on_ipistall (set to 1 by default) looks better option to me. For systems
> > > where such delay is acceptable and system can eventually get back to sane state,
> > > this option (set to 0 after boot) would prevent crashing the system for
> > > apparently benign CSD hangs of long duration.
> > 
> > Good point!  How about like the following?
> > 
> > 							Thanx, Paul
> > 
> > ------------------------------------------------------------------------
> > 
> > commit 6bcf3786291b86f13b3e13d51e998737a8009ec3
> > Author: Rik van Riel <riel@surriel.com>
> > Date:   Mon Aug 21 16:04:09 2023 -0400
> > 
> >     smp,csd: Throw an error if a CSD lock is stuck for too long
> >     
> >     The CSD lock seems to get stuck in 2 "modes". When it gets stuck
> >     temporarily, it usually gets released in a few seconds, and sometimes
> >     up to one or two minutes.
> >     
> >     If the CSD lock stays stuck for more than several minutes, it never
> >     seems to get unstuck, and gradually more and more things in the system
> >     end up also getting stuck.
> >     
> >     In the latter case, we should just give up, so the system can dump out
> >     a little more information about what went wrong, and, with panic_on_oops
> >     and a kdump kernel loaded, dump a whole bunch more information about what
> >     might have gone wrong.  In addition, there is an smp.panic_on_ipistall
> >     kernel boot parameter that by default retains the old behavior, but when
> >     set enables the panic after the CSD lock has been stuck for more than
> >     five minutes.
> >     
> >     [ paulmck: Apply Imran Khan feedback. ]
> >     
> >     Link: https://lore.kernel.org/lkml/bc7cc8b0-f587-4451-8bcd-0daae627bcc7@paulmck-laptop/
> >     Signed-off-by: Rik van Riel <riel@surriel.com>
> >     Signed-off-by: Paul E. McKenney <paulmck@kernel.org>
> >     Cc: Peter Zijlstra <peterz@infradead.org>
> >     Cc: Valentin Schneider <vschneid@redhat.com>
> >     Cc: Juergen Gross <jgross@suse.com>
> >     Cc: Jonathan Corbet <corbet@lwn.net>
> >     Cc: Randy Dunlap <rdunlap@infradead.org>
> > 
> > diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt
> > index 0a1731a0f0ef..592935267ce2 100644
> > --- a/Documentation/admin-guide/kernel-parameters.txt
> > +++ b/Documentation/admin-guide/kernel-parameters.txt
> > @@ -5858,6 +5858,11 @@
> >  			This feature may be more efficiently disabled
> >  			using the csdlock_debug- kernel parameter.
> >  
> > +	smp.panic_on_ipistall= [KNL]
> > +			If a csd_lock_timeout extends for more than
> > +			five minutes, panic the system.  By default, let
> > +			CSD-lock acquisition take as long as they take.
> > +
> 
> It could be interesting to have it as an s64 parameter (in {mili,}seconds) 
> instead of bool, this way the user could pick the time to wait before the 
> panic happens. 0 or -1 could mean disabled.
> 
> What do you think?
> 
> Other than that,
> Reviewed-by: Leonardo Bras <leobras@redhat.com>

Thank you for looking this over!

How about with the diff shown below, to be folded into the original?
I went with int instead of s64 because I am having some difficulty
imagining anyone specifying more than a 24-day timeout.  ;-)

							Thanx, Paul

------------------------------------------------------------------------

diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt
index ccb7621eff79..ea5ae9deb753 100644
--- a/Documentation/admin-guide/kernel-parameters.txt
+++ b/Documentation/admin-guide/kernel-parameters.txt
@@ -5931,8 +5931,10 @@
 
 	smp.panic_on_ipistall= [KNL]
 			If a csd_lock_timeout extends for more than
-			five minutes, panic the system.  By default, let
-			CSD-lock acquisition take as long as they take.
+			the specified number of milliseconds, panic the
+			system.  By default, let CSD-lock acquisition
+			take as long as they take.  Specifying 300,000
+			for this value provides a 10-minute timeout.
 
 	smsc-ircc2.nopnp	[HW] Don't use PNP to discover SMC devices
 	smsc-ircc2.ircc_cfg=	[HW] Device configuration I/O port
diff --git a/kernel/smp.c b/kernel/smp.c
index b6a0773a7015..d3ca47f32f38 100644
--- a/kernel/smp.c
+++ b/kernel/smp.c
@@ -170,8 +170,8 @@ static DEFINE_PER_CPU(void *, cur_csd_info);
 
 static ulong csd_lock_timeout = 5000;  /* CSD lock timeout in milliseconds. */
 module_param(csd_lock_timeout, ulong, 0444);
-static bool panic_on_ipistall;
-module_param(panic_on_ipistall, bool, 0444);
+static int panic_on_ipistall;  /* CSD panic timeout in milliseconds, 300000 for ten minutes. */
+module_param(panic_on_ipistall, int, 0444);
 
 static atomic_t csd_bug_count = ATOMIC_INIT(0);
 
@@ -256,7 +256,7 @@ static bool csd_lock_wait_toolong(struct __call_single_data *csd, u64 ts0, u64 *
 	 * to become unstuck. Use a signed comparison to avoid triggering
 	 * on underflows when the TSC is out of sync between sockets.
 	 */
-	BUG_ON(panic_on_ipistall && (s64)ts_delta > 300000000000LL);
+	BUG_ON(panic_on_ipistall > 0 && (s64)ts_delta > ((s64)panic_on_ipistall * NSEC_PER_MSEC));
 	if (cpu_cur_csd && csd != cpu_cur_csd) {
 		pr_alert("\tcsd: CSD lock (#%d) handling prior %pS(%ps) request.\n",
 			 *bug_id, READ_ONCE(per_cpu(cur_csd_func, cpux)),

Re: [PATCH smp,csd] Throw an error if a CSD lock is stuck for too long

[Leonardo Bras]

On Mon, Oct 16, 2023 at 11:27:51AM -0700, Paul E. McKenney wrote:
> On Fri, Oct 13, 2023 at 12:26:22PM -0300, Leonardo Bras wrote:
> > On Mon, Oct 09, 2023 at 09:39:38AM -0700, Paul E. McKenney wrote:
> > > On Fri, Oct 06, 2023 at 10:32:07AM +1100, Imran Khan wrote:
> > > > Hello Paul,
> > > > 
> > > > On 6/10/2023 3:48 am, Paul E. McKenney wrote:
> > > > > The CSD lock seems to get stuck in 2 "modes". When it gets stuck
> > > > > temporarily, it usually gets released in a few seconds, and sometimes
> > > > > up to one or two minutes.
> > > > > 
> > > > > If the CSD lock stays stuck for more than several minutes, it never
> > > > > seems to get unstuck, and gradually more and more things in the system
> > > > > end up also getting stuck.
> > > > > 
> > > > > In the latter case, we should just give up, so the system can dump out
> > > > > a little more information about what went wrong, and, with panic_on_oops
> > > > > and a kdump kernel loaded, dump a whole bunch more information about
> > > > > what might have gone wrong.
> > > > > 
> > > > > Question: should this have its own panic_on_ipistall switch in
> > > > > /proc/sys/kernel, or maybe piggyback on panic_on_oops in a different
> > > > > way than via BUG_ON?
> > > > > 
> > > > panic_on_ipistall (set to 1 by default) looks better option to me. For systems
> > > > where such delay is acceptable and system can eventually get back to sane state,
> > > > this option (set to 0 after boot) would prevent crashing the system for
> > > > apparently benign CSD hangs of long duration.
> > > 
> > > Good point!  How about like the following?
> > > 
> > > 							Thanx, Paul
> > > 
> > > ------------------------------------------------------------------------
> > > 
> > > commit 6bcf3786291b86f13b3e13d51e998737a8009ec3
> > > Author: Rik van Riel <riel@surriel.com>
> > > Date:   Mon Aug 21 16:04:09 2023 -0400
> > > 
> > >     smp,csd: Throw an error if a CSD lock is stuck for too long
> > >     
> > >     The CSD lock seems to get stuck in 2 "modes". When it gets stuck
> > >     temporarily, it usually gets released in a few seconds, and sometimes
> > >     up to one or two minutes.
> > >     
> > >     If the CSD lock stays stuck for more than several minutes, it never
> > >     seems to get unstuck, and gradually more and more things in the system
> > >     end up also getting stuck.
> > >     
> > >     In the latter case, we should just give up, so the system can dump out
> > >     a little more information about what went wrong, and, with panic_on_oops
> > >     and a kdump kernel loaded, dump a whole bunch more information about what
> > >     might have gone wrong.  In addition, there is an smp.panic_on_ipistall
> > >     kernel boot parameter that by default retains the old behavior, but when
> > >     set enables the panic after the CSD lock has been stuck for more than
> > >     five minutes.
> > >     
> > >     [ paulmck: Apply Imran Khan feedback. ]
> > >     
> > >     Link: https://lore.kernel.org/lkml/bc7cc8b0-f587-4451-8bcd-0daae627bcc7@paulmck-laptop/
> > >     Signed-off-by: Rik van Riel <riel@surriel.com>
> > >     Signed-off-by: Paul E. McKenney <paulmck@kernel.org>
> > >     Cc: Peter Zijlstra <peterz@infradead.org>
> > >     Cc: Valentin Schneider <vschneid@redhat.com>
> > >     Cc: Juergen Gross <jgross@suse.com>
> > >     Cc: Jonathan Corbet <corbet@lwn.net>
> > >     Cc: Randy Dunlap <rdunlap@infradead.org>
> > > 
> > > diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt
> > > index 0a1731a0f0ef..592935267ce2 100644
> > > --- a/Documentation/admin-guide/kernel-parameters.txt
> > > +++ b/Documentation/admin-guide/kernel-parameters.txt
> > > @@ -5858,6 +5858,11 @@
> > >  			This feature may be more efficiently disabled
> > >  			using the csdlock_debug- kernel parameter.
> > >  
> > > +	smp.panic_on_ipistall= [KNL]
> > > +			If a csd_lock_timeout extends for more than
> > > +			five minutes, panic the system.  By default, let
> > > +			CSD-lock acquisition take as long as they take.
> > > +
> > 
> > It could be interesting to have it as an s64 parameter (in {mili,}seconds) 
> > instead of bool, this way the user could pick the time to wait before the 
> > panic happens. 0 or -1 could mean disabled.
> > 
> > What do you think?
> > 
> > Other than that,
> > Reviewed-by: Leonardo Bras <leobras@redhat.com>
> 
> Thank you for looking this over!
> 
> How about with the diff shown below, to be folded into the original?
> I went with int instead of s64 because I am having some difficulty
> imagining anyone specifying more than a 24-day timeout.  ;-)

I suggested s64 just because it was the type being used in
BUG_ON(panic_on_ipistall && (s64)ts_delta > 300000000000LL);

But anyway, int should be fine.


> 
> 							Thanx, Paul
> 
> ------------------------------------------------------------------------
> 
> diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt
> index ccb7621eff79..ea5ae9deb753 100644
> --- a/Documentation/admin-guide/kernel-parameters.txt
> +++ b/Documentation/admin-guide/kernel-parameters.txt
> @@ -5931,8 +5931,10 @@
>  
>  	smp.panic_on_ipistall= [KNL]
>  			If a csd_lock_timeout extends for more than
> -			five minutes, panic the system.  By default, let
> -			CSD-lock acquisition take as long as they take.
> +			the specified number of milliseconds, panic the
> +			system.  By default, let CSD-lock acquisition
> +			take as long as they take.  Specifying 300,000
> +			for this value provides a 10-minute timeout.

300,000 ms is 300s, which is 5 minutes, right?

>  
>  	smsc-ircc2.nopnp	[HW] Don't use PNP to discover SMC devices
>  	smsc-ircc2.ircc_cfg=	[HW] Device configuration I/O port
> diff --git a/kernel/smp.c b/kernel/smp.c
> index b6a0773a7015..d3ca47f32f38 100644
> --- a/kernel/smp.c
> +++ b/kernel/smp.c
> @@ -170,8 +170,8 @@ static DEFINE_PER_CPU(void *, cur_csd_info);
>  
>  static ulong csd_lock_timeout = 5000;  /* CSD lock timeout in milliseconds. */
>  module_param(csd_lock_timeout, ulong, 0444);
> -static bool panic_on_ipistall;
> -module_param(panic_on_ipistall, bool, 0444);
> +static int panic_on_ipistall;  /* CSD panic timeout in milliseconds, 300000 for ten minutes. */

s/ten/five

> +module_param(panic_on_ipistall, int, 0444);
>  
>  static atomic_t csd_bug_count = ATOMIC_INIT(0);
>  
> @@ -256,7 +256,7 @@ static bool csd_lock_wait_toolong(struct __call_single_data *csd, u64 ts0, u64 *
>  	 * to become unstuck. Use a signed comparison to avoid triggering
>  	 * on underflows when the TSC is out of sync between sockets.
>  	 */
> -	BUG_ON(panic_on_ipistall && (s64)ts_delta > 300000000000LL);
> +	BUG_ON(panic_on_ipistall > 0 && (s64)ts_delta > ((s64)panic_on_ipistall * NSEC_PER_MSEC));

s64 here would avoid casting (s64)panic_on_ipistall, but I think it does 
not really impact readability.  

IIUC ts_delta is an u64 being casted as s64, which could be an issue but no 
computer system will actually take over 2^31 ns (292 years) to run 1 
iteration, so it's safe.

I think it's a nice feaure :)

Thanks!
Leo


>  	if (cpu_cur_csd && csd != cpu_cur_csd) {
>  		pr_alert("\tcsd: CSD lock (#%d) handling prior %pS(%ps) request.\n",
>  			 *bug_id, READ_ONCE(per_cpu(cur_csd_func, cpux)),
> 

Re: [PATCH smp,csd] Throw an error if a CSD lock is stuck for too long

[Leonardo Bras]

On Mon, Oct 16, 2023 at 05:52:57PM -0300, Leonardo Bras wrote:
> On Mon, Oct 16, 2023 at 11:27:51AM -0700, Paul E. McKenney wrote:
> > On Fri, Oct 13, 2023 at 12:26:22PM -0300, Leonardo Bras wrote:
> > > On Mon, Oct 09, 2023 at 09:39:38AM -0700, Paul E. McKenney wrote:
> > > > On Fri, Oct 06, 2023 at 10:32:07AM +1100, Imran Khan wrote:
> > > > > Hello Paul,
> > > > > 
> > > > > On 6/10/2023 3:48 am, Paul E. McKenney wrote:
> > > > > > The CSD lock seems to get stuck in 2 "modes". When it gets stuck
> > > > > > temporarily, it usually gets released in a few seconds, and sometimes
> > > > > > up to one or two minutes.
> > > > > > 
> > > > > > If the CSD lock stays stuck for more than several minutes, it never
> > > > > > seems to get unstuck, and gradually more and more things in the system
> > > > > > end up also getting stuck.
> > > > > > 
> > > > > > In the latter case, we should just give up, so the system can dump out
> > > > > > a little more information about what went wrong, and, with panic_on_oops
> > > > > > and a kdump kernel loaded, dump a whole bunch more information about
> > > > > > what might have gone wrong.
> > > > > > 
> > > > > > Question: should this have its own panic_on_ipistall switch in
> > > > > > /proc/sys/kernel, or maybe piggyback on panic_on_oops in a different
> > > > > > way than via BUG_ON?
> > > > > > 
> > > > > panic_on_ipistall (set to 1 by default) looks better option to me. For systems
> > > > > where such delay is acceptable and system can eventually get back to sane state,
> > > > > this option (set to 0 after boot) would prevent crashing the system for
> > > > > apparently benign CSD hangs of long duration.
> > > > 
> > > > Good point!  How about like the following?
> > > > 
> > > > 							Thanx, Paul
> > > > 
> > > > ------------------------------------------------------------------------
> > > > 
> > > > commit 6bcf3786291b86f13b3e13d51e998737a8009ec3
> > > > Author: Rik van Riel <riel@surriel.com>
> > > > Date:   Mon Aug 21 16:04:09 2023 -0400
> > > > 
> > > >     smp,csd: Throw an error if a CSD lock is stuck for too long
> > > >     
> > > >     The CSD lock seems to get stuck in 2 "modes". When it gets stuck
> > > >     temporarily, it usually gets released in a few seconds, and sometimes
> > > >     up to one or two minutes.
> > > >     
> > > >     If the CSD lock stays stuck for more than several minutes, it never
> > > >     seems to get unstuck, and gradually more and more things in the system
> > > >     end up also getting stuck.
> > > >     
> > > >     In the latter case, we should just give up, so the system can dump out
> > > >     a little more information about what went wrong, and, with panic_on_oops
> > > >     and a kdump kernel loaded, dump a whole bunch more information about what
> > > >     might have gone wrong.  In addition, there is an smp.panic_on_ipistall
> > > >     kernel boot parameter that by default retains the old behavior, but when
> > > >     set enables the panic after the CSD lock has been stuck for more than
> > > >     five minutes.
> > > >     
> > > >     [ paulmck: Apply Imran Khan feedback. ]
> > > >     
> > > >     Link: https://lore.kernel.org/lkml/bc7cc8b0-f587-4451-8bcd-0daae627bcc7@paulmck-laptop/
> > > >     Signed-off-by: Rik van Riel <riel@surriel.com>
> > > >     Signed-off-by: Paul E. McKenney <paulmck@kernel.org>
> > > >     Cc: Peter Zijlstra <peterz@infradead.org>
> > > >     Cc: Valentin Schneider <vschneid@redhat.com>
> > > >     Cc: Juergen Gross <jgross@suse.com>
> > > >     Cc: Jonathan Corbet <corbet@lwn.net>
> > > >     Cc: Randy Dunlap <rdunlap@infradead.org>
> > > > 
> > > > diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt
> > > > index 0a1731a0f0ef..592935267ce2 100644
> > > > --- a/Documentation/admin-guide/kernel-parameters.txt
> > > > +++ b/Documentation/admin-guide/kernel-parameters.txt
> > > > @@ -5858,6 +5858,11 @@
> > > >  			This feature may be more efficiently disabled
> > > >  			using the csdlock_debug- kernel parameter.
> > > >  
> > > > +	smp.panic_on_ipistall= [KNL]
> > > > +			If a csd_lock_timeout extends for more than
> > > > +			five minutes, panic the system.  By default, let
> > > > +			CSD-lock acquisition take as long as they take.
> > > > +
> > > 
> > > It could be interesting to have it as an s64 parameter (in {mili,}seconds) 
> > > instead of bool, this way the user could pick the time to wait before the 
> > > panic happens. 0 or -1 could mean disabled.
> > > 
> > > What do you think?
> > > 
> > > Other than that,
> > > Reviewed-by: Leonardo Bras <leobras@redhat.com>
> > 
> > Thank you for looking this over!
> > 
> > How about with the diff shown below, to be folded into the original?
> > I went with int instead of s64 because I am having some difficulty
> > imagining anyone specifying more than a 24-day timeout.  ;-)
> 
> I suggested s64 just because it was the type being used in
> BUG_ON(panic_on_ipistall && (s64)ts_delta > 300000000000LL);
> 
> But anyway, int should be fine.
> 
> 
> > 
> > 							Thanx, Paul
> > 
> > ------------------------------------------------------------------------
> > 
> > diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt
> > index ccb7621eff79..ea5ae9deb753 100644
> > --- a/Documentation/admin-guide/kernel-parameters.txt
> > +++ b/Documentation/admin-guide/kernel-parameters.txt
> > @@ -5931,8 +5931,10 @@
> >  
> >  	smp.panic_on_ipistall= [KNL]
> >  			If a csd_lock_timeout extends for more than
> > -			five minutes, panic the system.  By default, let
> > -			CSD-lock acquisition take as long as they take.
> > +			the specified number of milliseconds, panic the
> > +			system.  By default, let CSD-lock acquisition
> > +			take as long as they take.  Specifying 300,000
> > +			for this value provides a 10-minute timeout.
> 
> 300,000 ms is 300s, which is 5 minutes, right?
> 
> >  
> >  	smsc-ircc2.nopnp	[HW] Don't use PNP to discover SMC devices
> >  	smsc-ircc2.ircc_cfg=	[HW] Device configuration I/O port
> > diff --git a/kernel/smp.c b/kernel/smp.c
> > index b6a0773a7015..d3ca47f32f38 100644
> > --- a/kernel/smp.c
> > +++ b/kernel/smp.c
> > @@ -170,8 +170,8 @@ static DEFINE_PER_CPU(void *, cur_csd_info);
> >  
> >  static ulong csd_lock_timeout = 5000;  /* CSD lock timeout in milliseconds. */
> >  module_param(csd_lock_timeout, ulong, 0444);
> > -static bool panic_on_ipistall;
> > -module_param(panic_on_ipistall, bool, 0444);
> > +static int panic_on_ipistall;  /* CSD panic timeout in milliseconds, 300000 for ten minutes. */
> 
> s/ten/five
> 
> > +module_param(panic_on_ipistall, int, 0444);
> >  
> >  static atomic_t csd_bug_count = ATOMIC_INIT(0);
> >  
> > @@ -256,7 +256,7 @@ static bool csd_lock_wait_toolong(struct __call_single_data *csd, u64 ts0, u64 *
> >  	 * to become unstuck. Use a signed comparison to avoid triggering
> >  	 * on underflows when the TSC is out of sync between sockets.
> >  	 */
> > -	BUG_ON(panic_on_ipistall && (s64)ts_delta > 300000000000LL);
> > +	BUG_ON(panic_on_ipistall > 0 && (s64)ts_delta > ((s64)panic_on_ipistall * NSEC_PER_MSEC));
> 
> s64 here would avoid casting (s64)panic_on_ipistall, but I think it does 
> not really impact readability.  
> 
> IIUC ts_delta is an u64 being casted as s64, which could be an issue but no 
> computer system will actually take over 2^31 ns (292 years) to run 1 
> iteration, so it's safe.
> 
> I think it's a nice feaure :)

s/feaure/feature

Also, FWIW:
Reviewed-by: Leonardo Bras <leobras@redhat.com>

> 
> Thanks!
> Leo
> 
> 
> >  	if (cpu_cur_csd && csd != cpu_cur_csd) {
> >  		pr_alert("\tcsd: CSD lock (#%d) handling prior %pS(%ps) request.\n",
> >  			 *bug_id, READ_ONCE(per_cpu(cur_csd_func, cpux)),
> > 

Re: [PATCH smp,csd] Throw an error if a CSD lock is stuck for too long

[Paul E. McKenney]

On Mon, Oct 16, 2023 at 05:58:23PM -0300, Leonardo Bras wrote:
> On Mon, Oct 16, 2023 at 05:52:57PM -0300, Leonardo Bras wrote:
> > On Mon, Oct 16, 2023 at 11:27:51AM -0700, Paul E. McKenney wrote:
> > > On Fri, Oct 13, 2023 at 12:26:22PM -0300, Leonardo Bras wrote:
> > > > On Mon, Oct 09, 2023 at 09:39:38AM -0700, Paul E. McKenney wrote:
> > > > > On Fri, Oct 06, 2023 at 10:32:07AM +1100, Imran Khan wrote:
> > > > > > Hello Paul,
> > > > > > 
> > > > > > On 6/10/2023 3:48 am, Paul E. McKenney wrote:
> > > > > > > The CSD lock seems to get stuck in 2 "modes". When it gets stuck
> > > > > > > temporarily, it usually gets released in a few seconds, and sometimes
> > > > > > > up to one or two minutes.
> > > > > > > 
> > > > > > > If the CSD lock stays stuck for more than several minutes, it never
> > > > > > > seems to get unstuck, and gradually more and more things in the system
> > > > > > > end up also getting stuck.
> > > > > > > 
> > > > > > > In the latter case, we should just give up, so the system can dump out
> > > > > > > a little more information about what went wrong, and, with panic_on_oops
> > > > > > > and a kdump kernel loaded, dump a whole bunch more information about
> > > > > > > what might have gone wrong.
> > > > > > > 
> > > > > > > Question: should this have its own panic_on_ipistall switch in
> > > > > > > /proc/sys/kernel, or maybe piggyback on panic_on_oops in a different
> > > > > > > way than via BUG_ON?
> > > > > > > 
> > > > > > panic_on_ipistall (set to 1 by default) looks better option to me. For systems
> > > > > > where such delay is acceptable and system can eventually get back to sane state,
> > > > > > this option (set to 0 after boot) would prevent crashing the system for
> > > > > > apparently benign CSD hangs of long duration.
> > > > > 
> > > > > Good point!  How about like the following?
> > > > > 
> > > > > 							Thanx, Paul
> > > > > 
> > > > > ------------------------------------------------------------------------
> > > > > 
> > > > > commit 6bcf3786291b86f13b3e13d51e998737a8009ec3
> > > > > Author: Rik van Riel <riel@surriel.com>
> > > > > Date:   Mon Aug 21 16:04:09 2023 -0400
> > > > > 
> > > > >     smp,csd: Throw an error if a CSD lock is stuck for too long
> > > > >     
> > > > >     The CSD lock seems to get stuck in 2 "modes". When it gets stuck
> > > > >     temporarily, it usually gets released in a few seconds, and sometimes
> > > > >     up to one or two minutes.
> > > > >     
> > > > >     If the CSD lock stays stuck for more than several minutes, it never
> > > > >     seems to get unstuck, and gradually more and more things in the system
> > > > >     end up also getting stuck.
> > > > >     
> > > > >     In the latter case, we should just give up, so the system can dump out
> > > > >     a little more information about what went wrong, and, with panic_on_oops
> > > > >     and a kdump kernel loaded, dump a whole bunch more information about what
> > > > >     might have gone wrong.  In addition, there is an smp.panic_on_ipistall
> > > > >     kernel boot parameter that by default retains the old behavior, but when
> > > > >     set enables the panic after the CSD lock has been stuck for more than
> > > > >     five minutes.
> > > > >     
> > > > >     [ paulmck: Apply Imran Khan feedback. ]
> > > > >     
> > > > >     Link: https://lore.kernel.org/lkml/bc7cc8b0-f587-4451-8bcd-0daae627bcc7@paulmck-laptop/
> > > > >     Signed-off-by: Rik van Riel <riel@surriel.com>
> > > > >     Signed-off-by: Paul E. McKenney <paulmck@kernel.org>
> > > > >     Cc: Peter Zijlstra <peterz@infradead.org>
> > > > >     Cc: Valentin Schneider <vschneid@redhat.com>
> > > > >     Cc: Juergen Gross <jgross@suse.com>
> > > > >     Cc: Jonathan Corbet <corbet@lwn.net>
> > > > >     Cc: Randy Dunlap <rdunlap@infradead.org>
> > > > > 
> > > > > diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt
> > > > > index 0a1731a0f0ef..592935267ce2 100644
> > > > > --- a/Documentation/admin-guide/kernel-parameters.txt
> > > > > +++ b/Documentation/admin-guide/kernel-parameters.txt
> > > > > @@ -5858,6 +5858,11 @@
> > > > >  			This feature may be more efficiently disabled
> > > > >  			using the csdlock_debug- kernel parameter.
> > > > >  
> > > > > +	smp.panic_on_ipistall= [KNL]
> > > > > +			If a csd_lock_timeout extends for more than
> > > > > +			five minutes, panic the system.  By default, let
> > > > > +			CSD-lock acquisition take as long as they take.
> > > > > +
> > > > 
> > > > It could be interesting to have it as an s64 parameter (in {mili,}seconds) 
> > > > instead of bool, this way the user could pick the time to wait before the 
> > > > panic happens. 0 or -1 could mean disabled.
> > > > 
> > > > What do you think?
> > > > 
> > > > Other than that,
> > > > Reviewed-by: Leonardo Bras <leobras@redhat.com>
> > > 
> > > Thank you for looking this over!
> > > 
> > > How about with the diff shown below, to be folded into the original?
> > > I went with int instead of s64 because I am having some difficulty
> > > imagining anyone specifying more than a 24-day timeout.  ;-)
> > 
> > I suggested s64 just because it was the type being used in
> > BUG_ON(panic_on_ipistall && (s64)ts_delta > 300000000000LL);
> > 
> > But anyway, int should be fine.
> > 
> > 
> > > 
> > > 							Thanx, Paul
> > > 
> > > ------------------------------------------------------------------------
> > > 
> > > diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt
> > > index ccb7621eff79..ea5ae9deb753 100644
> > > --- a/Documentation/admin-guide/kernel-parameters.txt
> > > +++ b/Documentation/admin-guide/kernel-parameters.txt
> > > @@ -5931,8 +5931,10 @@
> > >  
> > >  	smp.panic_on_ipistall= [KNL]
> > >  			If a csd_lock_timeout extends for more than
> > > -			five minutes, panic the system.  By default, let
> > > -			CSD-lock acquisition take as long as they take.
> > > +			the specified number of milliseconds, panic the
> > > +			system.  By default, let CSD-lock acquisition
> > > +			take as long as they take.  Specifying 300,000
> > > +			for this value provides a 10-minute timeout.
> > 
> > 300,000 ms is 300s, which is 5 minutes, right?

Right you are!  Fixed, please see replacement fixup patch below.

> > >  	smsc-ircc2.nopnp	[HW] Don't use PNP to discover SMC devices
> > >  	smsc-ircc2.ircc_cfg=	[HW] Device configuration I/O port
> > > diff --git a/kernel/smp.c b/kernel/smp.c
> > > index b6a0773a7015..d3ca47f32f38 100644
> > > --- a/kernel/smp.c
> > > +++ b/kernel/smp.c
> > > @@ -170,8 +170,8 @@ static DEFINE_PER_CPU(void *, cur_csd_info);
> > >  
> > >  static ulong csd_lock_timeout = 5000;  /* CSD lock timeout in milliseconds. */
> > >  module_param(csd_lock_timeout, ulong, 0444);
> > > -static bool panic_on_ipistall;
> > > -module_param(panic_on_ipistall, bool, 0444);
> > > +static int panic_on_ipistall;  /* CSD panic timeout in milliseconds, 300000 for ten minutes. */
> > 
> > s/ten/five

And right you are again!

> > > +module_param(panic_on_ipistall, int, 0444);
> > >  
> > >  static atomic_t csd_bug_count = ATOMIC_INIT(0);
> > >  
> > > @@ -256,7 +256,7 @@ static bool csd_lock_wait_toolong(struct __call_single_data *csd, u64 ts0, u64 *
> > >  	 * to become unstuck. Use a signed comparison to avoid triggering
> > >  	 * on underflows when the TSC is out of sync between sockets.
> > >  	 */
> > > -	BUG_ON(panic_on_ipistall && (s64)ts_delta > 300000000000LL);
> > > +	BUG_ON(panic_on_ipistall > 0 && (s64)ts_delta > ((s64)panic_on_ipistall * NSEC_PER_MSEC));
> > 
> > s64 here would avoid casting (s64)panic_on_ipistall, but I think it does 
> > not really impact readability.  
> > 
> > IIUC ts_delta is an u64 being casted as s64, which could be an issue but no 
> > computer system will actually take over 2^31 ns (292 years) to run 1 
> > iteration, so it's safe.

Back at you with s/2^31/2^63/.  ;-)

> > I think it's a nice feaure :)
> 
> s/feaure/feature
> 
> Also, FWIW:
> Reviewed-by: Leonardo Bras <leobras@redhat.com>

I have that down as well, and thank you again!

							Thanx, Paul

------------------------------------------------------------------------

diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt
index ccb7621eff79..e1fe26dae586 100644
--- a/Documentation/admin-guide/kernel-parameters.txt
+++ b/Documentation/admin-guide/kernel-parameters.txt
@@ -5931,8 +5931,10 @@
 
 	smp.panic_on_ipistall= [KNL]
 			If a csd_lock_timeout extends for more than
-			five minutes, panic the system.  By default, let
-			CSD-lock acquisition take as long as they take.
+			the specified number of milliseconds, panic the
+			system.  By default, let CSD-lock acquisition
+			take as long as they take.  Specifying 300,000
+			for this value provides a 5-minute timeout.
 
 	smsc-ircc2.nopnp	[HW] Don't use PNP to discover SMC devices
 	smsc-ircc2.ircc_cfg=	[HW] Device configuration I/O port
diff --git a/kernel/smp.c b/kernel/smp.c
index b6a0773a7015..695eb13a276d 100644
--- a/kernel/smp.c
+++ b/kernel/smp.c
@@ -170,8 +170,8 @@ static DEFINE_PER_CPU(void *, cur_csd_info);
 
 static ulong csd_lock_timeout = 5000;  /* CSD lock timeout in milliseconds. */
 module_param(csd_lock_timeout, ulong, 0444);
-static bool panic_on_ipistall;
-module_param(panic_on_ipistall, bool, 0444);
+static int panic_on_ipistall;  /* CSD panic timeout in milliseconds, 300000 for five minutes. */
+module_param(panic_on_ipistall, int, 0444);
 
 static atomic_t csd_bug_count = ATOMIC_INIT(0);
 
@@ -256,7 +256,7 @@ static bool csd_lock_wait_toolong(struct __call_single_data *csd, u64 ts0, u64 *
 	 * to become unstuck. Use a signed comparison to avoid triggering
 	 * on underflows when the TSC is out of sync between sockets.
 	 */
-	BUG_ON(panic_on_ipistall && (s64)ts_delta > 300000000000LL);
+	BUG_ON(panic_on_ipistall > 0 && (s64)ts_delta > ((s64)panic_on_ipistall * NSEC_PER_MSEC));
 	if (cpu_cur_csd && csd != cpu_cur_csd) {
 		pr_alert("\tcsd: CSD lock (#%d) handling prior %pS(%ps) request.\n",
 			 *bug_id, READ_ONCE(per_cpu(cur_csd_func, cpux)),

Re: [PATCH smp,csd] Throw an error if a CSD lock is stuck for too long

[Leonardo Bras]

On Mon, Oct 16, 2023 at 02:37:10PM -0700, Paul E. McKenney wrote:
> On Mon, Oct 16, 2023 at 05:58:23PM -0300, Leonardo Bras wrote:
> > On Mon, Oct 16, 2023 at 05:52:57PM -0300, Leonardo Bras wrote:
> > > On Mon, Oct 16, 2023 at 11:27:51AM -0700, Paul E. McKenney wrote:
> > > > On Fri, Oct 13, 2023 at 12:26:22PM -0300, Leonardo Bras wrote:
> > > > > On Mon, Oct 09, 2023 at 09:39:38AM -0700, Paul E. McKenney wrote:
> > > > > > On Fri, Oct 06, 2023 at 10:32:07AM +1100, Imran Khan wrote:
> > > > > > > Hello Paul,
> > > > > > > 
> > > > > > > On 6/10/2023 3:48 am, Paul E. McKenney wrote:
> > > > > > > > The CSD lock seems to get stuck in 2 "modes". When it gets stuck
> > > > > > > > temporarily, it usually gets released in a few seconds, and sometimes
> > > > > > > > up to one or two minutes.
> > > > > > > > 
> > > > > > > > If the CSD lock stays stuck for more than several minutes, it never
> > > > > > > > seems to get unstuck, and gradually more and more things in the system
> > > > > > > > end up also getting stuck.
> > > > > > > > 
> > > > > > > > In the latter case, we should just give up, so the system can dump out
> > > > > > > > a little more information about what went wrong, and, with panic_on_oops
> > > > > > > > and a kdump kernel loaded, dump a whole bunch more information about
> > > > > > > > what might have gone wrong.
> > > > > > > > 
> > > > > > > > Question: should this have its own panic_on_ipistall switch in
> > > > > > > > /proc/sys/kernel, or maybe piggyback on panic_on_oops in a different
> > > > > > > > way than via BUG_ON?
> > > > > > > > 
> > > > > > > panic_on_ipistall (set to 1 by default) looks better option to me. For systems
> > > > > > > where such delay is acceptable and system can eventually get back to sane state,
> > > > > > > this option (set to 0 after boot) would prevent crashing the system for
> > > > > > > apparently benign CSD hangs of long duration.
> > > > > > 
> > > > > > Good point!  How about like the following?
> > > > > > 
> > > > > > 							Thanx, Paul
> > > > > > 
> > > > > > ------------------------------------------------------------------------
> > > > > > 
> > > > > > commit 6bcf3786291b86f13b3e13d51e998737a8009ec3
> > > > > > Author: Rik van Riel <riel@surriel.com>
> > > > > > Date:   Mon Aug 21 16:04:09 2023 -0400
> > > > > > 
> > > > > >     smp,csd: Throw an error if a CSD lock is stuck for too long
> > > > > >     
> > > > > >     The CSD lock seems to get stuck in 2 "modes". When it gets stuck
> > > > > >     temporarily, it usually gets released in a few seconds, and sometimes
> > > > > >     up to one or two minutes.
> > > > > >     
> > > > > >     If the CSD lock stays stuck for more than several minutes, it never
> > > > > >     seems to get unstuck, and gradually more and more things in the system
> > > > > >     end up also getting stuck.
> > > > > >     
> > > > > >     In the latter case, we should just give up, so the system can dump out
> > > > > >     a little more information about what went wrong, and, with panic_on_oops
> > > > > >     and a kdump kernel loaded, dump a whole bunch more information about what
> > > > > >     might have gone wrong.  In addition, there is an smp.panic_on_ipistall
> > > > > >     kernel boot parameter that by default retains the old behavior, but when
> > > > > >     set enables the panic after the CSD lock has been stuck for more than
> > > > > >     five minutes.
> > > > > >     
> > > > > >     [ paulmck: Apply Imran Khan feedback. ]
> > > > > >     
> > > > > >     Link: https://lore.kernel.org/lkml/bc7cc8b0-f587-4451-8bcd-0daae627bcc7@paulmck-laptop/
> > > > > >     Signed-off-by: Rik van Riel <riel@surriel.com>
> > > > > >     Signed-off-by: Paul E. McKenney <paulmck@kernel.org>
> > > > > >     Cc: Peter Zijlstra <peterz@infradead.org>
> > > > > >     Cc: Valentin Schneider <vschneid@redhat.com>
> > > > > >     Cc: Juergen Gross <jgross@suse.com>
> > > > > >     Cc: Jonathan Corbet <corbet@lwn.net>
> > > > > >     Cc: Randy Dunlap <rdunlap@infradead.org>
> > > > > > 
> > > > > > diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt
> > > > > > index 0a1731a0f0ef..592935267ce2 100644
> > > > > > --- a/Documentation/admin-guide/kernel-parameters.txt
> > > > > > +++ b/Documentation/admin-guide/kernel-parameters.txt
> > > > > > @@ -5858,6 +5858,11 @@
> > > > > >  			This feature may be more efficiently disabled
> > > > > >  			using the csdlock_debug- kernel parameter.
> > > > > >  
> > > > > > +	smp.panic_on_ipistall= [KNL]
> > > > > > +			If a csd_lock_timeout extends for more than
> > > > > > +			five minutes, panic the system.  By default, let
> > > > > > +			CSD-lock acquisition take as long as they take.
> > > > > > +
> > > > > 
> > > > > It could be interesting to have it as an s64 parameter (in {mili,}seconds) 
> > > > > instead of bool, this way the user could pick the time to wait before the 
> > > > > panic happens. 0 or -1 could mean disabled.
> > > > > 
> > > > > What do you think?
> > > > > 
> > > > > Other than that,
> > > > > Reviewed-by: Leonardo Bras <leobras@redhat.com>
> > > > 
> > > > Thank you for looking this over!
> > > > 
> > > > How about with the diff shown below, to be folded into the original?
> > > > I went with int instead of s64 because I am having some difficulty
> > > > imagining anyone specifying more than a 24-day timeout.  ;-)
> > > 
> > > I suggested s64 just because it was the type being used in
> > > BUG_ON(panic_on_ipistall && (s64)ts_delta > 300000000000LL);
> > > 
> > > But anyway, int should be fine.
> > > 
> > > 
> > > > 
> > > > 							Thanx, Paul
> > > > 
> > > > ------------------------------------------------------------------------
> > > > 
> > > > diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt
> > > > index ccb7621eff79..ea5ae9deb753 100644
> > > > --- a/Documentation/admin-guide/kernel-parameters.txt
> > > > +++ b/Documentation/admin-guide/kernel-parameters.txt
> > > > @@ -5931,8 +5931,10 @@
> > > >  
> > > >  	smp.panic_on_ipistall= [KNL]
> > > >  			If a csd_lock_timeout extends for more than
> > > > -			five minutes, panic the system.  By default, let
> > > > -			CSD-lock acquisition take as long as they take.
> > > > +			the specified number of milliseconds, panic the
> > > > +			system.  By default, let CSD-lock acquisition
> > > > +			take as long as they take.  Specifying 300,000
> > > > +			for this value provides a 10-minute timeout.
> > > 
> > > 300,000 ms is 300s, which is 5 minutes, right?
> 
> Right you are!  Fixed, please see replacement fixup patch below.
> 
> > > >  	smsc-ircc2.nopnp	[HW] Don't use PNP to discover SMC devices
> > > >  	smsc-ircc2.ircc_cfg=	[HW] Device configuration I/O port
> > > > diff --git a/kernel/smp.c b/kernel/smp.c
> > > > index b6a0773a7015..d3ca47f32f38 100644
> > > > --- a/kernel/smp.c
> > > > +++ b/kernel/smp.c
> > > > @@ -170,8 +170,8 @@ static DEFINE_PER_CPU(void *, cur_csd_info);
> > > >  
> > > >  static ulong csd_lock_timeout = 5000;  /* CSD lock timeout in milliseconds. */
> > > >  module_param(csd_lock_timeout, ulong, 0444);
> > > > -static bool panic_on_ipistall;
> > > > -module_param(panic_on_ipistall, bool, 0444);
> > > > +static int panic_on_ipistall;  /* CSD panic timeout in milliseconds, 300000 for ten minutes. */
> > > 
> > > s/ten/five
> 
> And right you are again!
> 
> > > > +module_param(panic_on_ipistall, int, 0444);
> > > >  
> > > >  static atomic_t csd_bug_count = ATOMIC_INIT(0);
> > > >  
> > > > @@ -256,7 +256,7 @@ static bool csd_lock_wait_toolong(struct __call_single_data *csd, u64 ts0, u64 *
> > > >  	 * to become unstuck. Use a signed comparison to avoid triggering
> > > >  	 * on underflows when the TSC is out of sync between sockets.
> > > >  	 */
> > > > -	BUG_ON(panic_on_ipistall && (s64)ts_delta > 300000000000LL);
> > > > +	BUG_ON(panic_on_ipistall > 0 && (s64)ts_delta > ((s64)panic_on_ipistall * NSEC_PER_MSEC));
> > > 
> > > s64 here would avoid casting (s64)panic_on_ipistall, but I think it does 
> > > not really impact readability.  
> > > 
> > > IIUC ts_delta is an u64 being casted as s64, which could be an issue but no 
> > > computer system will actually take over 2^31 ns (292 years) to run 1 
> > > iteration, so it's safe.
> 
> Back at you with s/2^31/2^63/.  ;-)

Right you are! :)

> 
> > > I think it's a nice feaure :)
> > 
> > s/feaure/feature
> > 
> > Also, FWIW:
> > Reviewed-by: Leonardo Bras <leobras@redhat.com>
> 
> I have that down as well, and thank you again!
> 
> 							Thanx, Paul
> 
> ------------------------------------------------------------------------
> 
> diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt
> index ccb7621eff79..e1fe26dae586 100644
> --- a/Documentation/admin-guide/kernel-parameters.txt
> +++ b/Documentation/admin-guide/kernel-parameters.txt
> @@ -5931,8 +5931,10 @@
>  
>  	smp.panic_on_ipistall= [KNL]
>  			If a csd_lock_timeout extends for more than
> -			five minutes, panic the system.  By default, let
> -			CSD-lock acquisition take as long as they take.
> +			the specified number of milliseconds, panic the
> +			system.  By default, let CSD-lock acquisition
> +			take as long as they take.  Specifying 300,000
> +			for this value provides a 5-minute timeout.
>  
>  	smsc-ircc2.nopnp	[HW] Don't use PNP to discover SMC devices
>  	smsc-ircc2.ircc_cfg=	[HW] Device configuration I/O port
> diff --git a/kernel/smp.c b/kernel/smp.c
> index b6a0773a7015..695eb13a276d 100644
> --- a/kernel/smp.c
> +++ b/kernel/smp.c
> @@ -170,8 +170,8 @@ static DEFINE_PER_CPU(void *, cur_csd_info);
>  
>  static ulong csd_lock_timeout = 5000;  /* CSD lock timeout in milliseconds. */
>  module_param(csd_lock_timeout, ulong, 0444);
> -static bool panic_on_ipistall;
> -module_param(panic_on_ipistall, bool, 0444);
> +static int panic_on_ipistall;  /* CSD panic timeout in milliseconds, 300000 for five minutes. */
> +module_param(panic_on_ipistall, int, 0444);
>  
>  static atomic_t csd_bug_count = ATOMIC_INIT(0);
>  
> @@ -256,7 +256,7 @@ static bool csd_lock_wait_toolong(struct __call_single_data *csd, u64 ts0, u64 *
>  	 * to become unstuck. Use a signed comparison to avoid triggering
>  	 * on underflows when the TSC is out of sync between sockets.
>  	 */
> -	BUG_ON(panic_on_ipistall && (s64)ts_delta > 300000000000LL);
> +	BUG_ON(panic_on_ipistall > 0 && (s64)ts_delta > ((s64)panic_on_ipistall * NSEC_PER_MSEC));
>  	if (cpu_cur_csd && csd != cpu_cur_csd) {
>  		pr_alert("\tcsd: CSD lock (#%d) handling prior %pS(%ps) request.\n",
>  			 *bug_id, READ_ONCE(per_cpu(cur_csd_func, cpux)),
> 

It's great!

Thanks!

Re: [PATCH smp,csd] Throw an error if a CSD lock is stuck for too long

[Jonas Oberhauser]

Is this related to the qspinlock issue you described earlier?


jonas


Am 10/5/2023 um 6:48 PM schrieb Paul E. McKenney:
> The CSD lock seems to get stuck in 2 "modes". When it gets stuck
> temporarily, it usually gets released in a few seconds, and sometimes
> up to one or two minutes.
>
> If the CSD lock stays stuck for more than several minutes, it never
> seems to get unstuck, and gradually more and more things in the system
> end up also getting stuck.
>
> In the latter case, we should just give up, so the system can dump out
> a little more information about what went wrong, and, with panic_on_oops
> and a kdump kernel loaded, dump a whole bunch more information about
> what might have gone wrong.
>
> Question: should this have its own panic_on_ipistall switch in
> /proc/sys/kernel, or maybe piggyback on panic_on_oops in a different
> way than via BUG_ON?
>
> Signed-off-by: Rik van Riel <riel@surriel.com>
> Signed-off-by: Paul E. McKenney <paulmck@kernel.org>
>
> diff --git a/kernel/smp.c b/kernel/smp.c
> index 8455a53465af..059f1f53fc6b 100644
> --- a/kernel/smp.c
> +++ b/kernel/smp.c
> @@ -230,6 +230,7 @@ static bool csd_lock_wait_toolong(struct __call_single_data *csd, u64 ts0, u64 *
>   	}
>   
>   	ts2 = sched_clock();
> +	/* How long since we last checked for a stuck CSD lock.*/
>   	ts_delta = ts2 - *ts1;
>   	if (likely(ts_delta <= csd_lock_timeout_ns || csd_lock_timeout_ns == 0))
>   		return false;
> @@ -243,9 +244,17 @@ static bool csd_lock_wait_toolong(struct __call_single_data *csd, u64 ts0, u64 *
>   	else
>   		cpux = cpu;
>   	cpu_cur_csd = smp_load_acquire(&per_cpu(cur_csd, cpux)); /* Before func and info. */
> +	/* How long since this CSD lock was stuck. */
> +	ts_delta = ts2 - ts0;
>   	pr_alert("csd: %s non-responsive CSD lock (#%d) on CPU#%d, waiting %llu ns for CPU#%02d %pS(%ps).\n",
> -		 firsttime ? "Detected" : "Continued", *bug_id, raw_smp_processor_id(), ts2 - ts0,
> +		 firsttime ? "Detected" : "Continued", *bug_id, raw_smp_processor_id(), ts_delta,
>   		 cpu, csd->func, csd->info);
> +	/*
> +	 * If the CSD lock is still stuck after 5 minutes, it is unlikely
> +	 * to become unstuck. Use a signed comparison to avoid triggering
> +	 * on underflows when the TSC is out of sync between sockets.
> +	 */
> +	BUG_ON((s64)ts_delta > 300000000000LL);
>   	if (cpu_cur_csd && csd != cpu_cur_csd) {
>   		pr_alert("\tcsd: CSD lock (#%d) handling prior %pS(%ps) request.\n",
>   			 *bug_id, READ_ONCE(per_cpu(cur_csd_func, cpux)),

Re: [PATCH smp,csd] Throw an error if a CSD lock is stuck for too long

[Paul E. McKenney]

On Fri, Oct 06, 2023 at 08:48:23PM +0200, Jonas Oberhauser wrote:
> Is this related to the qspinlock issue you described earlier?

Kind of in that sometimes qspinlock issues trigger CSD-lock warnings,
but not really directly related.

							Thanx, Paul

> jonas
> 
> 
> Am 10/5/2023 um 6:48 PM schrieb Paul E. McKenney:
> > The CSD lock seems to get stuck in 2 "modes". When it gets stuck
> > temporarily, it usually gets released in a few seconds, and sometimes
> > up to one or two minutes.
> > 
> > If the CSD lock stays stuck for more than several minutes, it never
> > seems to get unstuck, and gradually more and more things in the system
> > end up also getting stuck.
> > 
> > In the latter case, we should just give up, so the system can dump out
> > a little more information about what went wrong, and, with panic_on_oops
> > and a kdump kernel loaded, dump a whole bunch more information about
> > what might have gone wrong.
> > 
> > Question: should this have its own panic_on_ipistall switch in
> > /proc/sys/kernel, or maybe piggyback on panic_on_oops in a different
> > way than via BUG_ON?
> > 
> > Signed-off-by: Rik van Riel <riel@surriel.com>
> > Signed-off-by: Paul E. McKenney <paulmck@kernel.org>
> > 
> > diff --git a/kernel/smp.c b/kernel/smp.c
> > index 8455a53465af..059f1f53fc6b 100644
> > --- a/kernel/smp.c
> > +++ b/kernel/smp.c
> > @@ -230,6 +230,7 @@ static bool csd_lock_wait_toolong(struct __call_single_data *csd, u64 ts0, u64 *
> >   	}
> >   	ts2 = sched_clock();
> > +	/* How long since we last checked for a stuck CSD lock.*/
> >   	ts_delta = ts2 - *ts1;
> >   	if (likely(ts_delta <= csd_lock_timeout_ns || csd_lock_timeout_ns == 0))
> >   		return false;
> > @@ -243,9 +244,17 @@ static bool csd_lock_wait_toolong(struct __call_single_data *csd, u64 ts0, u64 *
> >   	else
> >   		cpux = cpu;
> >   	cpu_cur_csd = smp_load_acquire(&per_cpu(cur_csd, cpux)); /* Before func and info. */
> > +	/* How long since this CSD lock was stuck. */
> > +	ts_delta = ts2 - ts0;
> >   	pr_alert("csd: %s non-responsive CSD lock (#%d) on CPU#%d, waiting %llu ns for CPU#%02d %pS(%ps).\n",
> > -		 firsttime ? "Detected" : "Continued", *bug_id, raw_smp_processor_id(), ts2 - ts0,
> > +		 firsttime ? "Detected" : "Continued", *bug_id, raw_smp_processor_id(), ts_delta,
> >   		 cpu, csd->func, csd->info);
> > +	/*
> > +	 * If the CSD lock is still stuck after 5 minutes, it is unlikely
> > +	 * to become unstuck. Use a signed comparison to avoid triggering
> > +	 * on underflows when the TSC is out of sync between sockets.
> > +	 */
> > +	BUG_ON((s64)ts_delta > 300000000000LL);
> >   	if (cpu_cur_csd && csd != cpu_cur_csd) {
> >   		pr_alert("\tcsd: CSD lock (#%d) handling prior %pS(%ps) request.\n",
> >   			 *bug_id, READ_ONCE(per_cpu(cur_csd_func, cpux)),
>