Re: [Qemu-devel] [PATCH RESEND V3 4/6] migration: add postcopy downtime into MigrationIncommingState

[Alexey]

On Tue, May 02, 2017 at 09:51:44AM +0100, Dr. David Alan Gilbert wrote:
> * Alexey (address@hidden) wrote:
> > On Fri, Apr 28, 2017 at 05:22:05PM +0100, Dr. David Alan Gilbert wrote:
> > > * Peter Xu (address@hidden) wrote:
> > > > On Fri, Apr 28, 2017 at 01:03:45PM +0300, Alexey Perevalov wrote:
> > > > 
> > > > [...]
> > > > 
> > > > > >>diff --git a/migration/postcopy-ram.c b/migration/postcopy-ram.c
> > > > > >>index 21e7150..f3688f5 100644
> > > > > >>--- a/migration/postcopy-ram.c
> > > > > >>+++ b/migration/postcopy-ram.c
> > > > > >>@@ -132,6 +132,14 @@ static bool ufd_version_check(int ufd, 
> > > > > >>MigrationIncomingState *mis)
> > > > > >>          return false;
> > > > > >>      }
> > > > > >>+#ifdef UFFD_FEATURE_THREAD_ID
> > > > > >>+    if (mis && UFFD_FEATURE_THREAD_ID & supported_features) {
> > > > > >>+        /* kernel supports that feature */
> > > > > >>+        mis->downtime_ctx = downtime_context_new();
> > > > > >>+        new_features |= UFFD_FEATURE_THREAD_ID;
> > > > > >So here I know why in patch 2 new_features == 0...
> > > > > >
> > > > > >If I were you, I would like the series be done in below 4 patches:
> > > > > >
> > > > > >1. update header
> > > > > >2. introduce THREAD_ID feature, and enable it conditionally
> > > > > >3. squash all the downtime thing (downtime context, calculation) in
> > > > > >    one patch here
> > > > > >4. introduce trace
> > > > > >
> > > > > >IMHO that's clearer and easier for review. But I'm okay with current
> > > > > >as well as long as the maintainers (Dave/Juan) won't disagree. :)
> > > > > In previous series, David asked me to split one patch into 2
> > > > > [Qemu-devel] [PATCH 3/6] migration: add UFFD_FEATURE_THREAD_ID feature
> > > > > support
> > > > > 
> > > > > >There seem to be two parts to this:
> > > > > >  a) Adding the mis parameter to ufd_version_check
> > > > > >  b) Asking for the feature
> > > > > 
> > > > > >Please split it into two patches.
> > > > > 
> > > > > So in current patch set, I also added re-factoring, which was missed 
> > > > > before
> > > > > "migration: split ufd_version_check onto receive/request features 
> > > > > part"
> > > > 
> > > > Sure. As long as Dave agrees, I'm okay with either way.
> > > 
> > > I'm OK with the split, it pretty much matches what I asked last time I 
> > > think.
> > > 
> > > The question I still have is how is this memory-expensive feature turned
> > > on and off by the user?
> > > Also I think Peter had some ideas for simpler data structures, how did
> > > that play out?
> > Maybe introduce it as extension of MigrationParameter,
> > I mean { "execute": "migrate-set-parameters" , "arguments":
> >     { "calculate-postcopy-downtime": 1 } }
> 
> Use migrate-set-capabilities, they're effectively the same but just booleans.

For me it's not so clear, where to set that capability, on destination or on 
source
side. User sets postcopy ram capability on source side, probably on
source side user wants to set postcopy-downtime.
If I'm not wrong, neither capabilities nor parameters are transferring
from source to destination.

I wanted to pass in in MIG_CMD_POSTCOPY_ADVISE, but it holds only 2
uint64, and they are already occupied.
Like with RETURN PATH protocol, MIG couldn't be extended w/o breaking backward
compatibility. Length for cmd is transmitted, but compared with
predefined len from mig_cmd_args.

Maybe just increase QEMU_VM_FILE_VERSION in this case, it will be
possible to return downtime back to source by return path.
For supporting backward compatibility keep several versions of mig_cmd_args
per QEMU_VM_FILE_VERSION. 


> Dave
> 
> > 
> > > 
> > > Dave
> > > 
> > > 
> > > > -- 
> > > > Peter Xu
> > > --
> > > Dr. David Alan Gilbert / address@hidden / Manchester, UK
> > > 
> > 
> > -- 
> > 
> > BR
> > Alexey
> --
> Dr. David Alan Gilbert / address@hidden / Manchester, UK
> 

-- 

BR
Alexey