Re: [Qemu-devel] [PATCH 3/6] rdma: core logic

[Paolo Bonzini]

Il 28/06/2013 00:44, address@hidden ha scritto:
> + *
> + * This will have a terrible impact on migration performance, so until future
> + * workload information or LRU information is available, do not attempt to 
> use
> + * this feature except for basic testing.
> + */
> +//#define RDMA_ALLOW_REGISTRATION_WINDOW

Out of curiosity, how terrible is the terrible impact?

> + * Perform a non-optimized memory registration after every transfer
> + * for demonsration purposes, only if pin-all is not requested.
> + */
> +static int qemu_rdma_unregister_waiting(RDMAContext *rdma) {
> +#ifdef RDMA_ALLOW_REGISTRATION_WINDOW

IIRC  function can be always enabled, it will just do nothing if
no one registers anything.  It is just this bit that needs the
#ifdef:

+
+        if (!rdma->pin_all) {
+            /*
+             * FYI: If one wanted to signal a specific chunk to be unregistered
+             * using LRU or workload-specific information, this is the function
+             * you would call to do so. That chunk would then get 
asynchronously
+             * unregistered later.
+             */
   >> #ifdef RDMA_ALLOW_REGISTRATION_WINDOW
+            qemu_rdma_signal_unregister(rdma, index, chunk, wc.wr_id);
   >> #endif
+        }

Then qemu_rdma_signal_unregister would be unused for
!RDMA_ALLOW_REGISTRATION_WINDOW, so you would need to put that
function within an #ifdef (the whole function, not just the
body).

At this point RDMA_ALLOW_REGISTRATION_WINDOW becomes a slightly
wrong name.  What about RDMA_UNREGISTRATION_EXAMPLE?

> 
> +        if(test_bit(chunk, block->transit_bitmap)) {
> +            DDPRINTF("Cannot unregister inflight chunk: %" PRIu64 "\n", 
> chunk);
> +            continue;
> +        }

Nobody will process this unregistration after this.  Should you move this

+        // perhaps test_and_clear_bit here?  and exit if the bit is zero
+        clear_bit(chunk, block->unregister_bitmap);
+
+        ret = ibv_dereg_mr(block->pmr[chunk]);
+        block->pmr[chunk] = NULL;
+        block->remote_keys[chunk] = 0;
+
+        if(ret != 0) {
+            perror("unregistration chunk failed");
+            return -ret;
+        }
+        rdma->total_registrations--;
+
+        reg.key.chunk = chunk;
+        ret = qemu_rdma_exchange_send(rdma, &head, (uint8_t *) &reg,
+                                &resp, &resp_idx, NULL);
+        if (ret < 0) {
+            return ret;
+        }
+
+        DDPRINTF("Unregister for chunk: %" PRIu64 " complete.\n", chunk);

to a separate function and check the unregister_bitmap when you get a
transfer completed message from IB?

Also, you do not need to receive the unregistration completed response
now, do you?  You can have another bitmap for "unregistration in progress"
and only wait for completion before re-pinning.  This way, the impact
of unregistration is low if the page is not going to be changed soon.

_However_, please make this new functionality a separate patch so that we
can discuss it better.

Paolo