Re: [Qemu-devel] [RFC PATCH RDMA support v6: 4/7] Introduce two new capabilities

["Michael R. Hines" <mrhines@linux.vnet.ibm.com>]

On 04/10/2013 08:40 AM, Paolo Bonzini wrote:
> Il 10/04/2013 14:34, Michael R. Hines ha scritto:
>> On 04/10/2013 03:50 AM, Paolo Bonzini wrote:
>>> Il 10/04/2013 06:29, mrhines@linux.vnet.ibm.com ha scritto:
>>>> From: "Michael R. Hines" <mrhines@us.ibm.com>
>>>>
>>>> RDMA performs very slowly with zero-page checking.
>>>> Without the ability to disable it, RDMA throughput and
>>>> latency promises and high performance links cannot be
>>>> fully realized.
>>>>
>>>> On the other hand, dynamic page registration support is also
>>>> included in the RDMA protocol. This second capability also
>>>> cannot be fully realized without the ability to enable zero
>>>> page scanning.
>>>>
>>>> So, we have two new capabilities which work together:
>>>>
>>>> 1. migrate_set_capability check_for_zero on|off (default on)
>>>> 2. migrate_set_capability chunk_register_destination on|off (default
>>>> off)
>>>>
>>>> Signed-off-by: Michael R. Hines <mrhines@us.ibm.com>
>>> Michael, please listen to _all_ review comments.
>>>
>>> 1) I asked you to place check_for_zero in a separate patch.
>>>
>>> 2) Again, patch 3 cannot compile without this one.  The code should
>>> compile after each patch, with and without --enable-rdma.
>> My apologies - I misunderstood the request. I am indeed addressing every
>> comment.
>>
>> When you said separate, I thought you meant a different patch series
>> altpgether.
>>
>> This is my first time, so the meaning of "separate" was not clear =)
>>
>> And yes, patch 3 does in fact compile both with and without --enable-rdma.
> How does this:
>
> +#ifdef CONFIG_RDMA
> +const QEMURamControlOps qemu_rdma_write_control = {
> +    .before_ram_iterate = qemu_ram_registration_start,
> +    .after_ram_iterate = qemu_rdma_registration_stop,
> +    .register_ram_iterate = qemu_rdma_registration_handle,
> +    .save_page = qemu_rdma_save_page,
> +};
>
> compile (and link) successfully without a definition of
> qemu_ram_registration_start, which is in patch 5?  (Honest question).
>
> Similarly, patch 5 cannot link without qemu_ram_foreach_block.
>
> Anyway, patch 1 does not compile with --enable-rdma. :)

Yes, all 7 patches do compile in both cases - that code is 
conditionalized with CONFIG_RDMA.

What exactly is the problem there?

I toggled the enable/disable flags several times and recompiled just to 
be sure.

Also about qemu_ram_foreach_block: Why does this depend on patch 5, exactly?

qemu_ram_foreach_block is not conditionalized by anything.

>> You flip-flopped on me =)
>> You said conditionalize it, then make a separate patch, then delete it
>> altogether =)
> Make qemu_set_nonblock conditional, dropping the assert.
>
> Do not touch the implementation of qemu_set_nonblock.  (Yes, this was a
> mess).
>
> Paolo

Acknowledged.