Re: [Qemu-devel] [PATCH v5 17/17] migration: Flush receive queue

[Juan Quintela]

"Dr. David Alan Gilbert" <address@hidden> wrote:
> * Juan Quintela (address@hidden) wrote:
>> Each time that we sync the bitmap, it is a possiblity that we receive
>> a page that is being processed by a different thread.  We fix this
>> problem just making sure that we wait for all receiving threads to
>> finish its work before we procedeed with the next stage.
>> 
>> We are low on page flags, so we use a combination that is not valid to
>> emit that message:  MULTIFD_PAGE and COMPRESSED.
>> 
>> I tried to make a migration command for it, but it don't work because
>> we sync the bitmap sometimes when we have already sent the beggining
>> of the section, so I just added a new page flag.
>> 
>> Signed-off-by: Juan Quintela <address@hidden>

>> +static int multifd_flush(void)
>> +{
>> +    int i, thread_count;
>> +
>> +    if (!migrate_use_multifd()) {
>> +        return 0;
>> +    }
>> +    thread_count = migrate_multifd_threads();
>> +    for (i = 0; i < thread_count; i++) {
>> +        MultiFDRecvParams *p = multifd_recv_state->params[i];
>> +
>> +        qemu_mutex_lock(&p->mutex);
>> +        while (!p->done) {
>> +            p->sync = true;
>> +            qemu_cond_wait(&p->cond_sync, &p->mutex);
>> +        }
>
> I don't think I understand how that works in the case where the
> recv_thread has already 'done' by the point you set sync=true; how does
> it get back to the check and do the signal?

We have two cases:
* done = true
* done = false

if done is false, we need to wait until it is done.  But if it is true,
we don't have to wait.  By definition, there is nothing on that thread
that we need to wait for.  It is not in the middle of receiving a page.



>
>> +        qemu_mutex_unlock(&p->mutex);
>> +    }
>> +    return 0;
>> +}
>> +
>>  /**
>>   * save_page_header: write page header to wire
>>   *
>> @@ -809,6 +847,12 @@ static size_t save_page_header(RAMState *rs, QEMUFile 
>> *f,  RAMBlock *block,
>>  {
>>      size_t size, len;
>>  
>> +    if (rs->multifd_needs_flush &&
>> +        (offset & RAM_SAVE_FLAG_MULTIFD_PAGE)) {
>> +        offset |= RAM_SAVE_FLAG_ZERO;
>
> In the comment near the top you say RAM_SAVE_FLAG_COMPRESS_PAGE;  it's
> probably best to add an alias at the top to make it clear, e.g.
>   #define RAM_SAVE_FLAG_MULTIFD_SYNC RAM_SAVE_FLAG_ZERO
>
>   or maybe (RAM_SAVE_FLAG_MULTIFD_PAGE | RAM_SAVE_FLAG_ZERO)

done.

But I only use it when we use the "or".

Thanks, Juan.