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:
> >> 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

  b) I don't think your answer explains why it's an object_unref?

Dave

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