Re: [Qemu-devel] [PATCH v3 2/3] target-i386: calculate vcpu's TSC rate to be migrated

[Haozhong Zhang]

On 11/11/15 12:54, Eduardo Habkost wrote:
> On Tue, Nov 10, 2015 at 09:08:58AM +0800, Haozhong Zhang wrote:
> > On 11/09/15 14:01, Eduardo Habkost wrote:
> > > On Mon, Nov 09, 2015 at 08:33:55AM +0800, address@hidden wrote:
> > > > On 11/06/15 13:12, Eduardo Habkost wrote:
> > > > > On Fri, Nov 06, 2015 at 10:32:24AM +0800, address@hidden wrote:
> > > > > > On 11/05/15 14:05, Eduardo Habkost wrote:
> > > > > > > On Thu, Nov 05, 2015 at 09:30:51AM +0800, Haozhong Zhang wrote:
> > > > > > > > On 11/04/15 19:42, Eduardo Habkost wrote:
> > > > > [...]
> > > > > > > > > > +        env->tsc_khz_saved = r;
> > > > > > > > > > +    }
> > > > > > > > > 
> > > > > > > > > Why do you need a separate tsc_khz_saved field, and don't 
> > > > > > > > > simply use
> > > > > > > > > tsc_khz? It would have the additional feature of letting QMP 
> > > > > > > > > clients
> > > > > > > > > query the current TSC rate by asking for the tsc-freq 
> > > > > > > > > property on CPU
> > > > > > > > > objects.
> > > > > > > > >
> > > > > > > > 
> > > > > > > > It's to avoid overriding env->tsc_khz on the destination in the
> > > > > > > > migration. I can change this line to
> > > > > > > >              env->tsc_khz = env->tsc_khz_saved = r;
> > > > > > > 
> > > > > > > You are already avoiding overriding env->tsc_khz, because you use
> > > > > > > KVM_GET_TSC_KHZ only if tsc_khz is not set yet. I still don't see 
> > > > > > > why
> > > > > > > you need a tsc_khz_saved field that requires duplicating the 
> > > > > > > SET_TSC_KHZ
> > > > > > > code, if you could just do this:
> > > > > > > 
> > > > > > >     if (!env->tsc_khz) {
> > > > > > >         env->tsc_khz = kvm_vcpu_ioctl(cs, KVM_GET_TSC_KHZ);
> > > > > > >     }
> > > > > > >
> > > > > > 
> > > > > > Consider an example that we migrate a VM from machine A to machine B
> > > > > > and then to machine C, and QEMU on machine B is launched with the 
> > > > > > cpu
> > > > > > option 'tsc-freq' (i.e. env->tsc_khz on B is non-zero at the
> > > > > > beginning):
> > > > > >  1) In the migration from B to C, the user-specified TSC frequency 
> > > > > > by
> > > > > >     'tsc-freq' on B is expected to be migrated to C. That is, the
> > > > > >     value of env->tsc_khz on B is migrated.
> > > > > >  2) If TSC frequency is migrated through env->tsc_khz, then
> > > > > >     env->tsc_khz on B will be overrode in the migration from A to B
> > > > > >     before kvm_arch_setup_tsc_khz(). If the guest TSC frequency is
> > > > > >     different than the user-specified TSC frequency on B, the
> > > > > >     expectation in 1) will not be satisfied anymore.
> > > > > 
> > > > > Setting tsc-freq on B when tsc-freq was not used on A is invalid 
> > > > > usage.
> > > > > This is not different from changing the CPU model and adding or 
> > > > > removing
> > > > > CPU flags when migrating, which is also incorrect. The command-line
> > > > > parameters defining the VM must be the same when you migrate.
> > > > >
> > > > 
> > > > Good to know it's an invalid usage. Then the question is what QEMU is
> > > > expected to do for this invalid usage?
> > > > 
> > > >  1) Abort the migration? But I find that the current QEMU does not
> > > >     abort the migration between different CPU models (e.g. Nehalem and
> > > >     Haswell).
> > > > 
> > > >  2) Or do not abort the migration and ignore tsc-freq option? If so,
> > > >     tsc_khz_saved will be not needed.
> > > 
> > > My first choice is to abort migration. If we decide to abort today and
> > > find it to cause problems, we can easily fix it. If we decide to
> > > continue without aborting, it is difficult to change that behavior
> > > without breaking existing setups.
> > 
> > Agree, but I would like to relax the abort condition to "abort the
> > migration only if QEMU on the destination uses a different TSC
> > frequency than the migrated one" so that the following usages would be
> > still valid:
> >  1) Only QEMU on the destination has 'tsc-freq' option, but it' set to
> >     the same value of the migrated one.
> >  2) Only QEMU on the source has 'tsc-freq' option.
> >  3) QEMU on both sides have 'tsc-freq' option, but they are set to the
> >     same value.
> > In all above usages, TSC frequency on the destination is the same as
> > both the value on the source and the value explicitly expected by
> > users on the destination (by 'tsc-freq' on the destination).
> 
> Yes, that's probably all we can do because we don't really know
> the command-line on the source. All we know is that the user is
> asking for a TSC frequency that doesn't match what we see in the
> migration stream.
> 
> > 
> > And I still need tsc_khz_saved to tell on the destination whether
> >  1) both tsc-freq option and migrated TSC frequency are present, and
> >  2) above two values are the same.
> > Even though we restrictively requires QEMU on both sides use the same
> > CPU options, tsc_khz_saved is still needed because of 1).
> > 
> 
> So, it looks like we need an extra field because of two things:
> the migration sanity check, and SET_TSC_KHZ error handling. But
> what bothers me is that we have two redundant fields that could
> contradict each other, and it is not clear which one we should
> use on which case. Sometimes only tsc_khz is valid, sometimes
> only tsc_khz is valid.
> 
> What if we set/use tsc_khz on all code (no exceptions), but add a
> new field just to indicate if tsc-freq was set by the user?
> Something like:
> 
> * SET_TSC_KHZ code uses tsc_khz only;
> * "tsc-freq" property getter returns tsc_khz;
> * VMState saves/loads tsc_khz only;
> * Initialization code set tsc_khz = ioctl(KVM_GET_TSC_KHZ) in
>   case tsc_khz is not set.
> 
> And a new user_tsc_khz field would be added and used only for
> error checking:
> * "tsc-freq" property setter changes both tsc_khz and
>   user_tsc_khz;
> * Sanity check code post migration-load ensures that
>   (!user_tsc_khz || tsc_khz == user_tsc_khz);
> * SET_TSC_KHZ error handling returns error if user_tsc_khz
>   is set.
>

Your suggestion looks more clear than my current implementation. I'll
turn to it in the next version.

Thanks,
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