Re: [Qemu-devel] [PATCH 07/46] Return path: Open a return path on QEMUFile for sockets

[Dr. David Alan Gilbert]

* Paolo Bonzini (address@hidden) wrote:
> Il 16/07/2014 11:37, Dr. David Alan Gilbert ha scritto:
> >>>
> >>>+
> >>>+    /* If it's already open, return it */
> >>>+    if (qfs->file->return_path) {
> >>>+        return qfs->file->return_path;
> >>
> >>Wouldn't this leave a dangling file descriptor if you call
> >>socket_dup_return_path twice, and then close the original QEMUFile?
> >
> >Hmm - how?
> 
> The problem is that there is no reference count on QEMUFile, so if you do
> 
>   f1 = open_return_path(f0);
>   f2 = open_return_path(f0);
>   /* now f1 == f2 */
>   qemu_fclose(f1);
>   /* now f2 is dangling */

I think from the way I'm using it, I can remove the optimisation, but I do
need to check.

I'm not too sure what your worry is about 'f2' in this case; I guess the caller
needs to know that it should only close the return path once - is that
your worry?

> The remark about "closing the original QEMUFile" is also related to this
> part:
> 
>     if (result) {
>         /* We are the reverse path of our reverse path (although I don't
>            expect this to be used, it would stop another dup if it was /
>         result->return_path = qfs->file;
> 
> which has a similar bug
> 
>   f1 = open_return_path(f0);
>   f2 = open_return_path(f1);
>   /* now f2 == f0 */
>   qemu_fclose(f0);
>   /* now f2 is dangling */
> 
> If this is correct, the simplest fix is to drop the optimization.

I'm more nervous about dropping that one, because the current scheme
does provide a clean way of finding the forward path if you've got the
reverse (although I don't think I make use of it).

> >  Source side
> >     Forward path - written by migration thread
> >            : It's OK for this to be blocking, but we end up with it being
> >              non-blocking, and modify the socket code to emulate blocking.
> 
> This likely has a performance impact though.  The first migration thread
> code drop from Juan already improved throughput a lot, even if it kept the
> iothread all the time and only converted from nonblocking writes to
> blocking.

Can you give some more reasoning as to why you think this will hit the
performance so much; I thought the output buffers were quite big anyway.

> >     Return path  - opened by main thread, read by fd_handler on main thread
> >            : Must be non-blocking so as not to block the main thread while
> >              waiting for a partially sent command.
> 
> Why can't you handle this in the migration thread (or a new postcopy thread
> on the source side)?  Then it can stay blocking.

Handling it within the migration thread would make it much more complicated
(which would be bad since it's already complex enough);

A return path thread on the source side, hmm yes that could do it  - especially
since the migration thread is already a separate thread from the main thread
handling this and thus already needs locking paraphernalia.

> >  Destination side
> >     Forward path - read by main thread
> 
> This must be nonblocking so that the monitor keeps responding.

Interesting, I suspect none of the code in there is set up for that at the
moment, so how does that work during migration at the moment?

Actually, I see I missed something here; this should be:

   Destination side
         Forward path - read by main thread, and listener thread (see the
             separate mail that described that listner thread)

and that means that once postcopy is going (and the listener thread started)
it can't block the monitor.

> >     Return path  - opened by main thread, written by main thread AND 
> > postcopy
> >                    thread (protected by rp_mutex)
> 
> When does the main thread needs to write?

Not much; the only things the main thread currently responds to are the
ReqAck (ping like) requests; those are turning out to be very useful during 
debug;
I've also got the ability for the destination to send a migration result back 
to the
source which seems useful to be able to 'fail' early.

> If it doesn't need that, you can just switch to blocking when you process
> the listen command (i.e. when the postcopy thread starts).

Why don't I just do it anyway? Prior to postcopy starting we're in the same
situation as we're in with precopy today, so can already get mainblock 
threading.

Dave

> Paolo
> 
> >     I think I'm OK with both these being blocking.
> 
> 
--
Dr. David Alan Gilbert / address@hidden / Manchester, UK