"Dr. David Alan Gilbert" <dgilb...@redhat.com> wrote: > * Juan Quintela (quint...@redhat.com) wrote: >> "Dr. David Alan Gilbert" <dgilb...@redhat.com> wrote: >> > * Juan Quintela (quint...@redhat.com) 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.
