Received-SPF: pass (google.com: domain of linux-kernel+bounces-168201-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:4601:e00::3 as permitted sender) client-ip=2604:1380:4601:e00::3;
From: Leonardo Bras <leobras@redhat.com>
To: Sean Christopherson <seanjc@google.com>
Cc: Leonardo Bras <leobras@redhat.com>,
	Marcelo Tosatti <mtosatti@redhat.com>,
	"Paul E. McKenney" <paulmck@kernel.org>,
	Paolo Bonzini <pbonzini@redhat.com>,
	Frederic Weisbecker <frederic@kernel.org>,
	Neeraj Upadhyay <quic_neeraju@quicinc.com>,
	Joel Fernandes <joel@joelfernandes.org>,
	Josh Triplett <josh@joshtriplett.org>,
	Boqun Feng <boqun.feng@gmail.com>,
	Steven Rostedt <rostedt@goodmis.org>,
	Mathieu Desnoyers <mathieu.desnoyers@efficios.com>,
	Lai Jiangshan <jiangshanlai@gmail.com>,
	Zqiang <qiang.zhang1211@gmail.com>,
	kvm@vger.kernel.org,
	linux-kernel@vger.kernel.org,
	rcu@vger.kernel.org
Subject: Re: [RFC PATCH v1 0/2] Avoid rcu_core() if CPU just left guest vcpu
Date: Fri,  3 May 2024 17:44:22 -0300
Message-ID: <ZjVMpj7zcSf-JYd_@LeoBras>
In-Reply-To: <ZiAFSlZwxyKzOTRL@google.com>
References: <ZhAAg8KNd8qHEGcO@tpad> <ZhAN28BcMsfl4gm-@google.com> <a7398da4-a72c-4933-bb8b-5bc8965d96d0@paulmck-laptop> <ZhQmaEXPCqmx1rTW@google.com> <Zh2EQVj5bC0z5R90@tpad> <Zh2cPJ-5xh72ojzu@google.com> <Zh5w6rAWL+08a5lj@tpad> <Zh6GC0NRonCpzpV4@google.com> <Zh/1U8MtPWQ/yN2T@tpad> <ZiAFSlZwxyKzOTRL@google.com>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
Content-Transfer-Encoding: 8bit

On Wed, Apr 17, 2024 at 10:22:18AM -0700, Sean Christopherson wrote:
> On Wed, Apr 17, 2024, Marcelo Tosatti wrote:
> > On Tue, Apr 16, 2024 at 07:07:32AM -0700, Sean Christopherson wrote:
> > > On Tue, Apr 16, 2024, Marcelo Tosatti wrote:
> > > > > Why not have
> > > > > KVM provide a "this task is in KVM_RUN" flag, and then let the existing timeout
> > > > > handle the (hopefully rare) case where KVM doesn't "immediately" re-enter the guest?
> > > > 
> > > > Do you mean something like:
> > > > 
> > > > diff --git a/kernel/rcu/tree.c b/kernel/rcu/tree.c
> > > > index d9642dd06c25..0ca5a6a45025 100644
> > > > --- a/kernel/rcu/tree.c
> > > > +++ b/kernel/rcu/tree.c
> > > > @@ -3938,7 +3938,7 @@ static int rcu_pending(int user)
> > > >                 return 1;
> > > >  
> > > >         /* Is this a nohz_full CPU in userspace or idle?  (Ignore RCU if so.) */
> > > > -       if ((user || rcu_is_cpu_rrupt_from_idle()) && rcu_nohz_full_cpu())
> > > > +       if ((user || rcu_is_cpu_rrupt_from_idle() || this_cpu->in_kvm_run) && rcu_nohz_full_cpu())
> > > >                 return 0;
> > > 
> > > Yes.  This, https://lore.kernel.org/all/ZhAN28BcMsfl4gm-@google.com, plus logic
> > > in kvm_sched_{in,out}().
> > 
> > Question: where is vcpu->wants_to_run set? (or, where is the full series
> > again?).
> 
> Precisely around the call to kvm_arch_vcpu_ioctl_run().  I am planning on applying
> the patch that introduces the code for 6.10[*], I just haven't yet for a variety
> of reasons.
> 
> [*] https://lore.kernel.org/all/20240307163541.92138-1-dmatlack@google.com
> 
> > So for guest HLT emulation, there is a window between
> > 
> > kvm_vcpu_block -> fire_sched_out_preempt_notifiers -> vcpu_put 
> > and the idle's task call to ct_cpuidle_enter, where 
> > 
> > ct_dynticks_nesting() != 0 and vcpu_put has already executed.
> > 
> > Even for idle=poll, the race exists.
> 
> Is waking rcuc actually problematic?

