Re: [Qemu-devel] [PATCH v13 11/12] migration: Remove not needed semaphore and quit

["Dr. David Alan Gilbert" <dgilbert@redhat.com>]

* Juan Quintela (quintela@redhat.com) wrote:
> "Dr. David Alan Gilbert" <dgilbert@redhat.com> wrote:
> > * Juan Quintela (quintela@redhat.com) wrote:
> >> "Dr. David Alan Gilbert" <dgilbert@redhat.com> wrote:
> >> > * Juan Quintela (quintela@redhat.com) wrote:
> >> >> We know quit closing the QIO.
> >> >
> >> > This patch does two different things; one of which I think I understand.
> >> >
> >> > The 'quit' has been removed - I think that makes sense because the
> >> > multifd threads terminate when either they come to the end of a stream
> >> > or hit an error; there's no case of asking them to quit explicitly.
> >> > The 'sem' was basically for kicking the recv-thread to quit; which again
> >> > isn't needed.
> >> >
> >> > Now, what about the object_unref on p->c ?
> >> > If I've got this right multifd_recv_terminate_threads is only called in
> >> > the error case; but doesn't multifd_load_cleanup also get called in that
> >> > case - it does the unref and p->c = NULL as well.
> >> 
> >> Adding another comment O:-)
> >> 
> >> In normal exit case: we don't care.
> >> 
> >> In error case, we do a close of the p->c.  Doing a close, means that the
> >> channel threads that are waiting on qio_channel_read_all_eof() will stop
> >> the waiting and return an error, so we are safe.
> >> 
> >>         ret = qio_channel_read_all_eof(p->c, (void *)p->packet,
> >>                                        p->packet_len, &local_err);
> >>         if (ret == 0) {   /* EOF */
> >>             break;
> >>         }
> >>         if (ret == -1) {   /* Error */
> >>             break;
> >>         }
> >> 
> >> We have to wait for three things:
> >> - we receive a synchronization packet
> >> - we receive EOF and finish
> >> - we got somehow an error and have to quit.
> >> 
> >> Old versions have a semaphore, and we have three places where we set
> >> that semaphore "we are ready", when we receive one error, when we have
> >> data waiting to be read (it had a qio whatcher) or we just got a normal
> >> exit.
> >> 
> >> Waiting on two things make code more complex that it used to be (and
> >> whatchers are really lame, because we know there are data ready, but not
> >> how much, dealing with non-whole packets/pages is a real mess).  So we
> >> moved to:
> >> -  quit is gone: we just close the channel, that makes the *_read_eof()
> >>    to end.  Notice that we don't care if the close is due to one error
> >>    or because we have finished.  It is just done.
> >> - Now reception channel only have to wait on _read_eof(), the three
> >>   cases that we talked before are handled correctly.
> >> 
> >> I understand the confusion, this only makes sense when you came from the
> >> previous version of the patchset.
> >
> > Two questions from that then:
> >   a) Are you sure it's safe to close the qio_channel while another
> > thread is in qio_channel_read_all_eof?  Is it really defined that it
> > causes the other thread to exit with an error;  close() in some stuff
> > frees data structures that the other thread is still reading; that's
> > why I've used shutdown(2) in the past rather than close on fd's
> 
> Dunno if it is safe (I think it is), but I agree that shutdown will also
> get what I need.
> 
> >   b) I don't think your answer explains why it's an object_unref?
> 
> That is the standard way to closing qios to not have to take into
> account who have it oppened.  See previous paragraph, it is better to
> use shutdown, done.

OK, great;  I suspect it's unsafe because as soon as you do the unref
it could free the object; actually you should have a ref from each of
the threads to sotp it being freed while they use it.

Dave

> Thanks, Juan.
> 
> > Dave
> >
> >> Later, Juan.
> > --
> > Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK