Re: [Qemu-devel] [PATCH v2 0/3] target-i386: save/restore vcpu's TSC rate during migration

[haozhong . zhang]

On Fri, Oct 23, 2015 at 12:45:13PM -0200, Eduardo Habkost wrote:
> On Fri, Oct 23, 2015 at 10:27:27AM +0800, Haozhong Zhang wrote:
> > On Thu, Oct 22, 2015 at 04:45:21PM -0200, Eduardo Habkost wrote:
> > > On Tue, Oct 20, 2015 at 03:22:51PM +0800, Haozhong Zhang wrote:
> > > > This patchset enables QEMU to save/restore vcpu's TSC rate during the
> > > > migration. When cooperating with KVM which supports TSC scaling, guest
> > > > programs can observe a consistent guest TSC rate even though they are
> > > > migrated among machines with different host TSC rates.
> > > > 
> > > > A pair of cpu options 'save-tsc-freq' and 'load-tsc-freq' are added to
> > > > control the migration of vcpu's TSC rate.
> > > 
> > > The requirements and goals aren't clear to me. I see two possible use
> > > cases, here:
> > > 
> > > 1) Best effort to keep TSC frequency constant if possible (but not
> > >    aborting migration if not possible). This would be an interesting
> > >    default, but a bit unpredictable.
> > > 2) Strictly ensuring TSC frequency stays constant on migration (and
> > >    aborting migration if not possible). This would be an useful feature,
> > >    but can't be enabled by default unless both hosts have the same TSC
> > >    frequency or support TSC scaling.
> > > 
> > > Which one(s) you are trying to implement?
> > >
> > 
> > The former. I agree that it's unpredictable if setting vcpu's TSC
> > frequency to the migrated value is enabled by default (but not in this
> > patchset). The cpu option 'load-tsc-freq' is introduced to allow users
> > to enable this behavior if they do know the underlying KVM and CPU
> > support TSC scaling. In this way, I think the behavior is predictable
> > as users do know what they are doing.
> 
> I'm confused. If load-tsc-freq doesn't abort when TSC scaling isn't
> available (use case #1), why isn't it enabled by default? On the other
> hand, if you expect the user to enable it only if the host supports TSC
> scaling, why doesn't it abort if TSC scaling isn't available?
>

Sorry for the confusion. For user case #1, load-tsc-freq is really not
needed and the migrated TSC frequency should be set if possible
(ie. if TSC scaling is supported and KVM_SET_TSC_KHZ succeeds). If
setting TSC frequency fails, the migration will not be aborted.

> I mean, we can implement both use cases above this way:
> 
> 1) If the user didn't ask for anything explicitly:
>   * If the tsc-freq value is available in the migration stream, try to
>     set it (but don't abort if it can't be set). (use case #1 above)
>     * Rationale: it won't hurt to try to make the VM behave nicely if
>       possible, without blocking migration if TSC scaling isn't
>       available.
> 2) If the user asked for the TSC frequency to be enforced, set it and
>   abort if it couldn't be set (use case #2 above). This could apply to
>   both cases:
>   2.1) If tsc-freq is explicitly set in the command-line.
>     * Rationale: if the user asked for a specific frequency, we
>       should do what was requested and not ignore errors silently.
>   2.2) If tsc-freq is available in the migration stream, and the
>     user asked explicitly for it to be enforced.
>     * Rationale: the user is telling us that the incoming tsc-freq
>       is important, so we shouldn't ignore it silently.
>     * Open question: how should we name the new option?
>       "load-tsc-freq" would be misleading because it won't be just about
>       _loading_ tsc-freq (we would be loading it on use case #1, too),
>       but about making sure it is enforced. "strict-tsc-freq"?
>       "enforce-tsc-freq"?
> 
> We don't need to implement both #1 and #2 at the same time. But if you
> just want to implement #1 first, I don't see the need for the
> "load-tsc-freq" option.
> 
> On the migration source, we need another option or internal machine flag
> for #1. I am not sure it should be an user-visible option. If
> user-visible, I don't know how to name it. "save-tsc-freq" describes it
> correctly, but it doesn't make its purpose very clear. Any suggestions?
> It can also be implemented first as an internal machine class flag (set
> in pc >= 2.5 only), and possibly become a user-visible option later.
>

Because the way I implements 'save-tsc-freq' in patch 1, it's exposed
to users. I'm not familiar the way to make a feature only available
for newer machine types. Could you provide some suggestions to hide
'save-tsc-freq' from users?

For the name, if we make the option internal only, could we still use
'save-tsc-freq' as it does mean saving the TSC frequency.

> > 
> > > In other words, what is the right behavior when KVM_SET_TSC_KHZ fails or
> > > KVM_CAP_TSC_CONTROL is not available? We can't answer that question if
> > > the requirements and goals are not clear.
> > >
> > 
> > If KVM_CAP_TSC_CONTROL is unavailable, QEMU and KVM will use the host
> > TSC frequency as vcpu's TSC frequency.
> > 
> > If KVM_CAP_TSC_CONTROL is available and KVM_SET_TSC_KHZ fails, the
> > setting of TSC frequency will fail and abort either the VM creation
> > (this is the case for cpu option 'tsc-freq') or the migration.
> 
> I don't see why the lack of KVM_CAP_TSC_CONTROL and failure of
> KVM_SET_TSC_KHZ should be treated differently. In both cases it means we
> the TSC frequency can't be set.
> 
> I mean: if KVM_SET_TSC_KHZ is important enough for the user to make QEMU
> abort, it should abort if KVM_CAP_TSC_CONTROL isn't even available. On
> the other hand, if the user doesn't care about the lack of
> KVM_CAP_TSC_CONTROL (meaning it isn't possible to call KVM_SET_TSC_KHZ
> at all), I don't see why they would care if KVM_SET_TSC_KHZ failed.
>

Yes, we should treat them equally. If either of KVM_CAP_TSC_CONTROL or
KVM_SET_TSC_KHZ fails, we should display a warning message (but not
fail the migration).

>
> > 
> > > Once we know what exactly is the goal, we could enable the new mode with
> > > a single option, instead of raw options to control migration stream
> > > loading/saving.
> > >
> > 
> > Saving vcpu's TSC frequency does not depend on TSC scaling but the
> > loading does. And that is why I introduce two cpu options to control
> > them separately.
> 
> I understand we probably need internal flags on the source and
> destination side to control saving/loading/setting the tsc-freq. But I
> am still trying to clarify what are the configuration semantics we need,
> exactly, to properly evaluate both the new configuration interface and
> its implementation.
>

As my above replies, we only need an internal flag 'save-tsc-freq' to
indicate whether it needs to save the TSC frequency to the migration
stream. While on the migration destination side, we set to the
migrated TSC frequency only if both TSC scaling is supported and
KVM_SET_TSC_KHZ succeeds; in other cases, we do not set TSC frequency
and not abort the migration.

Haozhong

> -- 
> Eduardo
> --
> To unsubscribe from this list: send the line "unsubscribe kvm" in
> the body of a message to address@hidden
> More majordomo info at  http://vger.kernel.org/majordomo-info.html