pci-dma-api-v1

RFC In review Andrea Arcangeli 5 participants 32 messages Nov 27, 2008 – Dec 2, 2008 Kbuild

lore

Share & tools

lore

Andrea Arcangeli

author

2008-11-27 12:35 UTC #0 lore raw

RFC 1/2 pci-dma-api-v1

Hello everyone,

One major limitation for KVM today is the lack of a proper way to write drivers in a way that allows the host OS to use direct DMA to the guest physical memory to avoid any intermediate copy. The only API provided to drivers seems to be the cpu_physical_memory_rw and that enforces all drivers to bounce and trash cpu caches and be memory bound. This new DMA API instead allows drivers to use a pci_dma_sg method for SG I/O that will translate the guest physical addresses to host virutal addresses and it will call two operation, one is a submit method and one is the complete method. The pci_dma_sg may have to bounce buffer internally and to limit the max bounce size it may have to submit I/O in pieces with multiple submit calls. The patch adapts the ide.c HD driver to use this. Once cdrom is converted too dma_buf_rw can be eliminated. As you can see the new ide_dma_submit and ide_dma_complete code is much more readable than the previous rearming callback.

This is only tested with KVM so far but qemu builds, in general there's nothing kvm specific here (with the exception of a single kvm_enabled), so it should all work well for both.

All we care about is the performance of the direct path, so I tried to avoid dynamic allocations there to avoid entering glibc, the current logic doesn't satisfy me yet but it should be at least faster than calling malloc (but I'm still working on it to avoid memory waste to detect when more than one iov should be cached). But in case of instabilities I recommend first thing to set MAX_IOVEC_IOVCNT 0 to disable that logic ;). I recommend to test with DEBUG_BOUNCE and with a 512 max bounce buffer too. It's running stable in all modes so far. However if ide.c end up calling aio_cancel things will likely fall apart but this is all because of bdrv_aio_readv/writev, and the astonishing lack of aio_readv/writev in glibc!

Once we finish fixing storage performance with a real bdrv_aio_readv/writev (now a blocker issue), a pci_dma_single can be added for zero copy networking (one NIC per VM, or VMDq, IOV etc..). The DMA API should allow for that too.

Signed-off-by Andrea Arcangeli <[email protected]>

qemu/hw/pci_dma.c new file +346 -0

new file mode 100644
index 0000000..2e5f100
@@ -0,0 +1,346 @@
+/*
+ * QEMU PCI DMA operations
+ *
+ * Copyright (c) 2008 Red Hat, Inc.
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining a copy
+ * of this software and associated documentation files (the "Software"), to deal
+ * in the Software without restriction, including without limitation the rights
+ * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
+ * copies of the Software, and to permit persons to whom the Software is
+ * furnished to do so, subject to the following conditions:
+ *
+ * The above copyright notice and this permission notice shall be included in
+ * all copies or substantial portions of the Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+ * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL
+ * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+ * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
+ * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
+ * THE SOFTWARE.
+ */
+
+#include "pci_dma.h"
+
+#define MAX_DMA_BOUNCE_BUFFER (1024*1024)
+//#define DEBUG_BOUNCE
+//#define MAX_DMA_BOUNCE_BUFFER (512)
+
+/*
+ * Too many entries will run slower and waste memory, this is really
+ * only about the fast path so it must be small, slow path is fine to
+ * allocate dynamically. Max memory used is around MAX_IOVEC_ENTRIES *
+ * MAX_IOVEC_IOVCNT * sizeof(struct iovec).
+ */
+#define MAX_IOVEC_ENTRIES 5
+
+/*
+ * Don't cache exceptionally large iovcnt used for huge DMA transfers
+ * as the DMA transfer may take much longer than malloc
+ * and huge memory could be wasted if it happens only once in a while. 
+ */
+#define MAX_IOVEC_IOVCNT 2048
+
+struct iovec_entry {
+    int iovcnt;
+    struct iovec_entry *next;
+    struct iovec iov;
+};
+
+struct iovec_entry *iovec_list;
+
+static struct iovec *qemu_get_iovec(int iovcnt)
+{
+    struct iovec_entry *entry = iovec_list;
+
+    while (entry) {
+	if (entry->iovcnt >= iovcnt)
+	    return &entry->iov;
+	entry = entry->next;
+    }
+
+    entry = qemu_malloc(sizeof(struct iovec_entry) + 
+			sizeof(struct iovec) * (iovcnt-1));
+    if (!entry)
+	return NULL;
+
+    return &entry->iov;
+}
+
+static void qemu_release_iovec(struct iovec *iov)
+{
+    struct iovec_entry *this, *min_entry = NULL, *entry = iovec_list;
+    struct iovec_entry *min_last = NULL, *last = NULL;
+    unsigned int min_iovcnt = -1, nr = 0;
+
+    if (!iov)
+	return;
+
+    this = (struct iovec_entry *)
+	(((char*)iov)-(unsigned long)(&((struct iovec_entry *)0)->iov));
+
+    if (this->iovcnt > MAX_IOVEC_IOVCNT) {
+	qemu_free(this);
+	return;
+    }
+
+    while (entry) {
+	nr += 1;
+	if ((unsigned int)entry->iovcnt < min_iovcnt) {
+	    min_entry = entry;
+	    min_last = last;
+	    min_iovcnt = entry->iovcnt;
+	}
+	last = entry;
+	entry = entry->next;
+    }
+
+    if (nr > MAX_IOVEC_ENTRIES) {
+	/* detail: replace even if it's equal as it's cache hot */
+	if (this->iovcnt < min_iovcnt)
+	    qemu_free(this);
+	else {
+	    if (min_entry == iovec_list) {
+		this->next = iovec_list->next;
+		iovec_list = this;
+	    } else {
+		min_last->next = min_entry->next;
+		this->next = iovec_list;
+		iovec_list = this;
+	    }
+	    qemu_free(min_entry);
+	}
+    } else {
+	if (!iovec_list) {
+	    this->next = NULL;
+	    iovec_list = this;
+	} else {
+	    this->next = iovec_list;
+	    iovec_list = this;
+	}
+    }
+}
+
+static struct iovec *pci_dma_sg_map_direct(QEMUPciDmaSg *sg,
+					   int iovcnt,
+					   int dma_to_memory,
+					   int alignment,
+					   size_t *len)
+{
+    int idx = 0;
+    struct iovec *dma_iov;
+    size_t _len = 0;
+
+#ifdef DEBUG_BOUNCE
+    return NULL;
+#endif
+
+    /* fixme: must not call malloc and cache them in some faster queue */
+    dma_iov = qemu_get_iovec(iovcnt);
+    if (!dma_iov)
+	goto out;
+
+    for (idx = 0; idx < iovcnt; idx++) {
+	void * addr;
+
+	if (_len + sg[idx].len <= _len)
+		goto err;
+	_len += sg[idx].len;
+
+	addr = cpu_physical_memory_can_dma(sg[idx].addr,
+					   sg[idx].len,
+					   dma_to_memory,
+					   alignment);
+	if (!addr)
+	    goto err;
+
+	dma_iov[idx].iov_base = addr;
+	dma_iov[idx].iov_len = sg[idx].len;
+    }
+
+    *len = _len;
+ out:
+    return dma_iov;
+err:
+    qemu_release_iovec(dma_iov);
+    dma_iov = NULL;
+    goto out;
+}
+
+static struct iovec *pci_dma_sg_map_bounce(QEMUPciDmaSgParam *param)
+{
+    int idx;
+    size_t len = 0;
+
+    param->curr_restart_iovcnt = param->restart_iovcnt;
+    param->curr_restart_offset = param->restart_offset;
+
+    for (idx = param->restart_iovcnt; idx < param->iovcnt; idx++) {
+	if (len & (param->alignment-1))
+	    return NULL;
+	if (len + param->sg[idx].len <= len)
+	    return NULL;
+	len += param->sg[idx].len - param->restart_offset;
+	param->restart_offset = 0;
+	if (len > MAX_DMA_BOUNCE_BUFFER) {
+	    size_t leftover = len - MAX_DMA_BOUNCE_BUFFER;
+	    param->restart_offset = param->sg[idx].len - leftover;
+	    len = MAX_DMA_BOUNCE_BUFFER;
+	    break;
+	}
+    }
+    param->restart_iovcnt = idx;
+    param->curr_len = len;
+
+    param->linearized.iov_len = len;
+    if (!param->bounce) {
+	param->bounce = qemu_memalign(param->alignment, len);
+	if (!param->bounce)
+	    return NULL;
+	param->linearized.iov_base = param->bounce;
+    }
+
+    if (!param->dma_to_memory) {
+	int idx;
+	size_t offset = 0;
+	for (idx = param->curr_restart_iovcnt;
+	     idx < param->iovcnt && offset < len; idx++) {
+	    size_t copy_len = param->sg[idx].len - param->curr_restart_offset;
+	    if (offset+copy_len > len)
+		copy_len = len;
+	    cpu_physical_memory_read(param->sg[idx].addr + 
+				     param->curr_restart_offset,
+				     param->bounce + offset,
+				     copy_len);
+	    param->curr_restart_offset = 0;
+	    offset += copy_len;
+	}
+    }
+
+    return &param->linearized;
+}
+
+static void pci_dma_sg_unmap_direct(QEMUPciDmaSgParam *param, int ret)
+{
+    if (!ret && param->dma_to_memory) {
+	int idx;
+	QEMUPciDmaSg *sg = param->sg;
+	for (idx = 0; idx < param->iovcnt; idx++)
+	    cpu_physical_memory_write_post_dma(sg[idx].addr,
+					       sg[idx].len);
+    }
+    qemu_release_iovec(param->dma_iov);
+}
+
+int pci_dma_sg_unmap_bounce(QEMUPciDmaSgParam *param, int ret)
+{
+    if (!ret && param->dma_to_memory) {
+	int idx;
+	size_t offset = 0;
+	for (idx = param->curr_restart_iovcnt;
+	     idx < param->iovcnt && offset < param->curr_len; idx++) {
+	    size_t copy_len = param->sg[idx].len - param->curr_restart_offset;
+	    if (offset+copy_len > param->curr_len)
+		copy_len = param->curr_len;
+	    cpu_physical_memory_write(param->sg[idx].addr +
+				      param->curr_restart_offset,
+				      param->bounce + offset,
+				      copy_len);
+	    param->curr_restart_offset = 0;
+	    offset += copy_len;
+	}
+    }
+    if (param->restart_iovcnt == param->iovcnt || ret) {
+	qemu_free(param->bounce);
+	return 0;
+    }
+    return 1;
+}
+
+static void pci_dma_sg_cb(void *opaque, int ret)
+{
+    QEMUPciDmaSgParam *param = opaque;
+    int restart = 0;
+
+    if (!param->bounce)
+	pci_dma_sg_unmap_direct(param, ret);
+    else
+	restart = pci_dma_sg_unmap_bounce(param, ret);
+
+    if (restart) {
+	ret = -1;
+	param->dma_iov = pci_dma_sg_map_bounce(param);
+	if (!param->dma_iov)
+	    goto out_free;
+	ret = param->pci_dma_sg_submit(param->pci_dma_sg_opaque,
+				       param->dma_iov, 1,
+				       param->curr_len,
+				       pci_dma_sg_cb,
+				       param);
+    }
+    if (ret || !restart) {
+    out_free:
+	param->pci_dma_sg_complete(param->pci_dma_sg_opaque, ret);
+	qemu_free(param);
+    }
+}
+
+/* PCIDevice is there in case we want to emulate an iommu later */
+void pci_dma_sg(PCIDevice *pci_dev,
+		QEMUPciDmaSg *sg, int iovcnt,
+		QEMUPciDmaSgSubmit pci_dma_sg_submit,
+		QEMUPciDmaSgComplete pci_dma_sg_complete,
+		void *pci_dma_sg_opaque,
+		int dma_to_memory, int alignment)
+{
+    int ret = -1;
+    QEMUPciDmaSgParam *param;
+
+    if ((unsigned int) dma_to_memory > 1)
+	goto err;
+    if (alignment < 0)
+	goto err;
+    if (iovcnt < 1)
+	goto err;
+
+    param = qemu_malloc(sizeof(QEMUPciDmaSgParam));
+    if (!param)
+	goto err;
+
+    param->pci_dma_sg_submit = pci_dma_sg_submit;
+    param->pci_dma_sg_complete = pci_dma_sg_complete;
+    param->pci_dma_sg_opaque = pci_dma_sg_opaque;
+    param->dma_to_memory = dma_to_memory;
+    param->alignment = alignment;
+    param->bounce = NULL;
+    param->sg = sg;
+    param->iovcnt = iovcnt;
+    param->restart_offset = param->restart_iovcnt = 0;
+
+    /* map the sg */
+    param->dma_iov = pci_dma_sg_map_direct(sg, iovcnt,
+					   dma_to_memory, alignment,
+					   &param->curr_len);
+    if (!param->dma_iov) {
+	param->dma_iov = pci_dma_sg_map_bounce(param);
+	if (!param->dma_iov)
+	    goto out_free;
+	iovcnt = 1;
+    }
+
+    /* run the I/O */
+    ret = pci_dma_sg_submit(pci_dma_sg_opaque,
+			    param->dma_iov, iovcnt, param->curr_len,
+			    pci_dma_sg_cb,
+			    param);
+    if (ret)
+    out_free:
+	pci_dma_sg_cb(param, ret);
+    return;
+
+ err:
+    pci_dma_sg_complete(pci_dma_sg_opaque, ret);
+    return;
+}

