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

[Paolo Bonzini <pbonzini@redhat.com>]

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

> 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