Yeah, it may introduce a lot (30us) of latency in some cases, causing a 
missed deadline.

When dealing with RT tasks, missing a deadline can be really bad, so we 
need to make sure it will happen as rarely as possible.

>  I agree it's not ideal, but it's a smallish
> window, i.e. is unlikely to happen frequently, and if rcuc is awakened, it will
> effectively steal cycles from the idle thread, not the vCPU thread.

It would be fine, but sometimes the idle thread will run very briefly, and 
stealing microseconds from it will still steal enough time from the vcpu 
thread to become a problem.

>  If the vCPU
> gets a wake event before rcuc completes, then the vCPU could experience jitter,
> but that could also happen if the CPU ends up in a deep C-state.

IIUC, if the scenario calls for a very short HLT, which is kind of usual, 
then the CPU will not get into deep C-state. 
For the scenarios longer HLT happens, then it would be fine.

> 
> And that race exists in general, i.e. any IRQ that arrives just as the idle task
> is being scheduled in will unnecessarily wakeup rcuc.

That's a race could be solved with the timeout (snapshot) solution, if we 
don't zero last_guest_exit on kvm_sched_out(), right?

> 
> > > >         /* Is the RCU core waiting for a quiescent state from this CPU? */
> > > > 
> > > > The problem is:
> > > > 
> > > > 1) You should only set that flag, in the VM-entry path, after the point
> > > > where no use of RCU is made: close to guest_state_enter_irqoff call.
> > > 
> > > Why?  As established above, KVM essentially has 1 second to enter the guest after
> > > setting in_guest_run_loop (or whatever we call it).  In the vast majority of cases,
> > > the time before KVM enters the guest can probably be measured in microseconds.
> > 
> > OK.
> > 
> > > Snapshotting the exit time has the exact same problem of depending on KVM to
> > > re-enter the guest soon-ish, so I don't understand why this would be considered
> > > a problem with a flag to note the CPU is in KVM's run loop, but not with a
> > > snapshot to say the CPU recently exited a KVM guest.
> > 
> > See the race above.
> 
> Ya, but if kvm_last_guest_exit is zeroed in kvm_sched_out(), then the snapshot
> approach ends up with the same race.  And not zeroing kvm_last_guest_exit is
> arguably much more problematic as encountering a false positive doesn't require
> hitting a small window.

For the false positive (only on nohz_full) the maximum delay for the  
rcu_core() to be run would be 1s, and that would be in case we don't schedule out for 
some userspace task or idle thread, in which case we have a quiescent state 
without the need of rcu_core().

Now, for not being an userspace nor idle thread, it would need to be one or 
more kernel threads, which I suppose aren't usually many, nor usually 
take that long for completing, if we consider to be running on an isolated (nohz_full) cpu. 

So, for the kvm_sched_out() case, I don't actually think we are  
statistically introducing that much of a delay in the RCU mechanism.

(I may be missing some point, though)

Thanks!
Leo

> 
> > > > 2) While handling a VM-exit, a host timer interrupt can occur before that,
> > > > or after the point where "this_cpu->in_kvm_run" is set to false.
> > > >
> > > > And a host timer interrupt calls rcu_sched_clock_irq which is going to
> > > > wake up rcuc.
> > > 
> > > If in_kvm_run is false when the IRQ is handled, then either KVM exited to userspace
> > > or the vCPU was scheduled out.  In the former case, rcuc won't be woken up if the
> > > CPU is in userspace.  And in the latter case, waking up rcuc is absolutely the
> > > correct thing to do as VM-Enter is not imminent.
> > > 
> > > For exits to userspace, there would be a small window where an IRQ could arrive
> > > between KVM putting the vCPU and the CPU actually returning to userspace, but
> > > unless that's problematic in practice, I think it's a reasonable tradeoff.
> > 
> > OK, your proposal looks alright except these races.
> > 
> > We don't want those races to occur in production (and they likely will).
> > 
> > Is there any way to fix the races? Perhaps cmpxchg?
> 
> I don't think an atomic switch from the vCPU task to the idle task is feasible,
> e.g. KVM would somehow have to know that the idle task is going to run next.
> This seems like something that needs a generic solution, e.g. to prevent waking
> rcuc if the idle task is in the process of being scheduled in.
>