Re: [RFC PATCH v2 1/6] migration/multifd: Remove channels_ready semaphore

[Peter Xu <peterx@redhat.com>]

Fabiano,

Sorry to look at this series late; I messed up my inbox after I reworked my
arrangement methodology of emails. ;)

On Thu, Oct 19, 2023 at 11:06:06AM +0200, Juan Quintela wrote:
> Fabiano Rosas <farosas@suse.de> wrote:
> > The channels_ready semaphore is a global variable not linked to any
> > single multifd channel. Waiting on it only means that "some" channel
> > has become ready to send data. Since we need to address the channels
> > by index (multifd_send_state->params[i]), that information adds
> > nothing of value.
> 
> NAK.
> 
> I disagree here O:-)
> 
> the reason why that channel exist is for multifd_send_pages()
> 
> And simplifying the function what it does is:
> 
> sem_wait(channels_ready);
> 
> for_each_channel()
>    look if it is empty()
> 
> But with the semaphore, we guarantee that when we go to the loop, there
> is a channel ready, so we know we donat busy wait searching for a
> channel that is free.
> 
> Notice that I fully agree that the sem is not needed for locking.
> Locking is done with the mutex.  It is just used to make sure that we
> don't busy loop on that loop.
> 
> And we use a sem, because it is the easiest way to know how many
> channels are ready (even when we only care if there is one when we
> arrive to that code).
> 
> We lost count of that counter, and we fixed that here:
> 
> commit d2026ee117147893f8d80f060cede6d872ecbd7f
> Author: Juan Quintela <quintela@redhat.com>
> Date:   Wed Apr 26 12:20:36 2023 +0200
> 
>     multifd: Fix the number of channels ready
> 
>     We don't wait in the sem when we are doing a sync_main.  Make it
> 
> And we were addressing the problem that some users where finding that we
> were busy waiting on that loop.

Juan,

I can understand why send_pages needs that sem, but not when sync main.
IOW, why multifd_send_sync_main() needs:

        qemu_sem_wait(&multifd_send_state->channels_ready);

If it has:

        qemu_sem_wait(&p->sem_sync);

How does a busy loop happen?

Thanks,

-- 
Peter Xu