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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 19.01.18 at 19:23, <dunlapg@xxxxxxxxx> wrote:
> On Fri, Jan 19, 2018 at 5:59 PM, George Dunlap <dunlapg@xxxxxxxxx> wrote:
>> On Fri, Jan 19, 2018 at 4:06 PM, Jan Beulich <JBeulich@xxxxxxxx> 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.

sync_vcpu_execstate() very certainly doesn't mean to flush the TLB,
other than if that was required to happen in context_switch() as
well. context_switch() itself doesn't need the flush _at that point_
either, as the write_ptbase() in __context_switch() does what is
actually needed. I've added

"; neither of the two places actually have a need to flush the TLB in
 any event (quite possibly FLUSH_TLB was being passed there solely
 for flush_area_mask() to make it past its no-op check)."

to the tail of the description.

Jan


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