Re: [PATCH 1/2] migration: Prioritize RDMA in ram_save_target_page()

[Zhijian Li (Fujitsu)]

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;
>   }