On 19/02/2025 21:23, Peter Xu wrote:
>> 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?
> Yes it looks almost good. What I meant is (please see below):
>
>> 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()) {
> Here instead of bypassing postcopy, we should fail the migrate cmd early if
> postcopy ever enabled:
>
> diff --git a/migration/migration.c b/migration/migration.c
> index 862f469ea7..3a82e71437 100644
> --- a/migration/migration.c
> +++ b/migration/migration.c
> @@ -257,6 +257,12 @@
> migration_channels_and_transport_compatible(MigrationAddress *addr,
> return false;
> }
>
> + if (addr->transport == MIGRATION_ADDRESS_TYPE_FILE &&
> + migrate_postcopy_ram()) {
I think there is a typo
s/MIGRATION_ADDRESS_TYPE_FILE/MIGRATION_ADDRESS_TYPE_RDMA
> + error_setg(errp, "RDMA migration doesn't support postcopy");
IIUC, your change means RDMA + postcopy is no longer supported. I didn't
realize this before.
Additionally, we might consider eliminating all remaining
`migration_in_postcopy()` conditions in the current `rdma.c` file.
Thanks
Zhijian
> + return false;
> + }
> +
> return true;
> }
