From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:47996) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Utf1Z-00022d-WC for qemu-devel@nongnu.org; Mon, 01 Jul 2013 10:24:18 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Utf1V-0007NG-M4 for qemu-devel@nongnu.org; Mon, 01 Jul 2013 10:24:13 -0400 Received: from e37.co.us.ibm.com ([32.97.110.158]:44439) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Utf1V-0007Ms-EL for qemu-devel@nongnu.org; Mon, 01 Jul 2013 10:24:09 -0400 Received: from /spool/local by e37.co.us.ibm.com with IBM ESMTP SMTP Gateway: Authorized Use Only! Violators will be prosecuted for from ; Mon, 1 Jul 2013 08:24:07 -0600 Received: from d03relay03.boulder.ibm.com (d03relay03.boulder.ibm.com [9.17.195.228]) by d03dlp03.boulder.ibm.com (Postfix) with ESMTP id 8D82A19D803E for ; Mon, 1 Jul 2013 08:23:24 -0600 (MDT) Received: from d03av03.boulder.ibm.com (d03av03.boulder.ibm.com [9.17.195.169]) by d03relay03.boulder.ibm.com (8.13.8/8.13.8/NCO v10.0) with ESMTP id r61ENWmg151766 for ; Mon, 1 Jul 2013 08:23:32 -0600 Received: from d03av03.boulder.ibm.com (loopback [127.0.0.1]) by d03av03.boulder.ibm.com (8.14.4/8.13.1/NCO v10.0 AVout) with ESMTP id r61ENSPo002092 for ; Mon, 1 Jul 2013 08:23:28 -0600 Message-ID: <51D190DF.3080208@linux.vnet.ibm.com> Date: Mon, 01 Jul 2013 10:23:27 -0400 From: "Michael R. Hines" MIME-Version: 1.0 References: <1372449603-20431-1-git-send-email-mrhines@linux.vnet.ibm.com> <1372449603-20431-5-git-send-email-mrhines@linux.vnet.ibm.com> <51D17048.7080609@redhat.com> In-Reply-To: <51D17048.7080609@redhat.com> Content-Type: text/plain; charset=ISO-8859-15; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH v2 4/8] rdma: unpin support List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Paolo Bonzini Cc: aliguori@us.ibm.com, quintela@redhat.com, qemu-devel@nongnu.org, owasserm@redhat.com, abali@us.ibm.com, mrhines@us.ibm.com, gokul@us.ibm.com, chegu_vinod@hp.com, knoel@redhat.com On 07/01/2013 08:04 AM, Paolo Bonzini wrote: > Il 28/06/2013 21:59, mrhines@linux.vnet.ibm.com ha scritto: >> +/* >> + * Perform a non-optimized memory unregistration after every transfer >> + * for demonsration purposes, only if pin-all is not requested. >> + * >> + * Potential optimizations: >> + * 1. Start a new thread to run this function continuously >> + - for bit clearing >> + - and for receipt of unregister messages >> + * 2. Use an LRU. >> + * 3. Use workload hints. >> + */ >> +#ifdef RDMA_UNREGISTRATION_EXAMPLE >> +static int qemu_rdma_unregister_waiting(RDMAContext *rdma) >> +{ >> + while (rdma->unregistrations[rdma->unregister_current]) { >> + int ret; >> + uint64_t wr_id = rdma->unregistrations[rdma->unregister_current]; >> + uint64_t chunk = >> + (wr_id & RDMA_WRID_CHUNK_MASK) >> RDMA_WRID_CHUNK_SHIFT; >> + uint64_t index = >> + (wr_id & RDMA_WRID_BLOCK_MASK) >> RDMA_WRID_BLOCK_SHIFT; >> + RDMALocalBlock *block = >> + &(rdma->local_ram_blocks.block[index]); >> + RDMARegister reg = { .current_index = index }; >> + RDMAControlHeader resp = { .type = RDMA_CONTROL_UNREGISTER_FINISHED, >> + }; >> + RDMAControlHeader head = { .len = sizeof(RDMARegister), >> + .type = RDMA_CONTROL_UNREGISTER_REQUEST, >> + .repeat = 1, >> + }; >> + >> + DDPRINTF("Processing unregister for chunk: %" PRIu64 " at position %d\n", >> + chunk, rdma->unregister_current); >> + >> + rdma->unregistrations[rdma->unregister_current] = 0; >> + rdma->unregister_current++; >> + >> + if (rdma->unregister_current == RDMA_SIGNALED_SEND_MAX) { >> + rdma->unregister_current = 0; >> + } >> + >> + DDPRINTF("Sending unregister for chunk: %" PRIu64 "\n", chunk); >> + >> + clear_bit(chunk, block->unregister_bitmap); > The chunk is still registered at this point, shouldn't it be after > the ibv_dereg_mr or something like that? No, this example is completely speculative (and single-threaded). If the unpin can be done safely (i.e. memory is not being actively transmitted), only then do we proceed with unregistering it. Otherwise, we just skip the entire region. After the "in transmit" memory completes later, we will *again* attempt to unregister that memory in a later iteration of this function. >> + if (test_bit(chunk, block->transit_bitmap)) { >> + DDPRINTF("Cannot unregister inflight chunk: %" PRIu64 "\n", chunk); >> + continue; >> + } > This was not clear from your answer: who exactly will unregister this > chunk? Why not call the 15 lines below this one also at this point: hehe: This is because of the infiniband programming model =) I know it's not clear at first. I'll explain: *Everything* in infiniband is asynchronous, so you have to think about infiniband code just like parallel code even when it's not explicitly parallel. All messages in infiniband must have what is called a "receive work request" posted on the receiver side of the connection *before* the sender attempts actually send anything. This requires infiniband code to keep a very precise count of how many work requests have been posted, otherwise the device will throw an error and the whole connection shuts down. The block of code below is part of a single-shot blocking call which is expecting a specific series of asynchronous responses from the other side, for which a specific number of receive work requests have already been posted on the receiver-side RDMA device. Without implementing a new thread, we cannot *initiate* a new exchange of protocol messages *in the middle* of this blocking call path without causing the number of receive work requests posted to the RDMA device to be incorrect. The only way to unregister the memory (without a new thread) is to *mark* the memory as "need to unregister" in the bitmap you see here and then wait for the *current* RDMA transfer to complete and then when the coast is clear, so to speak, then we can unregister all the memory that was previously registered by piggybacking the unregistrations on top of the same call path of NEW RDMA transfers that will happen in the future. Does that make sense? > + if (wr_id == RDMA_WRID_RDMA_WRITE) { > + uint64_t chunk = > + (wc.wr_id & RDMA_WRID_CHUNK_MASK) >> RDMA_WRID_CHUNK_SHIFT; > + uint64_t index = > + (wc.wr_id & RDMA_WRID_BLOCK_MASK) >> RDMA_WRID_BLOCK_SHIFT; > + RDMALocalBlock *block = &(rdma->local_ram_blocks.block[index]); > + > + DDDPRINTF("completions %s (%" PRId64 ") left %d, " > + "block %" PRIu64 ", chunk: %" PRIu64 "\n", > + print_wrid(wr_id), wr_id, rdma->nb_sent, index, chunk); > + > + clear_bit(chunk, block->transit_bitmap); > + > + if (rdma->nb_sent > 0) { > + rdma->nb_sent--; > + } > > ? > >> + >> + 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; >> + register_to_network(®); >> + ret = qemu_rdma_exchange_send(rdma, &head, (uint8_t *) ®, >> + &resp, NULL, NULL); >> + if (ret < 0) { >> + return ret; >> + } >> + >> + DDPRINTF("Unregister for chunk: %" PRIu64 " complete.\n", chunk); >> + } >> + >> + return 0; >> +} >> + >> +/* >> + * Set bit for unregistration in the next iteration. >> + * We cannot transmit right here, but will unpin later. >> + */ >> +static void qemu_rdma_signal_unregister(RDMAContext *rdma, uint64_t index, >> + uint64_t chunk, uint64_t wr_id) >> +{ >> + if (rdma->unregistrations[rdma->unregister_next] != 0) { >> + fprintf(stderr, "rdma migration: queue is full!\n"); >> + } else { >> + RDMALocalBlock *block = &(rdma->local_ram_blocks.block[index]); >> + >> + if (!test_and_set_bit(chunk, block->unregister_bitmap)) { >> + DDPRINTF("Appending unregister chunk %" PRIu64 >> + " at position %d\n", chunk, rdma->unregister_next); >> + >> + rdma->unregistrations[rdma->unregister_next++] = wr_id; >> + >> + if (rdma->unregister_next == RDMA_SIGNALED_SEND_MAX) { >> + rdma->unregister_next = 0; >> + } >> + } else { >> + DDPRINTF("Unregister chunk %" PRIu64 " already in queue.\n", >> + chunk); >> + } >> + } >> +} >> +#endif >> static int qemu_rdma_exchange_send(RDMAContext *rdma, RDMAControlHeader *head, >> uint8_t *data, RDMAControlHeader *resp, >> int *resp_idx, >> @@ -1006,6 +1132,17 @@ static uint64_t qemu_rdma_poll(RDMAContext *rdma, uint64_t *wr_id_out) >> if (rdma->nb_sent > 0) { >> rdma->nb_sent--; >> } >> + 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_UNREGISTRATION_EXAMPLE >> + qemu_rdma_signal_unregister(rdma, index, chunk, wc.wr_id); >> +#endif >> + } >> } else { >> DDPRINTF("other completion %s (%" PRId64 ") received left %d\n", >> print_wrid(wr_id), wr_id, rdma->nb_sent); >> @@ -1423,6 +1560,12 @@ retry: >> chunk_start = ram_chunk_start(block, chunk); >> chunk_end = ram_chunk_end(block, chunk); >> >> + if (!rdma->pin_all) { >> +#ifdef RDMA_UNREGISTRATION_EXAMPLE >> + qemu_rdma_unregister_waiting(rdma); >> +#endif >> + } >> + >> while (test_bit(chunk, block->transit_bitmap)) { >> (void)count; >> DDPRINTF("(%d) Not clobbering: block: %d chunk %" PRIu64 >> >