qemu/hw/pci_dma.h new file +47 -0

47 lines not on this page · raw message

block_int.h +2 -0

Index: block_int.h
===================================================================
@@ -55,6 +55,8 @@
         int64_t sector_num, const uint8_t *buf, int nb_sectors,
         BlockDriverCompletionFunc *cb, void *opaque);
     void (*bdrv_aio_cancel)(BlockDriverAIOCB *acb);
+    BlockDriverAIOIOV *bdrv_aio_readv;
+    BlockDriverAIOIOV *bdrv_aio_writev;
     int aiocb_size;
 
     const char *protocol_name;

Makefile.target +5 -5

Index: Makefile.target
===================================================================
@@ -659,7 +659,7 @@
 
 ifeq ($(TARGET_BASE_ARCH), i386)
 # Hardware support
-OBJS+= ide.o pckbd.o ps2.o vga.o $(SOUND_HW) dma.o
+OBJS+= ide.o pckbd.o ps2.o vga.o $(SOUND_HW) dma.o pci_dma.o
 OBJS+= fdc.o mc146818rtc.o serial.o i8259.o i8254.o pcspk.o pc.o
 OBJS+= cirrus_vga.o apic.o parallel.o acpi.o piix_pci.o
 OBJS+= usb-uhci.o vmmouse.o vmport.o vmware_vga.o

@@ -668,7 +668,7 @@
 ifeq ($(TARGET_BASE_ARCH), ppc)
 CPPFLAGS += -DHAS_AUDIO -DHAS_AUDIO_CHOICE
 # shared objects
-OBJS+= ppc.o ide.o vga.o $(SOUND_HW) dma.o openpic.o
+OBJS+= ppc.o ide.o vga.o $(SOUND_HW) dma.o openpic.o pci_dma.o
 # PREP target
 OBJS+= pckbd.o ps2.o serial.o i8259.o i8254.o fdc.o m48t59.o mc146818rtc.o
 OBJS+= prep_pci.o ppc_prep.o

@@ -686,7 +686,7 @@
 OBJS+= mips_timer.o mips_int.o dma.o vga.o serial.o i8254.o i8259.o rc4030.o
 OBJS+= g364fb.o jazz_led.o
 OBJS+= ide.o gt64xxx.o pckbd.o ps2.o fdc.o mc146818rtc.o usb-uhci.o acpi.o ds1225y.o
-OBJS+= piix_pci.o parallel.o cirrus_vga.o pcspk.o $(SOUND_HW)
+OBJS+= piix_pci.o parallel.o cirrus_vga.o pcspk.o pci_dma.o $(SOUND_HW)
 OBJS+= mipsnet.o
 OBJS+= pflash_cfi01.o
 CPPFLAGS += -DHAS_AUDIO -DHAS_AUDIO_CHOICE

@@ -710,7 +710,7 @@
 else
 OBJS+= sun4m.o tcx.o pcnet.o iommu.o m48t59.o slavio_intctl.o
 OBJS+= slavio_timer.o slavio_serial.o slavio_misc.o fdc.o sparc32_dma.o
-OBJS+= cs4231.o ptimer.o eccmemctl.o sbi.o sun4c_intctl.o
+OBJS+= cs4231.o ptimer.o eccmemctl.o sbi.o sun4c_intctl.o pci_dma.o
 endif
 endif
 ifeq ($(TARGET_BASE_ARCH), arm)

@@ -731,7 +731,7 @@
 OBJS+= nseries.o blizzard.o onenand.o vga.o cbus.o tusb6010.o usb-musb.o
 OBJS+= tsc2005.o bt-hci-csr.o
 OBJS+= mst_fpga.o mainstone.o
-OBJS+= musicpal.o pflash_cfi02.o
+OBJS+= musicpal.o pflash_cfi02.o pci_dma.o
 CPPFLAGS += -DHAS_AUDIO
 endif
 ifeq ($(TARGET_BASE_ARCH), sh4)

exec.c +107 -2

127 lines not on this page · raw message

block.c +44 -0

51 lines not on this page · raw message

block.h +13 -0

Index: block.h
===================================================================
@@ -83,6 +83,13 @@
 /* async block I/O */
 typedef struct BlockDriverAIOCB BlockDriverAIOCB;
 typedef void BlockDriverCompletionFunc(void *opaque, int ret);
+typedef BlockDriverAIOCB *BlockDriverAIOIOV(BlockDriverState *bs,
+					    int64_t sector_num,
+					    struct iovec *iov,
+					    int iovnct,
+					    size_t len,
+					    BlockDriverCompletionFunc *cb,
+					    void *opaque);
 
 BlockDriverAIOCB *bdrv_aio_read(BlockDriverState *bs, int64_t sector_num,
                                 uint8_t *buf, int nb_sectors,

@@ -91,6 +98,12 @@
                                  const uint8_t *buf, int nb_sectors,
                                  BlockDriverCompletionFunc *cb, void *opaque);
 void bdrv_aio_cancel(BlockDriverAIOCB *acb);
+BlockDriverAIOCB *bdrv_aio_readv(BlockDriverState *bs, int64_t sector_num,
+				 struct iovec *iov, int iovnct, size_t len,
+				 BlockDriverCompletionFunc *cb, void *opaque);
+BlockDriverAIOCB *bdrv_aio_writev(BlockDriverState *bs, int64_t sector_num,
+				  struct iovec *iov, int iovnct, size_t len,
+				  BlockDriverCompletionFunc *cb, void *opaque);
 
 int qemu_key_check(BlockDriverState *bs, const char *name);

ide.c +86 ~~-99~~

250 lines not on this page · raw message

cpu-all.h +8 -3

Index: cpu-all.h
===================================================================
@@ -891,14 +891,19 @@
 CPUReadMemoryFunc **cpu_get_io_memory_read(int io_index);
 
 void cpu_physical_memory_rw(target_phys_addr_t addr, uint8_t *buf,
-                            int len, int is_write);
+                            size_t len, int is_write);
+void cpu_physical_memory_write_post_dma(target_phys_addr_t addr,
+					size_t len);
+uint8_t *cpu_physical_memory_can_dma(target_phys_addr_t addr,
+				     size_t len, int is_write,
+				     int alignment);
 static inline void cpu_physical_memory_read(target_phys_addr_t addr,
-                                            uint8_t *buf, int len)
+                                            uint8_t *buf, size_t len)
 {
     cpu_physical_memory_rw(addr, buf, len, 0);
 }
 static inline void cpu_physical_memory_write(target_phys_addr_t addr,
-                                             const uint8_t *buf, int len)
+                                             const uint8_t *buf, size_t len)
 {
     cpu_physical_memory_rw(addr, (uint8_t *)buf, len, 1);
 }

Andrea Arcangeli

author

2008-11-27 12:43 UTC #1 lore raw

RFC 2/2 bdrv_aio_readv/writev_em

Hello,

this is the emulated bdrv_aio_readv/writev pure hack to be able to test the dma api in previous patch.

About the real thing there are two ways to go:

pthread_create() and do aio with pthreads by calling writev by hand.

Use kernel based linux aio (I think it's much better as it won't screwup with contiguous I/O, and it handles o_direct random writes and random reads by keeping the lowlevel I/O pipeline full without threads but by just queuing _in_order_ [in order only from the point of view of the I/O scheduler of course] and asynchronously the commands of every different direct-io aio_readv/writev in the lowlevel storage queue without needing any scheduler and thread synchronization involvement).

So who's going to add bdrv_aio_readv/writev instead of the below aberration that breaks on backend not supporting aio and breaks with bdrv_aio_cancel too, besides being horribly slow and making direct path slower than the bounce path?

Signed-off-by Andrea Arcangeli <[email protected]>

block.c +84 -0

Index: block.c
===================================================================
@@ -53,6 +53,20 @@
                         uint8_t *buf, int nb_sectors);
 static int bdrv_write_em(BlockDriverState *bs, int64_t sector_num,
                          const uint8_t *buf, int nb_sectors);
+static BlockDriverAIOCB *bdrv_aio_readv_em(BlockDriverState *bs,
+					   int64_t sector_num,
+					   struct iovec *iov,
+					   int iovnct,
+					   size_t len,
+					   BlockDriverCompletionFunc *cb,
+					   void *opaque);
+static BlockDriverAIOCB *bdrv_aio_writev_em(BlockDriverState *bs,
+					    int64_t sector_num,
+					    struct iovec *iov,
+					    int iovnct,
+					    size_t len,
+					    BlockDriverCompletionFunc *cb,
+					    void *opaque);
 
 BlockDriverState *bdrv_first;

@@ -135,6 +149,8 @@
         /* add synchronous IO emulation layer */
         bdrv->bdrv_read = bdrv_read_em;
         bdrv->bdrv_write = bdrv_write_em;
+        bdrv->bdrv_aio_readv = bdrv_aio_readv_em;
+        bdrv->bdrv_aio_writev = bdrv_aio_writev_em;
     }
     bdrv->next = first_drv;
     first_drv = bdrv;

@@ -1341,6 +1401,74 @@
     qemu_aio_release(acb);
 }
 
+static void bdrv_aio_iov_bh_cb(void *opaque)
+{
+    BlockDriverAIOCBSync *acb = opaque;
+    acb->common.cb(acb->common.opaque, acb->ret);
+    qemu_bh_delete(acb->bh);
+    qemu_free(acb);
+}
+
+static BlockDriverAIOCB *bdrv_aio_readv_em(BlockDriverState *bs,
+					   int64_t sector_num,
+					   struct iovec *iov,
+					   int iovcnt,
+					   size_t len,
+					   BlockDriverCompletionFunc *cb,
+					   void *opaque)
+{
+    BlockDriverAIOCBSync *acb;
+    int ret = -1, idx;
+
+    for (idx = 0; idx < iovcnt; idx++) {
+	size_t sectors = iov[idx].iov_len >> SECTOR_BITS;
+	ret = bdrv_read(bs, sector_num, iov[idx].iov_base, sectors);
+	if (ret)
+	    break;
+	sector_num += sectors;
+    }
+    acb = qemu_mallocz(sizeof(BlockDriverAIOCBSync));
+    if (!acb)
+            return NULL;
+    acb->common.bs = bs;
+    acb->common.cb = cb;
+    acb->common.opaque = opaque;
+    acb->bh = qemu_bh_new(bdrv_aio_iov_bh_cb, acb);
+    acb->ret = ret;
+    qemu_bh_schedule(acb->bh);
+    return &acb->common;
+}
+
+static BlockDriverAIOCB *bdrv_aio_writev_em(BlockDriverState *bs,
+					    int64_t sector_num,
+					    struct iovec *iov,
+					    int iovcnt,
+					    size_t len,
+					    BlockDriverCompletionFunc *cb,
+					    void *opaque)
+{
+    BlockDriverAIOCBSync *acb;
+    int ret = -1, idx;
+
+    for (idx = 0; idx < iovcnt; idx++) {
+	size_t sectors = iov[idx].iov_len >> SECTOR_BITS;
+	ret = bdrv_write(bs, sector_num, iov[idx].iov_base, sectors);
+	if (ret)
+	    break;
+	sector_num += sectors;
+    }
+    acb = qemu_mallocz(sizeof(BlockDriverAIOCBSync));
+    if (!acb)
+            return NULL;
+    acb->common.bs = bs;
+    acb->common.cb = cb;
+    acb->common.opaque = opaque;
+    acb->bh = qemu_bh_new(bdrv_aio_iov_bh_cb, acb);
+    acb->ret = ret;
+    qemu_bh_schedule(acb->bh);
+    return &acb->common;
+}
+
 /**************************************************************/
 /* sync block device emulation */

