On Fri, Apr 28, 2017 at 05:22:05PM +0100, Dr. David Alan Gilbert wrote:
> * Peter Xu (pet...@redhat.com) 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 } }
>
> Dave
>
>
> > --
> > Peter Xu
> --
> Dr. David Alan Gilbert / dgilb...@redhat.com / Manchester, UK
>
--
BR
Alexey
