| Message ID | 20180103093834.20879-1-quintela@redhat.com |
|---|---|
| Headers | show |
| Series | Migration pull request | expand |
On 01/03/2018 03:38 AM, Juan Quintela wrote: > Hi > > This are the changes for migration that are already reviewed. > > Please, apply. > > Alexey Perevalov (6): > migration: introduce postcopy-blocktime capability > migration: add postcopy blocktime ctx into MigrationIncomingState > migration: calculate vCPU blocktime on dst side > migration: postcopy_blocktime documentation > migration: add blocktime calculation into migration-test > migration: add postcopy total blocktime into query-migrate I had unanswered questions about these patches in the v12 series, where I'm not sure if the interface is still quite right. We're still early enough that we could adjust the interface after the fact depending on how the questions are answered; but we're also early enough that it may be smarter to get the interface right before including it in a pull request. I'll leave it to Peter and Juan to sort out whether this means an updated pull request is needed, or to take this as-is.
* Eric Blake (eblake@redhat.com) wrote: > On 01/03/2018 03:38 AM, Juan Quintela wrote: > > Hi > > > > This are the changes for migration that are already reviewed. > > > > Please, apply. > > > > > Alexey Perevalov (6): > > migration: introduce postcopy-blocktime capability > > migration: add postcopy blocktime ctx into MigrationIncomingState > > migration: calculate vCPU blocktime on dst side > > migration: postcopy_blocktime documentation > > migration: add blocktime calculation into migration-test > > migration: add postcopy total blocktime into query-migrate > > I had unanswered questions about these patches in the v12 series, where > I'm not sure if the interface is still quite right. We're still early > enough that we could adjust the interface after the fact depending on > how the questions are answered; but we're also early enough that it may > be smarter to get the interface right before including it in a pull > request. I'll leave it to Peter and Juan to sort out whether this means > an updated pull request is needed, or to take this as-is. Hadn't that discussion been done to death about 10 times during the (long) life of this series? Dave > -- > Eric Blake, Principal Software Engineer > Red Hat, Inc. +1-919-301-3266 > Virtualization: qemu.org | libvirt.org > -- Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
Eric Blake <eblake@redhat.com> wrote: > On 01/03/2018 03:38 AM, Juan Quintela wrote: >> Hi >> >> This are the changes for migration that are already reviewed. >> >> Please, apply. >> > >> Alexey Perevalov (6): >> migration: introduce postcopy-blocktime capability >> migration: add postcopy blocktime ctx into MigrationIncomingState >> migration: calculate vCPU blocktime on dst side >> migration: postcopy_blocktime documentation >> migration: add blocktime calculation into migration-test >> migration: add postcopy total blocktime into query-migrate > > I had unanswered questions about these patches in the v12 series, where > I'm not sure if the interface is still quite right. To be fair, I had alroady integrated the patches before I saw your questions. > We're still early > enough that we could adjust the interface after the fact depending on > how the questions are answered; I think this is the best approach, so far I can see two questions: - do we want to make it conditional? it requires some locking, but I haven't meassured it to see how slow/fast is it. - the other was documentation. I will like Alexey to answer. Depending of how slow it is, I can agree to make it non-optional. > but we're also early enough that it may > be smarter to get the interface right before including it in a pull > request. I'll leave it to Peter and Juan to sort out whether this means > an updated pull request is needed, or to take this as-is. Thanks, Juan.
On 5 January 2018 at 09:59, Juan Quintela <quintela@redhat.com> wrote: > Eric Blake <eblake@redhat.com> wrote: >> On 01/03/2018 03:38 AM, Juan Quintela wrote: >>> Hi >>> >>> This are the changes for migration that are already reviewed. >>> >>> Please, apply. >>> >> >>> Alexey Perevalov (6): >>> migration: introduce postcopy-blocktime capability >>> migration: add postcopy blocktime ctx into MigrationIncomingState >>> migration: calculate vCPU blocktime on dst side >>> migration: postcopy_blocktime documentation >>> migration: add blocktime calculation into migration-test >>> migration: add postcopy total blocktime into query-migrate >> >> I had unanswered questions about these patches in the v12 series, where >> I'm not sure if the interface is still quite right. > > To be fair, I had alroady integrated the patches before I saw your questions. > >> We're still early >> enough that we could adjust the interface after the fact depending on >> how the questions are answered; > > I think this is the best approach, so far I can see two questions: > > - do we want to make it conditional? it requires some locking, but I > haven't meassured it to see how slow/fast is it. > > - the other was documentation. > > I will like Alexey to answer. Depending of how slow it is, I can agree > to make it non-optional. So have you come to a consensus on whether you'd like me to apply this pull request or not ? thanks -- PMM
Peter Maydell <peter.maydell@linaro.org> wrote: > On 5 January 2018 at 09:59, Juan Quintela <quintela@redhat.com> wrote: >> Eric Blake <eblake@redhat.com> wrote: >>> On 01/03/2018 03:38 AM, Juan Quintela wrote: >>>> Hi >>>> >>>> This are the changes for migration that are already reviewed. >>>> >>>> Please, apply. >>>> >>> >>>> Alexey Perevalov (6): >>>> migration: introduce postcopy-blocktime capability >>>> migration: add postcopy blocktime ctx into MigrationIncomingState >>>> migration: calculate vCPU blocktime on dst side >>>> migration: postcopy_blocktime documentation >>>> migration: add blocktime calculation into migration-test >>>> migration: add postcopy total blocktime into query-migrate >>> >>> I had unanswered questions about these patches in the v12 series, where >>> I'm not sure if the interface is still quite right. >> >> To be fair, I had alroady integrated the patches before I saw your questions. >> >>> We're still early >>> enough that we could adjust the interface after the fact depending on >>> how the questions are answered; >> >> I think this is the best approach, so far I can see two questions: >> >> - do we want to make it conditional? it requires some locking, but I >> haven't meassured it to see how slow/fast is it. >> >> - the other was documentation. >> >> I will like Alexey to answer. Depending of how slow it is, I can agree >> to make it non-optional. > > So have you come to a consensus on whether you'd like me to apply > this pull request or not ? My understanding is that you should apply it. We will add a documentation patch later. Later, Juan.
On 01/05/2018 12:59 PM, Juan Quintela wrote: > Eric Blake <eblake@redhat.com> wrote: >> On 01/03/2018 03:38 AM, Juan Quintela wrote: >>> Hi >>> >>> This are the changes for migration that are already reviewed. >>> >>> Please, apply. >>> >>> Alexey Perevalov (6): >>> migration: introduce postcopy-blocktime capability >>> migration: add postcopy blocktime ctx into MigrationIncomingState >>> migration: calculate vCPU blocktime on dst side >>> migration: postcopy_blocktime documentation >>> migration: add blocktime calculation into migration-test >>> migration: add postcopy total blocktime into query-migrate >> I had unanswered questions about these patches in the v12 series, where >> I'm not sure if the interface is still quite right. > To be fair, I had alroady integrated the patches before I saw your questions. > >> We're still early >> enough that we could adjust the interface after the fact depending on >> how the questions are answered; > I think this is the best approach, so far I can see two questions: > > - do we want to make it conditional? it requires some locking, but I > haven't meassured it to see how slow/fast is it. > > - the other was documentation. > > I will like Alexey to answer. Depending of how slow it is, I can agree > to make it non-optional. Ok, I'll give a logs with traces, maybe gprof result, today or tomorrow. > >> but we're also early enough that it may >> be smarter to get the interface right before including it in a pull >> request. I'll leave it to Peter and Juan to sort out whether this means >> an updated pull request is needed, or to take this as-is. > Thanks, Juan. > > >
On 3 January 2018 at 09:38, Juan Quintela <quintela@redhat.com> wrote: > Hi > > This are the changes for migration that are already reviewed. > > Please, apply. > > Later, Juan. > > > The following changes since commit 281f327487c9c9b1599f93c589a408bbf4a651b8: > > Merge remote-tracking branch 'remotes/vivier/tags/m68k-for-2.12-pull-request' into staging (2017-12-22 00:11:36 +0000) > > are available in the Git repository at: > > git://github.com/juanquintela/qemu.git tags/migration/20180103 > > for you to fetch changes up to 8a18afdcd8d2d6ab31f9de89d2f20fdadb89beb8: > > migration: finalize current_migration object (2018-01-03 09:28:56 +0100) > > ---------------------------------------------------------------- > migration/next for 20180103 > This fails to build on 32-bit systems: /home/peter.maydell/qemu/migration/postcopy-ram.c: In function 'mark_postcopy_blocktime_begin': /home/peter.maydell/qemu/migration/postcopy-ram.c:658:54: error: cast to pointer from integer of different size [-Werror=int-to-pointer-cast] already_received = ramblock_recv_bitmap_test(rb, (void *)addr); ^ Keeping pointers in uint64_t and casting all over the place looks like the wrong thing -- using the uintptr_t type will likely make things cleaner. This also looks suspicious: atomic_xchg__nocheck(&dc->last_begin, now_ms); atomic_xchg__nocheck(&dc->page_fault_vcpu_time[cpu], now_ms); atomic_xchg__nocheck(&dc->vcpu_addr[cpu], addr); because atomic operations on types larger than the host pointer size are not portable to all architectures. This should probably use the atomic_cmpxchg(), not __nocheck, because the latter bypasses the build time assert for this problem and turns a "fails on any 32-bit host" into "fails obscurely on some architectures only". thanks -- PMM
Hi This are the changes for migration that are already reviewed. Please, apply. Later, Juan. The following changes since commit 281f327487c9c9b1599f93c589a408bbf4a651b8: Merge remote-tracking branch 'remotes/vivier/tags/m68k-for-2.12-pull-request' into staging (2017-12-22 00:11:36 +0000) are available in the Git repository at: git://github.com/juanquintela/qemu.git tags/migration/20180103 for you to fetch changes up to 8a18afdcd8d2d6ab31f9de89d2f20fdadb89beb8: migration: finalize current_migration object (2018-01-03 09:28:56 +0100) ---------------------------------------------------------------- migration/next for 20180103 ---------------------------------------------------------------- Alexey Perevalov (6): migration: introduce postcopy-blocktime capability migration: add postcopy blocktime ctx into MigrationIncomingState migration: calculate vCPU blocktime on dst side migration: postcopy_blocktime documentation migration: add blocktime calculation into migration-test migration: add postcopy total blocktime into query-migrate Dr. David Alan Gilbert (2): docs: Convert migration.txt to rst migration: Guard ram_bytes_remaining against early call Juan Quintela (4): migration: Use proper types in json migration: print features as on off migration: free addr in the same function that we created it migration: free result string Laurent Vivier (1): migration: fix analyze-migration.py script with radix table Vladimir Sementsov-Ogievskiy (1): migration: finalize current_migration object docs/devel/{migration.txt => migration.rst} | 484 +++++++++++++++------------- hmp.c | 37 ++- include/migration/misc.h | 1 + migration/migration.c | 118 ++++--- migration/migration.h | 13 + migration/postcopy-ram.c | 258 ++++++++++++++- migration/ram.c | 3 +- migration/socket.c | 4 +- migration/trace-events | 6 +- qapi/migration.json | 37 ++- scripts/analyze-migration.py | 4 + tests/migration-test.c | 19 +- vl.c | 1 + 13 files changed, 696 insertions(+), 289 deletions(-) rename docs/devel/{migration.txt => migration.rst} (58%)