Blue Swirl

2008-11-27 19:14 UTC #3 lore raw

In reply to Andrea Arcangeli

Quoted 34 lines · Andrea Arcangeli

Hello everyone,
One major limitation for KVM today is the lack of a proper way to write drivers in a way that allows the host OS to use direct DMA to the guest physical memory to avoid any intermediate copy. The only API provided to drivers seems to be the cpu_physical_memory_rw and that enforces all drivers to bounce and trash cpu caches and be memory bound. This new DMA API instead allows drivers to use a pci_dma_sg method for SG I/O that will translate the guest physical addresses to host virutal addresses and it will call two operation, one is a submit method and one is the complete method. The pci_dma_sg may have to bounce buffer internally and to limit the max bounce size it may have to submit I/O in pieces with multiple submit calls. The patch adapts the ide.c HD driver to use this. Once cdrom is converted too dma_buf_rw can be eliminated. As you can see the new ide_dma_submit and ide_dma_complete code is much more readable than the previous rearming callback.
This is only tested with KVM so far but qemu builds, in general there's nothing kvm specific here (with the exception of a single kvm_enabled), so it should all work well for both.
All we care about is the performance of the direct path, so I tried to avoid dynamic allocations there to avoid entering glibc, the current logic doesn't satisfy me yet but it should be at least faster than calling malloc (but I'm still working on it to avoid memory waste to detect when more than one iov should be cached). But in case of instabilities I recommend first thing to set MAX_IOVEC_IOVCNT 0 to disable that logic ;). I recommend to test with DEBUG_BOUNCE and with a 512 max bounce buffer too. It's running stable in all modes so far. However if ide.c end up calling aio_cancel things will likely fall apart but this is all because of bdrv_aio_readv/writev, and the astonishing lack of aio_readv/writev in glibc!

Quoted 4 lines

Once we finish fixing storage performance with a real bdrv_aio_readv/writev (now a blocker issue), a pci_dma_single can be added for zero copy networking (one NIC per VM, or VMDq, IOV etc..). The DMA API should allow for that too.

The previous similar attempt by Anthony for generic DMA using vectored IO was abandoned because the malloc/free overhead was more than the performance gain. Have you made any performance measurements? How does this version compare to the previous ones?

I think the pci_ prefix can be removed, there is little PCI specific.

For Sparc32 IOMMU (and probably other IOMMUS), it should be possible to register a function used in place of cpu_physical_memory_rw, c_p_m_can_dma etc. The goal is that it should be possible to stack the DMA resolvers (think of devices behind a number of buses).

Andrea Arcangeli

author

2008-11-28 01:56 UTC #4 lore raw

Quoted 2 lines · Blue Swirl

The previous similar attempt by Anthony for generic DMA using vectored IO was abandoned because the malloc/free overhead was more than the

Even if there were dynamic allocations in the fast path, the overhead of malloc/free is nothing if compared to running and waiting a host kernel syscall to return every 4k, not to tell with O_DIRECT enabled which is the whole point of having a direct-dma API that truly doesn't pollute the cache. With O_DIRECT, without a real readv/writev I/O performance would be destroyed going down to something like 10M/sec even on the fastest storage/CPU/ram combinations.

So the question is how those benchmarks were run, with or without a real readv/writev and with or without O_DIRECT to truly eliminate all CPU cache pollution out of the memory copies?

About malloc, all we care about is the direct-io fast path, and with my patch there is no allocation whatsoever in the fast path. About the bounce layer, that is there for correctness only (purely to do DMA to non-RAM or in non-linear RAM ranges with non-RAM holes in between) and we don't care about it in performance terms.

Quoted 2 lines

performance gain. Have you made any performance measurements? How does this version compare to the previous ones?

I run some minor benchmark but it's basically futile to benchmark with the bdrv_aio_readv/writev_em.

Quoted 1 line

I think the pci_ prefix can be removed, there is little PCI specific.

Adding the pci_ prefix looked a requirement in naming terms from previous threads on the topic. Before I learnt about it, I didn't want to have a pci_ prefix too so I can certainly agree with you ;).

There's is nothing PCI specific so far. Anyway this is just a naming matter, it's up to you to decide what you like :).

Quoted 4 lines

For Sparc32 IOMMU (and probably other IOMMUS), it should be possible to register a function used in place of cpu_physical_memory_rw, c_p_m_can_dma etc. The goal is that it should be possible to stack the DMA resolvers (think of devices behind a number of buses).

The hardware thing being emulated in the real world wouldn't attach to both buses I think, hence you can specify it in the driver what kind of iommu it has (then behind it you can emulate whatever hardware you want but still the original device was pci or not-pci). I personally don't see much difference as renaming later wouldn't be harder than a sed script...

Jamie Lokier

2008-11-28 11:09 UTC #2 lore raw

Re: [Qemu-devel] [RFC 2/2] bdrv_aio_readv/writev_em

In reply to Andrea Arcangeli

Quoted 10 lines · Andrea Arcangeli

pthread_create() and do aio with pthreads by calling writev by hand.
Use kernel based linux aio (I think it's much better as it won't screwup with contiguous I/O, and it handles o_direct random writes and random reads by keeping the lowlevel I/O pipeline full without threads but by just queuing _in_order_ [in order only from the point of view of the I/O scheduler of course] and asynchronously the commands of every different direct-io aio_readv/writev in the lowlevel storage queue without needing any scheduler and thread synchronization involvement).

The good things about linux-aio are as you described.

The bad thing is that I'm told linux-aio can block sometimes. Apart from memory pressure (which can block the calling process anyway), an aio implementation which can block is not so good.

Doing it in a pthread is less good for a lot of reasons. On the other hand, I/O in a pthread should never block the main thread. We discussed the "in order" part a while ago, and several people pointed out that once there are a few I/Os in flight, the order should stabilise automatically, but perhaps that's not so good in transient situations.

(Ideally we'd use scheduler activations (syslets!) to avoid blocking in any kernel I/O path no matter how complex, while keeping aio ordering and avoiding unnecessary task switching. I haven't heard much about progress on this since earlier this year...)

-- Jamie

Blue Swirl

2008-11-28 17:59 UTC #5 lore raw

In reply to Andrea Arcangeli

Quoted 13 lines · Andrea Arcangeli

Quoted 2 lines · Blue Swirl
The previous similar attempt by Anthony for generic DMA using vectored IO was abandoned because the malloc/free overhead was more than the
Even if there were dynamic allocations in the fast path, the overhead
of malloc/free is nothing if compared to running and waiting a host kernel syscall to return every 4k, not to tell with O_DIRECT enabled which is the whole point of having a direct-dma API that truly doesn't pollute the cache. With O_DIRECT, without a real readv/writev I/O performance would be destroyed going down to something like 10M/sec even on the fastest storage/CPU/ram combinations.

Quoted 3 lines

So the question is how those benchmarks were run, with or without a real readv/writev and with or without O_DIRECT to truly eliminate all CPU cache pollution out of the memory copies?

I don't know, here's a pointer:
http://lists.gnu.org/archive/html/qemu-devel/2008-08/msg00092.html

Quoted 32 lines

About malloc, all we care about is the direct-io fast path, and with my patch there is no allocation whatsoever in the fast path. About the bounce layer, that is there for correctness only (purely to do DMA to non-RAM or in non-linear RAM ranges with non-RAM holes in between) and we don't care about it in performance terms.
Quoted 2 lines
performance gain. Have you made any performance measurements? How does this version compare to the previous ones?
I run some minor benchmark but it's basically futile to benchmark with
the bdrv_aio_readv/writev_em.
Quoted 1 line
I think the pci_ prefix can be removed, there is little PCI specific.
Adding the pci_ prefix looked a requirement in naming terms from
previous threads on the topic. Before I learnt about it, I didn't want to have a pci_ prefix too so I can certainly agree with you ;).
There's is nothing PCI specific so far. Anyway this is just a naming matter, it's up to you to decide what you like :).
Quoted 4 lines
For Sparc32 IOMMU (and probably other IOMMUS), it should be possible to register a function used in place of cpu_physical_memory_rw, c_p_m_can_dma etc. The goal is that it should be possible to stack the DMA resolvers (think of devices behind a number of buses).

Quoted 6 lines

The hardware thing being emulated in the real world wouldn't attach to
both buses I think, hence you can specify it in the driver what kind of iommu it has (then behind it you can emulate whatever hardware you want but still the original device was pci or not-pci). I personally don't see much difference as renaming later wouldn't be harder than a sed script...

