On 19/02/2025 06:03, Peter Xu wrote:
> On Tue, Feb 18, 2025 at 05:30:40PM -0300, Fabiano Rosas wrote:
>> Li Zhijian via <qemu-devel@nongnu.org> writes:
>>
>>> Address an error in RDMA-based migration by ensuring RDMA is prioritized
>>> when saving pages in `ram_save_target_page()`.
>>>
>>> Previously, the RDMA protocol's page-saving step was placed after other
>>> protocols due to a refactoring in commit bc38dc2f5f3. This led to migration
>>> failures characterized by unknown control messages and state loading errors
>>> destination:
>>> (qemu) qemu-system-x86_64: Unknown control message QEMU FILE
>>> qemu-system-x86_64: error while loading state section id 1(ram)
>>> qemu-system-x86_64: load of migration failed: Operation not permitted
>>> source:
>>> (qemu) qemu-system-x86_64: RDMA is in an error state waiting migration to
>>> abort!
>>> qemu-system-x86_64: failed to save SaveStateEntry with id(name): 1(ram): -1
>>> qemu-system-x86_64: rdma migration: recv polling control error!
>>> qemu-system-x86_64: warning: Early error. Sending error.
>>> qemu-system-x86_64: warning: rdma migration: send polling control error
>>>
>>> RDMA migration implemented its own protocol/method to send pages to
>>> destination side, hand over to RDMA first to prevent pages being saved by
>>> other protocol.
>>>
>>> Fixes: bc38dc2f5f3 ("migration: refactor ram_save_target_page functions")
>>> Signed-off-by: Li Zhijian <lizhij...@fujitsu.com>
>>> ---
>>> migration/ram.c | 9 +++++----
>>> 1 file changed, 5 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/migration/ram.c b/migration/ram.c
>>> index 6f460fd22d2..635a2fe443a 100644
>>> --- a/migration/ram.c
>>> +++ b/migration/ram.c
>>> @@ -1964,6 +1964,11 @@ static int ram_save_target_page(RAMState *rs,
>>> PageSearchStatus *pss)
>>> ram_addr_t offset = ((ram_addr_t)pss->page) << TARGET_PAGE_BITS;
>>> int res;
>>>
>>> + /* Hand over to RDMA first */
>>> + if (control_save_page(pss, offset, &res)) {
>>> + return res;
>>> + }
>>> +
>>
>> Can we hoist that migrate_rdma() from inside the function? Since the
>> other paths already check first before calling their functions.
>
Yeah, it sounds good to me.
> If we're talking about hoist and stuff.. and if we want to go slightly
> further, I wonder if we could also drop RAM_SAVE_CONTROL_NOT_SUPP.
>
> if (!migrate_rdma() || migration_in_postcopy()) {
> return RAM_SAVE_CONTROL_NOT_SUPP;
> }
>
> We should make sure rdma_control_save_page() won't get invoked at all in
> either case above..
> For postcopy, maybe we could fail in the QMP migrate /
> migrate_incoming cmd, at migration_channels_and_transport_compatible()
I tried to kill RAM_SAVE_CONTROL_NOT_SUPP, but It seems it doesn't need to
touch any postcopy logic
"in the QMP migrate / migrate_incoming cmd, at
migration_channels_and_transport_compatible()"
Is there something I might have overlooked?
A whole draft diff would be like below:
It includes 3 parts:
migration/rdma: Remove unnecessary RAM_SAVE_CONTROL_NOT_SUPP check in
rdma_control_save_page()
migration: kill RAM_SAVE_CONTROL_NOT_SUPP
migration: open control_save_page() to ram_save_target_page()
diff --git a/migration/ram.c b/migration/ram.c
index 589b6505eb2..fc6a964fd64 100644
--- a/migration/ram.c
+++ b/migration/ram.c
@@ -1143,32 +1143,6 @@ static int save_zero_page(RAMState *rs, PageSearchStatus
*pss,
return len;
}
-/*
- * @pages: the number of pages written by the control path,
- * < 0 - error
- * > 0 - number of pages written
- *
- * Return true if the pages has been saved, otherwise false is returned.
- */
-static bool control_save_page(PageSearchStatus *pss,
- ram_addr_t offset, int *pages)
-{
- int ret;
-
- ret = rdma_control_save_page(pss->pss_channel, pss->block->offset, offset,
- TARGET_PAGE_SIZE);
- if (ret == RAM_SAVE_CONTROL_NOT_SUPP) {
- return false;
- }
-
- if (ret == RAM_SAVE_CONTROL_DELAYED) {
- *pages = 1;
- return true;
- }
- *pages = ret;
- return true;
-}
-
/*
* directly send the page to the stream
*
@@ -1964,6 +1938,16 @@ static int ram_save_target_page(RAMState *rs,
PageSearchStatus *pss)
ram_addr_t offset = ((ram_addr_t)pss->page) << TARGET_PAGE_BITS;
int res;
+ if (migrate_rdma() && !migration_in_postcopy()) {
+ res = rdma_control_save_page(pss->pss_channel, pss->block->offset,
+ offset, TARGET_PAGE_SIZE);
+
+ if (res == RAM_SAVE_CONTROL_DELAYED) {
+ res = 1;
+ }
+ return res;
+ }
+
if (!migrate_multifd()
|| migrate_zero_page_detection() == ZERO_PAGE_DETECTION_LEGACY) {
if (save_zero_page(rs, pss, offset)) {
@@ -1976,10 +1960,6 @@ static int ram_save_target_page(RAMState *rs,
PageSearchStatus *pss)
return ram_save_multifd_page(block, offset);
}
}
- if (control_save_page(pss, offset, &res)) {
- return res;
- }
-
return ram_save_page(rs, pss);
}
diff --git a/migration/rdma.c b/migration/rdma.c
index 76fb0349238..c6876347e1e 100644
--- a/migration/rdma.c
+++ b/migration/rdma.c
@@ -3284,14 +3284,11 @@ err:
int rdma_control_save_page(QEMUFile *f, ram_addr_t block_offset,
ram_addr_t offset, size_t size)
{
- if (!migrate_rdma() || migration_in_postcopy()) {
- return RAM_SAVE_CONTROL_NOT_SUPP;
- }
+ assert(migrate_rdma());
int ret = qemu_rdma_save_page(f, block_offset, offset, size);
- if (ret != RAM_SAVE_CONTROL_DELAYED &&
- ret != RAM_SAVE_CONTROL_NOT_SUPP) {
+ if (ret != RAM_SAVE_CONTROL_DELAYED) {
if (ret < 0) {
qemu_file_set_error(f, ret);
}
diff --git a/migration/rdma.h b/migration/rdma.h
index f55f28bbed1..bb0296c3726 100644
--- a/migration/rdma.h
+++ b/migration/rdma.h
@@ -33,7 +33,6 @@ void rdma_start_incoming_migration(InetSocketAddress
*host_port, Error **errp);
#define RAM_CONTROL_ROUND 1
#define RAM_CONTROL_FINISH 3
-#define RAM_SAVE_CONTROL_NOT_SUPP -1000
#define RAM_SAVE_CONTROL_DELAYED -2000
#ifdef CONFIG_RDMA
@@ -56,7 +55,9 @@ static inline
int rdma_control_save_page(QEMUFile *f, ram_addr_t block_offset,
ram_addr_t offset, size_t size)
{
- return RAM_SAVE_CONTROL_NOT_SUPP;
+ /* never reach */
+ assert(0);
+ return -1;
}
#endif
#endif
Thanks
Zhijian
>
>>
>>> if (!migrate_multifd()
>>> || migrate_zero_page_detection() == ZERO_PAGE_DETECTION_LEGACY) {
>>> if (save_zero_page(rs, pss, offset)) {
>>> @@ -1976,10 +1981,6 @@ static int ram_save_target_page(RAMState *rs,
>>> PageSearchStatus *pss)
>>> return ram_save_multifd_page(block, offset);
>>> }
>>>
>>> - if (control_save_page(pss, offset, &res)) {
>>> - return res;
>>> - }
>>> -
>>> return ram_save_page(rs, pss);
>>> }
>>
>
