| Message ID | 20200116154616.11569-4-quintela@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | Fix multifd + cancel + multifd | expand |
* Juan Quintela (quintela@redhat.com) wrote: > If p->quit is true for any channel, we know that it has finished for > any reason. So don't wait for it, just continue. > > Signed-off-by: Juan Quintela <quintela@redhat.com> > > --- > > I could be convinced that the right thing to do in that case is to > just do a break instead of a continue. Each option has its own > advantages/disadvantanges. > --- > migration/ram.c | 6 ++++++ > 1 file changed, 6 insertions(+) > > diff --git a/migration/ram.c b/migration/ram.c > index 44ca56e1ea..bc918ef28d 100644 > --- a/migration/ram.c > +++ b/migration/ram.c > @@ -1118,6 +1118,12 @@ static void multifd_send_sync_main(RAMState *rs) > MultiFDSendParams *p = &multifd_send_state->params[i]; > > trace_multifd_send_sync_main_wait(p->id); > + qemu_mutex_lock(&p->mutex); > + if (p->quit) { > + qemu_mutex_unlock(&p->mutex); > + continue; > + } > + qemu_mutex_unlock(&p->mutex); Why is this needed/helps? You can't depend on the p->quit happening before the sem_wait, so the main thread still has to do a post on sem_sync before the join, even with the addition of the check for p->quit. Dave > qemu_sem_wait(&p->sem_sync); > } > trace_multifd_send_sync_main(multifd_send_state->packet_num); > -- > 2.24.1 > -- Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
"Dr. David Alan Gilbert" <dgilbert@redhat.com> wrote: > * Juan Quintela (quintela@redhat.com) wrote: >> If p->quit is true for any channel, we know that it has finished for >> any reason. So don't wait for it, just continue. >> >> Signed-off-by: Juan Quintela <quintela@redhat.com> >> >> --- >> >> I could be convinced that the right thing to do in that case is to >> just do a break instead of a continue. Each option has its own >> advantages/disadvantanges. >> --- >> migration/ram.c | 6 ++++++ >> 1 file changed, 6 insertions(+) >> >> diff --git a/migration/ram.c b/migration/ram.c >> index 44ca56e1ea..bc918ef28d 100644 >> --- a/migration/ram.c >> +++ b/migration/ram.c >> @@ -1118,6 +1118,12 @@ static void multifd_send_sync_main(RAMState *rs) >> MultiFDSendParams *p = &multifd_send_state->params[i]; >> >> trace_multifd_send_sync_main_wait(p->id); >> + qemu_mutex_lock(&p->mutex); >> + if (p->quit) { >> + qemu_mutex_unlock(&p->mutex); >> + continue; >> + } >> + qemu_mutex_unlock(&p->mutex); > > Why is this needed/helps? > You can't depend on the p->quit happening before the > sem_wait, so the main thread still has to do a post on sem_sync before > the join, even with the addition of the check for p->quit. if we have asked the thread to quit, it is inside posibility that it has already quit, so it is not going to be able to do the ->post() for this sem. if ->quit == true, then we know that we are exiting. On _normal_ exit, we know that everything is ok. On cancel/error, we don't really know, it deppends how lucky we are. Later, Juan.
diff --git a/migration/ram.c b/migration/ram.c index 44ca56e1ea..bc918ef28d 100644 --- a/migration/ram.c +++ b/migration/ram.c @@ -1118,6 +1118,12 @@ static void multifd_send_sync_main(RAMState *rs) MultiFDSendParams *p = &multifd_send_state->params[i]; trace_multifd_send_sync_main_wait(p->id); + qemu_mutex_lock(&p->mutex); + if (p->quit) { + qemu_mutex_unlock(&p->mutex); + continue; + } + qemu_mutex_unlock(&p->mutex); qemu_sem_wait(&p->sem_sync); } trace_multifd_send_sync_main(multifd_send_state->packet_num);
If p->quit is true for any channel, we know that it has finished for any reason. So don't wait for it, just continue. Signed-off-by: Juan Quintela <quintela@redhat.com> --- I could be convinced that the right thing to do in that case is to just do a break instead of a continue. Each option has its own advantages/disadvantanges. --- migration/ram.c | 6 ++++++ 1 file changed, 6 insertions(+)