Re: [PATCH v4 19/34] migration/multifd: Allow receiving pages without packets

[Fabiano Rosas <farosas@suse.de>]

Fabiano Rosas <farosas@suse.de> writes:

> Peter Xu <peterx@redhat.com> writes:
>
>> On Tue, Feb 20, 2024 at 07:41:23PM -0300, Fabiano Rosas wrote:
>>> Currently multifd does not need to have knowledge of pages on the
>>> receiving side because all the information needed is within the
>>> packets that come in the stream.
>>> 
>>> We're about to add support to fixed-ram migration, which cannot use
>>> packets because it expects the ramblock section in the migration file
>>> to contain only the guest pages data.
>>> 
>>> Add a data structure to transfer pages between the ram migration code
>>> and the multifd receiving threads.
>>> 
>>> We don't want to reuse MultiFDPages_t for two reasons:
>>> 
>>> a) multifd threads don't really need to know about the data they're
>>>    receiving.
>>> 
>>> b) the receiving side has to be stopped to load the pages, which means
>>>    we can experiment with larger granularities than page size when
>>>    transferring data.
>>> 
>>> Signed-off-by: Fabiano Rosas <farosas@suse.de>
>>> ---
>>> @Peter: a 'quit' flag cannot be used instead of pending_job. The
>>> receiving thread needs know there's no more data coming. If the
>>> migration thread sets a 'quit' flag, the multifd thread would see the
>>> flag right away and exit.
>>
>> Hmm.. isn't this exactly what we want?  I'll comment for this inline below.
>>
>>> The only way is to clear pending_job on the
>>> thread and spin once more.
>>> ---
>>>  migration/file.c    |   1 +
>>>  migration/multifd.c | 122 +++++++++++++++++++++++++++++++++++++++++---
>>>  migration/multifd.h |  15 ++++++
>>>  3 files changed, 131 insertions(+), 7 deletions(-)
>>> 
>>> diff --git a/migration/file.c b/migration/file.c
>>> index 5d4975f43e..22d052a71f 100644
>>> --- a/migration/file.c
>>> +++ b/migration/file.c
>>> @@ -6,6 +6,7 @@
>>>   */
>>>  
>>>  #include "qemu/osdep.h"
>>> +#include "exec/ramblock.h"
>>>  #include "qemu/cutils.h"
>>>  #include "qapi/error.h"
>>>  #include "channel.h"
>>> diff --git a/migration/multifd.c b/migration/multifd.c
>>> index 0a5279314d..45a0c7aaa8 100644
>>> --- a/migration/multifd.c
>>> +++ b/migration/multifd.c
>>> @@ -81,9 +81,15 @@ struct {
>>>  
>>>  struct {
>>>      MultiFDRecvParams *params;
>>> +    MultiFDRecvData *data;
>>>      /* number of created threads */
>>>      int count;
>>> -    /* syncs main thread and channels */
>>> +    /*
>>> +     * For sockets: this is posted once for each MULTIFD_FLAG_SYNC flag.
>>> +     *
>>> +     * For files: this is only posted at the end of the file load to mark
>>> +     *            completion of the load process.
>>> +     */
>>>      QemuSemaphore sem_sync;
>>>      /* global number of generated multifd packets */
>>>      uint64_t packet_num;
>>> @@ -1110,6 +1116,53 @@ bool multifd_send_setup(void)
>>>      return true;
>>>  }
>>>  
>>> +bool multifd_recv(void)
>>> +{
>>> +    int i;
>>> +    static int next_recv_channel;
>>> +    MultiFDRecvParams *p = NULL;
>>> +    MultiFDRecvData *data = multifd_recv_state->data;
>>
>> [1]
>>
>>> +
>>> +    /*
>>> +     * next_channel can remain from a previous migration that was
>>> +     * using more channels, so ensure it doesn't overflow if the
>>> +     * limit is lower now.
>>> +     */
>>> +    next_recv_channel %= migrate_multifd_channels();
>>> +    for (i = next_recv_channel;; i = (i + 1) % migrate_multifd_channels()) {
>>> +        if (multifd_recv_should_exit()) {
>>> +            return false;
>>> +        }
>>> +
>>> +        p = &multifd_recv_state->params[i];
>>> +
>>> +        /*
>>> +         * Safe to read atomically without a lock because the flag is
>>> +         * only set by this function below. Reading an old value of
>>> +         * true is not an issue because it would only send us looking
>>> +         * for the next idle channel.
>>> +         */
>>> +        if (qatomic_read(&p->pending_job) == false) {
>>> +            next_recv_channel = (i + 1) % migrate_multifd_channels();
>>> +            break;
>>> +        }
>>> +    }
>>
>> IIUC you'll need an smp_mb_acquire() here.  The ordering of "reading
>> pending_job" and below must be guaranteed, similar to the sender side.
>>
>
> I've been thinking about this even on the sending side.
>
> We shouldn't need the barrier here because there's a control flow
> dependency on breaking the loop. I think pending_job *must* be read
> prior to here, otherwise the program is just wrong. Does that make
> sense?

Hm, nevermind actually. We need to order this against data->size update
on the other thread anyway.