linux-kernel - Re: [PATCH] rcu/tree: consider time a VM was suspended

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Thu, 20 May 2021 07:53:18 -0700
From:   "Paul E. McKenney" <paulmck@...nel.org>
To:     Sergey Senozhatsky <senozhatsky@...omium.org>
Cc:     Josh Triplett <josh@...htriplett.org>,
        Steven Rostedt <rostedt@...dmis.org>,
        Mathieu Desnoyers <mathieu.desnoyers@...icios.com>,
        Lai Jiangshan <jiangshanlai@...il.com>,
        Joel Fernandes <joel@...lfernandes.org>,
        Suleiman Souhlal <suleiman@...gle.com>, rcu@...r.kernel.org,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH] rcu/tree: consider time a VM was suspended

On Thu, May 20, 2021 at 03:18:07PM +0900, Sergey Senozhatsky wrote:
> On (21/05/18 16:15), Paul E. McKenney wrote:
> > 
> > In the shorter term...  PVCLOCK_GUEST_STOPPED is mostly for things like
> > guest migration and debugger breakpoints, correct?  Either way, I am
> > wondering if rcu_cpu_stall_reset() should take a lighter touch.  Right
> > now, it effectively disables all stalls for the current grace period.
> > Why not make it restart the stall timeout when the stoppage is detected?
> 
> rcu_cpu_stall_reset() is used in many other places, not sure if we can
> change its behaviour rcu_cpu_stall_reset().

There was some use case back in the day where they wanted an indefinite
suppression of RCU CPU stall warnings for the current grace period, but
all the current use cases look fine with restarting the stall timeout.

However, please see below.

> Maybe it'll be possible to just stop calling it from PV-clock and do
> something like this

This was in fact one of the things I was considering, at least until
I convinced myself that I needed to ask some questions.

One point of possibly unnecessary nervousness on my part is resetting
the start of the grace period, which might confuse diagnostics.

But how about something like this?

void rcu_cpu_stall_reset(void)
{
	WRITE_ONCE(rcu_state.jiffies_stall,
		   jiffies + rcu_jiffies_till_stall_check());
}

Would something like that work?

(One issue with this is if there has already been an RCU CPU stall
warning, in which case the timeout for repeat warnings is tripled.  But in
the current use cases, I don't see that this matters.  If it turns out to
matter, I would just add a flag to rcu_state saying whether the current
grace period had seen a stall, and use that to decide whether or not
to triple the timeout.)

							Thanx, Paul

> ---
> 
> diff --git a/arch/x86/kernel/pvclock.c b/arch/x86/kernel/pvclock.c
> index eda37df016f0..2d2489eda8e6 100644
> --- a/arch/x86/kernel/pvclock.c
> +++ b/arch/x86/kernel/pvclock.c
> @@ -40,7 +40,7 @@ void pvclock_touch_watchdogs(void)
>  {
>  	touch_softlockup_watchdog_sync();
>  	clocksource_touch_watchdog();
> -	rcu_cpu_stall_reset();
> +	record_gp_stall_check_time();
>  	reset_hung_task_detector();
>  }
>  
> diff --git a/kernel/rcu/tree_stall.h b/kernel/rcu/tree_stall.h
> index cb233c79f0bc..6b3165c7a2c3 100644
> --- a/kernel/rcu/tree_stall.h
> +++ b/kernel/rcu/tree_stall.h
> @@ -137,7 +137,7 @@ void rcu_cpu_stall_reset(void)
>  // Interaction with RCU grace periods
>  
>  /* Start of new grace period, so record stall time (and forcing times). */
> -static void record_gp_stall_check_time(void)
> +void record_gp_stall_check_time(void)
>  {
>  	unsigned long j = jiffies;
>  	unsigned long j1;