Re: [PATCH 5/6] x86: avoid explicit TLB flush when saving exec state

From: George Dunlap <dunlapg@umich.edu>
To: Jan Beulich <JBeulich@suse.com>
Cc: xen-devel <xen-devel@lists.xenproject.org>,
	Andrew Cooper <andrew.cooper3@citrix.com>
Subject: Re: [PATCH 5/6] x86: avoid explicit TLB flush when saving exec state
Date: Fri, 19 Jan 2018 18:23:11 +0000	[thread overview]
Message-ID: <CAFLBxZb369H33HigM4Y-Oq8wOK+bZuxorhobyzwXfntzeswhnQ@mail.gmail.com> (raw)
In-Reply-To: <CAFLBxZbrFz7cYWn51b+cebzPepVL32ahs8oZEnS_M1gJ9MXr6g@mail.gmail.com>

On Fri, Jan 19, 2018 at 5:59 PM, George Dunlap <dunlapg@umich.edu> wrote:
> On Fri, Jan 19, 2018 at 4:06 PM, Jan Beulich <JBeulich@suse.com> wrote:
>> Now that it's obvious that only a single dirty CPU can exist for a vCPU,
>> it becomes clear that flush_mask() doesn't need to be invoked when
>> sync_local_execstate() was already run.  And with the IPI handler
>> clearing FLUSH_TLB from the passed flags anyway if
>> __sync_local_execstate() returns true, it also becomes clear that
>> FLUSH_TLB doesn't need to be passed here in the first place.
>
> I think the naming here is a bit confusing.  In theory, the fact that
> __sync_local_execstate() uses __context_switch() to sync the registers
> (and thus also flushes the TLB) is an internal implementation detail.
> But when viewed further back, it's clear that 1) syncing the state
> always happens because you didn't call __context_switch() before, and
> 2) the most robust way to make sure that the 'sync' works correctly is
> for it to use the same path as the actual context switch.
>
> I was originally going to object to removing the flag on the grounds
> that the implementation of __sync_local_execstate() could in theory
> change (such that no TLB was flushed); but I now think that's pretty
> unlikely.

OTOH, while there is certainly a good reason for
__sync_local_execstate() to share *state saving* code with
__context_switch(), there's no real reason for
__sync_local_execstate() to actually flush the TLB -- it would be a
minor performance optimization to allow other pcpus to read a vcpu's
registers without making it re-fill its TLB after resuming.

So it seems to me that keeping the FLUSH_TLB flag for the callers that
actually want to flush the TLB is a better idea.  It doesn't cost
anything, it makes it more clear what's going on, and it future-proofs
those calls against the kind of optimization I described above.

However...

> Are either of these examples explicitly trying to flush the TLB in the
> first case?  They both look like they care only about the vcpu state,
> and the FLUSH_TLB previously was to pass the nop check.

This would be an excellent reason to remove the flag.

 -George

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel