[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH v5 10/17] migration: Create ram_multifd_page
From: |
Juan Quintela |
Subject: |
Re: [Qemu-devel] [PATCH v5 10/17] migration: Create ram_multifd_page |
Date: |
Tue, 08 Aug 2017 20:02:31 +0200 |
User-agent: |
Gnus/5.13 (Gnus v5.13) Emacs/25.2 (gnu/linux) |
"Dr. David Alan Gilbert" <address@hidden> wrote:
> * Juan Quintela (address@hidden) wrote:
>> "Dr. David Alan Gilbert" <address@hidden> wrote:
>> > * Juan Quintela (address@hidden) wrote:
...
>> > My feeling, without having fully thought it through, is that
>> > the locking around 'address' can be simplified; especially if the
>> > sending-thread never actually changes it.
>> >
>> > http://pubs.opengroup.org/onlinepubs/9699919799/basedefs/V1_chap04.html#tag_04_11
>> > defines that most of the pthread_ functions act as barriers;
>> > including the sem_post and pthread_cond_signal that qemu_sem_post
>> > uses.
>>
>> At the end of the series the code is this:
>>
>> qemu_mutex_lock(&p->mutex);
>> p->pages.num = pages->num;
>> iov_copy(p->pages.iov, pages->num, pages->iov, pages->num, 0,
>> iov_size(pages->iov, pages->num));
****** HERE ******
>> pages->num = 0;
>> qemu_mutex_unlock(&p->mutex);
>>
>> Are you sure that it looks like a good idea to drop the mutex?
>>
>> The other thread uses pages->num to know if things are ready.
>
> Well, I wont push it too hard, but; if you:
> a) Know that the other thread isn't accessing the iov
> (because you previously know that it had set done)
This bit I know it is true.
> b) Know the other thread wont access it until pages->num gets
> set
> c) Ensure that all changes to the iov are visible before
> the pages->num write is visible - appropriate barriers/ordering
There is no barrier there that I can see. I know that it probably work
on x86, but in others? I think that it *** HERE **** we need that
memory barrier that we don't have.
> then you're good. However, the mutex might be simpler.
Code (after all the changes) is:
qemu_sem_wait(&multifd_send_state->sem);
qemu_mutex_lock(&multifd_send_state->mutex);
for (i = 0; i < multifd_send_state->count; i++) {
p = &multifd_send_state->params[i];
if (p->done) {
p->done = false;
break;
}
}
qemu_mutex_unlock(&multifd_send_state->mutex);
qemu_mutex_lock(&p->mutex);
p->pages.num = pages->num; /* we could probably switch this
statement with the next, but I doubt
this would make a big difference */
iov_copy(p->pages.iov, pages->num, pages->iov, pages->num, 0,
iov_size(pages->iov, pages->num));
pages->num = 0;
qemu_mutex_unlock(&p->mutex);
qemu_sem_post(&p->sem);
And the other thread
qemu_mutex_lock(&p->mutex);
[...]
if (p->pages.num) {
int num;
num = p->pages.num;
p->pages.num = 0;
qemu_mutex_unlock(&p->mutex);
if (qio_channel_writev_all(p->c, p->pages.iov,
num, &error_abort)
!= num * TARGET_PAGE_SIZE) {
MigrationState *s = migrate_get_current();
migrate_set_state(&s->state, MIGRATION_STATUS_ACTIVE,
MIGRATION_STATUS_FAILED);
terminate_multifd_send_threads();
return NULL;
}
qemu_mutex_lock(&multifd_send_state->mutex);
p->done = true;
qemu_mutex_unlock(&multifd_send_state->mutex);
qemu_sem_post(&multifd_send_state->sem);
continue;
}
qemu_mutex_unlock(&p->mutex);
qemu_sem_wait(&p->sem);
This code used to have condition variables for waiting. With
semaphores, we can probably remove the p->mutex, but then we need to
think a lot each time that we do a change.
Later, Juan.