Re: [PATCH v2 10/17] migration/multifd: Convert multifd_send()::next_channel to atomic

[Fabiano Rosas <farosas@suse.de>]

"Maciej S. Szmigiero" <mail@maciej.szmigiero.name> writes:

> On 30.08.2024 20:13, Fabiano Rosas wrote:
>> "Maciej S. Szmigiero" <mail@maciej.szmigiero.name> writes:
>> 
>>> From: "Maciej S. Szmigiero" <maciej.szmigiero@oracle.com>
>>>
>>> This is necessary for multifd_send() to be able to be called
>>> from multiple threads.
>>>
>>> Signed-off-by: Maciej S. Szmigiero <maciej.szmigiero@oracle.com>
>>> ---
>>>   migration/multifd.c | 24 ++++++++++++++++++------
>>>   1 file changed, 18 insertions(+), 6 deletions(-)
>>>
>>> diff --git a/migration/multifd.c b/migration/multifd.c
>>> index d5a8e5a9c9b5..b25789dde0b3 100644
>>> --- a/migration/multifd.c
>>> +++ b/migration/multifd.c
>>> @@ -343,26 +343,38 @@ bool multifd_send(MultiFDSendData **send_data)
>>>           return false;
>>>       }
>>>   
>>> -    /* We wait here, until at least one channel is ready */
>>> -    qemu_sem_wait(&multifd_send_state->channels_ready);
>>> -
>>>       /*
>>>        * 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_channel %= migrate_multifd_channels();
>>> -    for (i = next_channel;; i = (i + 1) % migrate_multifd_channels()) {
>>> +    i = qatomic_load_acquire(&next_channel);
>>> +    if (unlikely(i >= migrate_multifd_channels())) {
>>> +        qatomic_cmpxchg(&next_channel, i, 0);
>>> +    }
>> 
>> Do we still need this? It seems not, because the mod down below would
>> already truncate to a value less than the number of channels. We don't
>> need it to start at 0 always, the channels are equivalent.
>
> The "modulo" operation below forces i_next to be in the proper range,
> not i.
>
> If the qatomic_cmpxchg() ends up succeeding then we use the (now out of
> bounds) i value to index multifd_send_state->params[].

Indeed.

>
>>> +
>>> +    /* We wait here, until at least one channel is ready */
>>> +    qemu_sem_wait(&multifd_send_state->channels_ready);
>>> +
>>> +    while (true) {
>>> +        int i_next;
>>> +
>>>           if (multifd_send_should_exit()) {
>>>               return false;
>>>           }
>>> +
>>> +        i = qatomic_load_acquire(&next_channel);
>>> +        i_next = (i + 1) % migrate_multifd_channels();
>>> +        if (qatomic_cmpxchg(&next_channel, i, i_next) != i) {
>>> +            continue;
>>> +        }
>> 
>> Say channel 'i' is the only one that's idle. What's stopping the other
>> thread(s) to race at this point and loop around to the same index?
>
> See the reply below.
>
>>> +
>>>           p = &multifd_send_state->params[i];
>>>           /*
>>>            * Lockless read to p->pending_job is safe, because only multifd
>>>            * sender thread can clear it.
>>>            */
>>>           if (qatomic_read(&p->pending_job) == false) {
>> 
>> With the cmpxchg your other patch adds here, then the race I mentioned
>> above should be harmless. But we'd need to bring that code into this
>> patch.
>> 
>
> You're right - the sender code with this patch alone isn't thread safe
> yet but this commit is only literally about "converting
> multifd_send()::next_channel to atomic".
>
> At the time of this patch there aren't any multifd_send() calls from
> multiple threads, and the commit that introduces such possible call
> site (multifd_queue_device_state()) also modifies multifd_send()
> to be fully thread safe by introducing p->pending_job_preparing.

In general this would be a bad practice because this commit can end up
being moved around due to backporting or bisecting. It would be better
if it were complete from the start. It also affects backporting due to
ambiguity on where the Fixes tag should point to if someone eventually
finds a bug.

I already asked you to extract the other code into a separate patch, so
it's not that bad. If you prefer to keep both changes separate for
clarity, please note on the commit message that the next patch is
necessary for correctness.

>
> Thanks,
> Maciej