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

[George Dunlap <dunlapg@xxxxxxxxx>]

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.

But I do think that the naming the way it is may lead people to
believe that FLUSH_STATE will *not* result in a flushed TLB.

At very least a comment near FLUSH_STATE's definition should mention this fact.

Other than that, LGTM.

 -George

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