* Juan Quintela (quint...@redhat.com) wrote:
> "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.
Yes, I think that's smp_mb_release() - and you have to do an
smp_mb_acquire after reading the pages->num before accessing the iov.
(Probably worth checking with Paolo).
Or just stick with mutex's.
> > 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.
Dave
--
Dr. David Alan Gilbert / dgilb...@redhat.com / Manchester, UK
