Re: [Qemu-devel] [PATCH v4 16/47] Return path: Source handling of return path

[Dr. David Alan Gilbert]

* David Gibson (address@hidden) wrote:
> On Mon, Nov 03, 2014 at 01:22:45PM +0000, Dr. David Alan Gilbert wrote:
> > * David Gibson (address@hidden) wrote:
> > > On Fri, Oct 03, 2014 at 06:47:22PM +0100, Dr. David Alan Gilbert (git) 
> > > wrote:
> > > > From: "Dr. David Alan Gilbert" <address@hidden>
> > > > 
> > > > Open a return path, and handle messages that are received upon it.
> > > > 
> > > > Signed-off-by: Dr. David Alan Gilbert <address@hidden>
> > > 
> > > [snip]
> > > > @@ -414,6 +448,11 @@ static void migrate_fd_cancel(MigrationState *s)
> > > >      int old_state ;
> > > >      trace_migrate_fd_cancel();
> > > >  
> > > > +    if (s->return_path) {
> > > > +        /* shutdown the rp socket, so causing the rp thread to 
> > > > shutdown */
> > > > +        qemu_file_shutdown(s->return_path);
> > > 
> > > Terminating the rp thread via shutting down its file seems roundabout,
> > > and kind of dependent on the socket file implementation.
> > 
> > The rp thread might be in the middle of a blocking read()/recv()
> > so I'm doing a shutdown() to cause those to exit; once I have to do that
> > anyway it didn't seem necessary to add anything etra.
> 
> Hm.  I don't recall, does the rp thread need to do some cleanup at
> this point?  Otherwise pthread_cancel() should kill a thread, even if
> it's blocked at the moment.

It was Paolo's idea to use shutdown() and I agree - it works well;
I'd originally thought about using pthread_cancel but it seemed to be
generally disliked - you have to be very careful to either know exactly
the points at which it might be killed (if you use the deferred version)
or be prepared to deal with your thread disappearing at any time and
ensure your data structures are always consistent.  In addition there
was some concern that there was no Windows equivalent to pthread_cancel.

> > > [snip]
> > > > +__attribute__ (( unused )) /* Until later in patch series */
> > > > +static int open_outgoing_return_path(MigrationState *ms)
> > > > +{
> > > > +
> > > > +    ms->return_path = qemu_file_get_return_path(ms->file);
> > > 
> > > So, another reason this get_return_path abstraction doesn't seem right
> > > to me, is that it's not obvious that for non-socket file types, the
> > > source and destination side "get return path" operations would
> > > necessarily be the same.
> > 
> > However, since the implementation of the get_return_path is a method
> > on the particular implementation, and it can be different for a 
> > qemu_file opened for read or write, then that non-socket file type
> > could implement it how it likes including something like shutdown).
> 
> So, I'm a little less bothered by this since I realised that QemuFile
> is basically only used for migration streams, not for other file type
> operations.  The fact that that makes QemuFile a really bad name is a
> different matter.

Yes, but hey we've got FILE* in C anyway, so it might be bad, but it's
not inconsitent.

> The return path operation is quite specific to a migration stream, and
> doesn't really belong with a "file" abstraction.

I think the bit that's specific, is as you say that I don't know whether
I need it until later.

> The case I've been considering where it's not easy to see how to
> abstract this is that of a pipe - in that case it will be necessary to
> open a second pipe from destination to source, which probably needs
> some preliminary work when first opening the connection, and therefore
> can't easily be encapsulated into a "get return path" callback.

I'm OK with some transports not supporting this; I check for it and
error out.  At a higher level I do send an 'open_return_path' command
from src->dest early on to say I'm going to want a return path, I guess
a pipe might be able to open that fd then and pass it back over the original
fd? But that might be hairy.

> The abstraction of the shutdown is another question again - I can't
> think of any other file type which has an operation similar in effect
> to shutdown(), so it seems really socket specific. Which is another
> reason I'm not convinced telling the rp thread to die via its stream
> is a good idea.

I'd be OK with setting some flag or similar at the same time if that
would help; but I don't think there's a safe posix'y way of killing a
thread that might be stuck in a recv()/read() other than shutdown().

Dave

--
Dr. David Alan Gilbert / address@hidden / Manchester, UK