From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:53840) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1aYtxL-0003fo-EJ for qemu-devel@nongnu.org; Thu, 25 Feb 2016 06:19:40 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1aYtxH-0005GW-Ea for qemu-devel@nongnu.org; Thu, 25 Feb 2016 06:19:39 -0500 Received: from mx0.arrikto.com ([2a01:7e00::f03c:91ff:fe6e:d7ab]:37334) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1aYtxH-0005GR-7t for qemu-devel@nongnu.org; Thu, 25 Feb 2016 06:19:35 -0500 References: <1450284917-10508-1-git-send-email-apyrgio@arrikto.com> <1450284917-10508-2-git-send-email-apyrgio@arrikto.com> <56BC6DC0.4070204@redhat.com> <56C70168.2050408@arrikto.com> <56CAE64B.4070608@redhat.com> <56CED32C.6020001@arrikto.com> <56CED5CD.1070400@redhat.com> From: Alex Pyrgiotis Message-ID: <56CEE344.9010702@arrikto.com> Date: Thu, 25 Feb 2016 13:19:32 +0200 MIME-Version: 1.0 In-Reply-To: <56CED5CD.1070400@redhat.com> Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH 1/9] dma-helpers: Expose the sg mapping logic List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Paolo Bonzini Cc: kwolf@redhat.com, famz@redhat.com, qemu-devel@nongnu.org Hi Paolo, On 02/25/2016 12:22 PM, Paolo Bonzini wrote: > > > On 25/02/2016 11:10, Alex Pyrgiotis wrote: >> 8<... snip ...>8 >> >> Sure, you're right, there's no sensible way to break an ioctl() >> operation in many. However, I'd argue that we shouldn't need to, as it >> would be much better if the DMA operation was never restarted in the >> first place. Instead, if we dealt with the bigger issue of the global >> bounce buffer, we could kill two birds with one stone. >> >> I see that there was an attempt [1] to replace the global bounce buffer >> with something more dynamic. In short, the objections [2] were the >> following: >> >> 1. It introduced locking/unlocking a global mutex in the hot path as >> well as a hash table lookup. >> 2. It allowed for unbounded memory allocations. >> >> An improvement that would address (1) is to get rid of any global state: >> >> Since the mapping operation takes place in the context of a DMA >> operation, we could provide a ctx-type struct to the dma_memory_(un)map >> --> address_space_(un)map functions that would contain a hash table. If >> any memory allocations were needed, we would track them using that hash >> table, which would require no locks. Moreover, if the initialization of >> the hash table hurts the performance in the general case, we could use >> instead a skip list, if the number of memory allocations is small (e.g. >> < 100). > > You don't need a hash table either if you manage the bounce buffer list > per DMA transfer, and the simplest way to achieve that is to move the > bounce buffer from exec.c to dma-helpers.c entirely. > > The patch could first introduce address_space_map_direct that never uses > the bounce buffer. dma-helpers.c can call address_space_map_direct and, > if it fails, proceed to allocate (and fill if writing to the device) a > bounce buffer. You mean that address_space_map_direct() is a copy of the address_space_map() code, minus the bounce buffer part which will be handled in dma-helpers.c? If so, I agree. Also, the buffer should be removed from address_space_unmap. > Since the QEMUSGList is mapped and unmapped > beginning-to-end, you can just use a FIFO queue. The FIFO queue stores > a (QEMUSGList, buffer) tuple. I suppose that this queue is stored in the dbs structure of dma-helpers? If so, I agree. > When unmapping a QEMUSGList you check if > it matches the head of the queue; if it does, you write back the > contents of the bounce buffer (for reads from the device) and free it. > If it doesn't match, you call address_space_unmap. Agree. > Then, once the bounce buffer is implemented within dma-helpers.c, you > remove address_space_map and rename address_space_map_direct to > address_space_map. cpu_register_map_client goes away. What about the other users of address_space_map()? Is the bounce buffer used only for DMA operations? If so, I agree. Else, we need a fallback. > The unbounded memory allocation can be avoided by bounding the number of > entries in the queue. Agree. Thanks, Alex