Sorry, my description seems to have lead you to a totally wrong track. I meant this scenario: device (Lance Ethernet) -> DMA controller (MACIO) -> IOMMU -> physical memory. (In this case vectored DMA won't be useful since there is byte swapping involved, but serves as an example about generic DMA). At each step the DMA address is rewritten. It would be nice if the interface between Lance and DMA, DMA and IOMMU and IOMMU and memory was the same.

Here's some history, please have a look.

My first failed attempt:
http://lists.gnu.org/archive/html/qemu-devel/2007-08/msg00179.html

My second failed rough sketch:
http://lists.gnu.org/archive/html/qemu-devel/2007-10/msg00626.html

Anthony's version:
http://lists.gnu.org/archive/html/qemu-devel/2008-03/msg00474.html

Anthony's second version:
http://lists.gnu.org/archive/html/qemu-devel/2008-04/msg00077.html

Andrea Arcangeli

author

2008-11-28 18:50 UTC #6 lore raw

Quoted 2 lines · Blue Swirl

I don't know, here's a pointer:
http://lists.gnu.org/archive/html/qemu-devel/2008-08/msg00092.html

I'm in total agreement with it. The missing "proper vectored AIO operations" are bdrv_aio_readv/writev ;).

I wonder how can possibly aio_readv/writev be missing in posix aio? Unbelievable. It'd be totally trivial to add those to glibc, much easier infact than to pthread_create by hand, but how can we add a dependency on a certain glibc version? Ironically it'll be more user-friendly to add dependency on linux kernel-aio implementation that is already available for ages and it's guaranteed to run faster (or at least not slower).

Quoted 7 lines

Sorry, my description seems to have lead you to a totally wrong track. I meant this scenario: device (Lance Ethernet) -> DMA controller (MACIO) -> IOMMU -> physical memory. (In this case vectored DMA won't be useful since there is byte swapping involved, but serves as an example about generic DMA). At each step the DMA address is rewritten. It would be nice if the interface between Lance and DMA, DMA and IOMMU and IOMMU and memory was the same.

No problem. So you think I should change it to qemu_dma_sg instead of pci_dma_sg? We can decide it later, but surely we can think about it in the meantime ;).

Quoted 5 lines

Here's some history, please have a look.
My first failed attempt:
http://lists.gnu.org/archive/html/qemu-devel/2007-08/msg00179.html

Quoted 8 lines

My second failed rough sketch:
http://lists.gnu.org/archive/html/qemu-devel/2007-10/msg00626.html
Anthony's version:
http://lists.gnu.org/archive/html/qemu-devel/2008-03/msg00474.html
Anthony's second version:
http://lists.gnu.org/archive/html/qemu-devel/2008-04/msg00077.html

Thanks a lot for the pointers.

BTW, lots of the credit in the design of my current implementation goes to Avi, I forgot to mention it in previous emails.

The little cache layer I added at the last minute was very buggy so don't look much of it, just assume it works when reading the patch ;). I think I fixed it now in my tree, so next version will be much better. I've also noticed some problems with windows (I didn't test windows before posting), those aren't related to the cache layer as I added a #define to disable it and replace it with malloc/free. But that's not the cache layer, as soon as windows runs completely flawlessy I post an update.

The iov cache layer is now also improved so that it only caches at most N elements, where N is the max number of simultaneous in-flight dma that ever happened during the runtime, so it's a bit smarter than a generic slab cache.

Last but not the least there's still one malloc in the direct fast path but the plan is to eliminate that too, by embedding the iov inside the param (keeping it at the end of the struct like if I'd be extending the linear_iov), so then the cache layer will handle it all and there will be zero mallocs. The bounce path will be penalized as it'll have to allocate the direct-iov too, but we don't care.

Thanks!
Andrea

Blue Swirl

2008-11-28 19:03 UTC #7 lore raw

Quoted 8 lines · Andrea Arcangeli

Quoted 2 lines · Blue Swirl
I don't know, here's a pointer:
http://lists.gnu.org/archive/html/qemu-devel/2008-08/msg00092.html
I'm in total agreement with it. The missing "proper vectored AIO
operations" are bdrv_aio_readv/writev ;).

Quoted 7 lines

I wonder how can possibly aio_readv/writev be missing in posix aio? Unbelievable. It'd be totally trivial to add those to glibc, much easier infact than to pthread_create by hand, but how can we add a dependency on a certain glibc version? Ironically it'll be more user-friendly to add dependency on linux kernel-aio implementation that is already available for ages and it's guaranteed to run faster (or at least not slower).

There's also lio_listio that provides for vectored AIO.

Quoted 9 lines

Quoted 7 lines
Sorry, my description seems to have lead you to a totally wrong track. I meant this scenario: device (Lance Ethernet) -> DMA controller (MACIO) -> IOMMU -> physical memory. (In this case vectored DMA won't be useful since there is byte swapping involved, but serves as an example about generic DMA). At each step the DMA address is rewritten. It would be nice if the interface between Lance and DMA, DMA and IOMMU and IOMMU and memory was the same.

Quoted 3 lines

No problem. So you think I should change it to qemu_dma_sg instead of
pci_dma_sg? We can decide it later, but surely we can think about it in the meantime ;).

Yes.

Quoted 8 lines

Quoted 8 lines
Here's some history, please have a look.
My first failed attempt:
http://lists.gnu.org/archive/html/qemu-devel/2007-08/msg00179.html
My second failed rough sketch:
http://lists.gnu.org/archive/html/qemu-devel/2007-10/msg00626.html

Quoted 8 lines

Quoted 5 lines
Anthony's version:
http://lists.gnu.org/archive/html/qemu-devel/2008-03/msg00474.html
Anthony's second version:
http://lists.gnu.org/archive/html/qemu-devel/2008-04/msg00077.html
Thanks a lot for the pointers.

Perhaps you could point out why the previous attempts failed, but yours won't? ;-)

Jamie Lokier

2008-11-28 19:18 UTC #8 lore raw

Quoted 9 lines · Blue Swirl

Quoted 7 lines
I wonder how can possibly aio_readv/writev be missing in posix aio? Unbelievable. It'd be totally trivial to add those to glibc, much easier infact than to pthread_create by hand, but how can we add a dependency on a certain glibc version? Ironically it'll be more user-friendly to add dependency on linux kernel-aio implementation that is already available for ages and it's guaranteed to run faster (or at least not slower).
There's also lio_listio that provides for vectored AIO.

I think lio_listio is the missing aio_readv/writev.

It's more versatile, and that'll by why POSIX never bothered with aio_readv/writev.

Doesn't explain why they didn't _start_ with aio_readv before inventing lio_listio, but there you go. Unix history.

-- Jamie

Avi Kivity

2008-11-29 19:48 UTC #27 lore raw

In reply to Blue Swirl

Quoted 6 lines · Blue Swirl

The previous similar attempt by Anthony for generic DMA using vectored IO was abandoned because the malloc/free overhead was more than the performance gain. Have you made any performance measurements? How does this version compare to the previous ones?

The pointers you gave don't blame malloc/free, instead the lack of aio readv/writev.

Since Andrea's patches contain emulation for aio readv/writev, the performance degradation will not occur (though we will not see the benefit either).

I doubt you can get measure malloc overhead with anything less a thousand disks (even there, other overheads are likely to drown that malloc).

Signature · 2 lines

I have a truly marvellous patch that fixes the bug which this
signature is too narrow to contain.

Avi Kivity

2008-11-29 19:49 UTC #9 lore raw

In reply to Jamie Lokier

Quoted 2 lines · Jamie Lokier

I think lio_listio is the missing aio_readv/writev.

No, it isn't. A 1MB read divided into 4k pages will have 256 completions.

Signature · 2 lines

I have a truly marvellous patch that fixes the bug which this
signature is too narrow to contain.

Andrea Arcangeli

author

2008-11-30 17:20 UTC #10 lore raw

In reply to Jamie Lokier

Quoted 11 lines · Jamie Lokier

Quoted 9 lines · Blue Swirl
Quoted 7 lines
I wonder how can possibly aio_readv/writev be missing in posix aio? Unbelievable. It'd be totally trivial to add those to glibc, much easier infact than to pthread_create by hand, but how can we add a dependency on a certain glibc version? Ironically it'll be more user-friendly to add dependency on linux kernel-aio implementation that is already available for ages and it's guaranteed to run faster (or at least not slower).
There's also lio_listio that provides for vectored AIO.

Quoted 7 lines

I think lio_listio is the missing aio_readv/writev.
It's more versatile, and that'll by why POSIX never bothered with aio_readv/writev.
Doesn't explain why they didn't _start_ with aio_readv before inventing lio_listio, but there you go. Unix history.

Well, I before grepped for readv or writev syscalls inside the glibc-2.6.1/sysdeps/pthread and there was nothing there, so lio_listio doesn't seem to be helpful at all. If that was a _kernel_ API then the kernel could see the whole queue immediately and coalesce all oustanding contiguous I/O in a single DMA operation, but the userland queue here will not be visible to the kernel. So unless we can execute the readv and writev syscalls, O_DIRECT performance with direct DMA API will be destroyed compared to bounce buffering because the guest OS will submit large DMA operations that will be executed as 4k DMA operation in the storage hardware, the memcpy overhead that we're eliminating is minor compared to such a major I/O bottleneck with qemu cache=off.

The only way we could possibly use lio_listio, would be to improve glibc so the lio_listio op will be smart enough to call readv/writev if it finds contiguous I/O being queued, but overall this would be still largely inefficient. If you check the dma api, I'm preparing struct iovec *iov ready to submit to the kernel either through the inexistent aio_readv/writev or with the kernel-API IOCB_CMD_PREADV/WRITEV (they obviously both take the well defined struct iovec as param so there's zero overhead).

So even if we improve lio_listio, lio_listio would introduce artificial splitting-recolaescing overhead just because of its weird API. Entirely different would be if lio_listio would resemble the kernel sys_iosubmit API and had a PREADV/WRITEV type to submit iovecs, but this only has a LIO_READ/WRITE, no sign of LIO_READV/WRITEV unfortunately :(. Amittedly it's not so common having to use readv/writev on contiguous I/O but the emulated DMA with SG truly requires this. Anything that can't handle a native iovec we can't use.

Likely we'll have to add a pthread_create based our own aio implementation for non-linux and kernel-AIO for linux, and get rid of librt as a whole. It's pointless to mix our own userland aio (that will support readv/writev too), with the posix one. And if this was just a linux project kernel AIO would suffice. All DB that I know need to use readv/writev with AIO and O_DIRECT for similar reasons as us, already used kernel AIO.

Andrea Arcangeli

author

2008-11-30 17:29 UTC #28 lore raw

In reply to Avi Kivity

Quoted 3 lines · Avi Kivity

Since Andrea's patches contain emulation for aio readv/writev, the performance degradation will not occur (though we will not see the benefit either).

It will not occur especially if you #define DEBUG_BOUNCE. The way I emulated bdrv_aio_readv/writev didn't involve a bounce, but it's serially submitting the I/O so that it truly runs zerocopy when DEBUG_BOUNCE is not defined ;).

IF you enable DEBUG_BOUNCE then the bounce layer will be forced on, and the iovec will be linearized and the DMA command executed on the hardware will be allowed to be as large as MAX_DMA_BOUNCE_BUFFER like before. So until we have a real bdrv_aio_readv/writev #defining DEBUG_BOUNCE is a must with cache=off.

Quoted 2 lines

I doubt you can get measure malloc overhead with anything less a thousand disks (even there, other overheads are likely to drown that malloc).

I also eliminated any sign of malloc in the direct path with a small cache layer that caches as many pci dma sg params (with the max iovcnt seen so far embedded into it) as the max number of simultaneous in-flight I/O seen for the whole runtime. With a max param of 10 (so only if there are more than 10 simultaneous sg dma I/O operations in flight, malloc will have to run). Only params with iov arrays with a iovcnt < 2048 are cached. So worst case ram waste is limited, and it's auto-tuning at runtime to remain near zero for single disk setups etc..

Andrea Arcangeli

author

2008-11-30 17:41 UTC #13 lore raw

RFC 1/1 pci-dma-api-v2

In reply to Andrea Arcangeli

Quoted 5 lines · Andrea Arcangeli

better. I've also noticed some problems with windows (I didn't test windows before posting), those aren't related to the cache layer as I added a #define to disable it and replace it with malloc/free. But that's not the cache layer, as soon as windows runs completely flawlessy I post an update.

As promised here an updated. I fixed all outstanding issues, it's running rock solid. Before there were a few spots on ide.c plus the cache layer was very buggy with more than one dma in-flight.

One of the issues was also that the sg[].len may not be aligned. It's the total DMA length that has to be aligned. So while linux never submits memory regions with sg[].len that isn't 512byte aligned, other OS submitted 512 bytes large I/O scattered over different memory regions each one smaller than 512bytes. So the alignment check is moved on the whole DMA operation (param->curr_len) and the max alignment depends on the max size of the buffer (currently is hardcoded to 1M and it can't be too big for security reasons). The way host direct DMA works, the memory region length must be a multiple of the alignment, so bounce buffering is a must for those few times the buffer is smaller than blocksize. I guess the hardware behaves similarly and those dma op with sg entries with length < 512bytes runs slower in the hardware too.

I tested on various OS all combinations, direct dma, bounce buffering, and bounce buffering with 512byte size (which runs so much slower as expected, visibly even during boot, showing how much overhead there is even with cache=off by reading in small chunks and entering/exiting the host kenrel so frequently). I didn't split the _em out of the patch this time as it's so small anyway and I doubt we're ready for merging in qemu svn, but those aren't meant to be merged. However I think the below is close to good enough, the reason I don't think we're ready for merging is that this shall be merged at the same time we add a real bdrv_aio_readv/writev. So once we can agree on the below to be the way to go, next thing to discuss is how to add readv/writev ;).

Signed-off-by Andrea Arcangeli <[email protected]>

block_int.h +2 -0

Index: block_int.h
===================================================================
@@ -55,6 +55,8 @@
         int64_t sector_num, const uint8_t *buf, int nb_sectors,
         BlockDriverCompletionFunc *cb, void *opaque);
     void (*bdrv_aio_cancel)(BlockDriverAIOCB *acb);
+    BlockDriverAIOIOV *bdrv_aio_readv;
+    BlockDriverAIOIOV *bdrv_aio_writev;
     int aiocb_size;
 
     const char *protocol_name;

Makefile.target +5 -5

Index: Makefile.target
===================================================================
@@ -659,7 +659,7 @@
 
 ifeq ($(TARGET_BASE_ARCH), i386)
 # Hardware support
-OBJS+= ide.o pckbd.o ps2.o vga.o $(SOUND_HW) dma.o
+OBJS+= ide.o pckbd.o ps2.o vga.o $(SOUND_HW) dma.o pci_dma.o
 OBJS+= fdc.o mc146818rtc.o serial.o i8259.o i8254.o pcspk.o pc.o
 OBJS+= cirrus_vga.o apic.o parallel.o acpi.o piix_pci.o
 OBJS+= usb-uhci.o vmmouse.o vmport.o vmware_vga.o

