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
