aboutsummaryrefslogtreecommitdiff
path: root/migration
AgeCommit message (Collapse)Author
2023-10-17migration/rdma: Unfold ram_control_before_iterate()Juan Quintela
Once there: - Remove unused data parameter - unfold it in its callers. - change all callers to call qemu_rdma_registration_start() - We need to call QIO_CHANNEL_RDMA() after we check for migrate_rdma() Reviewed-by: Li Zhijian <lizhijian@fujitsu.com> Reviewed-by: Fabiano Rosas <farosas@suse.de> Signed-off-by: Juan Quintela <quintela@redhat.com> Message-ID: <20231011203527.9061-3-quintela@redhat.com>
2023-10-17migration: Create migrate_rdma()Juan Quintela
Helper to say if we are doing a migration over rdma. Reviewed-by: Peter Xu <peterx@redhat.com> Reviewed-by: Li Zhijian <lizhijian@fujitsu.com> Signed-off-by: Juan Quintela <quintela@redhat.com> Message-ID: <20231011203527.9061-2-quintela@redhat.com>
2023-10-17migration: Non multifd migration don't care about multifd flushesJuan Quintela
RDMA was having trouble because migrate_multifd_flush_after_each_section() can only be true or false, but we don't want to send any flush when we are not in multifd migration. CC: Fabiano Rosas <farosas@suse.de Fixes: 294e5a4034e81 ("multifd: Only flush once each full round of memory") Reported-by: Li Zhijian <lizhijian@fujitsu.com> Reviewed-by: Li Zhijian <lizhijian@fujitsu.com> Reviewed-by: Peter Xu <peterx@redhat.com> Signed-off-by: Juan Quintela <quintela@redhat.com> Message-ID: <20231011205548.10571-2-quintela@redhat.com>
2023-10-17migration: hold the BQL during setupFiona Ebner
This is intended to be a semantic revert of commit 9b09503752 ("migration: run setup callbacks out of big lock"). There have been so many changes since that commit (e.g. a new setup callback dirty_bitmap_save_setup() that also needs to be adapted now), it's easier to do the revert manually. For snapshots, the bdrv_writev_vmstate() function is used during setup (in QIOChannelBlock backing the QEMUFile), but not holding the BQL while calling it could lead to an assertion failure. To understand how, first note the following: 1. Generated coroutine wrappers for block layer functions spawn the coroutine and use AIO_WAIT_WHILE()/aio_poll() to wait for it. 2. If the host OS switches threads at an inconvenient time, it can happen that a bottom half scheduled for the main thread's AioContext is executed as part of a vCPU thread's aio_poll(). An example leading to the assertion failure is as follows: main thread: 1. A snapshot-save QMP command gets issued. 2. snapshot_save_job_bh() is scheduled. vCPU thread: 3. aio_poll() for the main thread's AioContext is called (e.g. when the guest writes to a pflash device, as part of blk_pwrite which is a generated coroutine wrapper). 4. snapshot_save_job_bh() is executed as part of aio_poll(). 3. qemu_savevm_state() is called. 4. qemu_mutex_unlock_iothread() is called. Now qemu_get_current_aio_context() returns 0x0. 5. bdrv_writev_vmstate() is executed during the usual savevm setup via qemu_fflush(). But this function is a generated coroutine wrapper, so it uses AIO_WAIT_WHILE. There, the assertion assert(qemu_get_current_aio_context() == qemu_get_aio_context()); will fail. To fix it, ensure that the BQL is held during setup. While it would only be needed for snapshots, adapting migration too avoids additional logic for conditional locking/unlocking in the setup callbacks. Writing the header could (in theory) also trigger qemu_fflush() and thus bdrv_writev_vmstate(), so the locked section also covers the qemu_savevm_state_header() call, even for migration for consistency. The section around multifd_send_sync_main() needs to be unlocked to avoid a deadlock. In particular, the multifd_save_setup() function calls socket_send_channel_create() using multifd_new_send_channel_async() as a callback and then waits for the callback to signal via the channels_ready semaphore. The connection happens via qio_task_run_in_thread(), but the callback is only executed via qio_task_thread_result() which is scheduled for the main event loop. Without unlocking the section, the main thread would never get to process the task result and the callback meaning there would be no signal via the channels_ready semaphore. The comment in ram_init_bitmaps() was introduced by 4987783400 ("migration: fix incorrect memory_global_dirty_log_start outside BQL") and is removed, because it referred to the qemu_mutex_lock_iothread() call. Signed-off-by: Fiona Ebner <f.ebner@proxmox.com> Reviewed-by: Fabiano Rosas <farosas@suse.de> Reviewed-by: Juan Quintela <quintela@redhat.com> Signed-off-by: Juan Quintela <quintela@redhat.com> Message-ID: <20231013105839.415989-1-f.ebner@proxmox.com>
2023-10-17migration: Add the configuration vmstate to the json writerNikolay Borisov
Make the migration json writer part of MigrationState struct, allowing the 'configuration' object be serialized to json. This will facilitate the parsing of the 'configuration' object in the next patch that fixes analyze-migration.py for arm. Signed-off-by: Nikolay Borisov <nborisov@suse.com> Signed-off-by: Fabiano Rosas <farosas@suse.de> Reviewed-by: Juan Quintela <quintela@redhat.com> Signed-off-by: Juan Quintela <quintela@redhat.com> Message-ID: <20231009184326.15777-2-farosas@suse.de>
2023-10-17migration: fix RAMBlock add NULL checkDmitry Frolov
qemu_ram_block_from_host() may return NULL, which will be dereferenced w/o check. Usualy return value is checked for this function. Found by Linux Verification Center (linuxtesting.org) with SVACE. Signed-off-by: Dmitry Frolov <frolov@swemel.ru> Reviewed-by: Fabiano Rosas <farosas@suse.de> Reviewed-by: Peter Xu <peterx@redhat.com> Reviewed-by: Juan Quintela <quintela@redhat.com> Signed-off-by: Juan Quintela <quintela@redhat.com> Message-ID: <20231010104851.802947-1-frolov@swemel.ru>
2023-10-17migration: Allow user to specify available switchover bandwidthPeter Xu
Migration bandwidth is a very important value to live migration. It's because it's one of the major factors that we'll make decision on when to switchover to destination in a precopy process. This value is currently estimated by QEMU during the whole live migration process by monitoring how fast we were sending the data. This can be the most accurate bandwidth if in the ideal world, where we're always feeding unlimited data to the migration channel, and then it'll be limited to the bandwidth that is available. However in reality it may be very different, e.g., over a 10Gbps network we can see query-migrate showing migration bandwidth of only a few tens of MB/s just because there are plenty of other things the migration thread might be doing. For example, the migration thread can be busy scanning zero pages, or it can be fetching dirty bitmap from other external dirty sources (like vhost or KVM). It means we may not be pushing data as much as possible to migration channel, so the bandwidth estimated from "how many data we sent in the channel" can be dramatically inaccurate sometimes. With that, the decision to switchover will be affected, by assuming that we may not be able to switchover at all with such a low bandwidth, but in reality we can. The migration may not even converge at all with the downtime specified, with that wrong estimation of bandwidth, keeping iterations forever with a low estimation of bandwidth. The issue is QEMU itself may not be able to avoid those uncertainties on measuing the real "available migration bandwidth". At least not something I can think of so far. One way to fix this is when the user is fully aware of the available bandwidth, then we can allow the user to help providing an accurate value. For example, if the user has a dedicated channel of 10Gbps for migration for this specific VM, the user can specify this bandwidth so QEMU can always do the calculation based on this fact, trusting the user as long as specified. It may not be the exact bandwidth when switching over (in which case qemu will push migration data as fast as possible), but much better than QEMU trying to wildly guess, especially when very wrong. A new parameter "avail-switchover-bandwidth" is introduced just for this. So when the user specified this parameter, instead of trusting the estimated value from QEMU itself (based on the QEMUFile send speed), it trusts the user more by using this value to decide when to switchover, assuming that we'll have such bandwidth available then. Note that specifying this value will not throttle the bandwidth for switchover yet, so QEMU will always use the full bandwidth possible for sending switchover data, assuming that should always be the most important way to use the network at that time. This can resolve issues like "unconvergence migration" which is caused by hilarious low "migration bandwidth" detected for whatever reason. Reported-by: Zhiyi Guo <zhguo@redhat.com> Reviewed-by: Joao Martins <joao.m.martins@oracle.com> Reviewed-by: Juan Quintela <quintela@redhat.com> Signed-off-by: Peter Xu <peterx@redhat.com> Signed-off-by: Juan Quintela <quintela@redhat.com> Message-ID: <20231010221922.40638-1-peterx@redhat.com>
2023-10-17migration: Use g_autofree to simplify ram_dirty_bitmap_reload()Philippe Mathieu-Daudé
Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org> Reviewed-by: Markus Armbruster <armbru@redhat.com> Reviewed-by: Fabiano Rosas <farosas@suse.de> Reviewed-by: Juan Quintela <quintela@redhat.com> Signed-off-by: Juan Quintela <quintela@redhat.com> Message-ID: <20231011023627.86691-1-philmd@linaro.org>
2023-10-17migration: refactor migration_completionWei Wang
Current migration_completion function is a bit long. Refactor the long implementation into different subfunctions: - migration_completion_precopy: completion code related to precopy - migration_completion_postcopy: completion code related to postcopy Rename await_return_path_close_on_source to close_return_path_on_source: It is renamed to match with open_return_path_on_source. This improves readability and is easier for future updates (e.g. add new subfunctions when completion code related to new features are needed). No functional changes intended. Signed-off-by: Wei Wang <wei.w.wang@intel.com> Reviewed-by: Peter Xu <peterx@redhat.com> Reviewed-by: Isaku Yamahata <isaku.yamahata@intel.com> Reviewed-by: Juan Quintela <quintela@redhat.com> Message-ID: <20230804093053.5037-1-wei.w.wang@intel.com> Signed-off-by: Juan Quintela <quintela@redhat.com>
2023-10-12block: Mark bdrv_first_blk() and bdrv_is_root_node() GRAPH_RDLOCKKevin Wolf
This adds GRAPH_RDLOCK annotations to declare that callers of bdrv_first_blk() and bdrv_is_root_node() need to hold a reader lock for the graph. These functions are the only functions in block-backend.c that access the parent list of a node. Signed-off-by: Kevin Wolf <kwolf@redhat.com> Message-ID: <20230929145157.45443-5-kwolf@redhat.com> Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
2023-10-11migration: Add migration_rp_wait|kick()Peter Xu
It's just a simple wrapper for rp_sem on either wait() or kick(), make it even clearer on how it is used. Prepared to be used even for other things. Reviewed-by: Fabiano Rosas <farosas@suse.de> Signed-off-by: Peter Xu <peterx@redhat.com> Message-ID: <20231004220240.167175-8-peterx@redhat.com> Signed-off-by: Juan Quintela <quintela@redhat.com>
2023-10-11migration: Remember num of ramblocks to sync during recoveryPeter Xu
Instead of only relying on the count of rp_sem, make the counter be part of RAMState so it can be used in both threads to synchronize on the process. rp_sem will be further reused in follow up patches, as a way to kick the main thread, e.g., on recovery failures. Reviewed-by: Fabiano Rosas <farosas@suse.de> Reviewed-by: Juan Quintela <quintela@redhat.com> Signed-off-by: Peter Xu <peterx@redhat.com> Signed-off-by: Juan Quintela <quintela@redhat.com> Message-ID: <20231004220240.167175-7-peterx@redhat.com>
2023-10-11qemufile: Always return a verbose errorPeter Xu
There're a lot of cases where we only have an errno set in last_error but without a detailed error description. When this happens, try to generate an error contains the errno as a descriptive error. This will be helpful in cases where one relies on the Error*. E.g., migration state only caches Error* in MigrationState.error. With this, we'll display correct error messages in e.g. query-migrate when the error was only set by qemu_file_set_error(). Reviewed-by: Fabiano Rosas <farosas@suse.de> Reviewed-by: Juan Quintela <quintela@redhat.com> Signed-off-by: Peter Xu <peterx@redhat.com> Signed-off-by: Juan Quintela <quintela@redhat.com> Message-ID: <20231004220240.167175-6-peterx@redhat.com>
2023-10-11migration: Introduce migrate_has_error()Peter Xu
Introduce a helper to detect whether MigrationState.error is set for whatever reason. This is preparation work for any thread (e.g. source return path thread) to setup errors in an unified way to MigrationState, rather than relying on its own way to set errors (mark_source_rp_bad()). Reviewed-by: Fabiano Rosas <farosas@suse.de> Reviewed-by: Juan Quintela <quintela@redhat.com> Signed-off-by: Peter Xu <peterx@redhat.com> Signed-off-by: Juan Quintela <quintela@redhat.com> Message-ID: <20231004220240.167175-3-peterx@redhat.com>
2023-10-11migration: Display error in query-migrate irrelevant of statusPeter Xu
Display it as long as being set, irrelevant of FAILED status. E.g., it may also be applicable to PAUSED stage of postcopy, to provide hint on what has gone wrong. The error_mutex seems to be overlooked when referencing the error, add it to be very safe. This will change QAPI behavior by showing up error message outside !FAILED status, but it's intended and doesn't expect to break anyone. Bugzilla: https://bugzilla.redhat.com/show_bug.cgi?id=2018404 Reviewed-by: Fabiano Rosas <farosas@suse.de> Reviewed-by: Juan Quintela <quintela@redhat.com> Signed-off-by: Peter Xu <peterx@redhat.com> Signed-off-by: Juan Quintela <quintela@redhat.com> Message-ID: <20231004220240.167175-2-peterx@redhat.com>
2023-10-11migration/rdma: Replace flawed device detail dump by tracingMarkus Armbruster
qemu_rdma_dump_id() dumps RDMA device details to stdout. rdma_start_outgoing_migration() calls it via qemu_rdma_source_init() and qemu_rdma_resolve_host() to show source device details. rdma_start_incoming_migration() arranges its call via rdma_accept_incoming_migration() and qemu_rdma_accept() to show destination device details. Two issues: 1. rdma_start_outgoing_migration() can run in HMP context. The information should arguably go the monitor, not stdout. 2. ibv_query_port() failure is reported as error. Its callers remain unaware of this failure (qemu_rdma_dump_id() can't fail), so reporting this to the user as an error is problematic. Fixable, but the device detail dump is noise, except when troubleshooting. Tracing is a better fit. Similar function qemu_rdma_dump_id() was converted to tracing in commit 733252deb8b (Tracify migration/rdma.c). Convert qemu_rdma_dump_id(), too. While there, touch up qemu_rdma_dump_gid()'s outdated comment. Signed-off-by: Markus Armbruster <armbru@redhat.com> Reviewed-by: Fabiano Rosas <farosas@suse.de> Reviewed-by: Juan Quintela <quintela@redhat.com> Signed-off-by: Juan Quintela <quintela@redhat.com> Message-ID: <20230928132019.2544702-54-armbru@redhat.com>
2023-10-11migration/rdma: Use error_report() & friends instead of stderrMarkus Armbruster
error_report() obeys -msg, reports the current error location if any, and reports to the current monitor if any. Reporting to stderr directly with fprintf() or perror() is wrong, because it loses all this. Fix the offenders. Bonus: resolves a FIXME about problematic use of errno. Signed-off-by: Markus Armbruster <armbru@redhat.com> Reviewed-by: Juan Quintela <quintela@redhat.com> Signed-off-by: Juan Quintela <quintela@redhat.com> Message-ID: <20230928132019.2544702-53-armbru@redhat.com>
2023-10-11migration/rdma: Downgrade qemu_rdma_cleanup() errors to warningsMarkus Armbruster
Functions that use an Error **errp parameter to return errors should not also report them to the user, because reporting is the caller's job. When the caller does, the error is reported twice. When it doesn't (because it recovered from the error), there is no error to report, i.e. the report is bogus. qemu_rdma_source_init(), qemu_rdma_connect(), rdma_start_incoming_migration(), and rdma_start_outgoing_migration() violate this principle: they call error_report() via qemu_rdma_cleanup(). Moreover, qemu_rdma_cleanup() can't fail. It is called on error paths, and QIOChannel close and finalization. Are the conditions it reports really errors? I doubt it. Downgrade qemu_rdma_cleanup()'s errors to warnings. Signed-off-by: Markus Armbruster <armbru@redhat.com> Reviewed-by: Fabiano Rosas <farosas@suse.de> Reviewed-by: Juan Quintela <quintela@redhat.com> Signed-off-by: Juan Quintela <quintela@redhat.com> Message-ID: <20230928132019.2544702-52-armbru@redhat.com>
2023-10-11migration/rdma: Silence qemu_rdma_register_and_get_keys()Markus Armbruster
Functions that use an Error **errp parameter to return errors should not also report them to the user, because reporting is the caller's job. When the caller does, the error is reported twice. When it doesn't (because it recovered from the error), there is no error to report, i.e. the report is bogus. qemu_rdma_write_one() violates this principle: it reports errors to stderr via qemu_rdma_register_and_get_keys(). I elected not to investigate how callers handle the error, i.e. precise impact is not known. Clean this up: silence qemu_rdma_register_and_get_keys(). I believe the caller's error reports suffice. If they don't, we need to convert to Error instead. Signed-off-by: Markus Armbruster <armbru@redhat.com> Reviewed-by: Li Zhijian <lizhijian@fujitsu.com> Reviewed-by: Juan Quintela <quintela@redhat.com> Signed-off-by: Juan Quintela <quintela@redhat.com> Message-ID: <20230928132019.2544702-51-armbru@redhat.com>
2023-10-11migration/rdma: Silence qemu_rdma_block_for_wrid()Markus Armbruster
Functions that use an Error **errp parameter to return errors should not also report them to the user, because reporting is the caller's job. When the caller does, the error is reported twice. When it doesn't (because it recovered from the error), there is no error to report, i.e. the report is bogus. qemu_rdma_post_send_control(), qemu_rdma_exchange_get_response(), and qemu_rdma_write_one() violate this principle: they call error_report(), fprintf(stderr, ...), and perror() via qemu_rdma_block_for_wrid(), qemu_rdma_poll(), and qemu_rdma_wait_comp_channel(). I elected not to investigate how callers handle the error, i.e. precise impact is not known. Clean this up by dropping the error reporting from qemu_rdma_poll(), qemu_rdma_wait_comp_channel(), and qemu_rdma_block_for_wrid(). I believe the callers' error reports suffice. If they don't, we need to convert to Error instead. Bonus: resolves a FIXME about problematic use of errno. Signed-off-by: Markus Armbruster <armbru@redhat.com> Reviewed-by: Li Zhijian <lizhijian@fujitsu.com> Reviewed-by: Juan Quintela <quintela@redhat.com> Signed-off-by: Juan Quintela <quintela@redhat.com> Message-ID: <20230928132019.2544702-50-armbru@redhat.com>
2023-10-11migration/rdma: Don't report received completion events as errorMarkus Armbruster
When qemu_rdma_wait_comp_channel() receives an event from the completion channel, it reports an error "receive cm event while wait comp channel,cm event is T", where T is the numeric event type. However, the function fails only when T is a disconnect or device removal. Events other than these two are not actually an error, and reporting them as an error is wrong. If we need to report them to the user, we should use something else, and what to use depends on why we need to report them to the user. For now, report this error only when the function actually fails. Signed-off-by: Markus Armbruster <armbru@redhat.com> Reviewed-by: Li Zhijian <lizhijian@fujitsu.com> Reviewed-by: Juan Quintela <quintela@redhat.com> Signed-off-by: Juan Quintela <quintela@redhat.com> Message-ID: <20230928132019.2544702-49-armbru@redhat.com>
2023-10-11migration/rdma: Silence qemu_rdma_reg_control()Markus Armbruster
Functions that use an Error **errp parameter to return errors should not also report them to the user, because reporting is the caller's job. When the caller does, the error is reported twice. When it doesn't (because it recovered from the error), there is no error to report, i.e. the report is bogus. qemu_rdma_source_init() and qemu_rdma_accept() violate this principle: they call error_report() via qemu_rdma_reg_control(). I elected not to investigate how callers handle the error, i.e. precise impact is not known. Clean this up by dropping the error reporting from qemu_rdma_reg_control(). I believe the callers' error reports suffice. If they don't, we need to convert to Error instead. Signed-off-by: Markus Armbruster <armbru@redhat.com> Reviewed-by: Li Zhijian <lizhijian@fujitsu.com> Reviewed-by: Juan Quintela <quintela@redhat.com> Signed-off-by: Juan Quintela <quintela@redhat.com> Message-ID: <20230928132019.2544702-48-armbru@redhat.com>
2023-10-11migration/rdma: Silence qemu_rdma_connect()Markus Armbruster
Functions that use an Error **errp parameter to return errors should not also report them to the user, because reporting is the caller's job. When the caller does, the error is reported twice. When it doesn't (because it recovered from the error), there is no error to report, i.e. the report is bogus. qemu_rdma_connect() violates this principle: it calls error_report() and perror(). I elected not to investigate how callers handle the error, i.e. precise impact is not known. Clean this up: replace perror() by changing error_setg() to error_setg_errno(), and drop error_report(). I believe the callers' error reports suffice then. If they don't, we need to convert to Error instead. Bonus: resolves a FIXME about problematic use of errno. Signed-off-by: Markus Armbruster <armbru@redhat.com> Reviewed-by: Li Zhijian <lizhijian@fujitsu.com> Reviewed-by: Juan Quintela <quintela@redhat.com> Signed-off-by: Juan Quintela <quintela@redhat.com> Message-ID: <20230928132019.2544702-47-armbru@redhat.com>
2023-10-11migration/rdma: Silence qemu_rdma_resolve_host()Markus Armbruster
Functions that use an Error **errp parameter to return errors should not also report them to the user, because reporting is the caller's job. When the caller does, the error is reported twice. When it doesn't (because it recovered from the error), there is no error to report, i.e. the report is bogus. qemu_rdma_resolve_host() violates this principle: it calls error_report(). Clean this up: drop error_report(). Signed-off-by: Markus Armbruster <armbru@redhat.com> Reviewed-by: Li Zhijian <lizhijian@fujitsu.com> Reviewed-by: Juan Quintela <quintela@redhat.com> Signed-off-by: Juan Quintela <quintela@redhat.com> Message-ID: <20230928132019.2544702-46-armbru@redhat.com>
2023-10-11migration/rdma: Convert qemu_rdma_alloc_pd_cq() to ErrorMarkus Armbruster
Functions that use an Error **errp parameter to return errors should not also report them to the user, because reporting is the caller's job. When the caller does, the error is reported twice. When it doesn't (because it recovered from the error), there is no error to report, i.e. the report is bogus. qemu_rdma_source_init() violates this principle: it calls error_report() via qemu_rdma_alloc_pd_cq(). I elected not to investigate how callers handle the error, i.e. precise impact is not known. Clean this up by converting qemu_rdma_alloc_pd_cq() to Error. The conversion loses a piece of advice on one of two failure paths: Your mlock() limits may be too low. Please check $ ulimit -a # and search for 'ulimit -l' in the output Not worth retaining. Signed-off-by: Markus Armbruster <armbru@redhat.com> Reviewed-by: Li Zhijian <lizhijian@fujitsu.com> Reviewed-by: Juan Quintela <quintela@redhat.com> Signed-off-by: Juan Quintela <quintela@redhat.com> Message-ID: <20230928132019.2544702-45-armbru@redhat.com>
2023-10-11migration/rdma: Convert qemu_rdma_post_recv_control() to ErrorMarkus Armbruster
Just for symmetry with qemu_rdma_post_send_control(). Error messages lose detail I consider of no use to users. Signed-off-by: Markus Armbruster <armbru@redhat.com> Reviewed-by: Li Zhijian <lizhijian@fujitsu.com> Reviewed-by: Juan Quintela <quintela@redhat.com> Signed-off-by: Juan Quintela <quintela@redhat.com> Message-ID: <20230928132019.2544702-44-armbru@redhat.com>
2023-10-11migration/rdma: Convert qemu_rdma_post_send_control() to ErrorMarkus Armbruster
Functions that use an Error **errp parameter to return errors should not also report them to the user, because reporting is the caller's job. When the caller does, the error is reported twice. When it doesn't (because it recovered from the error), there is no error to report, i.e. the report is bogus. qemu_rdma_exchange_send() violates this principle: it calls error_report() via qemu_rdma_post_send_control(). I elected not to investigate how callers handle the error, i.e. precise impact is not known. Clean this up by converting qemu_rdma_post_send_control() to Error. Signed-off-by: Markus Armbruster <armbru@redhat.com> Reviewed-by: Li Zhijian <lizhijian@fujitsu.com> Reviewed-by: Juan Quintela <quintela@redhat.com> Signed-off-by: Juan Quintela <quintela@redhat.com> Message-ID: <20230928132019.2544702-43-armbru@redhat.com>
2023-10-11migration/rdma: Convert qemu_rdma_write() to ErrorMarkus Armbruster
Just for consistency with qemu_rdma_write_one() and qemu_rdma_write_flush(), and for slightly simpler code. Signed-off-by: Markus Armbruster <armbru@redhat.com> Reviewed-by: Li Zhijian <lizhijian@fujitsu.com> Reviewed-by: Juan Quintela <quintela@redhat.com> Signed-off-by: Juan Quintela <quintela@redhat.com> Message-ID: <20230928132019.2544702-42-armbru@redhat.com>
2023-10-11migration/rdma: Convert qemu_rdma_write_one() to ErrorMarkus Armbruster
Functions that use an Error **errp parameter to return errors should not also report them to the user, because reporting is the caller's job. When the caller does, the error is reported twice. When it doesn't (because it recovered from the error), there is no error to report, i.e. the report is bogus. qemu_rdma_write_flush() violates this principle: it calls error_report() via qemu_rdma_write_one(). I elected not to investigate how callers handle the error, i.e. precise impact is not known. Clean this up by converting qemu_rdma_write_one() to Error. Bonus: resolves a FIXME about problematic use of errno. Signed-off-by: Markus Armbruster <armbru@redhat.com> Reviewed-by: Li Zhijian <lizhijian@fujitsu.com> Reviewed-by: Juan Quintela <quintela@redhat.com> Signed-off-by: Juan Quintela <quintela@redhat.com> Message-ID: <20230928132019.2544702-41-armbru@redhat.com>
2023-10-11migration/rdma: Convert qemu_rdma_write_flush() to ErrorMarkus Armbruster
Functions that use an Error **errp parameter to return errors should not also report them to the user, because reporting is the caller's job. When the caller does, the error is reported twice. When it doesn't (because it recovered from the error), there is no error to report, i.e. the report is bogus. qio_channel_rdma_writev() violates this principle: it calls error_report() via qemu_rdma_write_flush(). I elected not to investigate how callers handle the error, i.e. precise impact is not known. Clean this up by converting qemu_rdma_write_flush() to Error. Necessitates setting an error when qemu_rdma_write_one() failed. Since this error will go away later in this series, simply use "FIXME temporary error message" there. Signed-off-by: Markus Armbruster <armbru@redhat.com> Reviewed-by: Li Zhijian <lizhijian@fujitsu.com> Reviewed-by: Juan Quintela <quintela@redhat.com> Signed-off-by: Juan Quintela <quintela@redhat.com> Message-ID: <20230928132019.2544702-40-armbru@redhat.com>
2023-10-11migration/rdma: Convert qemu_rdma_reg_whole_ram_blocks() to ErrorMarkus Armbruster
Functions that use an Error **errp parameter to return errors should not also report them to the user, because reporting is the caller's job. When the caller does, the error is reported twice. When it doesn't (because it recovered from the error), there is no error to report, i.e. the report is bogus. qemu_rdma_exchange_send() violates this principle: it calls error_report() via callback qemu_rdma_reg_whole_ram_blocks(). I elected not to investigate how callers handle the error, i.e. precise impact is not known. Clean this up by converting the callback to Error. Signed-off-by: Markus Armbruster <armbru@redhat.com> Reviewed-by: Li Zhijian <lizhijian@fujitsu.com> Reviewed-by: Juan Quintela <quintela@redhat.com> Signed-off-by: Juan Quintela <quintela@redhat.com> Message-ID: <20230928132019.2544702-39-armbru@redhat.com>
2023-10-11migration/rdma: Convert qemu_rdma_exchange_get_response() to ErrorMarkus Armbruster
Functions that use an Error **errp parameter to return errors should not also report them to the user, because reporting is the caller's job. When the caller does, the error is reported twice. When it doesn't (because it recovered from the error), there is no error to report, i.e. the report is bogus. qemu_rdma_exchange_send() and qemu_rdma_exchange_recv() violate this principle: they call error_report() via qemu_rdma_exchange_get_response(). I elected not to investigate how callers handle the error, i.e. precise impact is not known. Clean this up by converting qemu_rdma_exchange_get_response() to Error. Signed-off-by: Markus Armbruster <armbru@redhat.com> Reviewed-by: Li Zhijian <lizhijian@fujitsu.com> Reviewed-by: Juan Quintela <quintela@redhat.com> Signed-off-by: Juan Quintela <quintela@redhat.com> Message-ID: <20230928132019.2544702-38-armbru@redhat.com>
2023-10-11migration/rdma: Convert qemu_rdma_exchange_send() to ErrorMarkus Armbruster
Functions that use an Error **errp parameter to return errors should not also report them to the user, because reporting is the caller's job. When the caller does, the error is reported twice. When it doesn't (because it recovered from the error), there is no error to report, i.e. the report is bogus. qio_channel_rdma_writev() violates this principle: it calls error_report() via qemu_rdma_exchange_send(). I elected not to investigate how callers handle the error, i.e. precise impact is not known. Clean this up by converting qemu_rdma_exchange_send() to Error. Necessitates setting an error when qemu_rdma_post_recv_control(), callback(), or qemu_rdma_exchange_get_response() failed. Since these errors will go away later in this series, simply use "FIXME temporary error message" there. Signed-off-by: Markus Armbruster <armbru@redhat.com> Reviewed-by: Li Zhijian <lizhijian@fujitsu.com> Reviewed-by: Juan Quintela <quintela@redhat.com> Signed-off-by: Juan Quintela <quintela@redhat.com> Message-ID: <20230928132019.2544702-37-armbru@redhat.com>
2023-10-11migration/rdma: Convert qemu_rdma_exchange_recv() to ErrorMarkus Armbruster
Functions that use an Error **errp parameter to return errors should not also report them to the user, because reporting is the caller's job. When the caller does, the error is reported twice. When it doesn't (because it recovered from the error), there is no error to report, i.e. the report is bogus. qio_channel_rdma_readv() violates this principle: it calls error_report() via qemu_rdma_exchange_recv(). I elected not to investigate how callers handle the error, i.e. precise impact is not known. Clean this up by converting qemu_rdma_exchange_recv() to Error. Necessitates setting an error when qemu_rdma_exchange_get_response() failed. Since this error will go away later in this series, simply use "FIXME temporary error message" there. Signed-off-by: Markus Armbruster <armbru@redhat.com> Reviewed-by: Li Zhijian <lizhijian@fujitsu.com> Reviewed-by: Juan Quintela <quintela@redhat.com> Signed-off-by: Juan Quintela <quintela@redhat.com> Message-ID: <20230928132019.2544702-36-armbru@redhat.com>
2023-10-11migration/rdma: Drop "@errp is clear" guards around error_setg()Markus Armbruster
These guards are all redundant now. Signed-off-by: Markus Armbruster <armbru@redhat.com> Reviewed-by: Li Zhijian <lizhijian@fujitsu.com> Reviewed-by: Juan Quintela <quintela@redhat.com> Signed-off-by: Juan Quintela <quintela@redhat.com> Message-ID: <20230928132019.2544702-35-armbru@redhat.com>
2023-10-11migration/rdma: Fix error handling around rdma_getaddrinfo()Markus Armbruster
qemu_rdma_resolve_host() and qemu_rdma_dest_init() iterate over addresses to find one that works, holding onto the first Error from qemu_rdma_broken_ipv6_kernel() for use when no address works. Issues: 1. If @errp was &error_abort or &error_fatal, we'd terminate instead of trying the next address. Can't actually happen, since no caller passes these arguments. 2. When @errp is a pointer to a variable containing NULL, and qemu_rdma_broken_ipv6_kernel() fails, the variable no longer contains NULL. Subsequent iterations pass it again, violating Error usage rules. Dangerous, as setting an error would then trip error_setv()'s assertion. Works only because qemu_rdma_broken_ipv6_kernel() and the code following the loops carefully avoids setting a second error. 3. If qemu_rdma_broken_ipv6_kernel() fails, and then a later iteration finds a working address, @errp still holds the first error from qemu_rdma_broken_ipv6_kernel(). If we then run into another error, we report the qemu_rdma_broken_ipv6_kernel() failure instead. 4. If we don't run into another error, we leak the Error object. Use a local error variable, and propagate to @errp. This fixes 3. and also cleans up 1 and partly 2. Free this error when we have a working address. This fixes 4. Pass the local error variable to qemu_rdma_broken_ipv6_kernel() only until it fails. Pass null on any later iterations. This cleans up the remainder of 2. Signed-off-by: Markus Armbruster <armbru@redhat.com> Reviewed-by: Li Zhijian <lizhijian@fujitsu.com> Reviewed-by: Juan Quintela <quintela@redhat.com> Signed-off-by: Juan Quintela <quintela@redhat.com> Message-ID: <20230928132019.2544702-34-armbru@redhat.com>
2023-10-11migration/rdma: Retire macro ERROR()Markus Armbruster
ERROR() has become "error_setg() unless an error has been set already". Hiding the conditional in the macro is in the way of further work. Replace the macro uses by their expansion, and delete the macro. Signed-off-by: Markus Armbruster <armbru@redhat.com> Reviewed-by: Li Zhijian <lizhijian@fujitsu.com> Reviewed-by: Juan Quintela <quintela@redhat.com> Signed-off-by: Juan Quintela <quintela@redhat.com> Message-ID: <20230928132019.2544702-33-armbru@redhat.com>
2023-10-11migration/rdma: Delete inappropriate error_report() in macro ERROR()Markus Armbruster
Functions that use an Error **errp parameter to return errors should not also report them to the user, because reporting is the caller's job. When the caller does, the error is reported twice. When it doesn't (because it recovered from the error), there is no error to report, i.e. the report is bogus. Macro ERROR() violates this principle. Delete the error_report() there. Signed-off-by: Markus Armbruster <armbru@redhat.com> Reviewed-by: Li Zhijian <lizhijian@fujitsu.com> Tested-by: Li Zhijian <lizhijian@fujitsu.com> Reviewed-by: Juan Quintela <quintela@redhat.com> Signed-off-by: Juan Quintela <quintela@redhat.com> Message-ID: <20230928132019.2544702-32-armbru@redhat.com>
2023-10-11migration/rdma: Plug a memory leak and improve a messageMarkus Armbruster
When migration capability @rdma-pin-all is true, but the server cannot honor it, qemu_rdma_connect() calls macro ERROR(), then returns success. ERROR() sets an error. Since qemu_rdma_connect() returns success, its caller rdma_start_outgoing_migration() duly assumes @errp is still clear. The Error object leaks. ERROR() additionally reports the situation to the user as an error: RDMA ERROR: Server cannot support pinning all memory. Will register memory dynamically. Is this an error or not? It actually isn't; we disable @rdma-pin-all and carry on. "Correcting" the user's configuration decisions that way feels problematic, but that's a topic for another day. Replace ERROR() by warn_report(). This plugs the memory leak, and emits a clearer message to the user. Signed-off-by: Markus Armbruster <armbru@redhat.com> Reviewed-by: Li Zhijian <lizhijian@fujitsu.com> Reviewed-by: Juan Quintela <quintela@redhat.com> Signed-off-by: Juan Quintela <quintela@redhat.com> Message-ID: <20230928132019.2544702-31-armbru@redhat.com>
2023-10-11migration/rdma: Check negative error values the same way everywhereMarkus Armbruster
When a function returns 0 on success, negative value on error, checking for non-zero suffices, but checking for negative is clearer. So do that. Signed-off-by: Markus Armbruster <armbru@redhat.com> Reviewed-by: Fabiano Rosas <farosas@suse.de> Reviewed-by: Juan Quintela <quintela@redhat.com> Signed-off-by: Juan Quintela <quintela@redhat.com> Message-ID: <20230928132019.2544702-30-armbru@redhat.com>
2023-10-11migration/rdma: Drop superfluous assignments to @retMarkus Armbruster
Signed-off-by: Markus Armbruster <armbru@redhat.com> Reviewed-by: Li Zhijian <lizhijian@fujitsu.com> Reviewed-by: Juan Quintela <quintela@redhat.com> Signed-off-by: Juan Quintela <quintela@redhat.com> Message-ID: <20230928132019.2544702-29-armbru@redhat.com>
2023-10-11migration/rdma: Replace int error_state by bool erroredMarkus Armbruster
All we do with the value of RDMAContext member @error_state is test whether it's zero. Change to bool and rename to @errored. Signed-off-by: Markus Armbruster <armbru@redhat.com> Reviewed-by: Li Zhijian <lizhijian@fujitsu.com> Reviewed-by: Juan Quintela <quintela@redhat.com> Signed-off-by: Juan Quintela <quintela@redhat.com> Message-ID: <20230928132019.2544702-28-armbru@redhat.com>
2023-10-11migration/rdma: Dumb down remaining int error values to -1Markus Armbruster
This is just to make the error value more obvious. Callers don't mind. Signed-off-by: Markus Armbruster <armbru@redhat.com> Reviewed-by: Li Zhijian <lizhijian@fujitsu.com> Reviewed-by: Juan Quintela <quintela@redhat.com> Signed-off-by: Juan Quintela <quintela@redhat.com> Message-ID: <20230928132019.2544702-27-armbru@redhat.com>
2023-10-11migration/rdma: Return -1 instead of negative errno codeMarkus Armbruster
Several functions return negative errno codes on failure. Callers check for specific codes exactly never. For some of the functions, callers couldn't check even if they wanted to, because the functions also return negative values that aren't errno codes, leaving readers confused on what the function actually returns. Clean up and simplify: return -1 instead of negative errno code. Signed-off-by: Markus Armbruster <armbru@redhat.com> Reviewed-by: Li Zhijian <lizhijian@fujitsu.com> Reviewed-by: Juan Quintela <quintela@redhat.com> Signed-off-by: Juan Quintela <quintela@redhat.com> Message-ID: <20230928132019.2544702-26-armbru@redhat.com>
2023-10-11migration/rdma: Fix rdma_getaddrinfo() error checkingMarkus Armbruster
rdma_getaddrinfo() returns 0 on success. On error, it returns one of the EAI_ error codes like getaddrinfo() does, or -1 with errno set. This is broken by design: POSIX implicitly specifies the EAI_ error codes to be non-zero, no more. They could clash with -1. Nothing we can do about this design flaw. Both callers of rdma_getaddrinfo() only recognize negative values as error. Works only because systems elect to make the EAI_ error codes negative. Best not to rely on that: change the callers to treat any non-zero value as failure. Also change them to return -1 instead of the value received from getaddrinfo() on failure, to avoid positive error values. Signed-off-by: Markus Armbruster <armbru@redhat.com> Reviewed-by: Li Zhijian <lizhijian@fujitsu.com> Reviewed-by: Juan Quintela <quintela@redhat.com> Signed-off-by: Juan Quintela <quintela@redhat.com> Message-ID: <20230928132019.2544702-25-armbru@redhat.com>
2023-10-11migration/rdma: Fix QEMUFileHooks method return valuesMarkus Armbruster
The QEMUFileHooks methods don't come with a written contract. Digging through the code calling them, we find: * save_page(): Negative values RAM_SAVE_CONTROL_DELAYED and RAM_SAVE_CONTROL_NOT_SUPP are special. Any other negative value is an unspecified error. qemu_rdma_save_page() returns -EIO or rdma->error_state on error. I believe the latter is always negative. Nothing stops either of them to clash with the special values, though. Feels unlikely, but fix it anyway to return only the special values and -1. * before_ram_iterate(), after_ram_iterate(): Negative value means error. qemu_rdma_registration_start() and qemu_rdma_registration_stop() comply as far as I can tell. Make them comply *obviously*, by returning -1 on error. * hook_ram_load: Negative value means error. rdma_load_hook() already returns -1 on error. Leave it alone. Signed-off-by: Markus Armbruster <armbru@redhat.com> Reviewed-by: Li Zhijian <lizhijian@fujitsu.com> Reviewed-by: Juan Quintela <quintela@redhat.com> Signed-off-by: Juan Quintela <quintela@redhat.com> Message-ID: <20230928132019.2544702-24-armbru@redhat.com>
2023-10-11migration/rdma: Drop dead qemu_rdma_data_init() code for !@host_portMarkus Armbruster
qemu_rdma_data_init() neglects to set an Error when it fails because @host_port is null. Fortunately, no caller passes null, so this is merely a latent bug. Drop the flawed code handling null argument. Signed-off-by: Markus Armbruster <armbru@redhat.com> Reviewed-by: Fabiano Rosas <farosas@suse.de> Reviewed-by: Juan Quintela <quintela@redhat.com> Signed-off-by: Juan Quintela <quintela@redhat.com> Message-ID: <20230928132019.2544702-23-armbru@redhat.com>
2023-10-11migration/rdma: Fix qemu_get_cm_event_timeout() to always set errorMarkus Armbruster
qemu_get_cm_event_timeout() neglects to set an error when it fails because rdma_get_cm_event() fails. Harmless, as its caller qemu_rdma_connect() substitutes a generic error then. Fix it anyway. qemu_rdma_connect() also sets the generic error when its own call of rdma_get_cm_event() fails. Make the error handling more obvious: set a specific error right after rdma_get_cm_event() fails. Delete the generic error. Signed-off-by: Markus Armbruster <armbru@redhat.com> Reviewed-by: Peter Xu <peterx@redhat.com> Reviewed-by: Li Zhijian <lizhijian@fujitsu.com> Reviewed-by: Juan Quintela <quintela@redhat.com> Signed-off-by: Juan Quintela <quintela@redhat.com> Message-ID: <20230928132019.2544702-22-armbru@redhat.com>
2023-10-11migration/rdma: Fix qemu_rdma_broken_ipv6_kernel() to set errorMarkus Armbruster
qemu_rdma_resolve_host() and qemu_rdma_dest_init() try addresses until they find on that works. If none works, they return the first Error set by qemu_rdma_broken_ipv6_kernel(), or else return a generic one. qemu_rdma_broken_ipv6_kernel() neglects to set an Error when ibv_open_device() fails. If a later address fails differently, we use that Error instead, or else the generic one. Harmless enough, but needs fixing all the same. Signed-off-by: Markus Armbruster <armbru@redhat.com> Reviewed-by: Fabiano Rosas <farosas@suse.de> Reviewed-by: Li Zhijian <lizhijian@fujitsu.com> Reviewed-by: Juan Quintela <quintela@redhat.com> Signed-off-by: Juan Quintela <quintela@redhat.com> Message-ID: <20230928132019.2544702-21-armbru@redhat.com>
2023-10-11migration/rdma: Replace dangerous macro CHECK_ERROR_STATE()Markus Armbruster
Hiding return statements in macros is a bad idea. Use a function instead, and open code the return part. Signed-off-by: Markus Armbruster <armbru@redhat.com> Reviewed-by: Fabiano Rosas <farosas@suse.de> Reviewed-by: Li Zhijian <lizhijian@fujitsu.com> Reviewed-by: Juan Quintela <quintela@redhat.com> Signed-off-by: Juan Quintela <quintela@redhat.com> Message-ID: <20230928132019.2544702-20-armbru@redhat.com>