Re: [PATCH v2 15/16] migration/rdma: Simplify the function that saves a page

[Juan Quintela]

Leonardo Brás <leobras@redhat.com> wrote:
> On Mon, 2023-05-15 at 21:57 +0200, Juan Quintela wrote:
>> When we sent a page through QEMUFile hooks (RDMA) there are three
>> posiblities:
>> - We are not using RDMA. return RAM_SAVE_CONTROL_DELAYED and
>>   control_save_page() returns false to let anything else to proceed.
>> - There is one error but we are using RDMA.  Then we return a negative
>>   value, control_save_page() needs to return true.
>> - Everything goes well and RDMA start the sent of the page
>>   asynchronously.  It returns RAM_SAVE_CONTROL_DELAYED and we need to
>>   return 1 for ram_save_page_legacy.
>> 
>> Clear?
>> 
>> I know, I know, the interfaz is as bad as it gets.  I think that now
>> it is a bit clearer, but this needs to be done some other way.
>
> interface?

Yeap.  I used the Spanish spelling, that, you know, in English is wrong O:-)

Thanks.
>> diff --git a/migration/rdma.c b/migration/rdma.c
>> index 416dec00a2..12d3c23fdc 100644
>> --- a/migration/rdma.c
>> +++ b/migration/rdma.c
>> @@ -3239,13 +3239,12 @@ qio_channel_rdma_shutdown(QIOChannel *ioc,
>>   *
>>   *    @size : Number of bytes to transfer
>>   *
>> - *    @bytes_sent : User-specificed pointer to indicate how many bytes were
>> + *    @pages_sent : User-specificed pointer to indicate how many pages were
>>   *                  sent. Usually, this will not be more than a few bytes of
>>   *                  the protocol because most transfers are sent 
>> asynchronously.
>>   */
>
> There is new doc to pages_sent but the parameter is not added to the signature
> bellow. Am I missing something?

Good catch.

I redid this patch several times.  And it appears that I forgot some leftovers.

>
>> -static size_t qemu_rdma_save_page(QEMUFile *f,
>> -                                  ram_addr_t block_offset, ram_addr_t 
>> offset,
>> -                                  size_t size, uint64_t *bytes_sent)
>> +static int qemu_rdma_save_page(QEMUFile *f, ram_addr_t block_offset,
>> +                               ram_addr_t offset, size_t size)
>>  {
>>      QIOChannelRDMA *rioc = QIO_CHANNEL_RDMA(qemu_file_get_ioc(f));
>>      RDMAContext *rdma;
>> @@ -3277,18 +3276,6 @@ static size_t qemu_rdma_save_page(QEMUFile *f,
>>          goto err;
>>      }
>>  
>> -    /*
>> -     * We always return 1 bytes because the RDMA
>> -     * protocol is completely asynchronous. We do not yet know
>> -     * whether an  identified chunk is zero or not because we're
>> -     * waiting for other pages to potentially be merged with
>> -     * the current chunk. So, we have to call qemu_update_position()
>> -     * later on when the actual write occurs.
>> -     */
>> -    if (bytes_sent) {
>> -        *bytes_sent = 1;
>> -    }
>> -
>>      /*
>>       * Drain the Completion Queue if possible, but do not block,
>>       * just poll.
>
> Oh, so this one complements 13/16. 
> Since it doesn't do imaginary transfers anymore, there is no need to use
> bytes_sent pointer to keep track of them anymore.
>
> Other than the pages_sent above that I couldn't understand:
> Reviewed-by: Leonardo Bras <leobras@redhat.com>

Dropping that bit.

Thanks.