On Wed, Jul 12, 2017 at 04:03:57PM +0200, Paolo Bonzini wrote: > On 12/07/2017 03:07, Fam Zheng wrote: > > On Tue, 07/11 12:28, Paolo Bonzini wrote: > >> On 11/07/2017 12:05, Stefan Hajnoczi wrote: > >>> On Mon, Jul 10, 2017 at 05:08:56PM +0200, Paolo Bonzini wrote: > >>>> On 10/07/2017 17:07, Stefan Hajnoczi wrote: > >>>>> On Wed, Jul 05, 2017 at 09:36:32PM +0800, Fam Zheng wrote: > >>>>>> Allow block driver to map and unmap a buffer for later I/O, as a > >>>>>> performance > >>>>>> hint. > >>>>> The name blk_dma_map() is confusing since other "dma" APIs like > >>>>> dma_addr_t and dma_blk_io() deal with guest physical addresses instead > >>>>> of host addresses. They are about DMA to/from guest RAM. > >>>>> > >>>>> Have you considered hiding this cached mapping in block/nvme.c so that > >>>>> it isn't exposed? block/nvme.c could keep the last buffer mapped and > >>>>> callers would get the performance benefit without a new blk_dma_map() > >>>>> API. > >>>> > >>>> One buffer is enough for qemu-img bench, but not for more complex cases > >>>> (e.g. fio). > >>> > >>> I don't see any other blk_dma_map() callers. > >> > >> Indeed, the fio plugin is not part of this series, but it also used > >> blk_dma_map. Without it, performance is awful. > > > > How many buffers does fio use, typically? If it's not too many, > > block/nvme.c can > > cache the last N buffers. I'm with Stefan that hiding the mapping logic from > > block layer callers makes a nicer API, especially such that qemu-img is much > > easier to maintain good performance across subcommmands. > > It depends on the queue depth. > > I think the API addition is necessary, otherwise we wouldn't have added > the RAMBlockNotifier which is a layering violation that does the same > thing (create permanent HVA->IOVA mappings). In fact, the > RAMBlockNotifier could be moved out of nvme.c and made to use > blk_dma_map/unmap, though I'm not proposing to do it now. > > I don't think qemu-img convert and dd are impacted by IOMMU map/unmap as > heavily as bench, because they operate with queue depth 1. But adding > map/unmap there would not be hard.
I'm not against an API existing for this. I would just ask: 1. It's documented so the purpose and semantics are clear. 2. The name cannot be confused with dma-helpers.c APIs. Maybe blk_register_buf() or blk_add_buf_hint()?
signature.asc
Description: PGP signature
