From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:42633) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Zk4qA-0002Pg-2w for qemu-devel@nongnu.org; Thu, 08 Oct 2015 02:38:11 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Zk4q6-0001n4-S1 for qemu-devel@nongnu.org; Thu, 08 Oct 2015 02:38:10 -0400 Received: from szxga02-in.huawei.com ([119.145.14.65]:34069) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Zk4q6-0001gW-2F for qemu-devel@nongnu.org; Thu, 08 Oct 2015 02:38:06 -0400 References: <1441182199-8328-1-git-send-email-zhang.zhanghailiang@huawei.com> <1441182199-8328-3-git-send-email-zhang.zhanghailiang@huawei.com> <560EAA9A.4080708@redhat.com> From: zhanghailiang Message-ID: <56160E80.8080309@huawei.com> Date: Thu, 8 Oct 2015 14:34:40 +0800 MIME-Version: 1.0 In-Reply-To: <560EAA9A.4080708@redhat.com> Content-Type: text/plain; charset="utf-8"; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH COLO-Frame v9 02/32] migration: Introduce capability 'colo' to migration List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Eric Blake , qemu-devel@nongnu.org Cc: lizhijian@cn.fujitsu.com, quintela@redhat.com, Markus Armbruster , yunhong.jiang@intel.com, eddie.dong@intel.com, peter.huangpeng@huawei.com, dgilbert@redhat.com, arei.gonglei@huawei.com, stefanha@redhat.com, amit.shah@redhat.com, yanghy@cn.fujitsu.com On 2015/10/3 0:02, Eric Blake wrote: > On 09/02/2015 02:22 AM, zhanghailiang wrote: >> We add helper function colo_supported() to indicate whether >> colo is supported or not, with which we use to control whether or not >> showing 'colo' string to users, they can use qmp command >> 'query-migrate-capabilities' or hmp command 'info migrate_capabilities' >> to learn if colo is supported. >> >> Cc: Juan Quintela >> Cc: Amit Shah >> Cc: Eric Blake >> Cc: Markus Armbruster >> Signed-off-by: zhanghailiang >> Signed-off-by: Yang Hongyang >> Signed-off-by: Gonglei >> --- > >> +++ b/migration/colo.c >> @@ -0,0 +1,18 @@ >> +/* >> + * COarse-grain LOck-stepping Virtual Machines for Non-stop Service (COLO) >> + * (a.k.a. Fault Tolerance or Continuous Replication) >> + * >> + * Copyright (c) 2015 HUAWEI TECHNOLOGIES CO., LTD. >> + * Copyright (c) 2015 FUJITSU LIMITED >> + * Copyright (c) 2015 Intel Corporation >> + * >> + * This work is licensed under the terms of the GNU GPL, version 2 or >> + * later. See the COPYING file in the top-level directory. >> + */ >> + >> +#include "migration/colo.h" >> + >> +bool colo_supported(void) >> +{ >> + return true; >> +} > > So this exists because you have a configure option that says whether to > include or exclude this .o file (the backup stub version returns false > if this file is not included)... > >> diff --git a/migration/migration.c b/migration/migration.c >> index 662e77e..593cac0 100644 >> --- a/migration/migration.c >> +++ b/migration/migration.c >> @@ -29,6 +29,7 @@ >> #include "trace.h" >> #include "qapi/util.h" >> #include "qapi-event.h" >> +#include "migration/colo.h" >> >> #define MAX_THROTTLE (32 << 20) /* Migration speed throttling */ >> >> @@ -345,6 +346,9 @@ MigrationCapabilityStatusList *qmp_query_migrate_capabilities(Error **errp) >> >> caps = NULL; /* silence compiler warning */ >> for (i = 0; i < MIGRATION_CAPABILITY_MAX; i++) { >> + if (i == MIGRATION_CAPABILITY_COLO && !colo_supported()) { >> + continue; >> + } > > ...and here, you use the result to explicitly remove the colo migration > property from the runtime result of query-migrate-capabilities if > support was not compiled in. That's a good thing, because it means that > a client can call query-migrate-capabilities, and immediately know if > qemu supports colo by whether the migration property is present. > > Especially since the new query-qmp-schema WILL show colo as a member of > the enum of possible values. Knowing that the enum supports a value > doesn't tell you whether runtime supports use of the value, so your > approach definitely helps in a place where static introspection doesn't > solve everything. > Yes, that is the reason we add it. >> if (head == NULL) { >> head = g_malloc0(sizeof(*caps)); >> caps = head; >> @@ -485,6 +489,13 @@ void qmp_migrate_set_capabilities(MigrationCapabilityStatusList *params, >> } >> >> for (cap = params; cap; cap = cap->next) { >> + if (cap->value->capability == MIGRATION_CAPABILITY_COLO && >> + !colo_supported()) { >> + error_setg(errp, "COLO is not currently supported, please" >> + " configure with --enable-colo option in order to" >> + " support COLO feature"); >> + continue; > > Likewise, you explicitly reject attempts to change the property, where > introspection says the enum supports the property. It might be > acceptable to silently permit attempts to set the capability to false > (the value it already has) and only reject attempts to set it to true, > but I'm not sure if the difference is worth it. > Ha, maybe you have forgotten, in last version, you pointed out that maybe it was better to give this error message regardless of true/false being requested, and i think it is reasonable, so i will keep it like this ;) >> + } >> s->enabled_capabilities[cap->value->capability] = cap->value->state; >> } >> } >> @@ -913,6 +924,12 @@ int64_t migrate_xbzrle_cache_size(void) >> return s->xbzrle_cache_size; >> } >> >> +bool migrate_enable_colo(void) >> +{ >> + MigrationState *s = migrate_get_current(); >> + return s->enabled_capabilities[MIGRATION_CAPABILITY_COLO]; > > Function name is wrong - it sounds like an imperative command (call this > to enable colo), when it is really a query command (call this to learn > if colo is enabled). Maybe migrate_colo_enabled()? > Sounds reasonable, will fix in next version. >> +} >> + >> /* migration thread support */ >> >> static void *migration_thread(void *opaque) >> diff --git a/qapi-schema.json b/qapi-schema.json >> index 5f45571..b14d1f4 100644 >> --- a/qapi-schema.json >> +++ b/qapi-schema.json >> @@ -529,11 +529,15 @@ >> # @auto-converge: If enabled, QEMU will automatically throttle down the guest >> # to speed up convergence of RAM migration. (since 1.6) >> # >> +# @colo: If enabled, migration will never end, and the state of the VM on the >> +# primary side will be migrated continuously to the VM on secondary >> +# side. (since 2.5) > > You may want to name this property 'x-colo' to mark it experimental; > especially since there are still a lot of other things waiting to go in > and we are getting closer to 2.5 freeze. Making the property > experimental will relieve the pressure of having to get everything right > on the first try, although it also means that libvirt won't use the > property until we graduate it from experimental 'x-colo' to > fully-supported 'colo'. > Got it, will fix it in next version, thanks.