Fabiano Rosas <faro...@suse.de> writes:
> Peter Xu <pet...@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 <faro...@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.
