Re: [Qemu-devel] [PULL 5/8] rdma: core rdma logic

[Michael R. Hines]

On 04/17/2013 04:58 AM, Paolo Bonzini wrote:
> No, I really mean it.  No dead code.  We included the one useful option
> as a capability, and that should be it.

Acknowledged =)

> > Failure already happens for unknown capabilities.
> Maybe I misread the code, but all I saw is:
>
> +        if (cap.flags & RDMA_CAPABILITY_CHUNK_REGISTER) {
> +            rdma->chunk_register_destination = true;
> +        } else if (cap.flags & RDMA_CAPABILITY_NEXT_FEATURE) {
> +            /* handle new capability */
> +        }
>
> and no failure.  I may well be wrong of course.

Sorry I miscommunicated. By "failure" I meant "disagreement".
My design allows the two sides to "agree" with each other as I
included in the documentation like this:

1. User sets capability on source (or not)

2. If 'true', source transmits our "intent" to activate capability to 
destination

          If (destination knows about the capability)
                 then, enable on both sides
          else
                 disable on both sides

3. If 'false', destination will never see the flag and will never know

> > > +            goto err_rdma_server_wait;
> > > +    }
> > > +
> > > +    if (cap.version == RDMA_CONTROL_VERSION_1) {
> > > +        if (cap.flags & RDMA_CAPABILITY_CHUNK_REGISTER) {
> > > +            rdma->chunk_register_destination = true;
> > > +        } else if (cap.flags & RDMA_CAPABILITY_NEXT_FEATURE) {
> > > +            /* handle new capability */
> > > +        }
> > > As mentioned above, please drop this "else if".  But in general, this
> > > "if" is useless.  Please replace it with an
> > >
> > >       /* We only support one version, and we rejected all
> > >        * others above.
> > >        */
> > >       assert(cap.version == RDMA_CONTROL_VERSION_CURRENT);
> > We don't want to kill QEMU with an assertion, do we?
> > Shouldn't we throw the error back to the user?
> You already filtered versions < min and > current with a decent error,
> so the only reamining version at this point is 1.  You know cap.version
> == RDMA_CONTROL_VERSION_CURRENT ("we rejected all others above").

So, as I described above, when version 2 comes out, both QEMU
instances are able to "agree" only on the minimum capabilities
that both sides support and still allow the migration to begin.

For that reason, I don't think we should be doing assert() here.
We want this to be compatible across multiple QEMU versions.


> > Yes, in my next patch, you will see a new version which uses
> > that file descriptor by calling <poll.h>'s poll().
> Ok.  Then we can improve it to support coroutines, too, so that the
> monitor remains responsive on the incoming side.  The idea is that if
> poll says the file descriptor is not available you do
> yield_until_fd_readable(fd) where yield_until_fd_readable is currently
> static in qemu-file.c but could be made public and moved to
> qemu-coroutine-io.c and include/qemu-common.h:
>
>      typedef struct {
>          Coroutine *co;
>          int fd;
>      } FDYieldUntilData;
>
>      static void fd_coroutine_enter(void *opaque)
>      {
>          FDYieldUntilData *data = opaque;
>          qemu_set_fd_handler(data->fd, NULL, NULL, NULL);
>          qemu_coroutine_enter(data->co, NULL);
>      }
>
>      /**
>       * Yield until a file descriptor becomes readable
>       *
>       * Note that this function clobbers the handlers for the file
> descriptor.
>       */
>      static void coroutine_fn yield_until_fd_readable(int fd)
>      {
>          FDYieldUntilData data;
>
>          assert(qemu_in_coroutine());
>          data.co = qemu_coroutine_self();
>          data.fd = fd;
>          qemu_set_fd_handler(fd, fd_coroutine_enter, NULL, &data);
>          qemu_coroutine_yield();
>      }
>
> Please feel free to include such a patch (separate from this one) in
> your next submission.

Excellent. I include the patch. Thanks.

- Michael