@@ -668,7 +668,7 @@
 ifeq ($(TARGET_BASE_ARCH), ppc)
 CPPFLAGS += -DHAS_AUDIO -DHAS_AUDIO_CHOICE
 # shared objects
-OBJS+= ppc.o ide.o vga.o $(SOUND_HW) dma.o openpic.o
+OBJS+= ppc.o ide.o vga.o $(SOUND_HW) dma.o openpic.o pci_dma.o
 # PREP target
 OBJS+= pckbd.o ps2.o serial.o i8259.o i8254.o fdc.o m48t59.o mc146818rtc.o
 OBJS+= prep_pci.o ppc_prep.o

@@ -686,7 +686,7 @@
 OBJS+= mips_timer.o mips_int.o dma.o vga.o serial.o i8254.o i8259.o rc4030.o
 OBJS+= g364fb.o jazz_led.o
 OBJS+= ide.o gt64xxx.o pckbd.o ps2.o fdc.o mc146818rtc.o usb-uhci.o acpi.o ds1225y.o
-OBJS+= piix_pci.o parallel.o cirrus_vga.o pcspk.o $(SOUND_HW)
+OBJS+= piix_pci.o parallel.o cirrus_vga.o pcspk.o pci_dma.o $(SOUND_HW)
 OBJS+= mipsnet.o
 OBJS+= pflash_cfi01.o
 CPPFLAGS += -DHAS_AUDIO -DHAS_AUDIO_CHOICE

@@ -710,7 +710,7 @@
 else
 OBJS+= sun4m.o tcx.o pcnet.o iommu.o m48t59.o slavio_intctl.o
 OBJS+= slavio_timer.o slavio_serial.o slavio_misc.o fdc.o sparc32_dma.o
-OBJS+= cs4231.o ptimer.o eccmemctl.o sbi.o sun4c_intctl.o
+OBJS+= cs4231.o ptimer.o eccmemctl.o sbi.o sun4c_intctl.o pci_dma.o
 endif
 endif
 ifeq ($(TARGET_BASE_ARCH), arm)

@@ -731,7 +731,7 @@
 OBJS+= nseries.o blizzard.o onenand.o vga.o cbus.o tusb6010.o usb-musb.o
 OBJS+= tsc2005.o bt-hci-csr.o
 OBJS+= mst_fpga.o mainstone.o
-OBJS+= musicpal.o pflash_cfi02.o
+OBJS+= musicpal.o pflash_cfi02.o pci_dma.o
 CPPFLAGS += -DHAS_AUDIO
 endif
 ifeq ($(TARGET_BASE_ARCH), sh4)

exec.c +107 -2

127 lines not on this page · raw message

block.c +129 -0

154 lines not on this page · raw message

block.h +13 -0

Index: block.h
===================================================================
@@ -83,6 +83,13 @@
 /* async block I/O */
 typedef struct BlockDriverAIOCB BlockDriverAIOCB;
 typedef void BlockDriverCompletionFunc(void *opaque, int ret);
+typedef BlockDriverAIOCB *BlockDriverAIOIOV(BlockDriverState *bs,
+					    int64_t sector_num,
+					    struct iovec *iov,
+					    int iovnct,
+					    size_t len,
+					    BlockDriverCompletionFunc *cb,
+					    void *opaque);
 
 BlockDriverAIOCB *bdrv_aio_read(BlockDriverState *bs, int64_t sector_num,
                                 uint8_t *buf, int nb_sectors,

@@ -91,6 +98,12 @@
                                  const uint8_t *buf, int nb_sectors,
                                  BlockDriverCompletionFunc *cb, void *opaque);
 void bdrv_aio_cancel(BlockDriverAIOCB *acb);
+BlockDriverAIOCB *bdrv_aio_readv(BlockDriverState *bs, int64_t sector_num,
+				 struct iovec *iov, int iovnct, size_t len,
+				 BlockDriverCompletionFunc *cb, void *opaque);
+BlockDriverAIOCB *bdrv_aio_writev(BlockDriverState *bs, int64_t sector_num,
+				  struct iovec *iov, int iovnct, size_t len,
+				  BlockDriverCompletionFunc *cb, void *opaque);
 
 int qemu_key_check(BlockDriverState *bs, const char *name);

ide.c +93 ~~-98~~

268 lines not on this page · raw message

cpu-all.h +8 -3

Index: cpu-all.h
===================================================================
@@ -891,14 +891,19 @@
 CPUReadMemoryFunc **cpu_get_io_memory_read(int io_index);
 
 void cpu_physical_memory_rw(target_phys_addr_t addr, uint8_t *buf,
-                            int len, int is_write);
+                            size_t len, int is_write);
+void cpu_physical_memory_write_post_dma(target_phys_addr_t addr,
+					size_t len);
+uint8_t *cpu_physical_memory_can_dma(target_phys_addr_t addr,
+				     size_t len, int is_write,
+				     int alignment);
 static inline void cpu_physical_memory_read(target_phys_addr_t addr,
-                                            uint8_t *buf, int len)
+                                            uint8_t *buf, size_t len)
 {
     cpu_physical_memory_rw(addr, buf, len, 0);
 }
 static inline void cpu_physical_memory_write(target_phys_addr_t addr,
-                                             const uint8_t *buf, int len)
+                                             const uint8_t *buf, size_t len)
 {
     cpu_physical_memory_rw(addr, (uint8_t *)buf, len, 1);
 }

qemu/hw/pci_dma.c new file +366 -0

366 lines not on this page · raw message

qemu/hw/pci_dma.h new file +29 -0

new file mode 100644
index 0000000..5cc8413
@@ -0,0 +1,29 @@
+#ifndef QEMU_PCI_DMA_H
+#define QEMU_PCI_DMA_H
+
+#include "qemu-common.h"
+#include "block.h"
+#include <sys/uio.h> /* struct iovec */
+
+typedef int QEMUPciDmaSgSubmit(void *pci_dma_sg_opaque,
+			       struct iovec *iov, int iovcnt,
+			       size_t len,
+			       BlockDriverCompletionFunc dma_cb,
+			       void *dma_cb_param);
+
+typedef void QEMUPciDmaSgComplete(void *pci_dma_sg_opaque, int ret);
+
+typedef struct QEMUPciDmaSg {
+    target_phys_addr_t addr;
+    size_t len;
+} QEMUPciDmaSg;
+
+/* pci_dma.c */
+void pci_dma_sg(PCIDevice *pci_dev,
+		QEMUPciDmaSg *sg, int iovcnt,
+		QEMUPciDmaSgSubmit *pci_dma_sg_submit,
+		QEMUPciDmaSgComplete *pci_dma_sg_complete,
+		void *pci_dma_sg_opaque,
+		int dma_to_memory, int alignment);
+
+#endif

Andrea Arcangeli

author

2008-11-30 18:04 UTC #12 lore raw

In reply to Blue Swirl

Quoted 1 line · Blue Swirl

There's also lio_listio that provides for vectored AIO.

Discussed this in answer to Jamie, basically no LIO_READV/WRITEV, no way to submit 'struct iovec' to the kernel with it still, which is a must to perform with cache=off.

Quoted 2 lines

Quoted 2 lines
Quoted 2 lines
Anthony's second version:
http://lists.gnu.org/archive/html/qemu-devel/2008-04/msg00077.html

Actually this version of the emulated bdrv_writev/readv should run faster thanks to malloc+memcpy instead of not having any memcpy and running more syscalls. I opted for an emulated bdrv_aio_readv/writev that does true zerocopy. But it doesn't make a whole lot of difference as neither one should run on any host kernel supporting readv/writev syscalls, this is just to we can test the rest of the zerocopy dma api. The bdrv_aio_readv/writev support has to be in a separated from the pci dma api anyway and surely I intend to reject my version of bdrv_aio_readv/writev as I think all qemu targets supports at least pthread posix API and readv/writev sycsalls allowing not having to do hacks like my current _em.

Quoted 2 lines

Perhaps you could point out why the previous attempts failed, but yours won't? ;-)

One can always hope to be more lucky? ;)

Seriously, just try to apply my last patch to your qemu tree (kvm rejects in Makefile.target ppc section but it'll work on kvm too for x86* targets) and try to test it. As an example also look at the below IDE code, much of an improvement compared to current code IMHO and it leaves all aiocb knowledge outside of the dma API itself, as it has to be.

static int build_dma_sg(BMDMAState *bm)

{
    struct {
        uint32_t addr;
        uint32_t size;
    } prd;
    int len;
    int idx;

    for (idx = 1; idx <= IDE_DMA_BUF_SECTORS; idx++) {
	cpu_physical_memory_read(bm->cur_addr, (uint8_t *)&prd, 8);
	bm->cur_addr += 8;
	bm->sg[idx-1].addr = le32_to_cpu(prd.addr);
	prd.size = le32_to_cpu(prd.size);
	len = prd.size & 0xfffe;
	if (len == 0)
	    len = 0x10000;
	bm->sg[idx-1].len = len;
	/* end of table (with a fail safe of one page) */
	if ((prd.size & 0x80000000) ||
	    (bm->cur_addr - bm->addr) >= 4096)
	    break;
    }
    if (idx > IDE_DMA_BUF_SECTORS)
	printf("build_dma_sg: too many sg entries\n");
    return idx;
}

static void ide_dma_complete(void *opaque, int ret)

{
    BMDMAState *bm = opaque;
    IDEState *s = bm->ide_if;

    bm->bdrv_aio_iov = NULL;
    bm->ide_if = NULL;
    bm->aiocb = NULL;
    /* end of transfer ? */
    if (s->nsector == 0 && !ret) {
        s->status = READY_STAT | SEEK_STAT;
        ide_set_irq(s);
        bm->status &= ~BM_STATUS_DMAING;
        bm->status |= BM_STATUS_INT;
    } else {
	ide_dma_error(s);
	printf("ide_dma_complete error: nsector %d err %d\n", s->nsector, ret);
    }
}

static int ide_dma_submit(void *opaque, struct iovec *dma_iov,
int iovcnt, size_t len, BlockDriverCompletionFunc dma_cb, void *dma_cb_param)

{
    BMDMAState *bm = opaque;
    IDEState *s = bm->ide_if;
    size_t sectors;
    int64_t sector_num;

    sectors = len >> 9;
    if (s->nsector < sectors)
	return -3000;
    sector_num = ide_get_sector(s);
    ide_set_sector(s, sector_num  + sectors);
    s->nsector -= sectors;

#ifdef DEBUG_AIO
    printf("ide_dma_submit_write: sector_num=%lld n=%d\n", sector_num, sectors);
#endif
    bm->aiocb = bm->bdrv_aio_iov(s->bs, sector_num, dma_iov, iovcnt, len,
				 dma_cb, dma_cb_param);
    if (!bm->aiocb)
	return -3001;

    return 0;
}

Blue Swirl

2008-11-30 18:37 UTC #14 lore raw

[Qemu-devel] Re: [RFC 1/1] pci-dma-api-v2

In reply to Andrea Arcangeli

Quoted 10 lines · Andrea Arcangeli

Quoted 5 lines · Andrea Arcangeli
better. I've also noticed some problems with windows (I didn't test windows before posting), those aren't related to the cache layer as I added a #define to disable it and replace it with malloc/free. But that's not the cache layer, as soon as windows runs completely flawlessy I post an update.
As promised here an updated. I fixed all outstanding issues, it's running rock solid. Before there were a few spots on ide.c plus the cache layer was very buggy with more than one dma in-flight.

The patch does not apply as is:

Quoted 4 lines

--- hw/ide.c (revision 5818)
+++ hw/ide.c (working copy)
diff --git a/qemu/hw/pci_dma.c b/qemu/hw/pci_dma.c new file mode 100644

Quoted 8 lines

index 0000000..48762a8
--- /dev/null
+++ b/qemu/hw/pci_dma.c
diff --git a/qemu/hw/pci_dma.h b/qemu/hw/pci_dma.h new file mode 100644
index 0000000..5cc8413
--- /dev/null
+++ b/qemu/hw/pci_dma.h

