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

[David Gibson]

On Wed, Nov 19, 2014 at 05:06:50PM +0000, Dr. David Alan Gilbert wrote:
> * 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.

Hmm, yeah all right.

> > > > [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.

Uh.. not following the analogy here, sorry.

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

You can pass fds over Unix sockets, but not over pipes AFAIK.

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

-- 
David Gibson                    | I'll have my music baroque, and my code
david AT gibson.dropbear.id.au  | minimalist, thank you.  NOT _the_ _other_
                                | _way_ _around_!
http://www.ozlabs.org/~dgibson

pgp56x5s_5Tzq.pgp
Description: PGP signature