From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from lists.gnu.org (lists.gnu.org [209.51.188.17]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.lore.kernel.org (Postfix) with ESMTPS id ED4E8CA5FC1 for ; Tue, 20 Jan 2026 17:53:48 +0000 (UTC) Received: from localhost ([::1] helo=lists1p.gnu.org) by lists.gnu.org with esmtp (Exim 4.90_1) (envelope-from ) id 1viFug-0004sS-Li; Tue, 20 Jan 2026 12:53:10 -0500 Received: from eggs.gnu.org ([2001:470:142:3::10]) by lists.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.90_1) (envelope-from ) id 1viFuX-0004nB-Hd for qemu-devel@nongnu.org; Tue, 20 Jan 2026 12:53:02 -0500 Received: from smtp-out2.suse.de ([2a07:de40:b251:101:10:150:64:2]) by eggs.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_128_GCM_SHA256:128) (Exim 4.90_1) (envelope-from ) id 1viFuV-0007YC-GP for qemu-devel@nongnu.org; Tue, 20 Jan 2026 12:53:01 -0500 Received: from imap1.dmz-prg2.suse.org (unknown [10.150.64.97]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (4096 bits) server-digest SHA256) (No client certificate requested) by smtp-out2.suse.de (Postfix) with ESMTPS id 089D75BCCA; Tue, 20 Jan 2026 17:52:56 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=suse.de; s=susede2_rsa; t=1768931576; h=from:from:reply-to:date:date:message-id:message-id:to:to:cc:cc: mime-version:mime-version:content-type:content-type: in-reply-to:in-reply-to:references:references; bh=rRsR6b5NNrt/FEP3jDslfQsJoyFvA62ojcim4VaxA0Y=; b=Iz63dpQDNEjWUc1CiBE3+B41EMrxP9tNlnDe52Hj688Z81oDrFR4VnVVq1GKimRcRwcnW8 xOCz/utsxVYGXjnSecIBSBvXqC9EZAXg43UlyysRND13zScpGEMI1tLv1IevOBjTJcgFPv jpCG0Fi7MKUxch32w7ENoimTK/fLh7Y= DKIM-Signature: v=1; a=ed25519-sha256; c=relaxed/relaxed; d=suse.de; s=susede2_ed25519; t=1768931576; h=from:from:reply-to:date:date:message-id:message-id:to:to:cc:cc: mime-version:mime-version:content-type:content-type: in-reply-to:in-reply-to:references:references; bh=rRsR6b5NNrt/FEP3jDslfQsJoyFvA62ojcim4VaxA0Y=; b=l8SZOCTwCgq6X1NsGfph2JweAjGS6ZgNgYkaTAI2AvmV/FIOuI5tHmcwmKKrzcBQTqe7UQ PZwP2aNST5f1vHAQ== Authentication-Results: smtp-out2.suse.de; none DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=suse.de; s=susede2_rsa; t=1768931576; h=from:from:reply-to:date:date:message-id:message-id:to:to:cc:cc: mime-version:mime-version:content-type:content-type: in-reply-to:in-reply-to:references:references; bh=rRsR6b5NNrt/FEP3jDslfQsJoyFvA62ojcim4VaxA0Y=; b=Iz63dpQDNEjWUc1CiBE3+B41EMrxP9tNlnDe52Hj688Z81oDrFR4VnVVq1GKimRcRwcnW8 xOCz/utsxVYGXjnSecIBSBvXqC9EZAXg43UlyysRND13zScpGEMI1tLv1IevOBjTJcgFPv jpCG0Fi7MKUxch32w7ENoimTK/fLh7Y= DKIM-Signature: v=1; a=ed25519-sha256; c=relaxed/relaxed; d=suse.de; s=susede2_ed25519; t=1768931576; h=from:from:reply-to:date:date:message-id:message-id:to:to:cc:cc: mime-version:mime-version:content-type:content-type: in-reply-to:in-reply-to:references:references; bh=rRsR6b5NNrt/FEP3jDslfQsJoyFvA62ojcim4VaxA0Y=; b=l8SZOCTwCgq6X1NsGfph2JweAjGS6ZgNgYkaTAI2AvmV/FIOuI5tHmcwmKKrzcBQTqe7UQ PZwP2aNST5f1vHAQ== Received: from imap1.dmz-prg2.suse.org (localhost [127.0.0.1]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (4096 bits) server-digest SHA256) (No client certificate requested) by imap1.dmz-prg2.suse.org (Postfix) with ESMTPS id 70DC83EA63; Tue, 20 Jan 2026 17:52:55 +0000 (UTC) Received: from dovecot-director2.suse.de ([2a07:de40:b281:106:10:150:64:167]) by imap1.dmz-prg2.suse.org with ESMTPSA id ESdjC/fAb2mnKwAAD6G6ig (envelope-from ); Tue, 20 Jan 2026 17:52:55 +0000 From: Fabiano Rosas To: Prasad Pandit Cc: qemu-devel@nongnu.org, peterx@redhat.com, berrange@redhat.com, Mark Kanda , Ben Chaney Subject: Re: [PATCH v3 06/25] migration: Use migrate_mode() to query for cpr-transfer In-Reply-To: References: <20260109124043.25019-1-farosas@suse.de> <20260109124043.25019-7-farosas@suse.de> Date: Tue, 20 Jan 2026 14:52:52 -0300 Message-ID: <87o6mo893f.fsf@suse.de> MIME-Version: 1.0 Content-Type: text/plain X-Spamd-Result: default: False [-4.30 / 50.00]; BAYES_HAM(-3.00)[100.00%]; NEURAL_HAM_LONG(-1.00)[-1.000]; NEURAL_HAM_SHORT(-0.20)[-1.000]; MIME_GOOD(-0.10)[text/plain]; ARC_NA(0.00)[]; MISSING_XM_UA(0.00)[]; RCVD_TLS_ALL(0.00)[]; MIME_TRACE(0.00)[0:+]; RCVD_VIA_SMTP_AUTH(0.00)[]; TO_DN_SOME(0.00)[]; FUZZY_RATELIMITED(0.00)[rspamd.com]; MID_RHS_MATCH_FROM(0.00)[]; DKIM_SIGNED(0.00)[suse.de:s=susede2_rsa,suse.de:s=susede2_ed25519]; FROM_HAS_DN(0.00)[]; RCPT_COUNT_FIVE(0.00)[6]; FROM_EQ_ENVFROM(0.00)[]; TO_MATCH_ENVRCPT_ALL(0.00)[]; RCVD_COUNT_TWO(0.00)[2]; DBL_BLOCKED_OPENRESOLVER(0.00)[oracle.com:email,suse.de:mid,suse.de:email] Received-SPF: pass client-ip=2a07:de40:b251:101:10:150:64:2; envelope-from=farosas@suse.de; helo=smtp-out2.suse.de X-Spam_score_int: -20 X-Spam_score: -2.1 X-Spam_bar: -- X-Spam_report: (-2.1 / 5.0 requ) BAYES_00=-1.9, DKIM_SIGNED=0.1, DKIM_VALID=-0.1, DKIM_VALID_AU=-0.1, DKIM_VALID_EF=-0.1, SPF_HELO_NONE=0.001, SPF_PASS=-0.001 autolearn=ham autolearn_force=no X-Spam_action: no action X-BeenThere: qemu-devel@nongnu.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: qemu development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: qemu-devel-bounces+qemu-devel=archiver.kernel.org@nongnu.org Sender: qemu-devel-bounces+qemu-devel=archiver.kernel.org@nongnu.org Prasad Pandit writes: > Hi, > > On Fri, 9 Jan 2026 at 18:12, Fabiano Rosas wrote: >> cpr_set_incoming_mode() is only called on the target side, so >> migrate_mode() on the source side is the same as s->parameters.mode. > > * This message is a little confusing, as the connection between > cpr_set_incoming_mode() and migrate_mode() is not evident. IIUC, > during CPR migration mode is not set explicitly on the source side, so > cpr_get_incoming_mode() returns none, so migrate_mode() sets it to > s->parameters.mode and returns it. Ideally it should be set to > s->parameters.mode. /* I wonder why we need such differention between > incoming and outgoing modes. */ > Good question. I had to go check. It's subtle and not properly documented: cpr_state_load() is called before current_migration exists. There is no parameters.mode at that point. >> Use the function to reduce explicit access to s->parameters, we have >> options.c for that. >> >> Cc: Mark Kanda >> Cc: Ben Chaney >> Reviewed-by: Peter Xu >> Signed-off-by: Fabiano Rosas >> --- >> migration/cpr-exec.c | 2 +- >> migration/migration.c | 27 +++++++++++++-------------- >> migration/migration.h | 5 ++--- >> 3 files changed, 16 insertions(+), 18 deletions(-) >> >> diff --git a/migration/cpr-exec.c b/migration/cpr-exec.c >> index da287d8031..e315a30f92 100644 >> --- a/migration/cpr-exec.c >> +++ b/migration/cpr-exec.c >> @@ -164,7 +164,7 @@ static void cpr_exec_cb(void *opaque) >> err = NULL; >> >> /* Note, we can go from state COMPLETED to FAILED */ > > /* Going from COMPLETED -> FAILED says something not right here, > maybe we are prematurely moving to COMPLETED state. */ > I agree there's a dissonance, but there is no "after" to counterpart this "prematurely". cpr-exec happens after migration completes, it needs to receive the MIG_EVENT_PRECOPY_DONE completion notifier. But then there's a slight chance that the execvp() may fail. If it fails, then migration is naturally FAILED and the MIG_EVENT_PRECOPY_FAILED notifiers are called below. If it succeeds, then it's another process image, s->state is no more. There might also be considerations from the management layer side in seeing the completion event and from the device migration code in seeing the completion notifier, I'm not sure. >> - migration_call_notifiers(s, MIG_EVENT_PRECOPY_FAILED, NULL); >> + migration_call_notifiers(MIG_EVENT_PRECOPY_FAILED, NULL); >> >> if (!migration_block_activate(&err)) { >> /* error was already reported */ >> diff --git a/migration/migration.c b/migration/migration.c >> index 4af5baad59..388e0be5a2 100644 >> --- a/migration/migration.c >> +++ b/migration/migration.c >> @@ -1534,7 +1534,7 @@ static void migration_cleanup(MigrationState *s) >> } >> type = migration_has_failed(s) ? MIG_EVENT_PRECOPY_FAILED : >> MIG_EVENT_PRECOPY_DONE; >> - migration_call_notifiers(s, type, NULL); >> + migration_call_notifiers(type, NULL); >> yank_unregister_instance(MIGRATION_YANK_INSTANCE); >> } >> >> @@ -1696,10 +1696,9 @@ void migration_remove_notifier(NotifierWithReturn *notify) >> } >> } >> >> -int migration_call_notifiers(MigrationState *s, MigrationEventType type, >> - Error **errp) >> +int migration_call_notifiers(MigrationEventType type, Error **errp) >> { >> - MigMode mode = s->parameters.mode; >> + MigMode mode = migrate_mode(); >> MigrationEvent e; >> NotifierWithReturn *notifier; >> GSList *elem, *next; >> @@ -1780,9 +1779,9 @@ bool migration_thread_is_self(void) >> return qemu_thread_is_self(&s->thread); >> } >> >> -bool migrate_mode_is_cpr(MigrationState *s) >> +bool migrate_mode_is_cpr(void) >> { >> - MigMode mode = s->parameters.mode; >> + MigMode mode = migrate_mode(); >> return mode == MIG_MODE_CPR_REBOOT || >> mode == MIG_MODE_CPR_TRANSFER || >> mode == MIG_MODE_CPR_EXEC; >> @@ -2136,7 +2135,7 @@ static bool migrate_prepare(MigrationState *s, bool resume, Error **errp) >> } >> } >> >> - if (migrate_mode_is_cpr(s)) { >> + if (migrate_mode_is_cpr()) { >> const char *conflict = NULL; >> >> if (migrate_postcopy()) { >> @@ -2252,7 +2251,7 @@ void qmp_migrate(const char *uri, bool has_channels, >> return; >> } >> >> - if (s->parameters.mode == MIG_MODE_CPR_TRANSFER && !cpr_channel) { >> + if (migrate_mode() == MIG_MODE_CPR_TRANSFER && !cpr_channel) { >> error_setg(errp, "missing 'cpr' migration channel"); >> return; >> } >> @@ -2277,7 +2276,7 @@ void qmp_migrate(const char *uri, bool has_channels, >> * in which case the target will not listen for the incoming migration >> * connection, so qmp_migrate_finish will fail to connect, and then recover. >> */ >> - if (s->parameters.mode == MIG_MODE_CPR_TRANSFER) { >> + if (migrate_mode() == MIG_MODE_CPR_TRANSFER) { > > * Shouldn't we use migrate_mode_is_cpr() to avoid this comparison? > There are other CPR modes that I don't think we want to match here, only cpr-transfer. >> migrate_hup_add(s, cpr_state_ioc(), (GSourceFunc)qmp_migrate_finish_cb, >> QAPI_CLONE(MigrationAddress, addr)); >> >> @@ -2852,7 +2851,7 @@ static int postcopy_start(MigrationState *ms, Error **errp) >> * at the transition to postcopy and after the device state; in particular >> * spice needs to trigger a transition now >> */ >> - migration_call_notifiers(ms, MIG_EVENT_PRECOPY_DONE, NULL); >> + migration_call_notifiers(MIG_EVENT_PRECOPY_DONE, NULL); >> >> migration_downtime_end(ms); >> >> @@ -2901,7 +2900,7 @@ fail: >> migrate_set_state(&ms->state, ms->state, MIGRATION_STATUS_FAILED); >> } >> migration_block_activate(NULL); >> - migration_call_notifiers(ms, MIG_EVENT_PRECOPY_FAILED, NULL); >> + migration_call_notifiers(MIG_EVENT_PRECOPY_FAILED, NULL); >> bql_unlock(); >> return -1; >> } >> @@ -3003,7 +3002,7 @@ static int migration_completion_precopy(MigrationState *s) >> >> bql_lock(); >> >> - if (!migrate_mode_is_cpr(s)) { >> + if (!migrate_mode_is_cpr()) { >> ret = migration_stop_vm(s, RUN_STATE_FINISH_MIGRATE); >> if (ret < 0) { >> goto out_unlock; >> @@ -4049,7 +4048,7 @@ void migration_connect(MigrationState *s, Error *error_in) >> rate_limit = migrate_max_bandwidth(); >> >> /* Notify before starting migration thread */ >> - if (migration_call_notifiers(s, MIG_EVENT_PRECOPY_SETUP, &local_err)) { >> + if (migration_call_notifiers(MIG_EVENT_PRECOPY_SETUP, &local_err)) { >> goto fail; >> } >> } >> @@ -4085,7 +4084,7 @@ void migration_connect(MigrationState *s, Error *error_in) >> return; >> } >> >> - if (migrate_mode_is_cpr(s)) { >> + if (migrate_mode_is_cpr()) { >> ret = migration_stop_vm(s, RUN_STATE_FINISH_MIGRATE); >> if (ret < 0) { >> error_setg(&local_err, "migration_stop_vm failed, error %d", -ret); >> diff --git a/migration/migration.h b/migration/migration.h >> index ccc4e536a5..8b55d4741a 100644 >> --- a/migration/migration.h >> +++ b/migration/migration.h >> @@ -538,8 +538,7 @@ bool migrate_has_error(MigrationState *s); >> >> void migration_connect(MigrationState *s, Error *error_in); >> >> -int migration_call_notifiers(MigrationState *s, MigrationEventType type, >> - Error **errp); >> +int migration_call_notifiers(MigrationEventType type, Error **errp); >> >> int migrate_init(MigrationState *s, Error **errp); >> bool migration_is_blocked(Error **errp); >> @@ -548,7 +547,7 @@ bool migration_in_postcopy(void); >> bool migration_postcopy_is_alive(MigrationStatus state); >> MigrationState *migrate_get_current(void); >> bool migration_has_failed(MigrationState *); >> -bool migrate_mode_is_cpr(MigrationState *); >> +bool migrate_mode_is_cpr(void); >> >> uint64_t ram_get_total_transferred_pages(void); >> >> -- > > * Otherwise change looks okay. Needs minor fix above. > Reviewed-by: Prasad Pandit > > Thank you. > --- > - Prasad