Even as I fixed the patch, it still does not compile, for example:
/src/qemu/block.c:1335: warning: 'struct iovec' declared inside parameter list /src/qemu/block.c:1336: error: conflicting types for 'bdrv_aio_writev' /src/qemu/block.h:106: error: previous declaration of 'bdrv_aio_writev' was here /src/qemu/block.c:1425: error: invalid use of undefined type 'struct iovec'

Andrea Arcangeli

author

2008-11-30 19:04 UTC #15 lore raw

[Qemu-devel] Re: [RFC 1/1] pci-dma-api-v2

Quoted 13 lines · Blue Swirl

The patch does not apply as is:
Quoted 12 lines
--- hw/ide.c (revision 5818)
+++ hw/ide.c (working copy)
diff --git a/qemu/hw/pci_dma.c b/qemu/hw/pci_dma.c new file mode 100644
index 0000000..48762a8
--- /dev/null
+++ b/qemu/hw/pci_dma.c
diff --git a/qemu/hw/pci_dma.h b/qemu/hw/pci_dma.h new file mode 100644
index 0000000..5cc8413
--- /dev/null
+++ b/qemu/hw/pci_dma.h

Yes sorry it's because I generate the patches with 'git diff master' and I cut and pasted them in... hand editing the patch to remove 'b/qemu/' will fix it (to apply later with patch -p0 of course, not standard but the default with svn).

Quoted 5 lines

Even as I fixed the patch, it still does not compile, for example:
/src/qemu/block.c:1335: warning: 'struct iovec' declared inside parameter list /src/qemu/block.c:1336: error: conflicting types for 'bdrv_aio_writev' /src/qemu/block.h:106: error: previous declaration of 'bdrv_aio_writev' was here /src/qemu/block.c:1425: error: invalid use of undefined type 'struct iovec'

Weird... I don't get that error, is that an old compiler? Does it go away if you move the #include <sys/uio.h> from pci_dma.h to block.h?

Thanks!

Blue Swirl

2008-11-30 19:11 UTC #16 lore raw

[Qemu-devel] Re: [RFC 1/1] pci-dma-api-v2

Quoted 29 lines · Andrea Arcangeli

Quoted 13 lines · Blue Swirl
The patch does not apply as is:
Quoted 12 lines
--- hw/ide.c (revision 5818)
+++ hw/ide.c (working copy)
diff --git a/qemu/hw/pci_dma.c b/qemu/hw/pci_dma.c new file mode 100644
index 0000000..48762a8
--- /dev/null
+++ b/qemu/hw/pci_dma.c
diff --git a/qemu/hw/pci_dma.h b/qemu/hw/pci_dma.h new file mode 100644
index 0000000..5cc8413
--- /dev/null
+++ b/qemu/hw/pci_dma.h
Yes sorry it's because I generate the patches with 'git diff master'
and I cut and pasted them in... hand editing the patch to remove 'b/qemu/' will fix it (to apply later with patch -p0 of course, not standard but the default with svn).
Quoted 5 lines
Even as I fixed the patch, it still does not compile, for example:
/src/qemu/block.c:1335: warning: 'struct iovec' declared inside parameter list /src/qemu/block.c:1336: error: conflicting types for 'bdrv_aio_writev' /src/qemu/block.h:106: error: previous declaration of 'bdrv_aio_writev' was here /src/qemu/block.c:1425: error: invalid use of undefined type 'struct iovec'

Quoted 2 lines

Weird... I don't get that error, is that an old compiler? Does it go
away if you move the #include <sys/uio.h> from pci_dma.h to block.h?

Yes, this version (attached) works.

block_int.h +2 -0

Index: qemu/block_int.h
===================================================================
@@ -55,6 +55,8 @@
         int64_t sector_num, const uint8_t *buf, int nb_sectors,
         BlockDriverCompletionFunc *cb, void *opaque);
     void (*bdrv_aio_cancel)(BlockDriverAIOCB *acb);
+    BlockDriverAIOIOV *bdrv_aio_readv;
+    BlockDriverAIOIOV *bdrv_aio_writev;
     int aiocb_size;
 
     const char *protocol_name;

Makefile.target +5 -5

Index: qemu/Makefile.target
===================================================================
@@ -659,7 +659,7 @@
 
 ifeq ($(TARGET_BASE_ARCH), i386)
 # Hardware support
-OBJS+= ide.o pckbd.o ps2.o vga.o $(SOUND_HW) dma.o
+OBJS+= ide.o pckbd.o ps2.o vga.o $(SOUND_HW) dma.o pci_dma.o
 OBJS+= fdc.o mc146818rtc.o serial.o i8259.o i8254.o pcspk.o pc.o
 OBJS+= cirrus_vga.o apic.o parallel.o acpi.o piix_pci.o
 OBJS+= usb-uhci.o vmmouse.o vmport.o vmware_vga.o

@@ -668,7 +668,7 @@
 ifeq ($(TARGET_BASE_ARCH), ppc)
 CPPFLAGS += -DHAS_AUDIO -DHAS_AUDIO_CHOICE
 # shared objects
-OBJS+= ppc.o ide.o vga.o $(SOUND_HW) dma.o openpic.o
+OBJS+= ppc.o ide.o vga.o $(SOUND_HW) dma.o openpic.o pci_dma.o
 # PREP target
 OBJS+= pckbd.o ps2.o serial.o i8259.o i8254.o fdc.o m48t59.o mc146818rtc.o
 OBJS+= prep_pci.o ppc_prep.o

@@ -686,7 +686,7 @@
 OBJS+= mips_timer.o mips_int.o dma.o vga.o serial.o i8254.o i8259.o rc4030.o
 OBJS+= g364fb.o jazz_led.o
 OBJS+= ide.o gt64xxx.o pckbd.o ps2.o fdc.o mc146818rtc.o usb-uhci.o acpi.o ds1225y.o
-OBJS+= piix_pci.o parallel.o cirrus_vga.o pcspk.o $(SOUND_HW)
+OBJS+= piix_pci.o parallel.o cirrus_vga.o pcspk.o pci_dma.o $(SOUND_HW)
 OBJS+= mipsnet.o
 OBJS+= pflash_cfi01.o
 CPPFLAGS += -DHAS_AUDIO -DHAS_AUDIO_CHOICE

@@ -710,7 +710,7 @@
 else
 OBJS+= sun4m.o tcx.o pcnet.o iommu.o m48t59.o slavio_intctl.o
 OBJS+= slavio_timer.o slavio_serial.o slavio_misc.o fdc.o sparc32_dma.o
-OBJS+= cs4231.o ptimer.o eccmemctl.o sbi.o sun4c_intctl.o
+OBJS+= cs4231.o ptimer.o eccmemctl.o sbi.o sun4c_intctl.o pci_dma.o
 endif
 endif
 ifeq ($(TARGET_BASE_ARCH), arm)

@@ -731,7 +731,7 @@
 OBJS+= nseries.o blizzard.o onenand.o vga.o cbus.o tusb6010.o usb-musb.o
 OBJS+= tsc2005.o bt-hci-csr.o
 OBJS+= mst_fpga.o mainstone.o
-OBJS+= musicpal.o pflash_cfi02.o
+OBJS+= musicpal.o pflash_cfi02.o pci_dma.o
 CPPFLAGS += -DHAS_AUDIO
 endif
 ifeq ($(TARGET_BASE_ARCH), sh4)

exec.c +107 -2

127 lines not on this page · raw message

block.c +129 -0

153 lines not on this page · raw message

block.h +14 -0

Index: qemu/block.h
===================================================================
@@ -2,6 +2,7 @@
 #define BLOCK_H
 
 #include "qemu-aio.h"
+#include <sys/uio.h> /* struct iovec */
 
 /* block.c */
 typedef struct BlockDriver BlockDriver;

@@ -83,6 +84,13 @@
 /* async block I/O */
 typedef struct BlockDriverAIOCB BlockDriverAIOCB;
 typedef void BlockDriverCompletionFunc(void *opaque, int ret);
+typedef BlockDriverAIOCB *BlockDriverAIOIOV(BlockDriverState *bs,
+					    int64_t sector_num,
+					    struct iovec *iov,
+					    int iovnct,
+					    size_t len,
+					    BlockDriverCompletionFunc *cb,
+					    void *opaque);
 
 BlockDriverAIOCB *bdrv_aio_read(BlockDriverState *bs, int64_t sector_num,
                                 uint8_t *buf, int nb_sectors,

@@ -91,6 +99,12 @@
                                  const uint8_t *buf, int nb_sectors,
                                  BlockDriverCompletionFunc *cb, void *opaque);
 void bdrv_aio_cancel(BlockDriverAIOCB *acb);
+BlockDriverAIOCB *bdrv_aio_readv(BlockDriverState *bs, int64_t sector_num,
+				 struct iovec *iov, int iovnct, size_t len,
+				 BlockDriverCompletionFunc *cb, void *opaque);
+BlockDriverAIOCB *bdrv_aio_writev(BlockDriverState *bs, int64_t sector_num,
+				  struct iovec *iov, int iovnct, size_t len,
+				  BlockDriverCompletionFunc *cb, void *opaque);
 
 int qemu_key_check(BlockDriverState *bs, const char *name);

hw/ide.c +94 ~~-99~~

269 lines not on this page · raw message

cpu-all.h +8 -3

Index: qemu/cpu-all.h
===================================================================
@@ -891,14 +891,19 @@
 CPUReadMemoryFunc **cpu_get_io_memory_read(int io_index);
 
 void cpu_physical_memory_rw(target_phys_addr_t addr, uint8_t *buf,
-                            int len, int is_write);
+                            size_t len, int is_write);
+void cpu_physical_memory_write_post_dma(target_phys_addr_t addr,
+					size_t len);
+uint8_t *cpu_physical_memory_can_dma(target_phys_addr_t addr,
+				     size_t len, int is_write,
+				     int alignment);
 static inline void cpu_physical_memory_read(target_phys_addr_t addr,
-                                            uint8_t *buf, int len)
+                                            uint8_t *buf, size_t len)
 {
     cpu_physical_memory_rw(addr, buf, len, 0);
 }
 static inline void cpu_physical_memory_write(target_phys_addr_t addr,
-                                             const uint8_t *buf, int len)
+                                             const uint8_t *buf, size_t len)
 {
     cpu_physical_memory_rw(addr, (uint8_t *)buf, len, 1);
 }

hw/pci_dma.c new file +366 -0

366 lines not on this page · raw message

hw/pci_dma.h new file +28 -0

Index: qemu/hw/pci_dma.h
===================================================================
@@ -0,0 +1,28 @@
+#ifndef QEMU_PCI_DMA_H
+#define QEMU_PCI_DMA_H
+
+#include "qemu-common.h"
+#include "block.h"
+
+typedef int QEMUPciDmaSgSubmit(void *pci_dma_sg_opaque,
+			       struct iovec *iov, int iovcnt,
+			       size_t len,
+			       BlockDriverCompletionFunc dma_cb,
+			       void *dma_cb_param);
+
+typedef void QEMUPciDmaSgComplete(void *pci_dma_sg_opaque, int ret);
+
+typedef struct QEMUPciDmaSg {
+    target_phys_addr_t addr;
+    size_t len;
+} QEMUPciDmaSg;
+
+/* pci_dma.c */
+void pci_dma_sg(PCIDevice *pci_dev,
+		QEMUPciDmaSg *sg, int iovcnt,
+		QEMUPciDmaSgSubmit *pci_dma_sg_submit,
+		QEMUPciDmaSgComplete *pci_dma_sg_complete,
+		void *pci_dma_sg_opaque,
+		int dma_to_memory, int alignment);
+
+#endif

Andrea Arcangeli

author

2008-11-30 19:20 UTC #17 lore raw

[Qemu-devel] Re: [RFC 1/1] pci-dma-api-v2

Quoted 1 line · Blue Swirl

Yes, this version (attached) works.

Ok great! ;)

Avi Kivity

2008-11-30 20:27 UTC #29 lore raw

In reply to Andrea Arcangeli

Quoted 10 lines · Andrea Arcangeli

Quoted 2 lines · Avi Kivity
Since Andrea's patches contain emulation for aio readv/writev, the performance degradation will not occur (though we will not see the benefit either).
It will not occur especially if you #define DEBUG_BOUNCE. The way I emulated bdrv_aio_readv/writev didn't involve a bounce, but it's serially submitting the I/O so that it truly runs zerocopy when DEBUG_BOUNCE is not defined ;).

Quoted 7 lines

IF you enable DEBUG_BOUNCE then the bounce layer will be forced on, and the iovec will be linearized and the DMA command executed on the hardware will be allowed to be as large as MAX_DMA_BOUNCE_BUFFER like before. So until we have a real bdrv_aio_readv/writev #defining DEBUG_BOUNCE is a must with cache=off.

Oh okay. In that case it should be committed with DEBUG_BOUNCE enabled, and that removed when we have proper aio *v.

Quoted 12 lines

Quoted 2 lines
I doubt you can get measure malloc overhead with anything less a thousand disks (even there, other overheads are likely to drown that malloc).
I also eliminated any sign of malloc in the direct path with a small cache layer that caches as many pci dma sg params (with the max iovcnt seen so far embedded into it) as the max number of simultaneous in-flight I/O seen for the whole runtime. With a max param of 10 (so only if there are more than 10 simultaneous sg dma I/O operations in flight, malloc will have to run). Only params with iov arrays with a iovcnt < 2048 are cached. So worst case ram waste is limited, and it's auto-tuning at runtime to remain near zero for single disk setups etc..

Overkill IMO (glibc likely caches mallocs), but can't hurt.

Signature · 2 lines

I have a truly marvellous patch that fixes the bug which this
signature is too narrow to contain.

Blue Swirl

2008-11-30 21:36 UTC #18 lore raw

[Qemu-devel] Re: [RFC 1/1] pci-dma-api-v2

In reply to Andrea Arcangeli

Quoted 5 lines · Andrea Arcangeli

Quoted 1 line · Blue Swirl
Yes, this version (attached) works.
Ok great! ;)

This looks promising. Maybe the previous attempts tried to solve several problems at once, like the complex address translation schemes I have in mind, in addition to the basic zero-copy DMA transfer.This version leaves the DMA address translation to the caller and operates using physical memory addresses. Another way would be to use directly host pointers.

Regarding the code, I'd drop the "PCI", this will fit SBus, ISA or whatever bus we are using.

Sparc64 does not compile, unless pci_dma.o is added to the Makefile.target list of Sparc64 objects.

The first function called is pci_dma_sg, which also performs some initialization. I'd move the function pointer setup to a separate init function.

Quilt complains about trailing whitespace, please trim.

Anthony Liguori

2008-11-30 22:31 UTC #11 lore raw

In reply to Jamie Lokier

Quoted 10 lines · Jamie Lokier

Quoted 8 lines · Blue Swirl
Quoted 8 lines
I wonder how can possibly aio_readv/writev be missing in posix aio? Unbelievable. It'd be totally trivial to add those to glibc, much easier infact than to pthread_create by hand, but how can we add a dependency on a certain glibc version? Ironically it'll be more user-friendly to add dependency on linux kernel-aio implementation that is already available for ages and it's guaranteed to run faster (or at least not slower).

Quoted 8 lines

Quoted 2 lines
There's also lio_listio that provides for vectored AIO.
I think lio_listio is the missing aio_readv/writev.
It's more versatile, and that'll by why POSIX never bothered with aio_readv/writev.

No. lio_listio is a batch submission mechanism, not an operation.

Each request maintains the same behavior as if they were submitted independently. This means if you're doing a write operation and you submit a vectored request via lio_listio, each portion of the vector has to complete before the other one.

But most importantly, pthreads doesn't use readv/writev to implement lio_listio as has been mentioned elsewhere.

Regards,

Anthony Liguori

Quoted 7 lines

Doesn't explain why they didn't _start_ with aio_readv before inventing lio_listio, but there you go. Unix history.
-- Jamie

Anthony Liguori

2008-11-30 22:33 UTC #31 lore raw

In reply to Blue Swirl

Quoted 10 lines · Blue Swirl

Quoted 7 lines · Andrea Arcangeli
Hello everyone,
Once we finish fixing storage performance with a real bdrv_aio_readv/writev (now a blocker issue), a pci_dma_single can be added for zero copy networking (one NIC per VM, or VMDq, IOV etc..). The DMA API should allow for that too.

Quoted 5 lines

The previous similar attempt by Anthony for generic DMA using vectored IO was abandoned because the malloc/free overhead was more than the performance gain. Have you made any performance measurements? How does this version compare to the previous ones?

No, I never measured any malloc/free overhead. I think people are more concerned with that than warranted. A good malloc implementation will usually have O(1) complexity for similar sized allocs so malloc/free should not be the bottleneck.

My attempt was abandoned because the API is complex and at the time, zero copy wasn't the bottle neck we needed to overcome. Fortunately, enough has progressed since then that it is now the bottle neck :-).

Regards,

Anthony Liguori

Quoted 9 lines

I think the pci_ prefix can be removed, there is little PCI specific.
For Sparc32 IOMMU (and probably other IOMMUS), it should be possible to register a function used in place of cpu_physical_memory_rw, c_p_m_can_dma etc. The goal is that it should be possible to stack the DMA resolvers (think of devices behind a number of buses).

Andrea Arcangeli

author

2008-11-30 22:33 UTC #30 lore raw

In reply to Avi Kivity

Quoted 2 lines · Avi Kivity

Oh okay. In that case it should be committed with DEBUG_BOUNCE enabled, and that removed when we have proper aio *v.

Exactly.

But before this can be merged the emulation must be solved. Currently bdrv_aio_cancel can't work safe (with last patch it works safe only for drivers using the dma api). I guess one way is to move the emulation to the block-raw-posix.c layer. But in general I'm not really sure how to best handle this bdrv readv/writev in short-term merging terms. Perhaps we should just get rid of posix aio and add the real thing that will handle readv/writev as well even if that will surely take a bit more than some emulation form in block-raw-posix.c that could handle a safe cancellation for all read/write/readv/writev.

Quoted 1 line

Overkill IMO (glibc likely caches mallocs), but can't hurt.

Yes.

Anthony Liguori

2008-11-30 22:34 UTC #26 lore raw

In reply to Blue Swirl

Quoted 15 lines · Blue Swirl

Quoted 13 lines · Andrea Arcangeli
Quoted 2 lines · Blue Swirl
The previous similar attempt by Anthony for generic DMA using vectored IO was abandoned because the malloc/free overhead was more than the
Even if there were dynamic allocations in the fast path, the overhead
of malloc/free is nothing if compared to running and waiting a host kernel syscall to return every 4k, not to tell with O_DIRECT enabled which is the whole point of having a direct-dma API that truly doesn't pollute the cache. With O_DIRECT, without a real readv/writev I/O performance would be destroyed going down to something like 10M/sec even on the fastest storage/CPU/ram combinations.

Quoted 8 lines

Quoted 4 lines
So the question is how those benchmarks were run, with or without a real readv/writev and with or without O_DIRECT to truly eliminate all CPU cache pollution out of the memory copies?
I don't know, here's a pointer:
http://lists.gnu.org/archive/html/qemu-devel/2008-08/msg00092.html

Yes. A vector IO API is not terribly useful in QEMU without an AIO implementation that can make use of it. posix-aio cannot.

Regards,

Anthony Liguori

2008-11-30 22:38 UTC #24 lore raw

In reply to Andrea Arcangeli

Quoted 11 lines · Andrea Arcangeli

Quoted 3 lines · Blue Swirl
I don't know, here's a pointer:
http://lists.gnu.org/archive/html/qemu-devel/2008-08/msg00092.html
I'm in total agreement with it. The missing "proper vectored AIO operations" are bdrv_aio_readv/writev ;).
I wonder how can possibly aio_readv/writev be missing in posix aio?

Because it sucks.

Quoted 3 lines

Unbelievable. It'd be totally trivial to add those to glibc, much easier infact than to pthread_create by hand, but how can we add a dependency on a certain glibc version?

I've already asked, Ulrich will not likely add this unless it first goes into the Posix standard. It won't go into the Posix standard without an existing implementation. Either way, we're talking about a multi-year process.

We need our own thread-pool based AIO implementation.

Quoted 5 lines

Ironically it'll be more
user-friendly to add dependency on linux kernel-aio implementation that is already available for ages and it's guaranteed to run faster (or at least not slower).

No, kernel-aio is more brain dead. io_submit may block if the underlying file descriptor does not support asynchronous IO. There is no way to detect this. This means the only safe way to use io_submit is to call it from a thread, and to have a thread pool that can do multiple io_submits in parallel.

Yes, that's ridiculous. There's a thread right now on [email protected] discussing the future of linux-aio. The prevailing wisdom seems to be that linux-aio does not have a future.

Regards,

Anthony Liguori

2008-11-30 22:50 UTC #20 lore raw

Re: [Qemu-devel] [RFC 1/1] pci-dma-api-v2

In reply to Andrea Arcangeli

Quoted 1 line · Andrea Arcangeli

On Fri, Nov 28, 2008 at 07:50:01PM +0100, Andrea Arcangeli wrote:

This patch is too large. It should be split up.

Quoted 7 lines

Signed-off-by Andrea Arcangeli <[email protected]>
Index: block_int.h
===================================================================

block_int.h line 61

Quoted 7 lines

block_int.h +2 -0

@@ -55,6 +55,8 @@
         int64_t sector_num, const uint8_t *buf, int nb_sectors,
         BlockDriverCompletionFunc *cb, void *opaque);
     void (*bdrv_aio_cancel)(BlockDriverAIOCB *acb);
+    BlockDriverAIOIOV *bdrv_aio_readv;
+    BlockDriverAIOIOV *bdrv_aio_writev;
     int aiocb_size;

Please maintain consistency with the rest of the struct by not introducing typedefs for these function pointers.

Quoted 23 lines

Index: exec.c
===================================================================

exec.c +2 -2

@@ -2807,7 +2807,7 @@
 /* physical memory access (slow version, mainly for debug) */
 #if defined(CONFIG_USER_ONLY)
 void cpu_physical_memory_rw(target_phys_addr_t addr, uint8_t *buf,
-                            int len, int is_write)
+                            size_t len, int is_write)
 {
     int l, flags;
     target_ulong page;

@@ -2848,7 +2848,7 @@
 
 #else
 void cpu_physical_memory_rw(target_phys_addr_t addr, uint8_t *buf,
-                            int len, int is_write)
+                            size_t len, int is_write)
 {
     int l, io_index;
     uint8_t *ptr;

exec.c line 2940

Quoted 3 lines

exec.c +0 -0

@@ -2938,6 +2938,111 @@
     }
 }

This API change should be a separate patch. It's orthogonal to this one.

exec.c

Quoted 4 lines

exec.c +3 -0

+uint8_t *cpu_physical_memory_can_dma(target_phys_addr_t addr,
+				     size_t len, int is_write,
+				     int alignment)

Your patch is tab-damaged. This API seems flawed. It's duplicating most of what is in cpu_physical_memory_rw.

I think you need something more sophisticated that can split up a scatter/gather list into a set of partial bounce buffers and partial direct copies. Just checking the vector once is a bit hackish.

exec.c

Quoted 6 lines

exec.c +5 -0

+	    /* qemu doesn't execute guest code directly, but kvm does
+	       therefore fluch instruction caches */
+	    if (kvm_enabled())
+		flush_icache_range((unsigned long)ptr,
+				   ((unsigned long)ptr)+l);

This is incorrect for upstream QEMU and just as wrong for kvm-userspace. This is a nasty hack for ia64. flush_icache_range() is dyngen generated. ia64 support is not in upstream QEMU.

exec.c line 155

Quoted 9 lines

exec.c +3 -0

@@ -135,6 +149,9 @@
         /* add synchronous IO emulation layer */
         bdrv->bdrv_read = bdrv_read_em;
         bdrv->bdrv_write = bdrv_write_em;
+        bdrv->bdrv_aio_readv = bdrv_aio_readv_em; /* FIXME */
+        bdrv->bdrv_aio_writev = bdrv_aio_writev_em; /* FIXME */
+        bdrv->bdrv_aio_cancel = bdrv_aio_cancel_em; /* FIXME */

What should be fixed? Are these emulated functions wrong?

There's a lack of symmetry here. We should have a bdrv_readv and bdrv_aio_readv. bdrv_read and bdrv_aio_read should disappear. We can maintain wrappers that create a compatible interface for older code but just adding a new API is wrong.

exec.c

Quoted 5 lines

exec.c +4 -0

+BlockDriverAIOCB *bdrv_aio_readv(BlockDriverState *bs, int64_t sector_num,
+				 struct iovec *iov, int iovcnt, size_t len,
+				 BlockDriverCompletionFunc *cb, void *opaque)
+{

struct iovec does not exist on Windows. I also don't think you've got the abstraction right. Reading and writing may require actions to happen. I don't think you can just introduce something as simple as an iovec here. I think we need something more active.

ide.c line 34

Quoted 9 lines

Index: hw/ide.c
===================================================================
ide.c +1 -0
@@ -31,6 +31,7 @@
 #include "qemu-timer.h"
 #include "sysemu.h"
 #include "ppc_mac.h"
+#include "pci_dma.h"

All of the changes to IDE/SCSI should be separate patches. That will help isolate failures when bisecting.

Quoted 49 lines

qemu/hw/pci_dma.c new file +41 -0

new file mode 100644
index 0000000..48762a8
@@ -0,0 +1,366 @@
+/*
+ * QEMU PCI DMA operations
+ *
+ * Copyright (c) 2008 Red Hat, Inc.
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining a copy
+ * of this software and associated documentation files (the "Software"), to deal
+ * in the Software without restriction, including without limitation the rights
+ * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
+ * copies of the Software, and to permit persons to whom the Software is
+ * furnished to do so, subject to the following conditions:
+ *
+ * The above copyright notice and this permission notice shall be included in
+ * all copies or substantial portions of the Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+ * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL
+ * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+ * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
+ * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
+ * THE SOFTWARE.
+ */
+
+#include <assert.h>
+#include "pci_dma.h"
+
+#define MAX_DMA_BOUNCE_BUFFER (1024*1024)
+//#define DEBUG_BOUNCE
+//#define MAX_DMA_BOUNCE_BUFFER (512)
+//#define DISABLE_IOVEC_CACHE
+
+typedef struct QEMUPciDmaSgParam {
+    QEMUPciDmaSgSubmit *pci_dma_sg_submit;
+    QEMUPciDmaSgComplete *pci_dma_sg_complete;
+    void *pci_dma_sg_opaque;
+    int dma_to_memory;
+    int alignment;
+    uint8_t *bounce;
+    QEMUPciDmaSg *sg;
+    int iovcnt;

qemu/hw/pci_dma.c line 51

Quoted 10 lines

qemu/hw/pci_dma.c new file +10 -0

+    int restart_iovcnt;
+    size_t restart_offset;
+    int curr_restart_iovcnt;
+    size_t curr_restart_offset;
+    size_t curr_len;
+#ifndef DISABLE_IOVEC_CACHE
+    struct QEMUPciDmaSgParam *next;
+#endif
+    struct iovec iov;
+} QEMUPciDmaSgParam;

What is the IOVEC_CACHE and why do we need it? Either way, it should be using sys-queue.

I think you missed the mark here. This API needs to go through the PCIBus and be pluggable at that level. There can be a default implementation but it may differ for each PCI bus.

Please read the previous threads about a DMA API. All of this has been discussed at length.

Regards,

Anthony Liguori

Jamie Lokier

2008-11-30 22:52 UTC #25 lore raw

In reply to Anthony Liguori

Quoted 3 lines · Anthony Liguori

Yes, that's ridiculous. There's a thread right now on [email protected] discussing the future of linux-aio. The prevailing wisdom seems to be that linux-aio does not have a future.

I think you meant [email protected], btw.

-- Jamie

Anthony Liguori

2008-11-30 22:55 UTC #19 lore raw

Re: [Qemu-devel] Re: [RFC 1/1] pci-dma-api-v2

In reply to Blue Swirl

Quoted 9 lines · Blue Swirl

Quoted 6 lines · Andrea Arcangeli
Quoted 1 line · Blue Swirl
Yes, this version (attached) works.
Ok great! ;)

Quoted 7 lines

This looks promising. Maybe the previous attempts tried to solve several problems at once, like the complex address translation schemes I have in mind, in addition to the basic zero-copy DMA transfer.This version leaves the DMA address translation to the caller and operates using physical memory addresses. Another way would be to use directly host pointers.

I'm happy to separate address translation from just zero-copy IO. I agree that a lot of the complexity is introduced trying to solve everything.

For me, just extending the BlockDriver API is a problem. I'd like to see the API properly structured around a single vector API. This is independent of zero copy.

I also think that in the very least, a DMA must be encapsulated in a structure, that has functions to pin()/unpin() that generate bounce buffers as necessary.

I think it's probably a good idea to separate a basic DMA api from a PCI DMA api.

Regards,

Anthony Liguori

Avi Kivity

2008-12-01 09:41 UTC #21 lore raw

Re: [Qemu-devel] [RFC 1/1] pci-dma-api-v2

In reply to Anthony Liguori

Quoted 3 lines · Anthony Liguori

I think you need something more sophisticated that can split up a scatter/gather list into a set of partial bounce buffers and partial direct copies. Just checking the vector once is a bit hackish.

That code path would never be used or tested. As it is, bouncing will only be invoked by dma to or from mmio, and I expect most guests never to invoke it. Why would we complicate the code even more?

Quoted 5 lines

What should be fixed? Are these emulated functions wrong?
There's a lack of symmetry here. We should have a bdrv_readv and bdrv_aio_readv. bdrv_read and bdrv_aio_read should disappear. We can maintain wrappers that create a compatible interface for older code but just adding a new API is wrong.

It was understood a real aio readv/writev was being worked on, so the emulation could be a temporary step.

Quoted 9 lines

block.c line 1314
Quoted 6 lines
+BlockDriverAIOCB *bdrv_aio_readv(BlockDriverState *bs, int64_t sector_num,
block.c +3 -0
+                 struct iovec *iov, int iovcnt, size_t len,
+                 BlockDriverCompletionFunc *cb, void *opaque)
+{
struct iovec does not exist on Windows. I also don't think you've got the abstraction right. Reading and writing may require actions to happen. I don't think you can just introduce something as simple as an iovec here. I think we need something more active.

Can you elaborate? Actions happen in the completion callback. This is just an straightforward extension of the block API, I don't think a dma api should materially change the block api.

Quoted 3 lines

I think you missed the mark here. This API needs to go through the PCIBus and be pluggable at that level. There can be a default implementation but it may differ for each PCI bus.

I think this can serve as the default implementation. Perhaps when a concrete user of pluggable dma emerges we can understand the requirements better.

Signature · 2 lines

I have a truly marvellous patch that fixes the bug which this
signature is too narrow to contain.

Anthony Liguori

2008-12-01 16:38 UTC #22 lore raw

Re: [Qemu-devel] [RFC 1/1] pci-dma-api-v2

Quoted 6 lines · Avi Kivity

Quoted 3 lines · Anthony Liguori
I think you need something more sophisticated that can split up a scatter/gather list into a set of partial bounce buffers and partial direct copies. Just checking the vector once is a bit hackish.
That code path would never be used or tested. As it is, bouncing will only be invoked by dma to or from mmio, and I expect most guests never to invoke it. Why would we complicate the code even more?

I'm not sure it's strictly required but I think it would be a simplificaction.

Quoted 7 lines

Quoted 5 lines
What should be fixed? Are these emulated functions wrong?
There's a lack of symmetry here. We should have a bdrv_readv and bdrv_aio_readv. bdrv_read and bdrv_aio_read should disappear. We can maintain wrappers that create a compatible interface for older code but just adding a new API is wrong.
It was understood a real aio readv/writev was being worked on, so the emulation could be a temporary step.

Yes, but that's orthogonal to what I'm saying here. I'm saying that instead of adding an optional ->aio_readv() member, we should eliminate the ->aio_read() member and replace it with ->aio_readv(). There are only three or four places in the code that implement aio so it's not a big change. It avoids introducing a new API too.

I also think we should have complimentary synchronous vector functions although I'd like to see the synchronous API disappear completely.

Quoted 11 lines

Quoted 9 lines
block.c line 1314
Quoted 6 lines
+BlockDriverAIOCB *bdrv_aio_readv(BlockDriverState *bs, int64_t sector_num,
block.c +3 -0
+                 struct iovec *iov, int iovcnt, size_t len,
+                 BlockDriverCompletionFunc *cb, void *opaque)
+{
struct iovec does not exist on Windows. I also don't think you've got the abstraction right. Reading and writing may require actions to happen. I don't think you can just introduce something as simple as an iovec here. I think we need something more active.
Can you elaborate? Actions happen in the completion callback. This is just an straightforward extension of the block API, I don't think a dma api should materially change the block api.

If we're not going to try and fold in the IOMMU/PCI BUS API at this pass, then this is less important. But to implement a proper PCI bus API, I think there has to be function pointers associated with each element of the scatter/gather list that control how data is copied in and out of each element.

Quoted 5 lines

Quoted 3 lines
I think you missed the mark here. This API needs to go through the PCIBus and be pluggable at that level. There can be a default implementation but it may differ for each PCI bus.
I think this can serve as the default implementation. Perhaps when a concrete user of pluggable dma emerges we can understand the requirements better.

I think if we drop the pci_ prefixes and just treat it as a basic DMA API as Blue Swirl suggested, then this is closer to what we need.

Regards,

Anthony Liguori

Avi Kivity

2008-12-02 09:45 UTC #23 lore raw

Re: [Qemu-devel] [RFC 1/1] pci-dma-api-v2

Quoted 9 lines · Anthony Liguori

Quoted 7 lines
Quoted 5 lines
What should be fixed? Are these emulated functions wrong?
There's a lack of symmetry here. We should have a bdrv_readv and bdrv_aio_readv. bdrv_read and bdrv_aio_read should disappear. We can maintain wrappers that create a compatible interface for older code but just adding a new API is wrong.
It was understood a real aio readv/writev was being worked on, so the emulation could be a temporary step.
Yes, but that's orthogonal to what I'm saying here. I'm saying that instead of adding an optional ->aio_readv() member, we should eliminate the ->aio_read() member and replace it with ->aio_readv(). There are only three or four places in the code that implement aio so it's not a big change. It avoids introducing a new API too.

The api replacement would make a lot more sense when there's an implementation backing it up. I have no concrete objection to this though, maybe Andrea can whip up a follow on patch to kill aio_read.

Quoted 1 line

I also think we should have complimentary synchronous vector functions although I'd like to see the synchronous API disappear completely.

I don't see a user for the synchronous ops. I think the sync ops are used only by the implementation of block formats, not by disk controller emulation, and these only use them to access metadata. Haven't checked this though.

Quoted 7 lines

Quoted 5 lines
Quoted 3 lines
struct iovec does not exist on Windows. I also don't think you've got the abstraction right. Reading and writing may require actions to happen. I don't think you can just introduce something as simple as an iovec here. I think we need something more active.
Can you elaborate? Actions happen in the completion callback. This is just an straightforward extension of the block API, I don't think a dma api should materially change the block api.
If we're not going to try and fold in the IOMMU/PCI BUS API at this pass, then this is less important. But to implement a proper PCI bus API, I think there has to be function pointers associated with each element of the scatter/gather list that control how data is copied in and out of each element.

Certainly not with each element, that would increase overhead for the common zero copy case. The way to do transformation is before/after bouncing, in the slow path.

The way I see it is:
- transformations that only manipulate addresses, maintaining alignment requirements, can be done at the sg->iovec transform phase without any need for further manipulation
- if there are alignment problems, we bounce, but again no need for further manipulation
- transformations that manipulate data are bounced and manipulated on the bounce buffer

It can all be done within the dma api, no need to involve the block layer with callbacks. That's not its job.

Signature · 1 line

error compiling committee.c: too many arguments to function