* [PATCH v3 RFC 2/2] nvme: improve performance for virtual NVMe devices
@ 2016-08-16 1:41 ` Helen Koike
0 siblings, 0 replies; 49+ messages in thread
From: Helen Koike @ 2016-08-16 1:41 UTC (permalink / raw)
From: Rob Nelson <rlnelson@google.com>
This change provides a mechanism to reduce the number of MMIO doorbell
writes for the NVMe driver. When running in a virtualized environment
like QEMU, the cost of an MMIO is quite hefy here. The main idea for
the patch is provide the device two memory location locations:
1) to store the doorbell values so they can be lookup without the doorbell
MMIO write
2) to store an event index.
I believe the doorbell value is obvious, the event index not so much.
Similar to the virtio specificaiton, the virtual device can tell the
driver (guest OS) not to write MMIO unless you are writing past this
value.
FYI: doorbell values are written by the nvme driver (guest OS) and the
event index is written by the virtual device (host OS).
The patch implements a new admin command that will communicate where
these two memory locations reside. If the command fails, the nvme
driver will work as before without any optimizations.
Contributions:
Eric Northup <digitaleric at google.com>
Frank Swiderski <fes at google.com>
Ted Tso <tytso at mit.edu>
Keith Busch <keith.busch at intel.com>
Just to give an idea on the performance boost with the vendor
extension: Running fio [1], a stock NVMe driver I get about 200K read
IOPs with my vendor patch I get about 1000K read IOPs. This was
running with a null device i.e. the backing device simply returned
success on every read IO request.
[1] Running on a 4 core machine:
fio --time_based --name=benchmark --runtime=30
--filename=/dev/nvme0n1 --nrfiles=1 --ioengine=libaio --iodepth=32
--direct=1 --invalidate=1 --verify=0 --verify_fatal=0 --numjobs=4
--rw=randread --blocksize=4k --randrepeat=false
Signed-off-by: Rob Nelson <rlnelson at google.com>
[mlin: port for upstream]
Signed-off-by: Ming Lin <mlin at kernel.org>
[koike: updated for upstream]
Signed-off-by: Helen Koike <helen.koike at collabora.co.uk>
---
Changes since v2:
- Add vdb.c and vdb.h, the idea is to let the code in pci.c clean and to
make it easier to integrate with the official nvme extention when nvme
consortium publishes it
- Remove rmb (I couldn't see why they were necessary here, please let me
know if I am wrong)
- Reposition wmb
- Transform specific code in helper functions
- Coding style (checkpatch, remove unecessary goto, change if statement
logic to decrease identation)
- Rename feature to CONFIG_NVME_VDB
- Remove some PCI_VENDOR_ID_GOOGLE checks
drivers/nvme/host/Kconfig | 11 ++++
drivers/nvme/host/Makefile | 1 +
drivers/nvme/host/pci.c | 29 ++++++++++-
drivers/nvme/host/vdb.c | 125 +++++++++++++++++++++++++++++++++++++++++++++
drivers/nvme/host/vdb.h | 118 ++++++++++++++++++++++++++++++++++++++++++
include/linux/nvme.h | 17 ++++++
6 files changed, 299 insertions(+), 2 deletions(-)
create mode 100644 drivers/nvme/host/vdb.c
create mode 100644 drivers/nvme/host/vdb.h
diff --git a/drivers/nvme/host/Kconfig b/drivers/nvme/host/Kconfig
index db39d53..d3f4da9 100644
--- a/drivers/nvme/host/Kconfig
+++ b/drivers/nvme/host/Kconfig
@@ -43,3 +43,14 @@ config NVME_RDMA
from https://github.com/linux-nvme/nvme-cli.
If unsure, say N.
+
+config NVME_VDB
+ bool "NVMe Virtual Doorbell Extension for Improved Virtualization"
+ depends on NVME_CORE
+ ---help---
+ This provides support for the Virtual Doorbell Extension which
+ reduces the number of required MMIOs to ring doorbells, improving
+ performance in virtualized environments where MMIO causes a high
+ overhead.
+
+ If unsure, say N.
diff --git a/drivers/nvme/host/Makefile b/drivers/nvme/host/Makefile
index 47abcec..d4d0e3d 100644
--- a/drivers/nvme/host/Makefile
+++ b/drivers/nvme/host/Makefile
@@ -8,6 +8,7 @@ nvme-core-$(CONFIG_BLK_DEV_NVME_SCSI) += scsi.o
nvme-core-$(CONFIG_NVM) += lightnvm.o
nvme-y += pci.o
+nvme-$(CONFIG_NVME_VDB) += vdb.o
nvme-fabrics-y += fabrics.o
diff --git a/drivers/nvme/host/pci.c b/drivers/nvme/host/pci.c
index cf8b3d7..20bbc33 100644
--- a/drivers/nvme/host/pci.c
+++ b/drivers/nvme/host/pci.c
@@ -44,6 +44,7 @@
#include <asm/unaligned.h>
#include "nvme.h"
+#include "vdb.h"
#define NVME_Q_DEPTH 1024
#define NVME_AQ_DEPTH 256
@@ -99,6 +100,7 @@ struct nvme_dev {
dma_addr_t cmb_dma_addr;
u64 cmb_size;
u32 cmbsz;
+ struct nvme_vdb_dev vdb_d;
struct nvme_ctrl ctrl;
struct completion ioq_wait;
};
@@ -131,6 +133,7 @@ struct nvme_queue {
u16 qid;
u8 cq_phase;
u8 cqe_seen;
+ struct nvme_vdb_queue vdb_q;
};
/*
@@ -171,6 +174,7 @@ static inline void _nvme_check_size(void)
BUILD_BUG_ON(sizeof(struct nvme_id_ns) != 4096);
BUILD_BUG_ON(sizeof(struct nvme_lba_range_type) != 64);
BUILD_BUG_ON(sizeof(struct nvme_smart_log) != 512);
+ BUILD_BUG_ON(sizeof(struct nvme_doorbell_memory) != 64);
}
/*
@@ -285,7 +289,7 @@ static void __nvme_submit_cmd(struct nvme_queue *nvmeq,
if (++tail == nvmeq->q_depth)
tail = 0;
- writel(tail, nvmeq->q_db);
+ nvme_write_doorbell_sq(&nvmeq->vdb_q, tail, nvmeq->q_db);
nvmeq->sq_tail = tail;
}
@@ -713,7 +717,8 @@ static void __nvme_process_cq(struct nvme_queue *nvmeq, unsigned int *tag)
return;
if (likely(nvmeq->cq_vector >= 0))
- writel(head, nvmeq->q_db + nvmeq->dev->db_stride);
+ nvme_write_doorbell_cq(&nvmeq->vdb_q, head,
+ nvmeq->q_db + nvmeq->dev->db_stride);
nvmeq->cq_head = head;
nvmeq->cq_phase = phase;
@@ -1068,6 +1073,8 @@ static struct nvme_queue *nvme_alloc_queue(struct nvme_dev *dev, int qid,
dev->queues[qid] = nvmeq;
dev->queue_count++;
+ nvme_init_doorbell_mem(&dev->vdb_d, &nvmeq->vdb_q, qid, dev->db_stride);
+
return nvmeq;
free_cqdma:
@@ -1098,6 +1105,7 @@ static void nvme_init_queue(struct nvme_queue *nvmeq, u16 qid)
nvmeq->cq_head = 0;
nvmeq->cq_phase = 1;
nvmeq->q_db = &dev->dbs[qid * 2 * dev->db_stride];
+ nvme_init_doorbell_mem(&dev->vdb_d, &nvmeq->vdb_q, qid, dev->db_stride);
memset((void *)nvmeq->cqes, 0, CQ_SIZE(nvmeq->q_depth));
dev->online_queues++;
spin_unlock_irq(&nvmeq->q_lock);
@@ -1588,6 +1596,9 @@ static int nvme_dev_add(struct nvme_dev *dev)
if (blk_mq_alloc_tag_set(&dev->tagset))
return 0;
dev->ctrl.tagset = &dev->tagset;
+
+ nvme_set_doorbell_memory(dev->dev, &dev->vdb_d,
+ &dev->ctrl, dev->db_stride);
} else {
blk_mq_update_nr_hw_queues(&dev->tagset, dev->online_queues - 1);
@@ -1655,6 +1666,18 @@ static int nvme_pci_enable(struct nvme_dev *dev)
pci_enable_pcie_error_reporting(pdev);
pci_save_state(pdev);
+
+ /*
+ * Google cloud support in memory doorbells extension, reducing the
+ * number of MMIOs, optimizing performance for virtualized environments
+ */
+ if (pdev->vendor == PCI_VENDOR_ID_GOOGLE) {
+ result = nvme_dma_alloc_doorbell_mem(dev->dev, &dev->vdb_d,
+ dev->db_stride);
+ if (result)
+ goto disable;
+ }
+
return 0;
disable:
@@ -1673,6 +1696,8 @@ static void nvme_pci_disable(struct nvme_dev *dev)
{
struct pci_dev *pdev = to_pci_dev(dev->dev);
+ nvme_dma_free_doorbell_mem(dev->dev, &dev->vdb_d, dev->db_stride);
+
if (pdev->msi_enabled)
pci_disable_msi(pdev);
else if (pdev->msix_enabled)
diff --git a/drivers/nvme/host/vdb.c b/drivers/nvme/host/vdb.c
new file mode 100644
index 0000000..e0c3fef
--- /dev/null
+++ b/drivers/nvme/host/vdb.c
@@ -0,0 +1,125 @@
+/*
+ * NVM Express device driver
+ * Copyright (C) 2015-2016, Google, Inc.
+ *
+ * This program is free software; you can redistribute it and/or modify it
+ * under the terms and conditions of the GNU General Public License,
+ * version 2, as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope it will be useful, but WITHOUT
+ * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
+ * FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License for
+ * more details.
+ */
+
+#include "nvme.h"
+#include "vdb.h"
+
+static inline unsigned int nvme_doorbell_memory_size(u32 stride)
+{
+ return ((num_possible_cpus() + 1) * 8 * stride);
+}
+
+int nvme_dma_alloc_doorbell_mem(struct device *dev,
+ struct nvme_vdb_dev *vdb_d,
+ u32 stride)
+{
+ unsigned int mem_size = nvme_doorbell_memory_size(stride);
+
+ vdb_d->db_mem = dma_alloc_coherent(dev, mem_size, &vdb_d->doorbell,
+ GFP_KERNEL);
+ if (!vdb_d->db_mem)
+ return -ENOMEM;
+ vdb_d->ei_mem = dma_alloc_coherent(dev, mem_size, &vdb_d->eventidx,
+ GFP_KERNEL);
+ if (!vdb_d->ei_mem) {
+ dma_free_coherent(dev, mem_size,
+ vdb_d->db_mem, vdb_d->doorbell);
+ vdb_d->db_mem = NULL;
+ return -ENOMEM;
+ }
+
+ return 0;
+}
+
+void nvme_dma_free_doorbell_mem(struct device *dev,
+ struct nvme_vdb_dev *vdb_d,
+ u32 stride)
+{
+ unsigned int mem_size = nvme_doorbell_memory_size(stride);
+
+ if (vdb_d->db_mem) {
+ dma_free_coherent(dev, mem_size,
+ vdb_d->db_mem, vdb_d->doorbell);
+ vdb_d->db_mem = NULL;
+ }
+ if (vdb_d->ei_mem) {
+ dma_free_coherent(dev, mem_size,
+ vdb_d->ei_mem, vdb_d->eventidx);
+ vdb_d->ei_mem = NULL;
+ }
+}
+
+void nvme_init_doorbell_mem(struct nvme_vdb_dev *vdb_d,
+ struct nvme_vdb_queue *vdb_q,
+ int qid, u32 stride)
+{
+ if (!vdb_d->db_mem || !qid)
+ return;
+
+ vdb_q->sq_doorbell_addr = &vdb_d->db_mem[SQ_IDX(qid, stride)];
+ vdb_q->cq_doorbell_addr = &vdb_d->db_mem[CQ_IDX(qid, stride)];
+ vdb_q->sq_eventidx_addr = &vdb_d->ei_mem[SQ_IDX(qid, stride)];
+ vdb_q->cq_eventidx_addr = &vdb_d->ei_mem[CQ_IDX(qid, stride)];
+}
+
+void nvme_set_doorbell_memory(struct device *dev,
+ struct nvme_vdb_dev *vdb_d,
+ struct nvme_ctrl *ctrl,
+ u32 stride)
+{
+ struct nvme_command c;
+
+ if (!vdb_d->db_mem)
+ return;
+
+ memset(&c, 0, sizeof(c));
+ c.doorbell_memory.opcode = nvme_admin_doorbell_memory;
+ c.doorbell_memory.prp1 = cpu_to_le64(vdb_d->doorbell);
+ c.doorbell_memory.prp2 = cpu_to_le64(vdb_d->eventidx);
+
+ if (nvme_submit_sync_cmd(ctrl->admin_q, &c, NULL, 0))
+ /* Free memory and continue on */
+ nvme_dma_free_doorbell_mem(dev, vdb_d, stride);
+}
+
+static inline int nvme_ext_need_event(u16 event_idx, u16 new_idx, u16 old)
+{
+ /* Borrowed from vring_need_event */
+ return (u16)(new_idx - event_idx - 1) < (u16)(new_idx - old);
+}
+
+void nvme_write_doorbell(u16 value,
+ u32 __iomem *q_db,
+ u32 *db_addr,
+ volatile u32 *event_idx)
+{
+ u16 old_value;
+
+ if (!db_addr) {
+ writel(value, q_db);
+ return;
+ }
+
+ /*
+ * Ensure that the queue is written before updating
+ * the doorbell in memory
+ */
+ wmb();
+
+ old_value = *db_addr;
+ *db_addr = value;
+
+ if (nvme_ext_need_event(*event_idx, value, old_value))
+ writel(value, q_db);
+}
diff --git a/drivers/nvme/host/vdb.h b/drivers/nvme/host/vdb.h
new file mode 100644
index 0000000..37edd75
--- /dev/null
+++ b/drivers/nvme/host/vdb.h
@@ -0,0 +1,118 @@
+/*
+ * NVM Express device driver
+ * Copyright (C) 2015-2016, Google, Inc.
+ *
+ * This program is free software; you can redistribute it and/or modify it
+ * under the terms and conditions of the GNU General Public License,
+ * version 2, as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope it will be useful, but WITHOUT
+ * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
+ * FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License for
+ * more details.
+ */
+
+#ifndef _NVME_VDB_H
+#define _NVME_VDB_H
+
+#ifdef CONFIG_NVME_VDB
+
+#define SQ_IDX(qid, stride) ((qid) * 2 * (stride))
+#define CQ_IDX(qid, stride) (((qid) * 2 + 1) * (stride))
+
+struct nvme_vdb_dev {
+ u32 *db_mem;
+ dma_addr_t doorbell;
+ u32 *ei_mem;
+ dma_addr_t eventidx;
+};
+
+struct nvme_vdb_queue {
+ u32 *sq_doorbell_addr;
+ u32 *sq_eventidx_addr;
+ u32 *cq_doorbell_addr;
+ u32 *cq_eventidx_addr;
+};
+
+int nvme_dma_alloc_doorbell_mem(struct device *dev,
+ struct nvme_vdb_dev *vdb_d,
+ u32 stride);
+
+void nvme_dma_free_doorbell_mem(struct device *dev,
+ struct nvme_vdb_dev *vdb_d,
+ u32 stride);
+
+void nvme_init_doorbell_mem(struct nvme_vdb_dev *vdb_d,
+ struct nvme_vdb_queue *vdb_q,
+ int qid, u32 stride);
+
+void nvme_set_doorbell_memory(struct device *dev,
+ struct nvme_vdb_dev *vdb_d,
+ struct nvme_ctrl *ctrl,
+ u32 stride);
+
+void nvme_write_doorbell(u16 value,
+ u32 __iomem *q_db,
+ u32 *db_addr,
+ volatile u32 *event_idx);
+
+static inline void nvme_write_doorbell_cq(struct nvme_vdb_queue *vdb_q,
+ u16 value, u32 __iomem *q_db)
+{
+ nvme_write_doorbell(value, q_db,
+ vdb_q->cq_doorbell_addr,
+ vdb_q->cq_eventidx_addr);
+}
+
+static inline void nvme_write_doorbell_sq(struct nvme_vdb_queue *vdb_q,
+ u16 value, u32 __iomem *q_db)
+{
+ nvme_write_doorbell(value, q_db,
+ vdb_q->sq_doorbell_addr,
+ vdb_q->sq_eventidx_addr);
+}
+
+#else /* CONFIG_NVME_VDB */
+
+struct nvme_vdb_dev {};
+
+struct nvme_vdb_queue {};
+
+static inline int nvme_dma_alloc_doorbell_mem(struct device *dev,
+ struct nvme_vdb_dev *vdb_d,
+ u32 stride)
+{
+ return 0;
+}
+
+static inline void nvme_dma_free_doorbell_mem(struct device *dev,
+ struct nvme_vdb_dev *vdb_d,
+ u32 stride)
+{}
+
+static inline void nvme_set_doorbell_memory(struct device *dev,
+ struct nvme_vdb_dev *vdb_d,
+ struct nvme_ctrl *ctrl,
+ u32 stride)
+{}
+
+static inline void nvme_init_doorbell_mem(struct nvme_vdb_dev *vdb_d,
+ struct nvme_vdb_queue *vdb_q,
+ int qid, u32 stride)
+{}
+
+static inline void nvme_write_doorbell_cq(struct nvme_vdb_queue *vdb_q,
+ u16 value, u32 __iomem *q_db)
+{
+ writel(value, q_db);
+}
+
+static inline void nvme_write_doorbell_sq(struct nvme_vdb_queue *vdb_q,
+ u16 value, u32 __iomem *q_db)
+{
+ writel(value, q_db);
+}
+
+#endif /* CONFIG_NVME_VDB */
+
+#endif /* _NVME_VDB_H */
diff --git a/include/linux/nvme.h b/include/linux/nvme.h
index d8b37ba..46d0412 100644
--- a/include/linux/nvme.h
+++ b/include/linux/nvme.h
@@ -562,6 +562,9 @@ enum nvme_admin_opcode {
nvme_admin_format_nvm = 0x80,
nvme_admin_security_send = 0x81,
nvme_admin_security_recv = 0x82,
+#ifdef CONFIG_NVME_VDB
+ nvme_admin_doorbell_memory = 0xC0,
+#endif
};
enum {
@@ -827,6 +830,16 @@ struct nvmf_property_get_command {
__u8 resv4[16];
};
+struct nvme_doorbell_memory {
+ __u8 opcode;
+ __u8 flags;
+ __u16 command_id;
+ __u32 rsvd1[5];
+ __le64 prp1;
+ __le64 prp2;
+ __u32 rsvd12[6];
+};
+
struct nvme_command {
union {
struct nvme_common_command common;
@@ -845,6 +858,7 @@ struct nvme_command {
struct nvmf_connect_command connect;
struct nvmf_property_set_command prop_set;
struct nvmf_property_get_command prop_get;
+ struct nvme_doorbell_memory doorbell_memory;
};
};
@@ -934,6 +948,9 @@ enum {
/*
* Media and Data Integrity Errors:
*/
+#ifdef CONFIG_NVME_VDB
+ NVME_SC_DOORBELL_MEMORY_INVALID = 0x1C0,
+#endif
NVME_SC_WRITE_FAULT = 0x280,
NVME_SC_READ_ERROR = 0x281,
NVME_SC_GUARD_CHECK = 0x282,
--
1.9.1
^ permalink raw reply related [flat|nested] 49+ messages in thread
* Re: [PATCH v3 RFC 2/2] nvme: improve performance for virtual NVMe devices
2016-08-16 1:41 ` Helen Koike
@ 2016-08-16 20:45 ` J Freyensee
-1 siblings, 0 replies; 49+ messages in thread
From: J Freyensee @ 2016-08-16 20:45 UTC (permalink / raw)
To: Helen Koike, hch, mlin, fes, keith.busch, rlnelson, axboe,
digitaleric, tytso, mikew, monish
Cc: open list, open list:NVM EXPRESS DRIVER, Huffman, Amber, Minturn, Dave B
On Mon, 2016-08-15 at 22:41 -0300, Helen Koike wrote:
>
> +struct nvme_doorbell_memory {
> + __u8 opcode;
> + __u8 flags;
> + __u16 command_id;
> + __u32 rsvd1[5];
> + __le64 prp1;
> + __le64 prp2;
> + __u32 rsvd12[6];
> +};
> +
> struct nvme_command {
> union {
> struct nvme_common_command common;
> @@ -845,6 +858,7 @@ struct nvme_command {
> struct nvmf_connect_command connect;
> struct nvmf_property_set_command prop_set;
> struct nvmf_property_get_command prop_get;
> + struct nvme_doorbell_memory doorbell_memory;
> };
> };
This looks like a new NVMe command being introduced, not found in the
latest NVMe specs (NVMe 1.2.1 spec or NVMe-over-Fabrics 1.0 spec)?
This is a big NACK, the command needs to be part of the NVMe standard
before adding it to the NVMe code base (this is exactly how NVMe-over-
Fabrics standard got implemented). I would bring your proposal to
nvmexpress.org.
Jay
>
> @@ -934,6 +948,9 @@ enum {
> /*
> * Media and Data Integrity Errors:
> */
> +#ifdef CONFIG_NVME_VDB
> + NVME_SC_DOORBELL_MEMORY_INVALID = 0x1C0,
> +#endif
> NVME_SC_WRITE_FAULT = 0x280,
> NVME_SC_READ_ERROR = 0x281,
> NVME_SC_GUARD_CHECK = 0x282,
^ permalink raw reply [flat|nested] 49+ messages in thread
* [PATCH v3 RFC 2/2] nvme: improve performance for virtual NVMe devices
@ 2016-08-16 20:45 ` J Freyensee
0 siblings, 0 replies; 49+ messages in thread
From: J Freyensee @ 2016-08-16 20:45 UTC (permalink / raw)
On Mon, 2016-08-15@22:41 -0300, Helen Koike wrote:
> ?
> +struct nvme_doorbell_memory {
> + __u8 opcode;
> + __u8 flags;
> + __u16 command_id;
> + __u32 rsvd1[5];
> + __le64 prp1;
> + __le64 prp2;
> + __u32 rsvd12[6];
> +};
> +
> ?struct nvme_command {
> ? union {
> ? struct nvme_common_command common;
> @@ -845,6 +858,7 @@ struct nvme_command {
> ? struct nvmf_connect_command connect;
> ? struct nvmf_property_set_command prop_set;
> ? struct nvmf_property_get_command prop_get;
> + struct nvme_doorbell_memory doorbell_memory;
> ? };
> ?};
This looks like a new NVMe command being introduced, not found in the
latest NVMe specs (NVMe 1.2.1 spec or NVMe-over-Fabrics 1.0 spec)?
This is a big NACK, the command needs to be part of the NVMe standard
before adding it to the NVMe code base (this is exactly how NVMe-over-
Fabrics standard got implemented). ?I would bring your proposal to
nvmexpress.org.
Jay
> ?
> @@ -934,6 +948,9 @@ enum {
> ? /*
> ? ?* Media and Data Integrity Errors:
> ? ?*/
> +#ifdef CONFIG_NVME_VDB
> + NVME_SC_DOORBELL_MEMORY_INVALID = 0x1C0,
> +#endif
> ? NVME_SC_WRITE_FAULT = 0x280,
> ? NVME_SC_READ_ERROR = 0x281,
> ? NVME_SC_GUARD_CHECK = 0x282,
^ permalink raw reply [flat|nested] 49+ messages in thread
* Re: [PATCH v3 RFC 2/2] nvme: improve performance for virtual NVMe devices
2016-08-16 20:45 ` J Freyensee
@ 2016-08-16 23:45 ` Keith Busch
-1 siblings, 0 replies; 49+ messages in thread
From: Keith Busch @ 2016-08-16 23:45 UTC (permalink / raw)
To: J Freyensee
Cc: Helen Koike, hch, mlin, fes, rlnelson, axboe, digitaleric, tytso,
mikew, monish, open list, open list:NVM EXPRESS DRIVER, Huffman,
Amber, Minturn, Dave B
On Tue, Aug 16, 2016 at 01:45:03PM -0700, J Freyensee wrote:
> On Mon, 2016-08-15 at 22:41 -0300, Helen Koike wrote:
> > struct nvmf_connect_command connect;
> > struct nvmf_property_set_command prop_set;
> > struct nvmf_property_get_command prop_get;
> > + struct nvme_doorbell_memory doorbell_memory;
> > };
> > };
>
> This looks like a new NVMe command being introduced, not found in the
> latest NVMe specs (NVMe 1.2.1 spec or NVMe-over-Fabrics 1.0 spec)?
>
> This is a big NACK, the command needs to be part of the NVMe standard
> before adding it to the NVMe code base (this is exactly how NVMe-over-
> Fabrics standard got implemented). I would bring your proposal to
> nvmexpress.org.
While this is an approved TPAR up for consideration soon, I don't think we
want to include this in mainline before it is ratified lest the current
form conflicts with the future spec.
We can still review and comment on the code, though it's not normally
done publicly until the spec is also public. The proposal originated
from a public RFC, though, so it's not a secret.
^ permalink raw reply [flat|nested] 49+ messages in thread
* [PATCH v3 RFC 2/2] nvme: improve performance for virtual NVMe devices
@ 2016-08-16 23:45 ` Keith Busch
0 siblings, 0 replies; 49+ messages in thread
From: Keith Busch @ 2016-08-16 23:45 UTC (permalink / raw)
On Tue, Aug 16, 2016@01:45:03PM -0700, J Freyensee wrote:
> On Mon, 2016-08-15@22:41 -0300, Helen Koike wrote:
> > ? struct nvmf_connect_command connect;
> > ? struct nvmf_property_set_command prop_set;
> > ? struct nvmf_property_get_command prop_get;
> > + struct nvme_doorbell_memory doorbell_memory;
> > ? };
> > ?};
>
> This looks like a new NVMe command being introduced, not found in the
> latest NVMe specs (NVMe 1.2.1 spec or NVMe-over-Fabrics 1.0 spec)?
>
> This is a big NACK, the command needs to be part of the NVMe standard
> before adding it to the NVMe code base (this is exactly how NVMe-over-
> Fabrics standard got implemented). ?I would bring your proposal to
> nvmexpress.org.
While this is an approved TPAR up for consideration soon, I don't think we
want to include this in mainline before it is ratified lest the current
form conflicts with the future spec.
We can still review and comment on the code, though it's not normally
done publicly until the spec is also public. The proposal originated
from a public RFC, though, so it's not a secret.
^ permalink raw reply [flat|nested] 49+ messages in thread
* [PATCH v4 RFC] nvme: improve performance for virtual NVMe devices
2016-08-16 1:41 ` Helen Koike
(?)
(?)
@ 2017-03-17 21:44 ` Helen Koike
2017-03-17 22:28 ` Keith Busch
-1 siblings, 1 reply; 49+ messages in thread
From: Helen Koike @ 2017-03-17 21:44 UTC (permalink / raw)
From: Helen Koike <helen.koike@collabora.co.uk>
This change provides a mechanism to reduce the number of MMIO doorbell
writes for the NVMe driver. When running in a virtualized environment
like QEMU, the cost of an MMIO is quite hefy here. The main idea for
the patch is provide the device two memory location locations:
1) to store the doorbell values so they can be lookup without the doorbell
MMIO write
2) to store an event index.
I believe the doorbell value is obvious, the event index not so much.
Similar to the virtio specification, the virtual device can tell the
driver (guest OS) not to write MMIO unless you are writing past this
value.
FYI: doorbell values are written by the nvme driver (guest OS) and the
event index is written by the virtual device (host OS).
The patch implements a new admin command that will communicate where
these two memory locations reside. If the command fails, the nvme
driver will work as before without any optimizations.
Contributions:
Eric Northup <digitaleric at google.com>
Frank Swiderski <fes at google.com>
Ted Tso <tytso at mit.edu>
Keith Busch <keith.busch at intel.com>
Just to give an idea on the performance boost with the vendor
extension: Running fio [1], a stock NVMe driver I get about 200K read
IOPs with my vendor patch I get about 1000K read IOPs. This was
running with a null device i.e. the backing device simply returned
success on every read IO request.
[1] Running on a 4 core machine:
fio --time_based --name=benchmark --runtime=30
--filename=/dev/nvme0n1 --nrfiles=1 --ioengine=libaio --iodepth=32
--direct=1 --invalidate=1 --verify=0 --verify_fatal=0 --numjobs=4
--rw=randread --blocksize=4k --randrepeat=false
Signed-off-by: Rob Nelson <rlnelson at google.com>
[mlin: port for upstream]
Signed-off-by: Ming Lin <mlin at kernel.org>
[koike: updated for upstream]
Signed-off-by: Helen Koike <helen.koike at collabora.co.uk>
---
This patch is based on git://git.infradead.org/nvme.git master
Tested through Google Cloud Engine
TPAR is ratified by the NVME working group
Changes since last version
- Rename to dbbuf (be closer to the latest TPAR)
- Modify the opcodes according to the latest TPAR
- Check if the device support this feature through the OACS bit
- little cleanups
---
drivers/nvme/host/Kconfig | 9 ++++
drivers/nvme/host/Makefile | 1 +
drivers/nvme/host/dbbuf.c | 125 +++++++++++++++++++++++++++++++++++++++++++++
drivers/nvme/host/dbbuf.h | 118 ++++++++++++++++++++++++++++++++++++++++++
drivers/nvme/host/pci.c | 22 +++++++-
include/linux/nvme.h | 13 +++++
6 files changed, 286 insertions(+), 2 deletions(-)
create mode 100644 drivers/nvme/host/dbbuf.c
create mode 100644 drivers/nvme/host/dbbuf.h
diff --git a/drivers/nvme/host/Kconfig b/drivers/nvme/host/Kconfig
index 90745a6..7ada6f3 100644
--- a/drivers/nvme/host/Kconfig
+++ b/drivers/nvme/host/Kconfig
@@ -59,4 +59,13 @@ config NVME_FC
To configure a NVMe over Fabrics controller use the nvme-cli tool
from https://github.com/linux-nvme/nvme-cli.
+config NVME_DBBUF
+ bool "NVM Express Doorbell Buffer Config Command"
+ depends on NVME_CORE
+ ---help---
+ This provides support for the Doorbell Buffer Config Command, which
+ reduces the number of required MMIOs to ring doorbells, improving
+ performance for virtualised environments where MMIO causes a high
+ overhead.
+
If unsure, say N.
diff --git a/drivers/nvme/host/Makefile b/drivers/nvme/host/Makefile
index f1a7d94..ed9efe5 100644
--- a/drivers/nvme/host/Makefile
+++ b/drivers/nvme/host/Makefile
@@ -9,6 +9,7 @@ nvme-core-$(CONFIG_BLK_DEV_NVME_SCSI) += scsi.o
nvme-core-$(CONFIG_NVM) += lightnvm.o
nvme-y += pci.o
+nvme-$(CONFIG_NVME_DBBUF) += dbbuf.o
nvme-fabrics-y += fabrics.o
diff --git a/drivers/nvme/host/dbbuf.c b/drivers/nvme/host/dbbuf.c
new file mode 100644
index 0000000..3900bb5
--- /dev/null
+++ b/drivers/nvme/host/dbbuf.c
@@ -0,0 +1,125 @@
+/*
+ * NVM Express device driver
+ * Copyright (C) 2015-2017, Google, Inc.
+ *
+ * This program is free software; you can redistribute it and/or modify it
+ * under the terms and conditions of the GNU General Public License,
+ * version 2, as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope it will be useful, but WITHOUT
+ * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
+ * FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License for
+ * more details.
+ */
+
+#include "nvme.h"
+#include "dbbuf.h"
+
+static inline unsigned int nvme_dbbuf_size(u32 stride)
+{
+ return ((num_possible_cpus() + 1) * 8 * stride);
+}
+
+int nvme_dma_alloc_dbbuf(struct device *dev,
+ struct nvme_dbbuf_dev *dbbuf_d,
+ u32 stride)
+{
+ unsigned int mem_size = nvme_dbbuf_size(stride);
+
+ dbbuf_d->db_mem = dma_alloc_coherent(dev, mem_size, &dbbuf_d->doorbell,
+ GFP_KERNEL);
+ if (!dbbuf_d->db_mem)
+ return -ENOMEM;
+ dbbuf_d->ei_mem = dma_alloc_coherent(dev, mem_size, &dbbuf_d->eventidx,
+ GFP_KERNEL);
+ if (!dbbuf_d->ei_mem) {
+ dma_free_coherent(dev, mem_size,
+ dbbuf_d->db_mem, dbbuf_d->doorbell);
+ dbbuf_d->db_mem = NULL;
+ return -ENOMEM;
+ }
+
+ return 0;
+}
+
+void nvme_dma_free_dbbuf(struct device *dev,
+ struct nvme_dbbuf_dev *dbbuf_d,
+ u32 stride)
+{
+ unsigned int mem_size = nvme_dbbuf_size(stride);
+
+ if (dbbuf_d->db_mem) {
+ dma_free_coherent(dev, mem_size,
+ dbbuf_d->db_mem, dbbuf_d->doorbell);
+ dbbuf_d->db_mem = NULL;
+ }
+ if (dbbuf_d->ei_mem) {
+ dma_free_coherent(dev, mem_size,
+ dbbuf_d->ei_mem, dbbuf_d->eventidx);
+ dbbuf_d->ei_mem = NULL;
+ }
+}
+
+void nvme_init_dbbuf(struct nvme_dbbuf_dev *dbbuf_d,
+ struct nvme_dbbuf_queue *dbbuf_q,
+ int qid, u32 stride)
+{
+ if (!dbbuf_d->db_mem || !qid)
+ return;
+
+ dbbuf_q->sq_doorbell_addr = &dbbuf_d->db_mem[SQ_IDX(qid, stride)];
+ dbbuf_q->cq_doorbell_addr = &dbbuf_d->db_mem[CQ_IDX(qid, stride)];
+ dbbuf_q->sq_eventidx_addr = &dbbuf_d->ei_mem[SQ_IDX(qid, stride)];
+ dbbuf_q->cq_eventidx_addr = &dbbuf_d->ei_mem[CQ_IDX(qid, stride)];
+}
+
+void nvme_set_dbbuf(struct device *dev,
+ struct nvme_dbbuf_dev *dbbuf_d,
+ struct nvme_ctrl *ctrl,
+ u32 stride)
+{
+ struct nvme_command c;
+
+ if (!dbbuf_d->db_mem)
+ return;
+
+ memset(&c, 0, sizeof(c));
+ c.dbbuf.opcode = nvme_admin_dbbuf;
+ c.dbbuf.prp1 = cpu_to_le64(dbbuf_d->doorbell);
+ c.dbbuf.prp2 = cpu_to_le64(dbbuf_d->eventidx);
+
+ if (nvme_submit_sync_cmd(ctrl->admin_q, &c, NULL, 0))
+ /* Free memory and continue on */
+ nvme_dma_free_dbbuf(dev, dbbuf_d, stride);
+}
+
+static inline int nvme_ext_need_event(u16 event_idx, u16 new_idx, u16 old)
+{
+ /* Borrowed from vring_need_event */
+ return (u16)(new_idx - event_idx - 1) < (u16)(new_idx - old);
+}
+
+void nvme_write_doorbell(u16 value,
+ u32 __iomem *q_db,
+ u32 *db_addr,
+ volatile u32 *event_idx)
+{
+ u16 old_value;
+
+ if (!db_addr) {
+ writel(value, q_db);
+ return;
+ }
+
+ /*
+ * Ensure that the queue is written before updating
+ * the doorbell in memory
+ */
+ wmb();
+
+ old_value = *db_addr;
+ *db_addr = value;
+
+ if (nvme_ext_need_event(*event_idx, value, old_value))
+ writel(value, q_db);
+}
diff --git a/drivers/nvme/host/dbbuf.h b/drivers/nvme/host/dbbuf.h
new file mode 100644
index 0000000..0c0a83f
--- /dev/null
+++ b/drivers/nvme/host/dbbuf.h
@@ -0,0 +1,118 @@
+/*
+ * NVM Express device driver
+ * Copyright (C) 2015-2017, Google, Inc.
+ *
+ * This program is free software; you can redistribute it and/or modify it
+ * under the terms and conditions of the GNU General Public License,
+ * version 2, as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope it will be useful, but WITHOUT
+ * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
+ * FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License for
+ * more details.
+ */
+
+#ifndef _NVME_DBBUF_H
+#define _NVME_DBBUF_H
+
+#define SQ_IDX(qid, stride) ((qid) * 2 * (stride))
+#define CQ_IDX(qid, stride) (((qid) * 2 + 1) * (stride))
+
+#ifdef CONFIG_NVME_DBBUF
+
+struct nvme_dbbuf_dev {
+ u32 *db_mem;
+ dma_addr_t doorbell;
+ u32 *ei_mem;
+ dma_addr_t eventidx;
+};
+
+struct nvme_dbbuf_queue {
+ u32 *sq_doorbell_addr;
+ u32 *sq_eventidx_addr;
+ u32 *cq_doorbell_addr;
+ u32 *cq_eventidx_addr;
+};
+
+int nvme_dma_alloc_dbbuf(struct device *dev,
+ struct nvme_dbbuf_dev *dbbuf_d,
+ u32 stride);
+
+void nvme_dma_free_dbbuf(struct device *dev,
+ struct nvme_dbbuf_dev *dbbuf_d,
+ u32 stride);
+
+void nvme_init_dbbuf(struct nvme_dbbuf_dev *dbbuf_d,
+ struct nvme_dbbuf_queue *dbbuf_q,
+ int qid, u32 stride);
+
+void nvme_set_dbbuf(struct device *dev,
+ struct nvme_dbbuf_dev *dbbuf_d,
+ struct nvme_ctrl *ctrl,
+ u32 stride);
+
+void nvme_write_doorbell(u16 value,
+ u32 __iomem *q_db,
+ u32 *db_addr,
+ volatile u32 *event_idx);
+
+static inline void nvme_write_doorbell_cq(struct nvme_dbbuf_queue *dbbuf_q,
+ u16 value, u32 __iomem *q_db)
+{
+ nvme_write_doorbell(value, q_db,
+ dbbuf_q->cq_doorbell_addr,
+ dbbuf_q->cq_eventidx_addr);
+}
+
+static inline void nvme_write_doorbell_sq(struct nvme_dbbuf_queue *dbbuf_q,
+ u16 value, u32 __iomem *q_db)
+{
+ nvme_write_doorbell(value, q_db,
+ dbbuf_q->sq_doorbell_addr,
+ dbbuf_q->sq_eventidx_addr);
+}
+
+#else /* CONFIG_NVME_DBBUF */
+
+struct nvme_dbbuf_dev {};
+
+struct nvme_dbbuf_queue {};
+
+static inline int nvme_dma_alloc_dbbuf(struct device *dev,
+ struct nvme_dbbuf_dev *dbbuf_d,
+ u32 stride)
+{
+ return 0;
+}
+
+static inline void nvme_dma_free_dbbuf(struct device *dev,
+ struct nvme_dbbuf_dev *dbbuf_d,
+ u32 stride)
+{}
+
+static inline void nvme_set_dbbuf(struct device *dev,
+ struct nvme_dbbuf_dev *dbbuf_d,
+ struct nvme_ctrl *ctrl,
+ u32 stride)
+{}
+
+static inline void nvme_init_dbbuf(struct nvme_dbbuf_dev *dbbuf_d,
+ struct nvme_dbbuf_queue *dbbuf_q,
+ int qid, u32 stride)
+{}
+
+static inline void nvme_write_doorbell_cq(struct nvme_dbbuf_queue *dbbuf_q,
+ u16 value, u32 __iomem *q_db)
+{
+ writel(value, q_db);
+}
+
+static inline void nvme_write_doorbell_sq(struct nvme_dbbuf_queue *dbbuf_q,
+ u16 value, u32 __iomem *q_db)
+{
+ writel(value, q_db);
+}
+
+#endif /* CONFIG_NVME_DBBUF */
+
+#endif /* _NVME_DBBUF_H */
diff --git a/drivers/nvme/host/pci.c b/drivers/nvme/host/pci.c
index 5b7bd9c..3723697 100644
--- a/drivers/nvme/host/pci.c
+++ b/drivers/nvme/host/pci.c
@@ -46,6 +46,7 @@
#include <linux/sed-opal.h>
#include "nvme.h"
+#include "dbbuf.h"
#define NVME_Q_DEPTH 1024
#define NVME_AQ_DEPTH 256
@@ -103,6 +104,7 @@ struct nvme_dev {
u32 cmbloc;
struct nvme_ctrl ctrl;
struct completion ioq_wait;
+ struct nvme_dbbuf_dev dbbuf_d;
};
static inline struct nvme_dev *to_nvme_dev(struct nvme_ctrl *ctrl)
@@ -133,6 +135,7 @@ struct nvme_queue {
u16 qid;
u8 cq_phase;
u8 cqe_seen;
+ struct nvme_dbbuf_queue dbbuf_q;
};
/*
@@ -174,6 +177,7 @@ static inline void _nvme_check_size(void)
BUILD_BUG_ON(sizeof(struct nvme_id_ns) != 4096);
BUILD_BUG_ON(sizeof(struct nvme_lba_range_type) != 64);
BUILD_BUG_ON(sizeof(struct nvme_smart_log) != 512);
+ BUILD_BUG_ON(sizeof(struct nvme_dbbuf) != 64);
}
/*
@@ -300,7 +304,7 @@ static void __nvme_submit_cmd(struct nvme_queue *nvmeq,
if (++tail == nvmeq->q_depth)
tail = 0;
- writel(tail, nvmeq->q_db);
+ nvme_write_doorbell_sq(&nvmeq->dbbuf_q, tail, nvmeq->q_db);
nvmeq->sq_tail = tail;
}
@@ -716,7 +720,8 @@ static void __nvme_process_cq(struct nvme_queue *nvmeq, unsigned int *tag)
return;
if (likely(nvmeq->cq_vector >= 0))
- writel(head, nvmeq->q_db + nvmeq->dev->db_stride);
+ nvme_write_doorbell_cq(&nvmeq->dbbuf_q, head,
+ nvmeq->q_db + nvmeq->dev->db_stride);
nvmeq->cq_head = head;
nvmeq->cq_phase = phase;
@@ -1066,6 +1071,7 @@ static struct nvme_queue *nvme_alloc_queue(struct nvme_dev *dev, int qid,
nvmeq->q_depth = depth;
nvmeq->qid = qid;
nvmeq->cq_vector = -1;
+ nvme_init_dbbuf(&dev->dbbuf_d, &nvmeq->dbbuf_q, qid, dev->db_stride);
dev->queues[qid] = nvmeq;
dev->queue_count++;
@@ -1099,6 +1105,7 @@ static void nvme_init_queue(struct nvme_queue *nvmeq, u16 qid)
nvmeq->cq_phase = 1;
nvmeq->q_db = &dev->dbs[qid * 2 * dev->db_stride];
memset((void *)nvmeq->cqes, 0, CQ_SIZE(nvmeq->q_depth));
+ nvme_init_dbbuf(&dev->dbbuf_d, &nvmeq->dbbuf_q, qid, dev->db_stride);
dev->online_queues++;
spin_unlock_irq(&nvmeq->q_lock);
}
@@ -1568,6 +1575,9 @@ static int nvme_dev_add(struct nvme_dev *dev)
if (blk_mq_alloc_tag_set(&dev->tagset))
return 0;
dev->ctrl.tagset = &dev->tagset;
+
+ nvme_set_dbbuf(dev->dev, &dev->dbbuf_d,
+ &dev->ctrl, dev->db_stride);
} else {
blk_mq_update_nr_hw_queues(&dev->tagset, dev->online_queues - 1);
@@ -1700,6 +1710,7 @@ static void nvme_dev_disable(struct nvme_dev *dev, bool shutdown)
nvme_disable_admin_queue(dev, shutdown);
}
nvme_pci_disable(dev);
+ nvme_dma_free_dbbuf(dev->dev, &dev->dbbuf_d, dev->db_stride);
blk_mq_tagset_busy_iter(&dev->tagset, nvme_cancel_request, &dev->ctrl);
blk_mq_tagset_busy_iter(&dev->admin_tagset, nvme_cancel_request, &dev->ctrl);
@@ -1800,6 +1811,13 @@ static void nvme_reset_work(struct work_struct *work)
dev->ctrl.opal_dev = NULL;
}
+ if (dev->ctrl.oacs & NVME_CTRL_OACS_DBBUF_SUPP) {
+ result = nvme_dma_alloc_dbbuf(dev->dev, &dev->dbbuf_d,
+ dev->db_stride);
+ if (result)
+ goto out;
+ }
+
result = nvme_setup_io_queues(dev);
if (result)
goto out;
diff --git a/include/linux/nvme.h b/include/linux/nvme.h
index c43d435..43a6289 100644
--- a/include/linux/nvme.h
+++ b/include/linux/nvme.h
@@ -245,6 +245,7 @@ enum {
NVME_CTRL_ONCS_WRITE_ZEROES = 1 << 3,
NVME_CTRL_VWC_PRESENT = 1 << 0,
NVME_CTRL_OACS_SEC_SUPP = 1 << 0,
+ NVME_CTRL_OACS_DBBUF_SUPP = 1 << 7,
};
struct nvme_lbaf {
@@ -603,6 +604,7 @@ enum nvme_admin_opcode {
nvme_admin_download_fw = 0x11,
nvme_admin_ns_attach = 0x15,
nvme_admin_keep_alive = 0x18,
+ nvme_admin_dbbuf = 0x7C,
nvme_admin_format_nvm = 0x80,
nvme_admin_security_send = 0x81,
nvme_admin_security_recv = 0x82,
@@ -874,6 +876,16 @@ struct nvmf_property_get_command {
__u8 resv4[16];
};
+struct nvme_dbbuf {
+ __u8 opcode;
+ __u8 flags;
+ __u16 command_id;
+ __u32 rsvd1[5];
+ __le64 prp1;
+ __le64 prp2;
+ __u32 rsvd12[6];
+};
+
struct nvme_command {
union {
struct nvme_common_command common;
@@ -893,6 +905,7 @@ struct nvme_command {
struct nvmf_connect_command connect;
struct nvmf_property_set_command prop_set;
struct nvmf_property_get_command prop_get;
+ struct nvme_dbbuf dbbuf;
};
};
--
2.7.4
^ permalink raw reply related [flat|nested] 49+ messages in thread
* [PATCH v4 RFC] nvme: improve performance for virtual NVMe devices
2017-03-17 21:44 ` [PATCH v4 RFC] " Helen Koike
@ 2017-03-17 22:28 ` Keith Busch
2017-03-17 22:26 ` Helen Koike
2017-03-24 4:23 ` [PATCH v5 " Helen Koike
0 siblings, 2 replies; 49+ messages in thread
From: Keith Busch @ 2017-03-17 22:28 UTC (permalink / raw)
On Fri, Mar 17, 2017@06:44:59PM -0300, Helen Koike wrote:
> diff --git a/drivers/nvme/host/Kconfig b/drivers/nvme/host/Kconfig
> index 90745a6..7ada6f3 100644
> --- a/drivers/nvme/host/Kconfig
> +++ b/drivers/nvme/host/Kconfig
> @@ -59,4 +59,13 @@ config NVME_FC
> To configure a NVMe over Fabrics controller use the nvme-cli tool
> from https://github.com/linux-nvme/nvme-cli.
>
> +config NVME_DBBUF
> + bool "NVM Express Doorbell Buffer Config Command"
> + depends on NVME_CORE
> + ---help---
> + This provides support for the Doorbell Buffer Config Command, which
> + reduces the number of required MMIOs to ring doorbells, improving
> + performance for virtualised environments where MMIO causes a high
> + overhead.
> +
> If unsure, say N.
It looks like you took the last line from NVME_FC's help. But I don't
think this capabilty should enabled through kernel config, though. Just
have the driver react based on the device's advertised capabilities, ya?
^ permalink raw reply [flat|nested] 49+ messages in thread
* [PATCH v4 RFC] nvme: improve performance for virtual NVMe devices
2017-03-17 22:28 ` Keith Busch
@ 2017-03-17 22:26 ` Helen Koike
2017-03-24 4:23 ` [PATCH v5 " Helen Koike
1 sibling, 0 replies; 49+ messages in thread
From: Helen Koike @ 2017-03-17 22:26 UTC (permalink / raw)
On 2017-03-17 07:28 PM, Keith Busch wrote:
> On Fri, Mar 17, 2017@06:44:59PM -0300, Helen Koike wrote:
> > diff --git a/drivers/nvme/host/Kconfig b/drivers/nvme/host/Kconfig
> > index 90745a6..7ada6f3 100644
> > --- a/drivers/nvme/host/Kconfig
> > +++ b/drivers/nvme/host/Kconfig
> > @@ -59,4 +59,13 @@ config NVME_FC
> > To configure a NVMe over Fabrics controller use the nvme-cli tool
> > from https://github.com/linux-nvme/nvme-cli.
> >
> > +config NVME_DBBUF
> > + bool "NVM Express Doorbell Buffer Config Command"
> > + depends on NVME_CORE
> > + ---help---
> > + This provides support for the Doorbell Buffer Config Command, which
> > + reduces the number of required MMIOs to ring doorbells, improving
> > + performance for virtualised environments where MMIO causes a high
> > + overhead.
> > +
> > If unsure, say N.
>
> It looks like you took the last line from NVME_FC's help. But I don't
> think this capabilty should enabled through kernel config, though. Just
> have the driver react based on the device's advertised capabilities, ya?
>
The advantage is that you can chose to not use the feature (not sure if it has any usecase) and reduce the code size if unused. But the code is really small. I'll remove CONF_NVME_DBBUF then and send another version, the code will be much cleaner imho.
Helen
^ permalink raw reply [flat|nested] 49+ messages in thread
* [PATCH v5 RFC] nvme: improve performance for virtual NVMe devices
2017-03-17 22:28 ` Keith Busch
2017-03-17 22:26 ` Helen Koike
@ 2017-03-24 4:23 ` Helen Koike
2017-03-27 9:49 ` Christoph Hellwig
` (2 more replies)
1 sibling, 3 replies; 49+ messages in thread
From: Helen Koike @ 2017-03-24 4:23 UTC (permalink / raw)
This change provides a mechanism to reduce the number of MMIO doorbell
writes for the NVMe driver. When running in a virtualized environment
like QEMU, the cost of an MMIO is quite hefy here. The main idea for
the patch is provide the device two memory location locations:
1) to store the doorbell values so they can be lookup without the doorbell
MMIO write
2) to store an event index.
I believe the doorbell value is obvious, the event index not so much.
Similar to the virtio specification, the virtual device can tell the
driver (guest OS) not to write MMIO unless you are writing past this
value.
FYI: doorbell values are written by the nvme driver (guest OS) and the
event index is written by the virtual device (host OS).
The patch implements a new admin command that will communicate where
these two memory locations reside. If the command fails, the nvme
driver will work as before without any optimizations.
Contributions:
Eric Northup <digitaleric at google.com>
Frank Swiderski <fes at google.com>
Ted Tso <tytso at mit.edu>
Keith Busch <keith.busch at intel.com>
Just to give an idea on the performance boost with the vendor
extension: Running fio [1], a stock NVMe driver I get about 200K read
IOPs with my vendor patch I get about 1000K read IOPs. This was
running with a null device i.e. the backing device simply returned
success on every read IO request.
[1] Running on a 4 core machine:
fio --time_based --name=benchmark --runtime=30
--filename=/dev/nvme0n1 --nrfiles=1 --ioengine=libaio --iodepth=32
--direct=1 --invalidate=1 --verify=0 --verify_fatal=0 --numjobs=4
--rw=randread --blocksize=4k --randrepeat=false
Signed-off-by: Rob Nelson <rlnelson at google.com>
[mlin: port for upstream]
Signed-off-by: Ming Lin <mlin at kernel.org>
[koike: updated for upstream]
Signed-off-by: Helen Koike <helen.koike at collabora.com>
---
This patch is based on git://git.infradead.org/nvme.git master
Tested through Google Cloud Engine
TPAR is ratified by the NVME working group
Changes since v4
- Remove CONFIG_NVME_DBBUF
- Remove dbbuf.{c,h}, move the code to pci.c
- Coding style changes
- Functions and vaiables renamed
Changes since v3
- Rename to dbbuf (be closer to the latest TPAR)
- Modify the opcodes according to the latest TPAR
- Check if the device support this feature through the OACS bit
- little cleanups
---
drivers/nvme/host/pci.c | 144 +++++++++++++++++++++++++++++++++++++++++++++++-
include/linux/nvme.h | 13 +++++
2 files changed, 155 insertions(+), 2 deletions(-)
diff --git a/drivers/nvme/host/pci.c b/drivers/nvme/host/pci.c
index 5b7bd9c..4289eb1 100644
--- a/drivers/nvme/host/pci.c
+++ b/drivers/nvme/host/pci.c
@@ -51,6 +51,8 @@
#define NVME_AQ_DEPTH 256
#define SQ_SIZE(depth) (depth * sizeof(struct nvme_command))
#define CQ_SIZE(depth) (depth * sizeof(struct nvme_completion))
+#define SQ_IDX(qid, stride) ((qid) * 2 * (stride))
+#define CQ_IDX(qid, stride) (((qid) * 2 + 1) * (stride))
/*
* We handle AEN commands ourselves and don't even let the
@@ -103,6 +105,12 @@ struct nvme_dev {
u32 cmbloc;
struct nvme_ctrl ctrl;
struct completion ioq_wait;
+ struct {
+ u32 *dbs;
+ u32 *eis;
+ dma_addr_t dbs_dma_addr;
+ dma_addr_t eis_dma_addr;
+ } dbbuf;
};
static inline struct nvme_dev *to_nvme_dev(struct nvme_ctrl *ctrl)
@@ -133,6 +141,12 @@ struct nvme_queue {
u16 qid;
u8 cq_phase;
u8 cqe_seen;
+ struct {
+ u32 *sq_db;
+ u32 *cq_db;
+ u32 *sq_ei;
+ u32 *cq_ei;
+ } dbbuf;
};
/*
@@ -174,6 +188,121 @@ static inline void _nvme_check_size(void)
BUILD_BUG_ON(sizeof(struct nvme_id_ns) != 4096);
BUILD_BUG_ON(sizeof(struct nvme_lba_range_type) != 64);
BUILD_BUG_ON(sizeof(struct nvme_smart_log) != 512);
+ BUILD_BUG_ON(sizeof(struct nvme_dbbuf) != 64);
+}
+
+static inline unsigned int nvme_dbbuf_size(u32 stride)
+{
+ return ((num_possible_cpus() + 1) * 8 * stride);
+}
+
+static int nvme_dbbuf_dma_alloc(struct nvme_dev *dev)
+{
+ unsigned int mem_size = nvme_dbbuf_size(dev->db_stride);
+
+ dev->dbbuf.dbs = dma_alloc_coherent(dev->dev, mem_size,
+ &dev->dbbuf.dbs_dma_addr,
+ GFP_KERNEL);
+ if (!dev->dbbuf.dbs)
+ return -ENOMEM;
+ dev->dbbuf.eis = dma_alloc_coherent(dev->dev, mem_size,
+ &dev->dbbuf.eis_dma_addr,
+ GFP_KERNEL);
+ if (!dev->dbbuf.eis) {
+ dma_free_coherent(dev->dev, mem_size,
+ dev->dbbuf.dbs, dev->dbbuf.dbs_dma_addr);
+ dev->dbbuf.dbs = NULL;
+ return -ENOMEM;
+ }
+
+ return 0;
+}
+
+static void nvme_dbbuf_dma_free(struct nvme_dev *dev)
+{
+ unsigned int mem_size = nvme_dbbuf_size(dev->db_stride);
+
+ if (dev->dbbuf.dbs) {
+ dma_free_coherent(dev->dev, mem_size,
+ dev->dbbuf.dbs, dev->dbbuf.dbs_dma_addr);
+ dev->dbbuf.dbs = NULL;
+ }
+ if (dev->dbbuf.eis) {
+ dma_free_coherent(dev->dev, mem_size,
+ dev->dbbuf.eis, dev->dbbuf.eis_dma_addr);
+ dev->dbbuf.eis = NULL;
+ }
+}
+
+static void nvme_dbbuf_init(struct nvme_dev *dev,
+ struct nvme_queue *nvmeq, int qid)
+{
+ if (!dev->dbbuf.dbs || !qid)
+ return;
+
+ nvmeq->dbbuf.sq_db = &dev->dbbuf.dbs[SQ_IDX(qid, dev->db_stride)];
+ nvmeq->dbbuf.cq_db = &dev->dbbuf.dbs[CQ_IDX(qid, dev->db_stride)];
+ nvmeq->dbbuf.sq_ei = &dev->dbbuf.eis[SQ_IDX(qid, dev->db_stride)];
+ nvmeq->dbbuf.cq_ei = &dev->dbbuf.eis[CQ_IDX(qid, dev->db_stride)];
+}
+
+static void nvme_dbbuf_set(struct nvme_dev *dev)
+{
+ struct nvme_command c;
+
+ if (!dev->dbbuf.dbs)
+ return;
+
+ memset(&c, 0, sizeof(c));
+ c.dbbuf.opcode = nvme_admin_dbbuf;
+ c.dbbuf.prp1 = cpu_to_le64(dev->dbbuf.dbs_dma_addr);
+ c.dbbuf.prp2 = cpu_to_le64(dev->dbbuf.eis_dma_addr);
+
+ if (nvme_submit_sync_cmd(dev->ctrl.admin_q, &c, NULL, 0))
+ /* Free memory and continue on */
+ nvme_dbbuf_dma_free(dev);
+}
+
+static inline int nvme_dbbuf_need_event(u16 event_idx, u16 new_idx, u16 old)
+{
+ /* Borrowed from vring_need_event */
+ return (u16)(new_idx - event_idx - 1) < (u16)(new_idx - old);
+}
+
+static void nvme_write_doorbell(u16 value,
+ u32 __iomem *db,
+ u32 *dbbuf_db,
+ volatile u32 *dbbuf_ei)
+{
+ u16 old_value;
+
+ if (!dbbuf_db) {
+ writel(value, db);
+ return;
+ }
+
+ /*
+ * Ensure that the queue is written before updating
+ * the doorbell in memory
+ */
+ wmb();
+
+ old_value = *dbbuf_db;
+ *dbbuf_db = value;
+ if (nvme_dbbuf_need_event(*dbbuf_ei, value, old_value))
+ writel(value, db);
+}
+
+static inline void nvme_write_doorbell_cq(struct nvme_queue *nvmeq, u16 value)
+{
+ nvme_write_doorbell(value, nvmeq->q_db + nvmeq->dev->db_stride,
+ nvmeq->dbbuf.cq_db, nvmeq->dbbuf.cq_ei);
+}
+
+static inline void nvme_write_doorbell_sq(struct nvme_queue *nvmeq, u16 value)
+{
+ nvme_write_doorbell(value, nvmeq->q_db,
+ nvmeq->dbbuf.sq_db, nvmeq->dbbuf.sq_ei);
}
/*
@@ -300,7 +429,7 @@ static void __nvme_submit_cmd(struct nvme_queue *nvmeq,
if (++tail == nvmeq->q_depth)
tail = 0;
- writel(tail, nvmeq->q_db);
+ nvme_write_doorbell_sq(nvmeq, tail);
nvmeq->sq_tail = tail;
}
@@ -716,7 +845,7 @@ static void __nvme_process_cq(struct nvme_queue *nvmeq, unsigned int *tag)
return;
if (likely(nvmeq->cq_vector >= 0))
- writel(head, nvmeq->q_db + nvmeq->dev->db_stride);
+ nvme_write_doorbell_cq(nvmeq, head);
nvmeq->cq_head = head;
nvmeq->cq_phase = phase;
@@ -1066,6 +1195,7 @@ static struct nvme_queue *nvme_alloc_queue(struct nvme_dev *dev, int qid,
nvmeq->q_depth = depth;
nvmeq->qid = qid;
nvmeq->cq_vector = -1;
+ nvme_dbbuf_init(dev, nvmeq, qid);
dev->queues[qid] = nvmeq;
dev->queue_count++;
@@ -1099,6 +1229,7 @@ static void nvme_init_queue(struct nvme_queue *nvmeq, u16 qid)
nvmeq->cq_phase = 1;
nvmeq->q_db = &dev->dbs[qid * 2 * dev->db_stride];
memset((void *)nvmeq->cqes, 0, CQ_SIZE(nvmeq->q_depth));
+ nvme_dbbuf_init(dev, nvmeq, qid);
dev->online_queues++;
spin_unlock_irq(&nvmeq->q_lock);
}
@@ -1568,6 +1699,8 @@ static int nvme_dev_add(struct nvme_dev *dev)
if (blk_mq_alloc_tag_set(&dev->tagset))
return 0;
dev->ctrl.tagset = &dev->tagset;
+
+ nvme_dbbuf_set(dev);
} else {
blk_mq_update_nr_hw_queues(&dev->tagset, dev->online_queues - 1);
@@ -1700,6 +1833,7 @@ static void nvme_dev_disable(struct nvme_dev *dev, bool shutdown)
nvme_disable_admin_queue(dev, shutdown);
}
nvme_pci_disable(dev);
+ nvme_dbbuf_dma_free(dev);
blk_mq_tagset_busy_iter(&dev->tagset, nvme_cancel_request, &dev->ctrl);
blk_mq_tagset_busy_iter(&dev->admin_tagset, nvme_cancel_request, &dev->ctrl);
@@ -1800,6 +1934,12 @@ static void nvme_reset_work(struct work_struct *work)
dev->ctrl.opal_dev = NULL;
}
+ if (dev->ctrl.oacs & NVME_CTRL_OACS_DBBUF_SUPP) {
+ result = nvme_dbbuf_dma_alloc(dev);
+ if (result)
+ goto out;
+ }
+
result = nvme_setup_io_queues(dev);
if (result)
goto out;
diff --git a/include/linux/nvme.h b/include/linux/nvme.h
index c43d435..43a6289 100644
--- a/include/linux/nvme.h
+++ b/include/linux/nvme.h
@@ -245,6 +245,7 @@ enum {
NVME_CTRL_ONCS_WRITE_ZEROES = 1 << 3,
NVME_CTRL_VWC_PRESENT = 1 << 0,
NVME_CTRL_OACS_SEC_SUPP = 1 << 0,
+ NVME_CTRL_OACS_DBBUF_SUPP = 1 << 7,
};
struct nvme_lbaf {
@@ -603,6 +604,7 @@ enum nvme_admin_opcode {
nvme_admin_download_fw = 0x11,
nvme_admin_ns_attach = 0x15,
nvme_admin_keep_alive = 0x18,
+ nvme_admin_dbbuf = 0x7C,
nvme_admin_format_nvm = 0x80,
nvme_admin_security_send = 0x81,
nvme_admin_security_recv = 0x82,
@@ -874,6 +876,16 @@ struct nvmf_property_get_command {
__u8 resv4[16];
};
+struct nvme_dbbuf {
+ __u8 opcode;
+ __u8 flags;
+ __u16 command_id;
+ __u32 rsvd1[5];
+ __le64 prp1;
+ __le64 prp2;
+ __u32 rsvd12[6];
+};
+
struct nvme_command {
union {
struct nvme_common_command common;
@@ -893,6 +905,7 @@ struct nvme_command {
struct nvmf_connect_command connect;
struct nvmf_property_set_command prop_set;
struct nvmf_property_get_command prop_get;
+ struct nvme_dbbuf dbbuf;
};
};
--
2.7.4
^ permalink raw reply related [flat|nested] 49+ messages in thread
* [PATCH v5 RFC] nvme: improve performance for virtual NVMe devices
2017-03-24 4:23 ` [PATCH v5 " Helen Koike
@ 2017-03-27 9:49 ` Christoph Hellwig
2017-03-27 16:04 ` Helen Koike
2017-03-27 14:43 ` Keith Busch
2017-03-27 16:50 ` [PATCH v6 " Helen Koike
2 siblings, 1 reply; 49+ messages in thread
From: Christoph Hellwig @ 2017-03-27 9:49 UTC (permalink / raw)
> +#define SQ_IDX(qid, stride) ((qid) * 2 * (stride))
> +#define CQ_IDX(qid, stride) (((qid) * 2 + 1) * (stride))
Please use inline functions for these.
> + struct {
> + u32 *dbs;
> + u32 *eis;
> + dma_addr_t dbs_dma_addr;
> + dma_addr_t eis_dma_addr;
> + } dbbuf;
No need for a struct here, also please keep the field and its dma_addr_t
together.
> + struct {
> + u32 *sq_db;
> + u32 *cq_db;
> + u32 *sq_ei;
> + u32 *cq_ei;
> + } dbbuf;
No need for the struct here either.
> +
> +static int nvme_dbbuf_dma_alloc(struct nvme_dev *dev)
> +{
> + unsigned int mem_size = nvme_dbbuf_size(dev->db_stride);
> +
> + dev->dbbuf.dbs = dma_alloc_coherent(dev->dev, mem_size,
> + &dev->dbbuf.dbs_dma_addr,
> + GFP_KERNEL);
> + if (!dev->dbbuf.dbs)
> + return -ENOMEM;
> + dev->dbbuf.eis = dma_alloc_coherent(dev->dev, mem_size,
> + &dev->dbbuf.eis_dma_addr,
> + GFP_KERNEL);
> + if (!dev->dbbuf.eis) {
> + dma_free_coherent(dev->dev, mem_size,
> + dev->dbbuf.dbs, dev->dbbuf.dbs_dma_addr);
> + dev->dbbuf.dbs = NULL;
> + return -ENOMEM;
> + }
> +
> + return 0;
Please use normal kernel-style goto unwinding.
> +static void nvme_dbbuf_set(struct nvme_dev *dev)
> +{
> + struct nvme_command c;
> +
> + if (!dev->dbbuf.dbs)
> + return;
> +
> + memset(&c, 0, sizeof(c));
> + c.dbbuf.opcode = nvme_admin_dbbuf;
> + c.dbbuf.prp1 = cpu_to_le64(dev->dbbuf.dbs_dma_addr);
> + c.dbbuf.prp2 = cpu_to_le64(dev->dbbuf.eis_dma_addr);
> +
> + if (nvme_submit_sync_cmd(dev->ctrl.admin_q, &c, NULL, 0))
> + /* Free memory and continue on */
> + nvme_dbbuf_dma_free(dev);
At least log a warning.
> +static inline int nvme_dbbuf_need_event(u16 event_idx, u16 new_idx, u16 old)
> +{
> + /* Borrowed from vring_need_event */
I don't think this comment matters.
> +static void nvme_write_doorbell(u16 value,
> + u32 __iomem *db,
> + u32 *dbbuf_db,
> + volatile u32 *dbbuf_ei)
> +{
Very odd formatting. Why not:
static void nvme_write_doorbell(u16 value, u32 __iomem *db, u32 *dbbuf_db,
volatile u32 *dbbuf_ei)
?
> + u16 old_value;
> +
> + if (!dbbuf_db) {
> + writel(value, db);
> + return;
> + }
I'd prefer to keep this in the ultimate callers to make the flow
easier to read.
> +static inline void nvme_write_doorbell_cq(struct nvme_queue *nvmeq, u16 value)
> +{
> + nvme_write_doorbell(value, nvmeq->q_db + nvmeq->dev->db_stride,
> + nvmeq->dbbuf.cq_db, nvmeq->dbbuf.cq_ei);
> +}
> +
> +static inline void nvme_write_doorbell_sq(struct nvme_queue *nvmeq, u16 value)
> +{
> + nvme_write_doorbell(value, nvmeq->q_db,
> + nvmeq->dbbuf.sq_db, nvmeq->dbbuf.sq_ei);
> }
I'd skip these wrappers entirely.
> + if (dev->ctrl.oacs & NVME_CTRL_OACS_DBBUF_SUPP) {
> + result = nvme_dbbuf_dma_alloc(dev);
> + if (result)
> + goto out;
> + }
Should we really fail the init here or just print a warning?
^ permalink raw reply [flat|nested] 49+ messages in thread
* [PATCH v5 RFC] nvme: improve performance for virtual NVMe devices
2017-03-27 9:49 ` Christoph Hellwig
@ 2017-03-27 16:04 ` Helen Koike
2017-03-27 16:25 ` Helen Koike
0 siblings, 1 reply; 49+ messages in thread
From: Helen Koike @ 2017-03-27 16:04 UTC (permalink / raw)
Hi Keith,
Thanks for your review. Please, see my comments below
On 2017-03-27 06:49 AM, Christoph Hellwig wrote:
>> +#define SQ_IDX(qid, stride) ((qid) * 2 * (stride))
>> +#define CQ_IDX(qid, stride) (((qid) * 2 + 1) * (stride))
>
> Please use inline functions for these.
>
>> + struct {
>> + u32 *dbs;
>> + u32 *eis;
>> + dma_addr_t dbs_dma_addr;
>> + dma_addr_t eis_dma_addr;
>> + } dbbuf;
>
> No need for a struct here, also please keep the field and its dma_addr_t
> together.
>
>> + struct {
>> + u32 *sq_db;
>> + u32 *cq_db;
>> + u32 *sq_ei;
>> + u32 *cq_ei;
>> + } dbbuf;
>
> No need for the struct here either.
>
>> +
>> +static int nvme_dbbuf_dma_alloc(struct nvme_dev *dev)
>> +{
>> + unsigned int mem_size = nvme_dbbuf_size(dev->db_stride);
>> +
>> + dev->dbbuf.dbs = dma_alloc_coherent(dev->dev, mem_size,
>> + &dev->dbbuf.dbs_dma_addr,
>> + GFP_KERNEL);
>> + if (!dev->dbbuf.dbs)
>> + return -ENOMEM;
>> + dev->dbbuf.eis = dma_alloc_coherent(dev->dev, mem_size,
>> + &dev->dbbuf.eis_dma_addr,
>> + GFP_KERNEL);
>> + if (!dev->dbbuf.eis) {
>> + dma_free_coherent(dev->dev, mem_size,
>> + dev->dbbuf.dbs, dev->dbbuf.dbs_dma_addr);
>> + dev->dbbuf.dbs = NULL;
>> + return -ENOMEM;
>> + }
>> +
>> + return 0;
>
> Please use normal kernel-style goto unwinding.
I don't think it is necessary as there is only a single item to unwind.
From my experience in other parts of the kernel code we usually use
goto when there are several other steps to unwind. But I don't mind
changing it if you still prefer a goto here.
>
>> +static void nvme_dbbuf_set(struct nvme_dev *dev)
>> +{
>> + struct nvme_command c;
>> +
>> + if (!dev->dbbuf.dbs)
>> + return;
>> +
>> + memset(&c, 0, sizeof(c));
>> + c.dbbuf.opcode = nvme_admin_dbbuf;
>> + c.dbbuf.prp1 = cpu_to_le64(dev->dbbuf.dbs_dma_addr);
>> + c.dbbuf.prp2 = cpu_to_le64(dev->dbbuf.eis_dma_addr);
>> +
>> + if (nvme_submit_sync_cmd(dev->ctrl.admin_q, &c, NULL, 0))
>> + /* Free memory and continue on */
>> + nvme_dbbuf_dma_free(dev);
>
> At least log a warning.
>
>> +static inline int nvme_dbbuf_need_event(u16 event_idx, u16 new_idx, u16 old)
>> +{
>> + /* Borrowed from vring_need_event */
>
> I don't think this comment matters.
>
>> +static void nvme_write_doorbell(u16 value,
>> + u32 __iomem *db,
>> + u32 *dbbuf_db,
>> + volatile u32 *dbbuf_ei)
>> +{
>
> Very odd formatting. Why not:
>
> static void nvme_write_doorbell(u16 value, u32 __iomem *db, u32 *dbbuf_db,
> volatile u32 *dbbuf_ei)
>
> ?
>
>> + u16 old_value;
>> +
>> + if (!dbbuf_db) {
>> + writel(value, db);
>> + return;
>> + }
>
> I'd prefer to keep this in the ultimate callers to make the flow
> easier to read.
>
>> +static inline void nvme_write_doorbell_cq(struct nvme_queue *nvmeq, u16 value)
>> +{
>> + nvme_write_doorbell(value, nvmeq->q_db + nvmeq->dev->db_stride,
>> + nvmeq->dbbuf.cq_db, nvmeq->dbbuf.cq_ei);
>> +}
>> +
>> +static inline void nvme_write_doorbell_sq(struct nvme_queue *nvmeq, u16 value)
>> +{
>> + nvme_write_doorbell(value, nvmeq->q_db,
>> + nvmeq->dbbuf.sq_db, nvmeq->dbbuf.sq_ei);
>> }
>
> I'd skip these wrappers entirely.
I added them to avoid future mistakes as mixing nvmeq->dbbuf.sq_db with
nvmeq->dbbuf.sq_ei. But I don't mind to remove them either.
>
>> + if (dev->ctrl.oacs & NVME_CTRL_OACS_DBBUF_SUPP) {
>> + result = nvme_dbbuf_dma_alloc(dev);
>> + if (result)
>> + goto out;
>> + }
>
> Should we really fail the init here or just print a warning?
>
^ permalink raw reply [flat|nested] 49+ messages in thread
* [PATCH v5 RFC] nvme: improve performance for virtual NVMe devices
2017-03-27 16:04 ` Helen Koike
@ 2017-03-27 16:25 ` Helen Koike
0 siblings, 0 replies; 49+ messages in thread
From: Helen Koike @ 2017-03-27 16:25 UTC (permalink / raw)
Sorry, the last email was a reply to Christoph (thank you all for reviewing)
On 2017-03-27 01:04 PM, Helen Koike wrote:
> Hi Keith,
>
> Thanks for your review. Please, see my comments below
>
> On 2017-03-27 06:49 AM, Christoph Hellwig wrote:
> >> +#define SQ_IDX(qid, stride) ((qid) * 2 * (stride))
> >> +#define CQ_IDX(qid, stride) (((qid) * 2 + 1) * (stride))
> >
> > Please use inline functions for these.
> >
> >> + struct {
> >> + u32 *dbs;
> >> + u32 *eis;
> >> + dma_addr_t dbs_dma_addr;
> >> + dma_addr_t eis_dma_addr;
> >> + } dbbuf;
> >
> > No need for a struct here, also please keep the field and its dma_addr_t
> > together.
> >
> >> + struct {
> >> + u32 *sq_db;
> >> + u32 *cq_db;
> >> + u32 *sq_ei;
> >> + u32 *cq_ei;
> >> + } dbbuf;
> >
> > No need for the struct here either.
> >
> >> +
> >> +static int nvme_dbbuf_dma_alloc(struct nvme_dev *dev)
> >> +{
> >> + unsigned int mem_size = nvme_dbbuf_size(dev->db_stride);
> >> +
> >> + dev->dbbuf.dbs = dma_alloc_coherent(dev->dev, mem_size,
> >> + &dev->dbbuf.dbs_dma_addr,
> >> + GFP_KERNEL);
> >> + if (!dev->dbbuf.dbs)
> >> + return -ENOMEM;
> >> + dev->dbbuf.eis = dma_alloc_coherent(dev->dev, mem_size,
> >> + &dev->dbbuf.eis_dma_addr,
> >> + GFP_KERNEL);
> >> + if (!dev->dbbuf.eis) {
> >> + dma_free_coherent(dev->dev, mem_size,
> >> + dev->dbbuf.dbs, dev->dbbuf.dbs_dma_addr);
> >> + dev->dbbuf.dbs = NULL;
> >> + return -ENOMEM;
> >> + }
> >> +
> >> + return 0;
> >
> > Please use normal kernel-style goto unwinding.
>
> I don't think it is necessary as there is only a single item to unwind.
> From my experience in other parts of the kernel code we usually use goto
> when there are several other steps to unwind. But I don't mind changing
> it if you still prefer a goto here.
>
> >
> >> +static void nvme_dbbuf_set(struct nvme_dev *dev)
> >> +{
> >> + struct nvme_command c;
> >> +
> >> + if (!dev->dbbuf.dbs)
> >> + return;
> >> +
> >> + memset(&c, 0, sizeof(c));
> >> + c.dbbuf.opcode = nvme_admin_dbbuf;
> >> + c.dbbuf.prp1 = cpu_to_le64(dev->dbbuf.dbs_dma_addr);
> >> + c.dbbuf.prp2 = cpu_to_le64(dev->dbbuf.eis_dma_addr);
> >> +
> >> + if (nvme_submit_sync_cmd(dev->ctrl.admin_q, &c, NULL, 0))
> >> + /* Free memory and continue on */
> >> + nvme_dbbuf_dma_free(dev);
> >
> > At least log a warning.
> >
> >> +static inline int nvme_dbbuf_need_event(u16 event_idx, u16 new_idx,
> >> u16 old)
> >> +{
> >> + /* Borrowed from vring_need_event */
> >
> > I don't think this comment matters.
> >
> >> +static void nvme_write_doorbell(u16 value,
> >> + u32 __iomem *db,
> >> + u32 *dbbuf_db,
> >> + volatile u32 *dbbuf_ei)
> >> +{
> >
> > Very odd formatting. Why not:
> >
> > static void nvme_write_doorbell(u16 value, u32 __iomem *db, u32
> > *dbbuf_db,
> > volatile u32 *dbbuf_ei)
> >
> > ?
> >
> >> + u16 old_value;
> >> +
> >> + if (!dbbuf_db) {
> >> + writel(value, db);
> >> + return;
> >> + }
> >
> > I'd prefer to keep this in the ultimate callers to make the flow
> > easier to read.
> >
> >> +static inline void nvme_write_doorbell_cq(struct nvme_queue *nvmeq,
> >> u16 value)
> >> +{
> >> + nvme_write_doorbell(value, nvmeq->q_db + nvmeq->dev->db_stride,
> >> + nvmeq->dbbuf.cq_db, nvmeq->dbbuf.cq_ei);
> >> +}
> >> +
> >> +static inline void nvme_write_doorbell_sq(struct nvme_queue *nvmeq,
> >> u16 value)
> >> +{
> >> + nvme_write_doorbell(value, nvmeq->q_db,
> >> + nvmeq->dbbuf.sq_db, nvmeq->dbbuf.sq_ei);
> >> }
> >
> > I'd skip these wrappers entirely.
>
> I added them to avoid future mistakes as mixing nvmeq->dbbuf.sq_db with
> nvmeq->dbbuf.sq_ei. But I don't mind to remove them either.
>
> >
> >> + if (dev->ctrl.oacs & NVME_CTRL_OACS_DBBUF_SUPP) {
> >> + result = nvme_dbbuf_dma_alloc(dev);
> >> + if (result)
> >> + goto out;
> >> + }
> >
> > Should we really fail the init here or just print a warning?
> >
^ permalink raw reply [flat|nested] 49+ messages in thread
* [PATCH v5 RFC] nvme: improve performance for virtual NVMe devices
2017-03-24 4:23 ` [PATCH v5 " Helen Koike
2017-03-27 9:49 ` Christoph Hellwig
@ 2017-03-27 14:43 ` Keith Busch
2017-03-27 16:50 ` [PATCH v6 " Helen Koike
2 siblings, 0 replies; 49+ messages in thread
From: Keith Busch @ 2017-03-27 14:43 UTC (permalink / raw)
On Fri, Mar 24, 2017@01:23:43AM -0300, Helen Koike wrote:
> + if (dev->ctrl.oacs & NVME_CTRL_OACS_DBBUF_SUPP) {
> + result = nvme_dbbuf_dma_alloc(dev);
> + if (result)
> + goto out;
> + }
We are still functional without enabling this feature, so I don't think
we should error out. Just log a warning that it's less than optimal.
^ permalink raw reply [flat|nested] 49+ messages in thread
* [PATCH v6 RFC] nvme: improve performance for virtual NVMe devices
2017-03-24 4:23 ` [PATCH v5 " Helen Koike
2017-03-27 9:49 ` Christoph Hellwig
2017-03-27 14:43 ` Keith Busch
@ 2017-03-27 16:50 ` Helen Koike
2017-03-30 17:33 ` Keith Busch
2 siblings, 1 reply; 49+ messages in thread
From: Helen Koike @ 2017-03-27 16:50 UTC (permalink / raw)
From: Helen Koike <helen.koike@collabora.co.uk>
This change provides a mechanism to reduce the number of MMIO doorbell
writes for the NVMe driver. When running in a virtualized environment
like QEMU, the cost of an MMIO is quite hefy here. The main idea for
the patch is provide the device two memory location locations:
1) to store the doorbell values so they can be lookup without the doorbell
MMIO write
2) to store an event index.
I believe the doorbell value is obvious, the event index not so much.
Similar to the virtio specification, the virtual device can tell the
driver (guest OS) not to write MMIO unless you are writing past this
value.
FYI: doorbell values are written by the nvme driver (guest OS) and the
event index is written by the virtual device (host OS).
The patch implements a new admin command that will communicate where
these two memory locations reside. If the command fails, the nvme
driver will work as before without any optimizations.
Contributions:
Eric Northup <digitaleric at google.com>
Frank Swiderski <fes at google.com>
Ted Tso <tytso at mit.edu>
Keith Busch <keith.busch at intel.com>
Just to give an idea on the performance boost with the vendor
extension: Running fio [1], a stock NVMe driver I get about 200K read
IOPs with my vendor patch I get about 1000K read IOPs. This was
running with a null device i.e. the backing device simply returned
success on every read IO request.
[1] Running on a 4 core machine:
fio --time_based --name=benchmark --runtime=30
--filename=/dev/nvme0n1 --nrfiles=1 --ioengine=libaio --iodepth=32
--direct=1 --invalidate=1 --verify=0 --verify_fatal=0 --numjobs=4
--rw=randread --blocksize=4k --randrepeat=false
Signed-off-by: Rob Nelson <rlnelson at google.com>
[mlin: port for upstream]
Signed-off-by: Ming Lin <mlin at kernel.org>
[koike: updated for upstream]
Signed-off-by: Helen Koike <helen.koike at collabora.co.uk>
---
This patch is based on git://git.infradead.org/nvme.git master
Tested through Google Cloud Engine
TPAR is ratified by the NVME working group
changes since v5:
- remove anonymous dbbuf struct in struct nvme_dev and struct nvme_queue
- use inline functions for sq_idx and cq_idx instead of macros
- add a warning when nvme_dbbuf_set fails
- remove comment "Borrowed from vring_need_event"
- change formatting of the parameters in nvme_write_doorbell
- don't fail nvme_reset_work if nvme_dbbuf_dma_alloc fails
- remove nvme_write_doorbell_{sq,cq} wrappers
- in nvme_write_doorbell(), call writel last
Changes since v4
- Remove CONFIG_NVME_DBBUF
- Remove dbbuf.{c,h}, move the code to pci.c
- Coding style changes
- Functions and vaiables renamed
Changes since v3
- Rename to dbbuf (be closer to the latest TPAR)
- Modify the opcodes according to the latest TPAR
- Check if the device support this feature through the OACS bit
- little cleanups
---
drivers/nvme/host/pci.c | 137 +++++++++++++++++++++++++++++++++++++++++++++++-
include/linux/nvme.h | 13 +++++
2 files changed, 148 insertions(+), 2 deletions(-)
diff --git a/drivers/nvme/host/pci.c b/drivers/nvme/host/pci.c
index 5b7bd9c..00edeca 100644
--- a/drivers/nvme/host/pci.c
+++ b/drivers/nvme/host/pci.c
@@ -103,8 +103,22 @@ struct nvme_dev {
u32 cmbloc;
struct nvme_ctrl ctrl;
struct completion ioq_wait;
+ u32 *dbbuf_dbs;
+ dma_addr_t dbbuf_dbs_dma_addr;
+ u32 *dbbuf_eis;
+ dma_addr_t dbbuf_eis_dma_addr;
};
+static inline unsigned int sq_idx(unsigned int qid, u32 stride)
+{
+ return qid * 2 * stride;
+}
+
+static inline unsigned int cq_idx(unsigned int qid, u32 stride)
+{
+ return (qid * 2 + 1) * stride;
+}
+
static inline struct nvme_dev *to_nvme_dev(struct nvme_ctrl *ctrl)
{
return container_of(ctrl, struct nvme_dev, ctrl);
@@ -133,6 +147,10 @@ struct nvme_queue {
u16 qid;
u8 cq_phase;
u8 cqe_seen;
+ u32 *dbbuf_sq_db;
+ u32 *dbbuf_cq_db;
+ u32 *dbbuf_sq_ei;
+ u32 *dbbuf_cq_ei;
};
/*
@@ -174,6 +192,107 @@ static inline void _nvme_check_size(void)
BUILD_BUG_ON(sizeof(struct nvme_id_ns) != 4096);
BUILD_BUG_ON(sizeof(struct nvme_lba_range_type) != 64);
BUILD_BUG_ON(sizeof(struct nvme_smart_log) != 512);
+ BUILD_BUG_ON(sizeof(struct nvme_dbbuf) != 64);
+}
+
+static inline unsigned int nvme_dbbuf_size(u32 stride)
+{
+ return ((num_possible_cpus() + 1) * 8 * stride);
+}
+
+static int nvme_dbbuf_dma_alloc(struct nvme_dev *dev)
+{
+ unsigned int mem_size = nvme_dbbuf_size(dev->db_stride);
+
+ dev->dbbuf_dbs = dma_alloc_coherent(dev->dev, mem_size,
+ &dev->dbbuf_dbs_dma_addr,
+ GFP_KERNEL);
+ if (!dev->dbbuf_dbs)
+ return -ENOMEM;
+ dev->dbbuf_eis = dma_alloc_coherent(dev->dev, mem_size,
+ &dev->dbbuf_eis_dma_addr,
+ GFP_KERNEL);
+ if (!dev->dbbuf_eis) {
+ dma_free_coherent(dev->dev, mem_size,
+ dev->dbbuf_dbs, dev->dbbuf_dbs_dma_addr);
+ dev->dbbuf_dbs = NULL;
+ return -ENOMEM;
+ }
+
+ return 0;
+}
+
+static void nvme_dbbuf_dma_free(struct nvme_dev *dev)
+{
+ unsigned int mem_size = nvme_dbbuf_size(dev->db_stride);
+
+ if (dev->dbbuf_dbs) {
+ dma_free_coherent(dev->dev, mem_size,
+ dev->dbbuf_dbs, dev->dbbuf_dbs_dma_addr);
+ dev->dbbuf_dbs = NULL;
+ }
+ if (dev->dbbuf_eis) {
+ dma_free_coherent(dev->dev, mem_size,
+ dev->dbbuf_eis, dev->dbbuf_eis_dma_addr);
+ dev->dbbuf_eis = NULL;
+ }
+}
+
+static void nvme_dbbuf_init(struct nvme_dev *dev,
+ struct nvme_queue *nvmeq, int qid)
+{
+ if (!dev->dbbuf_dbs || !qid)
+ return;
+
+ nvmeq->dbbuf_sq_db = &dev->dbbuf_dbs[sq_idx(qid, dev->db_stride)];
+ nvmeq->dbbuf_cq_db = &dev->dbbuf_dbs[cq_idx(qid, dev->db_stride)];
+ nvmeq->dbbuf_sq_ei = &dev->dbbuf_eis[sq_idx(qid, dev->db_stride)];
+ nvmeq->dbbuf_cq_ei = &dev->dbbuf_eis[cq_idx(qid, dev->db_stride)];
+}
+
+static void nvme_dbbuf_set(struct nvme_dev *dev)
+{
+ struct nvme_command c;
+
+ if (!dev->dbbuf_dbs)
+ return;
+
+ memset(&c, 0, sizeof(c));
+ c.dbbuf.opcode = nvme_admin_dbbuf;
+ c.dbbuf.prp1 = cpu_to_le64(dev->dbbuf_dbs_dma_addr);
+ c.dbbuf.prp2 = cpu_to_le64(dev->dbbuf_eis_dma_addr);
+
+ if (nvme_submit_sync_cmd(dev->ctrl.admin_q, &c, NULL, 0)) {
+ dev_warn(dev->dev, "unable to set dbbuf\n");
+ /* Free memory and continue on */
+ nvme_dbbuf_dma_free(dev);
+ }
+}
+
+static inline int nvme_dbbuf_need_event(u16 event_idx, u16 new_idx, u16 old)
+{
+ return (u16)(new_idx - event_idx - 1) < (u16)(new_idx - old);
+}
+
+static void nvme_write_doorbell(u16 value, u32 __iomem *db, u32 *dbbuf_db,
+ volatile u32 *dbbuf_ei)
+{
+ if (dbbuf_db) {
+ u16 old_value;
+
+ /*
+ * Ensure that the queue is written before updating
+ * the doorbell in memory
+ */
+ wmb();
+
+ old_value = *dbbuf_db;
+ *dbbuf_db = value;
+ if (!nvme_dbbuf_need_event(*dbbuf_ei, value, old_value))
+ return;
+ }
+
+ writel(value, db);
}
/*
@@ -300,7 +419,8 @@ static void __nvme_submit_cmd(struct nvme_queue *nvmeq,
if (++tail == nvmeq->q_depth)
tail = 0;
- writel(tail, nvmeq->q_db);
+ nvme_write_doorbell(tail, nvmeq->q_db, nvmeq->dbbuf_sq_db,
+ nvmeq->dbbuf_sq_ei);
nvmeq->sq_tail = tail;
}
@@ -716,7 +836,8 @@ static void __nvme_process_cq(struct nvme_queue *nvmeq, unsigned int *tag)
return;
if (likely(nvmeq->cq_vector >= 0))
- writel(head, nvmeq->q_db + nvmeq->dev->db_stride);
+ nvme_write_doorbell(head, nvmeq->q_db + nvmeq->dev->db_stride,
+ nvmeq->dbbuf_cq_db, nvmeq->dbbuf_cq_ei);
nvmeq->cq_head = head;
nvmeq->cq_phase = phase;
@@ -1066,6 +1187,7 @@ static struct nvme_queue *nvme_alloc_queue(struct nvme_dev *dev, int qid,
nvmeq->q_depth = depth;
nvmeq->qid = qid;
nvmeq->cq_vector = -1;
+ nvme_dbbuf_init(dev, nvmeq, qid);
dev->queues[qid] = nvmeq;
dev->queue_count++;
@@ -1099,6 +1221,7 @@ static void nvme_init_queue(struct nvme_queue *nvmeq, u16 qid)
nvmeq->cq_phase = 1;
nvmeq->q_db = &dev->dbs[qid * 2 * dev->db_stride];
memset((void *)nvmeq->cqes, 0, CQ_SIZE(nvmeq->q_depth));
+ nvme_dbbuf_init(dev, nvmeq, qid);
dev->online_queues++;
spin_unlock_irq(&nvmeq->q_lock);
}
@@ -1568,6 +1691,8 @@ static int nvme_dev_add(struct nvme_dev *dev)
if (blk_mq_alloc_tag_set(&dev->tagset))
return 0;
dev->ctrl.tagset = &dev->tagset;
+
+ nvme_dbbuf_set(dev);
} else {
blk_mq_update_nr_hw_queues(&dev->tagset, dev->online_queues - 1);
@@ -1700,6 +1825,7 @@ static void nvme_dev_disable(struct nvme_dev *dev, bool shutdown)
nvme_disable_admin_queue(dev, shutdown);
}
nvme_pci_disable(dev);
+ nvme_dbbuf_dma_free(dev);
blk_mq_tagset_busy_iter(&dev->tagset, nvme_cancel_request, &dev->ctrl);
blk_mq_tagset_busy_iter(&dev->admin_tagset, nvme_cancel_request, &dev->ctrl);
@@ -1800,6 +1926,13 @@ static void nvme_reset_work(struct work_struct *work)
dev->ctrl.opal_dev = NULL;
}
+ if (dev->ctrl.oacs & NVME_CTRL_OACS_DBBUF_SUPP) {
+ result = nvme_dbbuf_dma_alloc(dev);
+ if (result)
+ dev_warn(dev->dev,
+ "unable to allocate dma for dbbuf\n");
+ }
+
result = nvme_setup_io_queues(dev);
if (result)
goto out;
diff --git a/include/linux/nvme.h b/include/linux/nvme.h
index c43d435..43a6289 100644
--- a/include/linux/nvme.h
+++ b/include/linux/nvme.h
@@ -245,6 +245,7 @@ enum {
NVME_CTRL_ONCS_WRITE_ZEROES = 1 << 3,
NVME_CTRL_VWC_PRESENT = 1 << 0,
NVME_CTRL_OACS_SEC_SUPP = 1 << 0,
+ NVME_CTRL_OACS_DBBUF_SUPP = 1 << 7,
};
struct nvme_lbaf {
@@ -603,6 +604,7 @@ enum nvme_admin_opcode {
nvme_admin_download_fw = 0x11,
nvme_admin_ns_attach = 0x15,
nvme_admin_keep_alive = 0x18,
+ nvme_admin_dbbuf = 0x7C,
nvme_admin_format_nvm = 0x80,
nvme_admin_security_send = 0x81,
nvme_admin_security_recv = 0x82,
@@ -874,6 +876,16 @@ struct nvmf_property_get_command {
__u8 resv4[16];
};
+struct nvme_dbbuf {
+ __u8 opcode;
+ __u8 flags;
+ __u16 command_id;
+ __u32 rsvd1[5];
+ __le64 prp1;
+ __le64 prp2;
+ __u32 rsvd12[6];
+};
+
struct nvme_command {
union {
struct nvme_common_command common;
@@ -893,6 +905,7 @@ struct nvme_command {
struct nvmf_connect_command connect;
struct nvmf_property_set_command prop_set;
struct nvmf_property_get_command prop_get;
+ struct nvme_dbbuf dbbuf;
};
};
--
2.7.4
^ permalink raw reply related [flat|nested] 49+ messages in thread
* [PATCH v6 RFC] nvme: improve performance for virtual NVMe devices
2017-03-27 16:50 ` [PATCH v6 " Helen Koike
@ 2017-03-30 17:33 ` Keith Busch
2017-03-30 17:46 ` [PATCH v7 " Helen Koike
0 siblings, 1 reply; 49+ messages in thread
From: Keith Busch @ 2017-03-30 17:33 UTC (permalink / raw)
On Mon, Mar 27, 2017@01:50:08PM -0300, Helen Koike wrote:
> changes since v5:
> - remove anonymous dbbuf struct in struct nvme_dev and struct nvme_queue
> - use inline functions for sq_idx and cq_idx instead of macros
> - add a warning when nvme_dbbuf_set fails
> - remove comment "Borrowed from vring_need_event"
> - change formatting of the parameters in nvme_write_doorbell
> - don't fail nvme_reset_work if nvme_dbbuf_dma_alloc fails
> - remove nvme_write_doorbell_{sq,cq} wrappers
> - in nvme_write_doorbell(), call writel last
I'm pretty much okay with this. The only thing I'd change is to call
nvme_dbbuf_dma_free only when we're unbinding so that we don't have to
reallocate it on a every controller reset:
> +static int nvme_dbbuf_dma_alloc(struct nvme_dev *dev)
> +{
> + unsigned int mem_size = nvme_dbbuf_size(dev->db_stride);
if (dev->dbbuf_dbs)
return 0;
> +
> + dev->dbbuf_dbs = dma_alloc_coherent(dev->dev, mem_size,
> + &dev->dbbuf_dbs_dma_addr,
> + GFP_KERNEL);
> + if (!dev->dbbuf_dbs)
> + return -ENOMEM;
> + dev->dbbuf_eis = dma_alloc_coherent(dev->dev, mem_size,
> + &dev->dbbuf_eis_dma_addr,
> + GFP_KERNEL);
> + if (!dev->dbbuf_eis) {
> + dma_free_coherent(dev->dev, mem_size,
> + dev->dbbuf_dbs, dev->dbbuf_dbs_dma_addr);
> + dev->dbbuf_dbs = NULL;
> + return -ENOMEM;
> + }
> +
> + return 0;
> +}
> @@ -1066,6 +1187,7 @@ static struct nvme_queue *nvme_alloc_queue(struct nvme_dev *dev, int qid,
> nvmeq->q_depth = depth;
> nvmeq->qid = qid;
> nvmeq->cq_vector = -1;
> + nvme_dbbuf_init(dev, nvmeq, qid);
> dev->queues[qid] = nvmeq;
> dev->queue_count++;
Remove the above since it's duplicated in nvme_init_queue.
> @@ -1700,6 +1825,7 @@ static void nvme_dev_disable(struct nvme_dev *dev, bool shutdown)
> nvme_disable_admin_queue(dev, shutdown);
> }
> nvme_pci_disable(dev);
> + nvme_dbbuf_dma_free(dev);
And move this to nvme_pci_free_ctrl().
^ permalink raw reply [flat|nested] 49+ messages in thread
* [PATCH v7 RFC] nvme: improve performance for virtual NVMe devices
2017-03-30 17:33 ` Keith Busch
@ 2017-03-30 17:46 ` Helen Koike
2017-03-31 7:01 ` Christoph Hellwig
0 siblings, 1 reply; 49+ messages in thread
From: Helen Koike @ 2017-03-30 17:46 UTC (permalink / raw)
This change provides a mechanism to reduce the number of MMIO doorbell
writes for the NVMe driver. When running in a virtualized environment
like QEMU, the cost of an MMIO is quite hefy here. The main idea for
the patch is provide the device two memory location locations:
1) to store the doorbell values so they can be lookup without the doorbell
MMIO write
2) to store an event index.
I believe the doorbell value is obvious, the event index not so much.
Similar to the virtio specification, the virtual device can tell the
driver (guest OS) not to write MMIO unless you are writing past this
value.
FYI: doorbell values are written by the nvme driver (guest OS) and the
event index is written by the virtual device (host OS).
The patch implements a new admin command that will communicate where
these two memory locations reside. If the command fails, the nvme
driver will work as before without any optimizations.
Contributions:
Eric Northup <digitaleric at google.com>
Frank Swiderski <fes at google.com>
Ted Tso <tytso at mit.edu>
Keith Busch <keith.busch at intel.com>
Just to give an idea on the performance boost with the vendor
extension: Running fio [1], a stock NVMe driver I get about 200K read
IOPs with my vendor patch I get about 1000K read IOPs. This was
running with a null device i.e. the backing device simply returned
success on every read IO request.
[1] Running on a 4 core machine:
fio --time_based --name=benchmark --runtime=30
--filename=/dev/nvme0n1 --nrfiles=1 --ioengine=libaio --iodepth=32
--direct=1 --invalidate=1 --verify=0 --verify_fatal=0 --numjobs=4
--rw=randread --blocksize=4k --randrepeat=false
Signed-off-by: Rob Nelson <rlnelson at google.com>
[mlin: port for upstream]
Signed-off-by: Ming Lin <mlin at kernel.org>
[koike: updated for upstream]
Signed-off-by: Helen Koike <helen.koike at collabora.com>
---
This patch is based on git://git.infradead.org/nvme.git master
Tested through Google Cloud Engine
TPAR is ratified by the NVME working group
changes since v6:
- remove nvme_dbbuf_init() from nvme_alloc_queue() as it is
already called in nvme_init_queue()
- free dbbuf_dbs only in nvme_pci_free_ctrl(), change
nvme_dbbuf_dma_alloc() to do nothing if the dbbuf_dbs is already
allocated
changes since v5:
- remove anonymous dbbuf struct in struct nvme_dev and struct nvme_queue
- use inline functions for sq_idx and cq_idx instead of macros
- add a warning when nvme_dbbuf_set fails
- remove comment "Borrowed from vring_need_event"
- change formatting of the parameters in nvme_write_doorbell
- don't fail nvme_reset_work if nvme_dbbuf_dma_alloc fails
- remove nvme_write_doorbell_{sq,cq} wrappers
- in nvme_write_doorbell(), call writel last
Changes since v4
- Remove CONFIG_NVME_DBBUF
- Remove dbbuf.{c,h}, move the code to pci.c
- Coding style changes
- Functions and vaiables renamed
Changes since v3
- Rename to dbbuf (be closer to the latest TPAR)
- Modify the opcodes according to the latest TPAR
- Check if the device support this feature through the OACS bit
- little cleanups
---
drivers/nvme/host/pci.c | 139 +++++++++++++++++++++++++++++++++++++++++++++++-
include/linux/nvme.h | 13 +++++
2 files changed, 150 insertions(+), 2 deletions(-)
diff --git a/drivers/nvme/host/pci.c b/drivers/nvme/host/pci.c
index 5b7bd9c..3ce705a 100644
--- a/drivers/nvme/host/pci.c
+++ b/drivers/nvme/host/pci.c
@@ -103,8 +103,22 @@ struct nvme_dev {
u32 cmbloc;
struct nvme_ctrl ctrl;
struct completion ioq_wait;
+ u32 *dbbuf_dbs;
+ dma_addr_t dbbuf_dbs_dma_addr;
+ u32 *dbbuf_eis;
+ dma_addr_t dbbuf_eis_dma_addr;
};
+static inline unsigned int sq_idx(unsigned int qid, u32 stride)
+{
+ return qid * 2 * stride;
+}
+
+static inline unsigned int cq_idx(unsigned int qid, u32 stride)
+{
+ return (qid * 2 + 1) * stride;
+}
+
static inline struct nvme_dev *to_nvme_dev(struct nvme_ctrl *ctrl)
{
return container_of(ctrl, struct nvme_dev, ctrl);
@@ -133,6 +147,10 @@ struct nvme_queue {
u16 qid;
u8 cq_phase;
u8 cqe_seen;
+ u32 *dbbuf_sq_db;
+ u32 *dbbuf_cq_db;
+ u32 *dbbuf_sq_ei;
+ u32 *dbbuf_cq_ei;
};
/*
@@ -174,6 +192,110 @@ static inline void _nvme_check_size(void)
BUILD_BUG_ON(sizeof(struct nvme_id_ns) != 4096);
BUILD_BUG_ON(sizeof(struct nvme_lba_range_type) != 64);
BUILD_BUG_ON(sizeof(struct nvme_smart_log) != 512);
+ BUILD_BUG_ON(sizeof(struct nvme_dbbuf) != 64);
+}
+
+static inline unsigned int nvme_dbbuf_size(u32 stride)
+{
+ return ((num_possible_cpus() + 1) * 8 * stride);
+}
+
+static int nvme_dbbuf_dma_alloc(struct nvme_dev *dev)
+{
+ unsigned int mem_size = nvme_dbbuf_size(dev->db_stride);
+
+ if (dev->dbbuf_dbs)
+ return 0;
+
+ dev->dbbuf_dbs = dma_alloc_coherent(dev->dev, mem_size,
+ &dev->dbbuf_dbs_dma_addr,
+ GFP_KERNEL);
+ if (!dev->dbbuf_dbs)
+ return -ENOMEM;
+ dev->dbbuf_eis = dma_alloc_coherent(dev->dev, mem_size,
+ &dev->dbbuf_eis_dma_addr,
+ GFP_KERNEL);
+ if (!dev->dbbuf_eis) {
+ dma_free_coherent(dev->dev, mem_size,
+ dev->dbbuf_dbs, dev->dbbuf_dbs_dma_addr);
+ dev->dbbuf_dbs = NULL;
+ return -ENOMEM;
+ }
+
+ return 0;
+}
+
+static void nvme_dbbuf_dma_free(struct nvme_dev *dev)
+{
+ unsigned int mem_size = nvme_dbbuf_size(dev->db_stride);
+
+ if (dev->dbbuf_dbs) {
+ dma_free_coherent(dev->dev, mem_size,
+ dev->dbbuf_dbs, dev->dbbuf_dbs_dma_addr);
+ dev->dbbuf_dbs = NULL;
+ }
+ if (dev->dbbuf_eis) {
+ dma_free_coherent(dev->dev, mem_size,
+ dev->dbbuf_eis, dev->dbbuf_eis_dma_addr);
+ dev->dbbuf_eis = NULL;
+ }
+}
+
+static void nvme_dbbuf_init(struct nvme_dev *dev,
+ struct nvme_queue *nvmeq, int qid)
+{
+ if (!dev->dbbuf_dbs || !qid)
+ return;
+
+ nvmeq->dbbuf_sq_db = &dev->dbbuf_dbs[sq_idx(qid, dev->db_stride)];
+ nvmeq->dbbuf_cq_db = &dev->dbbuf_dbs[cq_idx(qid, dev->db_stride)];
+ nvmeq->dbbuf_sq_ei = &dev->dbbuf_eis[sq_idx(qid, dev->db_stride)];
+ nvmeq->dbbuf_cq_ei = &dev->dbbuf_eis[cq_idx(qid, dev->db_stride)];
+}
+
+static void nvme_dbbuf_set(struct nvme_dev *dev)
+{
+ struct nvme_command c;
+
+ if (!dev->dbbuf_dbs)
+ return;
+
+ memset(&c, 0, sizeof(c));
+ c.dbbuf.opcode = nvme_admin_dbbuf;
+ c.dbbuf.prp1 = cpu_to_le64(dev->dbbuf_dbs_dma_addr);
+ c.dbbuf.prp2 = cpu_to_le64(dev->dbbuf_eis_dma_addr);
+
+ if (nvme_submit_sync_cmd(dev->ctrl.admin_q, &c, NULL, 0)) {
+ dev_warn(dev->dev, "unable to set dbbuf\n");
+ /* Free memory and continue on */
+ nvme_dbbuf_dma_free(dev);
+ }
+}
+
+static inline int nvme_dbbuf_need_event(u16 event_idx, u16 new_idx, u16 old)
+{
+ return (u16)(new_idx - event_idx - 1) < (u16)(new_idx - old);
+}
+
+static void nvme_write_doorbell(u16 value, u32 __iomem *db, u32 *dbbuf_db,
+ volatile u32 *dbbuf_ei)
+{
+ if (dbbuf_db) {
+ u16 old_value;
+
+ /*
+ * Ensure that the queue is written before updating
+ * the doorbell in memory
+ */
+ wmb();
+
+ old_value = *dbbuf_db;
+ *dbbuf_db = value;
+ if (!nvme_dbbuf_need_event(*dbbuf_ei, value, old_value))
+ return;
+ }
+
+ writel(value, db);
}
/*
@@ -300,7 +422,8 @@ static void __nvme_submit_cmd(struct nvme_queue *nvmeq,
if (++tail == nvmeq->q_depth)
tail = 0;
- writel(tail, nvmeq->q_db);
+ nvme_write_doorbell(tail, nvmeq->q_db, nvmeq->dbbuf_sq_db,
+ nvmeq->dbbuf_sq_ei);
nvmeq->sq_tail = tail;
}
@@ -716,7 +839,8 @@ static void __nvme_process_cq(struct nvme_queue *nvmeq, unsigned int *tag)
return;
if (likely(nvmeq->cq_vector >= 0))
- writel(head, nvmeq->q_db + nvmeq->dev->db_stride);
+ nvme_write_doorbell(head, nvmeq->q_db + nvmeq->dev->db_stride,
+ nvmeq->dbbuf_cq_db, nvmeq->dbbuf_cq_ei);
nvmeq->cq_head = head;
nvmeq->cq_phase = phase;
@@ -1099,6 +1223,7 @@ static void nvme_init_queue(struct nvme_queue *nvmeq, u16 qid)
nvmeq->cq_phase = 1;
nvmeq->q_db = &dev->dbs[qid * 2 * dev->db_stride];
memset((void *)nvmeq->cqes, 0, CQ_SIZE(nvmeq->q_depth));
+ nvme_dbbuf_init(dev, nvmeq, qid);
dev->online_queues++;
spin_unlock_irq(&nvmeq->q_lock);
}
@@ -1568,6 +1693,8 @@ static int nvme_dev_add(struct nvme_dev *dev)
if (blk_mq_alloc_tag_set(&dev->tagset))
return 0;
dev->ctrl.tagset = &dev->tagset;
+
+ nvme_dbbuf_set(dev);
} else {
blk_mq_update_nr_hw_queues(&dev->tagset, dev->online_queues - 1);
@@ -1733,6 +1860,7 @@ static void nvme_pci_free_ctrl(struct nvme_ctrl *ctrl)
{
struct nvme_dev *dev = to_nvme_dev(ctrl);
+ nvme_dbbuf_dma_free(dev);
put_device(dev->dev);
if (dev->tagset.tags)
blk_mq_free_tag_set(&dev->tagset);
@@ -1800,6 +1928,13 @@ static void nvme_reset_work(struct work_struct *work)
dev->ctrl.opal_dev = NULL;
}
+ if (dev->ctrl.oacs & NVME_CTRL_OACS_DBBUF_SUPP) {
+ result = nvme_dbbuf_dma_alloc(dev);
+ if (result)
+ dev_warn(dev->dev,
+ "unable to allocate dma for dbbuf\n");
+ }
+
result = nvme_setup_io_queues(dev);
if (result)
goto out;
diff --git a/include/linux/nvme.h b/include/linux/nvme.h
index c43d435..43a6289 100644
--- a/include/linux/nvme.h
+++ b/include/linux/nvme.h
@@ -245,6 +245,7 @@ enum {
NVME_CTRL_ONCS_WRITE_ZEROES = 1 << 3,
NVME_CTRL_VWC_PRESENT = 1 << 0,
NVME_CTRL_OACS_SEC_SUPP = 1 << 0,
+ NVME_CTRL_OACS_DBBUF_SUPP = 1 << 7,
};
struct nvme_lbaf {
@@ -603,6 +604,7 @@ enum nvme_admin_opcode {
nvme_admin_download_fw = 0x11,
nvme_admin_ns_attach = 0x15,
nvme_admin_keep_alive = 0x18,
+ nvme_admin_dbbuf = 0x7C,
nvme_admin_format_nvm = 0x80,
nvme_admin_security_send = 0x81,
nvme_admin_security_recv = 0x82,
@@ -874,6 +876,16 @@ struct nvmf_property_get_command {
__u8 resv4[16];
};
+struct nvme_dbbuf {
+ __u8 opcode;
+ __u8 flags;
+ __u16 command_id;
+ __u32 rsvd1[5];
+ __le64 prp1;
+ __le64 prp2;
+ __u32 rsvd12[6];
+};
+
struct nvme_command {
union {
struct nvme_common_command common;
@@ -893,6 +905,7 @@ struct nvme_command {
struct nvmf_connect_command connect;
struct nvmf_property_set_command prop_set;
struct nvmf_property_get_command prop_get;
+ struct nvme_dbbuf dbbuf;
};
};
--
2.7.4
^ permalink raw reply related [flat|nested] 49+ messages in thread
* [PATCH v7 RFC] nvme: improve performance for virtual NVMe devices
2017-03-30 17:46 ` [PATCH v7 " Helen Koike
@ 2017-03-31 7:01 ` Christoph Hellwig
2017-04-01 21:50 ` Helen Koike
2017-04-10 15:51 ` [PATCH v8] " Helen Koike
0 siblings, 2 replies; 49+ messages in thread
From: Christoph Hellwig @ 2017-03-31 7:01 UTC (permalink / raw)
I'd still prefer to not hide the MMIO path behind nvme_write_doorbell,
but except for that this looks fine to me:
Reviewed-by: Christoph Hellwig <hch at lst.de>
^ permalink raw reply [flat|nested] 49+ messages in thread
* [PATCH v7 RFC] nvme: improve performance for virtual NVMe devices
2017-03-31 7:01 ` Christoph Hellwig
@ 2017-04-01 21:50 ` Helen Koike
2017-04-10 15:31 ` Helen Koike
2017-04-10 15:37 ` Christoph Hellwig
2017-04-10 15:51 ` [PATCH v8] " Helen Koike
1 sibling, 2 replies; 49+ messages in thread
From: Helen Koike @ 2017-04-01 21:50 UTC (permalink / raw)
Hi Christoph,
Thanks for reviewing this patch
On 2017-03-31 04:01 AM, Christoph Hellwig wrote:
> I'd still prefer to not hide the MMIO path behind nvme_write_doorbell,
> but except for that this looks fine to me:
>
Do you mean wrapping the call to writel()? What do you think if I send a new
version with the changes below? It wouldn't hide the call to writel()
inside nvme_write_doorbell anymore.
Helen
diff --git a/drivers/nvme/host/pci.c b/drivers/nvme/host/pci.c
index 3ce705a..0e5f2a1 100644
--- a/drivers/nvme/host/pci.c
+++ b/drivers/nvme/host/pci.c
@@ -277,8 +277,9 @@ static inline int nvme_dbbuf_need_event(u16
event_idx, u16 new_idx, u16 old)
return (u16)(new_idx - event_idx - 1) < (u16)(new_idx - old);
}
-static void nvme_write_doorbell(u16 value, u32 __iomem *db, u32 *dbbuf_db,
- volatile u32 *dbbuf_ei)
+/* Update dbbuf and return true if a MMIO is required */
+static bool nvme_dbbuf_update_and_check_event(u16 value, u32 *dbbuf_db,
+ volatile u32 *dbbuf_ei)
{
if (dbbuf_db) {
u16 old_value;
@@ -291,11 +292,12 @@ static void nvme_write_doorbell(u16 value, u32
__iomem *db, u32 *dbbuf_db,
old_value = *dbbuf_db;
*dbbuf_db = value;
+
if (!nvme_dbbuf_need_event(*dbbuf_ei, value, old_value))
- return;
+ return false;
}
- writel(value, db);
+ return true;
}
/*
@@ -422,8 +424,9 @@ static void __nvme_submit_cmd(struct nvme_queue *nvmeq,
if (++tail == nvmeq->q_depth)
tail = 0;
- nvme_write_doorbell(tail, nvmeq->q_db, nvmeq->dbbuf_sq_db,
- nvmeq->dbbuf_sq_ei);
+ if (nvme_dbbuf_update_and_check_event(tail, nvmeq->dbbuf_sq_db,
+ nvmeq->dbbuf_sq_ei))
+ writel(tail, nvmeq->q_db);
nvmeq->sq_tail = tail;
}
@@ -839,8 +842,9 @@ static void __nvme_process_cq(struct nvme_queue
*nvmeq, unsigned int *tag)
return;
if (likely(nvmeq->cq_vector >= 0))
- nvme_write_doorbell(head, nvmeq->q_db +
nvmeq->dev->db_stride,
- nvmeq->dbbuf_cq_db, nvmeq->dbbuf_cq_ei);
+ if (nvme_dbbuf_update_and_check_event(head,
nvmeq->dbbuf_cq_db,
+ nvmeq->dbbuf_cq_ei))
+ writel(head, nvmeq->q_db + nvmeq->dev->db_stride);
nvmeq->cq_head = head;
nvmeq->cq_phase = phase;
^ permalink raw reply related [flat|nested] 49+ messages in thread
* [PATCH v7 RFC] nvme: improve performance for virtual NVMe devices
2017-04-01 21:50 ` Helen Koike
@ 2017-04-10 15:31 ` Helen Koike
2017-04-10 15:37 ` Christoph Hellwig
1 sibling, 0 replies; 49+ messages in thread
From: Helen Koike @ 2017-04-10 15:31 UTC (permalink / raw)
ping
On 2017-04-01 06:50 PM, Helen Koike wrote:
>
> Hi Christoph,
>
> Thanks for reviewing this patch
>
> On 2017-03-31 04:01 AM, Christoph Hellwig wrote:
>> I'd still prefer to not hide the MMIO path behind nvme_write_doorbell,
>> but except for that this looks fine to me:
>>
>
> Do you mean wrapping the call to writel()? What do you think if I send a
> new
> version with the changes below? It wouldn't hide the call to writel()
> inside nvme_write_doorbell anymore.
>
> Helen
>
> diff --git a/drivers/nvme/host/pci.c b/drivers/nvme/host/pci.c
> index 3ce705a..0e5f2a1 100644
> --- a/drivers/nvme/host/pci.c
> +++ b/drivers/nvme/host/pci.c
> @@ -277,8 +277,9 @@ static inline int nvme_dbbuf_need_event(u16
> event_idx, u16 new_idx, u16 old)
> return (u16)(new_idx - event_idx - 1) < (u16)(new_idx - old);
> }
>
> -static void nvme_write_doorbell(u16 value, u32 __iomem *db, u32 *dbbuf_db,
> - volatile u32 *dbbuf_ei)
> +/* Update dbbuf and return true if a MMIO is required */
> +static bool nvme_dbbuf_update_and_check_event(u16 value, u32 *dbbuf_db,
> + volatile u32 *dbbuf_ei)
> {
> if (dbbuf_db) {
> u16 old_value;
> @@ -291,11 +292,12 @@ static void nvme_write_doorbell(u16 value, u32
> __iomem *db, u32 *dbbuf_db,
>
> old_value = *dbbuf_db;
> *dbbuf_db = value;
> +
> if (!nvme_dbbuf_need_event(*dbbuf_ei, value, old_value))
> - return;
> + return false;
> }
>
> - writel(value, db);
> + return true;
> }
>
> /*
> @@ -422,8 +424,9 @@ static void __nvme_submit_cmd(struct nvme_queue *nvmeq,
>
> if (++tail == nvmeq->q_depth)
> tail = 0;
> - nvme_write_doorbell(tail, nvmeq->q_db, nvmeq->dbbuf_sq_db,
> - nvmeq->dbbuf_sq_ei);
> + if (nvme_dbbuf_update_and_check_event(tail, nvmeq->dbbuf_sq_db,
> + nvmeq->dbbuf_sq_ei))
> + writel(tail, nvmeq->q_db);
> nvmeq->sq_tail = tail;
> }
>
> @@ -839,8 +842,9 @@ static void __nvme_process_cq(struct nvme_queue
> *nvmeq, unsigned int *tag)
> return;
>
> if (likely(nvmeq->cq_vector >= 0))
> - nvme_write_doorbell(head, nvmeq->q_db +
> nvmeq->dev->db_stride,
> - nvmeq->dbbuf_cq_db,
> nvmeq->dbbuf_cq_ei);
> + if (nvme_dbbuf_update_and_check_event(head,
> nvmeq->dbbuf_cq_db,
> + nvmeq->dbbuf_cq_ei))
> + writel(head, nvmeq->q_db + nvmeq->dev->db_stride);
> nvmeq->cq_head = head;
> nvmeq->cq_phase = phase;
>
^ permalink raw reply [flat|nested] 49+ messages in thread
* [PATCH v7 RFC] nvme: improve performance for virtual NVMe devices
2017-04-01 21:50 ` Helen Koike
2017-04-10 15:31 ` Helen Koike
@ 2017-04-10 15:37 ` Christoph Hellwig
1 sibling, 0 replies; 49+ messages in thread
From: Christoph Hellwig @ 2017-04-10 15:37 UTC (permalink / raw)
On Sat, Apr 01, 2017@06:50:03PM -0300, Helen Koike wrote:
>
> Hi Christoph,
>
> Thanks for reviewing this patch
>
> On 2017-03-31 04:01 AM, Christoph Hellwig wrote:
> > I'd still prefer to not hide the MMIO path behind nvme_write_doorbell,
> > but except for that this looks fine to me:
> >
>
> Do you mean wrapping the call to writel()? What do you think if I send a new
> version with the changes below? It wouldn't hide the call to writel()
> inside nvme_write_doorbell anymore.
Yes, that's the version I'd prefer.
^ permalink raw reply [flat|nested] 49+ messages in thread
* [PATCH v8] nvme: improve performance for virtual NVMe devices
2017-03-31 7:01 ` Christoph Hellwig
2017-04-01 21:50 ` Helen Koike
@ 2017-04-10 15:51 ` Helen Koike
2017-04-14 18:10 ` Helen Koike
1 sibling, 1 reply; 49+ messages in thread
From: Helen Koike @ 2017-04-10 15:51 UTC (permalink / raw)
From: Helen Koike <helen.koike@collabora.co.uk>
This change provides a mechanism to reduce the number of MMIO doorbell
writes for the NVMe driver. When running in a virtualized environment
like QEMU, the cost of an MMIO is quite hefy here. The main idea for
the patch is provide the device two memory location locations:
1) to store the doorbell values so they can be lookup without the doorbell
MMIO write
2) to store an event index.
I believe the doorbell value is obvious, the event index not so much.
Similar to the virtio specification, the virtual device can tell the
driver (guest OS) not to write MMIO unless you are writing past this
value.
FYI: doorbell values are written by the nvme driver (guest OS) and the
event index is written by the virtual device (host OS).
The patch implements a new admin command that will communicate where
these two memory locations reside. If the command fails, the nvme
driver will work as before without any optimizations.
Contributions:
Eric Northup <digitaleric at google.com>
Frank Swiderski <fes at google.com>
Ted Tso <tytso at mit.edu>
Keith Busch <keith.busch at intel.com>
Just to give an idea on the performance boost with the vendor
extension: Running fio [1], a stock NVMe driver I get about 200K read
IOPs with my vendor patch I get about 1000K read IOPs. This was
running with a null device i.e. the backing device simply returned
success on every read IO request.
[1] Running on a 4 core machine:
fio --time_based --name=benchmark --runtime=30
--filename=/dev/nvme0n1 --nrfiles=1 --ioengine=libaio --iodepth=32
--direct=1 --invalidate=1 --verify=0 --verify_fatal=0 --numjobs=4
--rw=randread --blocksize=4k --randrepeat=false
Signed-off-by: Rob Nelson <rlnelson at google.com>
[mlin: port for upstream]
Signed-off-by: Ming Lin <mlin at kernel.org>
[koike: updated for upstream]
Signed-off-by: Helen Koike <helen.koike at collabora.co.uk>
Reviewed-by: Christoph Hellwig <hch at lst.de>
---
This patch is based on git://git.infradead.org/nvme.git master
Tested through Google Cloud Engine
TPAR is ratified by the NVME working group
changes since v7:
- remove nvme_write_doorbell(), add
nvme_dbbuf_update_and_check_event() instead that doesn't write
to the doorbell, only returns true if db needs to be updated
- add reviewed-by tag from Christoph
changes since v6:
- remove nvme_dbbuf_init() from nvme_alloc_queue() as it is
already called in nvme_init_queue()
- free dbbuf_dbs only in nvme_pci_free_ctrl(), change
nvme_dbbuf_dma_alloc() to do nothing the dbbuf_dbs is already
allocated
changes since v5:
- remove anonymous dbbuf struct in struct nvme_dev and struct nvme_queue
- use inline functions for sq_idx and cq_idx instead of macros
- add a warning when nvme_dbbuf_set fails
- remove comment "Borrowed from vring_need_event"
- change formatting of the parameters in nvme_write_doorbell
- don't fail nvme_reset_work if nvme_dbbuf_dma_alloc fails
- remove nvme_write_doorbell_{sq,cq} wrappers
- in nvme_write_doorbell(), call writel last
Changes since v4
- Remove CONFIG_NVME_DBBUF
- Remove dbbuf.{c,h}, move the code to pci.c
- Coding style changes
- Functions and vaiables renamed
Changes since v3
- Rename to dbbuf (be closer to the latest TPAR)
- Modify the opcodes according to the latest TPAR
- Check if the device support this feature through the OACS bit
- little cleanups
nvme_dbbuf_update_and_check_event
---
drivers/nvme/host/pci.c | 143 +++++++++++++++++++++++++++++++++++++++++++++++-
include/linux/nvme.h | 13 +++++
2 files changed, 154 insertions(+), 2 deletions(-)
diff --git a/drivers/nvme/host/pci.c b/drivers/nvme/host/pci.c
index 5b7bd9c..0e5f2a1 100644
--- a/drivers/nvme/host/pci.c
+++ b/drivers/nvme/host/pci.c
@@ -103,8 +103,22 @@ struct nvme_dev {
u32 cmbloc;
struct nvme_ctrl ctrl;
struct completion ioq_wait;
+ u32 *dbbuf_dbs;
+ dma_addr_t dbbuf_dbs_dma_addr;
+ u32 *dbbuf_eis;
+ dma_addr_t dbbuf_eis_dma_addr;
};
+static inline unsigned int sq_idx(unsigned int qid, u32 stride)
+{
+ return qid * 2 * stride;
+}
+
+static inline unsigned int cq_idx(unsigned int qid, u32 stride)
+{
+ return (qid * 2 + 1) * stride;
+}
+
static inline struct nvme_dev *to_nvme_dev(struct nvme_ctrl *ctrl)
{
return container_of(ctrl, struct nvme_dev, ctrl);
@@ -133,6 +147,10 @@ struct nvme_queue {
u16 qid;
u8 cq_phase;
u8 cqe_seen;
+ u32 *dbbuf_sq_db;
+ u32 *dbbuf_cq_db;
+ u32 *dbbuf_sq_ei;
+ u32 *dbbuf_cq_ei;
};
/*
@@ -174,6 +192,112 @@ static inline void _nvme_check_size(void)
BUILD_BUG_ON(sizeof(struct nvme_id_ns) != 4096);
BUILD_BUG_ON(sizeof(struct nvme_lba_range_type) != 64);
BUILD_BUG_ON(sizeof(struct nvme_smart_log) != 512);
+ BUILD_BUG_ON(sizeof(struct nvme_dbbuf) != 64);
+}
+
+static inline unsigned int nvme_dbbuf_size(u32 stride)
+{
+ return ((num_possible_cpus() + 1) * 8 * stride);
+}
+
+static int nvme_dbbuf_dma_alloc(struct nvme_dev *dev)
+{
+ unsigned int mem_size = nvme_dbbuf_size(dev->db_stride);
+
+ if (dev->dbbuf_dbs)
+ return 0;
+
+ dev->dbbuf_dbs = dma_alloc_coherent(dev->dev, mem_size,
+ &dev->dbbuf_dbs_dma_addr,
+ GFP_KERNEL);
+ if (!dev->dbbuf_dbs)
+ return -ENOMEM;
+ dev->dbbuf_eis = dma_alloc_coherent(dev->dev, mem_size,
+ &dev->dbbuf_eis_dma_addr,
+ GFP_KERNEL);
+ if (!dev->dbbuf_eis) {
+ dma_free_coherent(dev->dev, mem_size,
+ dev->dbbuf_dbs, dev->dbbuf_dbs_dma_addr);
+ dev->dbbuf_dbs = NULL;
+ return -ENOMEM;
+ }
+
+ return 0;
+}
+
+static void nvme_dbbuf_dma_free(struct nvme_dev *dev)
+{
+ unsigned int mem_size = nvme_dbbuf_size(dev->db_stride);
+
+ if (dev->dbbuf_dbs) {
+ dma_free_coherent(dev->dev, mem_size,
+ dev->dbbuf_dbs, dev->dbbuf_dbs_dma_addr);
+ dev->dbbuf_dbs = NULL;
+ }
+ if (dev->dbbuf_eis) {
+ dma_free_coherent(dev->dev, mem_size,
+ dev->dbbuf_eis, dev->dbbuf_eis_dma_addr);
+ dev->dbbuf_eis = NULL;
+ }
+}
+
+static void nvme_dbbuf_init(struct nvme_dev *dev,
+ struct nvme_queue *nvmeq, int qid)
+{
+ if (!dev->dbbuf_dbs || !qid)
+ return;
+
+ nvmeq->dbbuf_sq_db = &dev->dbbuf_dbs[sq_idx(qid, dev->db_stride)];
+ nvmeq->dbbuf_cq_db = &dev->dbbuf_dbs[cq_idx(qid, dev->db_stride)];
+ nvmeq->dbbuf_sq_ei = &dev->dbbuf_eis[sq_idx(qid, dev->db_stride)];
+ nvmeq->dbbuf_cq_ei = &dev->dbbuf_eis[cq_idx(qid, dev->db_stride)];
+}
+
+static void nvme_dbbuf_set(struct nvme_dev *dev)
+{
+ struct nvme_command c;
+
+ if (!dev->dbbuf_dbs)
+ return;
+
+ memset(&c, 0, sizeof(c));
+ c.dbbuf.opcode = nvme_admin_dbbuf;
+ c.dbbuf.prp1 = cpu_to_le64(dev->dbbuf_dbs_dma_addr);
+ c.dbbuf.prp2 = cpu_to_le64(dev->dbbuf_eis_dma_addr);
+
+ if (nvme_submit_sync_cmd(dev->ctrl.admin_q, &c, NULL, 0)) {
+ dev_warn(dev->dev, "unable to set dbbuf\n");
+ /* Free memory and continue on */
+ nvme_dbbuf_dma_free(dev);
+ }
+}
+
+static inline int nvme_dbbuf_need_event(u16 event_idx, u16 new_idx, u16 old)
+{
+ return (u16)(new_idx - event_idx - 1) < (u16)(new_idx - old);
+}
+
+/* Update dbbuf and return true if an MMIO is required */
+static bool nvme_dbbuf_update_and_check_event(u16 value, u32 *dbbuf_db,
+ volatile u32 *dbbuf_ei)
+{
+ if (dbbuf_db) {
+ u16 old_value;
+
+ /*
+ * Ensure that the queue is written before updating
+ * the doorbell in memory
+ */
+ wmb();
+
+ old_value = *dbbuf_db;
+ *dbbuf_db = value;
+
+ if (!nvme_dbbuf_need_event(*dbbuf_ei, value, old_value))
+ return false;
+ }
+
+ return true;
}
/*
@@ -300,7 +424,9 @@ static void __nvme_submit_cmd(struct nvme_queue *nvmeq,
if (++tail == nvmeq->q_depth)
tail = 0;
- writel(tail, nvmeq->q_db);
+ if (nvme_dbbuf_update_and_check_event(tail, nvmeq->dbbuf_sq_db,
+ nvmeq->dbbuf_sq_ei))
+ writel(tail, nvmeq->q_db);
nvmeq->sq_tail = tail;
}
@@ -716,7 +842,9 @@ static void __nvme_process_cq(struct nvme_queue *nvmeq, unsigned int *tag)
return;
if (likely(nvmeq->cq_vector >= 0))
- writel(head, nvmeq->q_db + nvmeq->dev->db_stride);
+ if (nvme_dbbuf_update_and_check_event(head, nvmeq->dbbuf_cq_db,
+ nvmeq->dbbuf_cq_ei))
+ writel(head, nvmeq->q_db + nvmeq->dev->db_stride);
nvmeq->cq_head = head;
nvmeq->cq_phase = phase;
@@ -1099,6 +1227,7 @@ static void nvme_init_queue(struct nvme_queue *nvmeq, u16 qid)
nvmeq->cq_phase = 1;
nvmeq->q_db = &dev->dbs[qid * 2 * dev->db_stride];
memset((void *)nvmeq->cqes, 0, CQ_SIZE(nvmeq->q_depth));
+ nvme_dbbuf_init(dev, nvmeq, qid);
dev->online_queues++;
spin_unlock_irq(&nvmeq->q_lock);
}
@@ -1568,6 +1697,8 @@ static int nvme_dev_add(struct nvme_dev *dev)
if (blk_mq_alloc_tag_set(&dev->tagset))
return 0;
dev->ctrl.tagset = &dev->tagset;
+
+ nvme_dbbuf_set(dev);
} else {
blk_mq_update_nr_hw_queues(&dev->tagset, dev->online_queues - 1);
@@ -1733,6 +1864,7 @@ static void nvme_pci_free_ctrl(struct nvme_ctrl *ctrl)
{
struct nvme_dev *dev = to_nvme_dev(ctrl);
+ nvme_dbbuf_dma_free(dev);
put_device(dev->dev);
if (dev->tagset.tags)
blk_mq_free_tag_set(&dev->tagset);
@@ -1800,6 +1932,13 @@ static void nvme_reset_work(struct work_struct *work)
dev->ctrl.opal_dev = NULL;
}
+ if (dev->ctrl.oacs & NVME_CTRL_OACS_DBBUF_SUPP) {
+ result = nvme_dbbuf_dma_alloc(dev);
+ if (result)
+ dev_warn(dev->dev,
+ "unable to allocate dma for dbbuf\n");
+ }
+
result = nvme_setup_io_queues(dev);
if (result)
goto out;
diff --git a/include/linux/nvme.h b/include/linux/nvme.h
index c43d435..43a6289 100644
--- a/include/linux/nvme.h
+++ b/include/linux/nvme.h
@@ -245,6 +245,7 @@ enum {
NVME_CTRL_ONCS_WRITE_ZEROES = 1 << 3,
NVME_CTRL_VWC_PRESENT = 1 << 0,
NVME_CTRL_OACS_SEC_SUPP = 1 << 0,
+ NVME_CTRL_OACS_DBBUF_SUPP = 1 << 7,
};
struct nvme_lbaf {
@@ -603,6 +604,7 @@ enum nvme_admin_opcode {
nvme_admin_download_fw = 0x11,
nvme_admin_ns_attach = 0x15,
nvme_admin_keep_alive = 0x18,
+ nvme_admin_dbbuf = 0x7C,
nvme_admin_format_nvm = 0x80,
nvme_admin_security_send = 0x81,
nvme_admin_security_recv = 0x82,
@@ -874,6 +876,16 @@ struct nvmf_property_get_command {
__u8 resv4[16];
};
+struct nvme_dbbuf {
+ __u8 opcode;
+ __u8 flags;
+ __u16 command_id;
+ __u32 rsvd1[5];
+ __le64 prp1;
+ __le64 prp2;
+ __u32 rsvd12[6];
+};
+
struct nvme_command {
union {
struct nvme_common_command common;
@@ -893,6 +905,7 @@ struct nvme_command {
struct nvmf_connect_command connect;
struct nvmf_property_set_command prop_set;
struct nvmf_property_get_command prop_get;
+ struct nvme_dbbuf dbbuf;
};
};
--
2.7.4
^ permalink raw reply related [flat|nested] 49+ messages in thread
* Re: [PATCH v8] nvme: improve performance for virtual NVMe devices
2017-04-10 15:51 ` [PATCH v8] " Helen Koike
@ 2017-04-14 18:10 ` Helen Koike
0 siblings, 0 replies; 49+ messages in thread
From: Helen Koike @ 2017-04-14 18:10 UTC (permalink / raw)
To: Christoph Hellwig
Cc: Keith Busch, fes, axboe, rlnelson,
open list : NVM EXPRESS DRIVER, mikew, digitaleric, monish,
james_p_freyensee @ linux . intel . com, tytso, mlin, sagi,
linux-kernel
On 2017-04-10 12:51 PM, Helen Koike wrote:
> From: Helen Koike <helen.koike@collabora.co.uk>
>
> This change provides a mechanism to reduce the number of MMIO doorbell
> writes for the NVMe driver. When running in a virtualized environment
> like QEMU, the cost of an MMIO is quite hefy here. The main idea for
> the patch is provide the device two memory location locations:
> 1) to store the doorbell values so they can be lookup without the doorbell
> MMIO write
> 2) to store an event index.
> I believe the doorbell value is obvious, the event index not so much.
> Similar to the virtio specification, the virtual device can tell the
> driver (guest OS) not to write MMIO unless you are writing past this
> value.
>
> FYI: doorbell values are written by the nvme driver (guest OS) and the
> event index is written by the virtual device (host OS).
>
> The patch implements a new admin command that will communicate where
> these two memory locations reside. If the command fails, the nvme
> driver will work as before without any optimizations.
>
> Contributions:
> Eric Northup <digitaleric@google.com>
> Frank Swiderski <fes@google.com>
> Ted Tso <tytso@mit.edu>
> Keith Busch <keith.busch@intel.com>
>
> Just to give an idea on the performance boost with the vendor
> extension: Running fio [1], a stock NVMe driver I get about 200K read
> IOPs with my vendor patch I get about 1000K read IOPs. This was
> running with a null device i.e. the backing device simply returned
> success on every read IO request.
>
> [1] Running on a 4 core machine:
> fio --time_based --name=benchmark --runtime=30
> --filename=/dev/nvme0n1 --nrfiles=1 --ioengine=libaio --iodepth=32
> --direct=1 --invalidate=1 --verify=0 --verify_fatal=0 --numjobs=4
> --rw=randread --blocksize=4k --randrepeat=false
>
> Signed-off-by: Rob Nelson <rlnelson@google.com>
> [mlin: port for upstream]
> Signed-off-by: Ming Lin <mlin@kernel.org>
> [koike: updated for upstream]
> Signed-off-by: Helen Koike <helen.koike@collabora.co.uk>
> Reviewed-by: Christoph Hellwig <hch@lst.de>
>
> ---
>
> This patch is based on git://git.infradead.org/nvme.git master
> Tested through Google Cloud Engine
> TPAR is ratified by the NVME working group
>
> changes since v7:
> - remove nvme_write_doorbell(), add
> nvme_dbbuf_update_and_check_event() instead that doesn't write
> to the doorbell, only returns true if db needs to be updated
> - add reviewed-by tag from Christoph
>
> changes since v6:
> - remove nvme_dbbuf_init() from nvme_alloc_queue() as it is
> already called in nvme_init_queue()
> - free dbbuf_dbs only in nvme_pci_free_ctrl(), change
> nvme_dbbuf_dma_alloc() to do nothing the dbbuf_dbs is already
> allocated
>
> changes since v5:
> - remove anonymous dbbuf struct in struct nvme_dev and struct nvme_queue
> - use inline functions for sq_idx and cq_idx instead of macros
> - add a warning when nvme_dbbuf_set fails
> - remove comment "Borrowed from vring_need_event"
> - change formatting of the parameters in nvme_write_doorbell
> - don't fail nvme_reset_work if nvme_dbbuf_dma_alloc fails
> - remove nvme_write_doorbell_{sq,cq} wrappers
> - in nvme_write_doorbell(), call writel last
>
> Changes since v4
> - Remove CONFIG_NVME_DBBUF
> - Remove dbbuf.{c,h}, move the code to pci.c
> - Coding style changes
> - Functions and vaiables renamed
>
> Changes since v3
> - Rename to dbbuf (be closer to the latest TPAR)
> - Modify the opcodes according to the latest TPAR
> - Check if the device support this feature through the OACS bit
> - little cleanups
>
> nvme_dbbuf_update_and_check_event
> ---
> drivers/nvme/host/pci.c | 143 +++++++++++++++++++++++++++++++++++++++++++++++-
> include/linux/nvme.h | 13 +++++
> 2 files changed, 154 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/nvme/host/pci.c b/drivers/nvme/host/pci.c
> index 5b7bd9c..0e5f2a1 100644
> --- a/drivers/nvme/host/pci.c
> +++ b/drivers/nvme/host/pci.c
> @@ -103,8 +103,22 @@ struct nvme_dev {
> u32 cmbloc;
> struct nvme_ctrl ctrl;
> struct completion ioq_wait;
> + u32 *dbbuf_dbs;
> + dma_addr_t dbbuf_dbs_dma_addr;
> + u32 *dbbuf_eis;
> + dma_addr_t dbbuf_eis_dma_addr;
> };
>
> +static inline unsigned int sq_idx(unsigned int qid, u32 stride)
> +{
> + return qid * 2 * stride;
> +}
> +
> +static inline unsigned int cq_idx(unsigned int qid, u32 stride)
> +{
> + return (qid * 2 + 1) * stride;
> +}
> +
> static inline struct nvme_dev *to_nvme_dev(struct nvme_ctrl *ctrl)
> {
> return container_of(ctrl, struct nvme_dev, ctrl);
> @@ -133,6 +147,10 @@ struct nvme_queue {
> u16 qid;
> u8 cq_phase;
> u8 cqe_seen;
> + u32 *dbbuf_sq_db;
> + u32 *dbbuf_cq_db;
> + u32 *dbbuf_sq_ei;
> + u32 *dbbuf_cq_ei;
> };
>
> /*
> @@ -174,6 +192,112 @@ static inline void _nvme_check_size(void)
> BUILD_BUG_ON(sizeof(struct nvme_id_ns) != 4096);
> BUILD_BUG_ON(sizeof(struct nvme_lba_range_type) != 64);
> BUILD_BUG_ON(sizeof(struct nvme_smart_log) != 512);
> + BUILD_BUG_ON(sizeof(struct nvme_dbbuf) != 64);
> +}
> +
> +static inline unsigned int nvme_dbbuf_size(u32 stride)
> +{
> + return ((num_possible_cpus() + 1) * 8 * stride);
> +}
> +
> +static int nvme_dbbuf_dma_alloc(struct nvme_dev *dev)
> +{
> + unsigned int mem_size = nvme_dbbuf_size(dev->db_stride);
> +
> + if (dev->dbbuf_dbs)
> + return 0;
> +
> + dev->dbbuf_dbs = dma_alloc_coherent(dev->dev, mem_size,
> + &dev->dbbuf_dbs_dma_addr,
> + GFP_KERNEL);
> + if (!dev->dbbuf_dbs)
> + return -ENOMEM;
> + dev->dbbuf_eis = dma_alloc_coherent(dev->dev, mem_size,
> + &dev->dbbuf_eis_dma_addr,
> + GFP_KERNEL);
> + if (!dev->dbbuf_eis) {
> + dma_free_coherent(dev->dev, mem_size,
> + dev->dbbuf_dbs, dev->dbbuf_dbs_dma_addr);
> + dev->dbbuf_dbs = NULL;
> + return -ENOMEM;
> + }
> +
> + return 0;
> +}
> +
> +static void nvme_dbbuf_dma_free(struct nvme_dev *dev)
> +{
> + unsigned int mem_size = nvme_dbbuf_size(dev->db_stride);
> +
> + if (dev->dbbuf_dbs) {
> + dma_free_coherent(dev->dev, mem_size,
> + dev->dbbuf_dbs, dev->dbbuf_dbs_dma_addr);
> + dev->dbbuf_dbs = NULL;
> + }
> + if (dev->dbbuf_eis) {
> + dma_free_coherent(dev->dev, mem_size,
> + dev->dbbuf_eis, dev->dbbuf_eis_dma_addr);
> + dev->dbbuf_eis = NULL;
> + }
> +}
> +
> +static void nvme_dbbuf_init(struct nvme_dev *dev,
> + struct nvme_queue *nvmeq, int qid)
> +{
> + if (!dev->dbbuf_dbs || !qid)
> + return;
> +
> + nvmeq->dbbuf_sq_db = &dev->dbbuf_dbs[sq_idx(qid, dev->db_stride)];
> + nvmeq->dbbuf_cq_db = &dev->dbbuf_dbs[cq_idx(qid, dev->db_stride)];
> + nvmeq->dbbuf_sq_ei = &dev->dbbuf_eis[sq_idx(qid, dev->db_stride)];
> + nvmeq->dbbuf_cq_ei = &dev->dbbuf_eis[cq_idx(qid, dev->db_stride)];
> +}
> +
> +static void nvme_dbbuf_set(struct nvme_dev *dev)
> +{
> + struct nvme_command c;
> +
> + if (!dev->dbbuf_dbs)
> + return;
> +
> + memset(&c, 0, sizeof(c));
> + c.dbbuf.opcode = nvme_admin_dbbuf;
> + c.dbbuf.prp1 = cpu_to_le64(dev->dbbuf_dbs_dma_addr);
> + c.dbbuf.prp2 = cpu_to_le64(dev->dbbuf_eis_dma_addr);
> +
> + if (nvme_submit_sync_cmd(dev->ctrl.admin_q, &c, NULL, 0)) {
> + dev_warn(dev->dev, "unable to set dbbuf\n");
> + /* Free memory and continue on */
> + nvme_dbbuf_dma_free(dev);
> + }
> +}
> +
> +static inline int nvme_dbbuf_need_event(u16 event_idx, u16 new_idx, u16 old)
> +{
> + return (u16)(new_idx - event_idx - 1) < (u16)(new_idx - old);
> +}
> +
> +/* Update dbbuf and return true if an MMIO is required */
> +static bool nvme_dbbuf_update_and_check_event(u16 value, u32 *dbbuf_db,
> + volatile u32 *dbbuf_ei)
> +{
> + if (dbbuf_db) {
> + u16 old_value;
> +
> + /*
> + * Ensure that the queue is written before updating
> + * the doorbell in memory
> + */
> + wmb();
> +
> + old_value = *dbbuf_db;
> + *dbbuf_db = value;
> +
> + if (!nvme_dbbuf_need_event(*dbbuf_ei, value, old_value))
> + return false;
> + }
> +
> + return true;
> }
>
> /*
> @@ -300,7 +424,9 @@ static void __nvme_submit_cmd(struct nvme_queue *nvmeq,
>
> if (++tail == nvmeq->q_depth)
> tail = 0;
> - writel(tail, nvmeq->q_db);
> + if (nvme_dbbuf_update_and_check_event(tail, nvmeq->dbbuf_sq_db,
> + nvmeq->dbbuf_sq_ei))
> + writel(tail, nvmeq->q_db);
> nvmeq->sq_tail = tail;
> }
>
> @@ -716,7 +842,9 @@ static void __nvme_process_cq(struct nvme_queue *nvmeq, unsigned int *tag)
> return;
>
> if (likely(nvmeq->cq_vector >= 0))
> - writel(head, nvmeq->q_db + nvmeq->dev->db_stride);
> + if (nvme_dbbuf_update_and_check_event(head, nvmeq->dbbuf_cq_db,
> + nvmeq->dbbuf_cq_ei))
> + writel(head, nvmeq->q_db + nvmeq->dev->db_stride);
> nvmeq->cq_head = head;
> nvmeq->cq_phase = phase;
>
> @@ -1099,6 +1227,7 @@ static void nvme_init_queue(struct nvme_queue *nvmeq, u16 qid)
> nvmeq->cq_phase = 1;
> nvmeq->q_db = &dev->dbs[qid * 2 * dev->db_stride];
> memset((void *)nvmeq->cqes, 0, CQ_SIZE(nvmeq->q_depth));
> + nvme_dbbuf_init(dev, nvmeq, qid);
> dev->online_queues++;
> spin_unlock_irq(&nvmeq->q_lock);
> }
> @@ -1568,6 +1697,8 @@ static int nvme_dev_add(struct nvme_dev *dev)
> if (blk_mq_alloc_tag_set(&dev->tagset))
> return 0;
> dev->ctrl.tagset = &dev->tagset;
> +
> + nvme_dbbuf_set(dev);
> } else {
> blk_mq_update_nr_hw_queues(&dev->tagset, dev->online_queues - 1);
>
> @@ -1733,6 +1864,7 @@ static void nvme_pci_free_ctrl(struct nvme_ctrl *ctrl)
> {
> struct nvme_dev *dev = to_nvme_dev(ctrl);
>
> + nvme_dbbuf_dma_free(dev);
> put_device(dev->dev);
> if (dev->tagset.tags)
> blk_mq_free_tag_set(&dev->tagset);
> @@ -1800,6 +1932,13 @@ static void nvme_reset_work(struct work_struct *work)
> dev->ctrl.opal_dev = NULL;
> }
>
> + if (dev->ctrl.oacs & NVME_CTRL_OACS_DBBUF_SUPP) {
> + result = nvme_dbbuf_dma_alloc(dev);
> + if (result)
> + dev_warn(dev->dev,
> + "unable to allocate dma for dbbuf\n");
> + }
> +
> result = nvme_setup_io_queues(dev);
> if (result)
> goto out;
> diff --git a/include/linux/nvme.h b/include/linux/nvme.h
> index c43d435..43a6289 100644
> --- a/include/linux/nvme.h
> +++ b/include/linux/nvme.h
> @@ -245,6 +245,7 @@ enum {
> NVME_CTRL_ONCS_WRITE_ZEROES = 1 << 3,
> NVME_CTRL_VWC_PRESENT = 1 << 0,
> NVME_CTRL_OACS_SEC_SUPP = 1 << 0,
> + NVME_CTRL_OACS_DBBUF_SUPP = 1 << 7,
> };
>
> struct nvme_lbaf {
> @@ -603,6 +604,7 @@ enum nvme_admin_opcode {
> nvme_admin_download_fw = 0x11,
> nvme_admin_ns_attach = 0x15,
> nvme_admin_keep_alive = 0x18,
> + nvme_admin_dbbuf = 0x7C,
> nvme_admin_format_nvm = 0x80,
> nvme_admin_security_send = 0x81,
> nvme_admin_security_recv = 0x82,
> @@ -874,6 +876,16 @@ struct nvmf_property_get_command {
> __u8 resv4[16];
> };
>
> +struct nvme_dbbuf {
> + __u8 opcode;
> + __u8 flags;
> + __u16 command_id;
> + __u32 rsvd1[5];
> + __le64 prp1;
> + __le64 prp2;
> + __u32 rsvd12[6];
> +};
> +
> struct nvme_command {
> union {
> struct nvme_common_command common;
> @@ -893,6 +905,7 @@ struct nvme_command {
> struct nvmf_connect_command connect;
> struct nvmf_property_set_command prop_set;
> struct nvmf_property_get_command prop_get;
> + struct nvme_dbbuf dbbuf;
> };
> };
>
>
+ Add missing maintainers from scripts/get_maintainer.pl in the email thread
Hi,
I would like to know if it would be possible to get this patch for
kernel 4.12.
Should I send a pull request? Or do you usually get the patch from the
email thread?
Thanks,
Helen
^ permalink raw reply [flat|nested] 49+ messages in thread
* [PATCH v8] nvme: improve performance for virtual NVMe devices
@ 2017-04-14 18:10 ` Helen Koike
0 siblings, 0 replies; 49+ messages in thread
From: Helen Koike @ 2017-04-14 18:10 UTC (permalink / raw)
On 2017-04-10 12:51 PM, Helen Koike wrote:
> From: Helen Koike <helen.koike at collabora.co.uk>
>
> This change provides a mechanism to reduce the number of MMIO doorbell
> writes for the NVMe driver. When running in a virtualized environment
> like QEMU, the cost of an MMIO is quite hefy here. The main idea for
> the patch is provide the device two memory location locations:
> 1) to store the doorbell values so they can be lookup without the doorbell
> MMIO write
> 2) to store an event index.
> I believe the doorbell value is obvious, the event index not so much.
> Similar to the virtio specification, the virtual device can tell the
> driver (guest OS) not to write MMIO unless you are writing past this
> value.
>
> FYI: doorbell values are written by the nvme driver (guest OS) and the
> event index is written by the virtual device (host OS).
>
> The patch implements a new admin command that will communicate where
> these two memory locations reside. If the command fails, the nvme
> driver will work as before without any optimizations.
>
> Contributions:
> Eric Northup <digitaleric at google.com>
> Frank Swiderski <fes at google.com>
> Ted Tso <tytso at mit.edu>
> Keith Busch <keith.busch at intel.com>
>
> Just to give an idea on the performance boost with the vendor
> extension: Running fio [1], a stock NVMe driver I get about 200K read
> IOPs with my vendor patch I get about 1000K read IOPs. This was
> running with a null device i.e. the backing device simply returned
> success on every read IO request.
>
> [1] Running on a 4 core machine:
> fio --time_based --name=benchmark --runtime=30
> --filename=/dev/nvme0n1 --nrfiles=1 --ioengine=libaio --iodepth=32
> --direct=1 --invalidate=1 --verify=0 --verify_fatal=0 --numjobs=4
> --rw=randread --blocksize=4k --randrepeat=false
>
> Signed-off-by: Rob Nelson <rlnelson at google.com>
> [mlin: port for upstream]
> Signed-off-by: Ming Lin <mlin at kernel.org>
> [koike: updated for upstream]
> Signed-off-by: Helen Koike <helen.koike at collabora.co.uk>
> Reviewed-by: Christoph Hellwig <hch at lst.de>
>
> ---
>
> This patch is based on git://git.infradead.org/nvme.git master
> Tested through Google Cloud Engine
> TPAR is ratified by the NVME working group
>
> changes since v7:
> - remove nvme_write_doorbell(), add
> nvme_dbbuf_update_and_check_event() instead that doesn't write
> to the doorbell, only returns true if db needs to be updated
> - add reviewed-by tag from Christoph
>
> changes since v6:
> - remove nvme_dbbuf_init() from nvme_alloc_queue() as it is
> already called in nvme_init_queue()
> - free dbbuf_dbs only in nvme_pci_free_ctrl(), change
> nvme_dbbuf_dma_alloc() to do nothing the dbbuf_dbs is already
> allocated
>
> changes since v5:
> - remove anonymous dbbuf struct in struct nvme_dev and struct nvme_queue
> - use inline functions for sq_idx and cq_idx instead of macros
> - add a warning when nvme_dbbuf_set fails
> - remove comment "Borrowed from vring_need_event"
> - change formatting of the parameters in nvme_write_doorbell
> - don't fail nvme_reset_work if nvme_dbbuf_dma_alloc fails
> - remove nvme_write_doorbell_{sq,cq} wrappers
> - in nvme_write_doorbell(), call writel last
>
> Changes since v4
> - Remove CONFIG_NVME_DBBUF
> - Remove dbbuf.{c,h}, move the code to pci.c
> - Coding style changes
> - Functions and vaiables renamed
>
> Changes since v3
> - Rename to dbbuf (be closer to the latest TPAR)
> - Modify the opcodes according to the latest TPAR
> - Check if the device support this feature through the OACS bit
> - little cleanups
>
> nvme_dbbuf_update_and_check_event
> ---
> drivers/nvme/host/pci.c | 143 +++++++++++++++++++++++++++++++++++++++++++++++-
> include/linux/nvme.h | 13 +++++
> 2 files changed, 154 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/nvme/host/pci.c b/drivers/nvme/host/pci.c
> index 5b7bd9c..0e5f2a1 100644
> --- a/drivers/nvme/host/pci.c
> +++ b/drivers/nvme/host/pci.c
> @@ -103,8 +103,22 @@ struct nvme_dev {
> u32 cmbloc;
> struct nvme_ctrl ctrl;
> struct completion ioq_wait;
> + u32 *dbbuf_dbs;
> + dma_addr_t dbbuf_dbs_dma_addr;
> + u32 *dbbuf_eis;
> + dma_addr_t dbbuf_eis_dma_addr;
> };
>
> +static inline unsigned int sq_idx(unsigned int qid, u32 stride)
> +{
> + return qid * 2 * stride;
> +}
> +
> +static inline unsigned int cq_idx(unsigned int qid, u32 stride)
> +{
> + return (qid * 2 + 1) * stride;
> +}
> +
> static inline struct nvme_dev *to_nvme_dev(struct nvme_ctrl *ctrl)
> {
> return container_of(ctrl, struct nvme_dev, ctrl);
> @@ -133,6 +147,10 @@ struct nvme_queue {
> u16 qid;
> u8 cq_phase;
> u8 cqe_seen;
> + u32 *dbbuf_sq_db;
> + u32 *dbbuf_cq_db;
> + u32 *dbbuf_sq_ei;
> + u32 *dbbuf_cq_ei;
> };
>
> /*
> @@ -174,6 +192,112 @@ static inline void _nvme_check_size(void)
> BUILD_BUG_ON(sizeof(struct nvme_id_ns) != 4096);
> BUILD_BUG_ON(sizeof(struct nvme_lba_range_type) != 64);
> BUILD_BUG_ON(sizeof(struct nvme_smart_log) != 512);
> + BUILD_BUG_ON(sizeof(struct nvme_dbbuf) != 64);
> +}
> +
> +static inline unsigned int nvme_dbbuf_size(u32 stride)
> +{
> + return ((num_possible_cpus() + 1) * 8 * stride);
> +}
> +
> +static int nvme_dbbuf_dma_alloc(struct nvme_dev *dev)
> +{
> + unsigned int mem_size = nvme_dbbuf_size(dev->db_stride);
> +
> + if (dev->dbbuf_dbs)
> + return 0;
> +
> + dev->dbbuf_dbs = dma_alloc_coherent(dev->dev, mem_size,
> + &dev->dbbuf_dbs_dma_addr,
> + GFP_KERNEL);
> + if (!dev->dbbuf_dbs)
> + return -ENOMEM;
> + dev->dbbuf_eis = dma_alloc_coherent(dev->dev, mem_size,
> + &dev->dbbuf_eis_dma_addr,
> + GFP_KERNEL);
> + if (!dev->dbbuf_eis) {
> + dma_free_coherent(dev->dev, mem_size,
> + dev->dbbuf_dbs, dev->dbbuf_dbs_dma_addr);
> + dev->dbbuf_dbs = NULL;
> + return -ENOMEM;
> + }
> +
> + return 0;
> +}
> +
> +static void nvme_dbbuf_dma_free(struct nvme_dev *dev)
> +{
> + unsigned int mem_size = nvme_dbbuf_size(dev->db_stride);
> +
> + if (dev->dbbuf_dbs) {
> + dma_free_coherent(dev->dev, mem_size,
> + dev->dbbuf_dbs, dev->dbbuf_dbs_dma_addr);
> + dev->dbbuf_dbs = NULL;
> + }
> + if (dev->dbbuf_eis) {
> + dma_free_coherent(dev->dev, mem_size,
> + dev->dbbuf_eis, dev->dbbuf_eis_dma_addr);
> + dev->dbbuf_eis = NULL;
> + }
> +}
> +
> +static void nvme_dbbuf_init(struct nvme_dev *dev,
> + struct nvme_queue *nvmeq, int qid)
> +{
> + if (!dev->dbbuf_dbs || !qid)
> + return;
> +
> + nvmeq->dbbuf_sq_db = &dev->dbbuf_dbs[sq_idx(qid, dev->db_stride)];
> + nvmeq->dbbuf_cq_db = &dev->dbbuf_dbs[cq_idx(qid, dev->db_stride)];
> + nvmeq->dbbuf_sq_ei = &dev->dbbuf_eis[sq_idx(qid, dev->db_stride)];
> + nvmeq->dbbuf_cq_ei = &dev->dbbuf_eis[cq_idx(qid, dev->db_stride)];
> +}
> +
> +static void nvme_dbbuf_set(struct nvme_dev *dev)
> +{
> + struct nvme_command c;
> +
> + if (!dev->dbbuf_dbs)
> + return;
> +
> + memset(&c, 0, sizeof(c));
> + c.dbbuf.opcode = nvme_admin_dbbuf;
> + c.dbbuf.prp1 = cpu_to_le64(dev->dbbuf_dbs_dma_addr);
> + c.dbbuf.prp2 = cpu_to_le64(dev->dbbuf_eis_dma_addr);
> +
> + if (nvme_submit_sync_cmd(dev->ctrl.admin_q, &c, NULL, 0)) {
> + dev_warn(dev->dev, "unable to set dbbuf\n");
> + /* Free memory and continue on */
> + nvme_dbbuf_dma_free(dev);
> + }
> +}
> +
> +static inline int nvme_dbbuf_need_event(u16 event_idx, u16 new_idx, u16 old)
> +{
> + return (u16)(new_idx - event_idx - 1) < (u16)(new_idx - old);
> +}
> +
> +/* Update dbbuf and return true if an MMIO is required */
> +static bool nvme_dbbuf_update_and_check_event(u16 value, u32 *dbbuf_db,
> + volatile u32 *dbbuf_ei)
> +{
> + if (dbbuf_db) {
> + u16 old_value;
> +
> + /*
> + * Ensure that the queue is written before updating
> + * the doorbell in memory
> + */
> + wmb();
> +
> + old_value = *dbbuf_db;
> + *dbbuf_db = value;
> +
> + if (!nvme_dbbuf_need_event(*dbbuf_ei, value, old_value))
> + return false;
> + }
> +
> + return true;
> }
>
> /*
> @@ -300,7 +424,9 @@ static void __nvme_submit_cmd(struct nvme_queue *nvmeq,
>
> if (++tail == nvmeq->q_depth)
> tail = 0;
> - writel(tail, nvmeq->q_db);
> + if (nvme_dbbuf_update_and_check_event(tail, nvmeq->dbbuf_sq_db,
> + nvmeq->dbbuf_sq_ei))
> + writel(tail, nvmeq->q_db);
> nvmeq->sq_tail = tail;
> }
>
> @@ -716,7 +842,9 @@ static void __nvme_process_cq(struct nvme_queue *nvmeq, unsigned int *tag)
> return;
>
> if (likely(nvmeq->cq_vector >= 0))
> - writel(head, nvmeq->q_db + nvmeq->dev->db_stride);
> + if (nvme_dbbuf_update_and_check_event(head, nvmeq->dbbuf_cq_db,
> + nvmeq->dbbuf_cq_ei))
> + writel(head, nvmeq->q_db + nvmeq->dev->db_stride);
> nvmeq->cq_head = head;
> nvmeq->cq_phase = phase;
>
> @@ -1099,6 +1227,7 @@ static void nvme_init_queue(struct nvme_queue *nvmeq, u16 qid)
> nvmeq->cq_phase = 1;
> nvmeq->q_db = &dev->dbs[qid * 2 * dev->db_stride];
> memset((void *)nvmeq->cqes, 0, CQ_SIZE(nvmeq->q_depth));
> + nvme_dbbuf_init(dev, nvmeq, qid);
> dev->online_queues++;
> spin_unlock_irq(&nvmeq->q_lock);
> }
> @@ -1568,6 +1697,8 @@ static int nvme_dev_add(struct nvme_dev *dev)
> if (blk_mq_alloc_tag_set(&dev->tagset))
> return 0;
> dev->ctrl.tagset = &dev->tagset;
> +
> + nvme_dbbuf_set(dev);
> } else {
> blk_mq_update_nr_hw_queues(&dev->tagset, dev->online_queues - 1);
>
> @@ -1733,6 +1864,7 @@ static void nvme_pci_free_ctrl(struct nvme_ctrl *ctrl)
> {
> struct nvme_dev *dev = to_nvme_dev(ctrl);
>
> + nvme_dbbuf_dma_free(dev);
> put_device(dev->dev);
> if (dev->tagset.tags)
> blk_mq_free_tag_set(&dev->tagset);
> @@ -1800,6 +1932,13 @@ static void nvme_reset_work(struct work_struct *work)
> dev->ctrl.opal_dev = NULL;
> }
>
> + if (dev->ctrl.oacs & NVME_CTRL_OACS_DBBUF_SUPP) {
> + result = nvme_dbbuf_dma_alloc(dev);
> + if (result)
> + dev_warn(dev->dev,
> + "unable to allocate dma for dbbuf\n");
> + }
> +
> result = nvme_setup_io_queues(dev);
> if (result)
> goto out;
> diff --git a/include/linux/nvme.h b/include/linux/nvme.h
> index c43d435..43a6289 100644
> --- a/include/linux/nvme.h
> +++ b/include/linux/nvme.h
> @@ -245,6 +245,7 @@ enum {
> NVME_CTRL_ONCS_WRITE_ZEROES = 1 << 3,
> NVME_CTRL_VWC_PRESENT = 1 << 0,
> NVME_CTRL_OACS_SEC_SUPP = 1 << 0,
> + NVME_CTRL_OACS_DBBUF_SUPP = 1 << 7,
> };
>
> struct nvme_lbaf {
> @@ -603,6 +604,7 @@ enum nvme_admin_opcode {
> nvme_admin_download_fw = 0x11,
> nvme_admin_ns_attach = 0x15,
> nvme_admin_keep_alive = 0x18,
> + nvme_admin_dbbuf = 0x7C,
> nvme_admin_format_nvm = 0x80,
> nvme_admin_security_send = 0x81,
> nvme_admin_security_recv = 0x82,
> @@ -874,6 +876,16 @@ struct nvmf_property_get_command {
> __u8 resv4[16];
> };
>
> +struct nvme_dbbuf {
> + __u8 opcode;
> + __u8 flags;
> + __u16 command_id;
> + __u32 rsvd1[5];
> + __le64 prp1;
> + __le64 prp2;
> + __u32 rsvd12[6];
> +};
> +
> struct nvme_command {
> union {
> struct nvme_common_command common;
> @@ -893,6 +905,7 @@ struct nvme_command {
> struct nvmf_connect_command connect;
> struct nvmf_property_set_command prop_set;
> struct nvmf_property_get_command prop_get;
> + struct nvme_dbbuf dbbuf;
> };
> };
>
>
+ Add missing maintainers from scripts/get_maintainer.pl in the email thread
Hi,
I would like to know if it would be possible to get this patch for
kernel 4.12.
Should I send a pull request? Or do you usually get the patch from the
email thread?
Thanks,
Helen
^ permalink raw reply [flat|nested] 49+ messages in thread
* Re: [PATCH v8] nvme: improve performance for virtual NVMe devices
2017-04-14 18:10 ` Helen Koike
@ 2017-04-17 23:01 ` Keith Busch
-1 siblings, 0 replies; 49+ messages in thread
From: Keith Busch @ 2017-04-17 23:01 UTC (permalink / raw)
To: Helen Koike
Cc: Christoph Hellwig, fes, axboe, rlnelson,
open list : NVM EXPRESS DRIVER, mikew, digitaleric, monish,
james_p_freyensee @ linux . intel . com, tytso, mlin, sagi,
linux-kernel
On Fri, Apr 14, 2017 at 03:10:30PM -0300, Helen Koike wrote:
> + Add missing maintainers from scripts/get_maintainer.pl in the email thread
>
> Hi,
>
> I would like to know if it would be possible to get this patch for kernel
> 4.12.
> Should I send a pull request? Or do you usually get the patch from the email
> thread?
Hi Helen,
This looks good to me. I pulled this into a branch for the next merge
window here:
http://git.infradead.org/nvme.git/shortlog/refs/heads/for-next
There's only one other commit at the moment in addition to yours, so
I'll just need to make sure we've got everything we want for 4.12 before
submitting our next pull request.
^ permalink raw reply [flat|nested] 49+ messages in thread
* [PATCH v8] nvme: improve performance for virtual NVMe devices
@ 2017-04-17 23:01 ` Keith Busch
0 siblings, 0 replies; 49+ messages in thread
From: Keith Busch @ 2017-04-17 23:01 UTC (permalink / raw)
On Fri, Apr 14, 2017@03:10:30PM -0300, Helen Koike wrote:
> + Add missing maintainers from scripts/get_maintainer.pl in the email thread
>
> Hi,
>
> I would like to know if it would be possible to get this patch for kernel
> 4.12.
> Should I send a pull request? Or do you usually get the patch from the email
> thread?
Hi Helen,
This looks good to me. I pulled this into a branch for the next merge
window here:
http://git.infradead.org/nvme.git/shortlog/refs/heads/for-next
There's only one other commit at the moment in addition to yours, so
I'll just need to make sure we've got everything we want for 4.12 before
submitting our next pull request.
^ permalink raw reply [flat|nested] 49+ messages in thread
* Re: [PATCH v8] nvme: improve performance for virtual NVMe devices
2017-04-17 23:01 ` Keith Busch
@ 2017-04-17 23:20 ` Helen Koike
-1 siblings, 0 replies; 49+ messages in thread
From: Helen Koike @ 2017-04-17 23:20 UTC (permalink / raw)
To: Keith Busch
Cc: Christoph Hellwig, fes, axboe, rlnelson,
open list : NVM EXPRESS DRIVER, mikew, digitaleric, monish,
james_p_freyensee @ linux . intel . com, tytso, mlin, sagi,
linux-kernel
On 2017-04-17 08:01 PM, Keith Busch wrote:
> On Fri, Apr 14, 2017 at 03:10:30PM -0300, Helen Koike wrote:
>> + Add missing maintainers from scripts/get_maintainer.pl in the email thread
>>
>> Hi,
>>
>> I would like to know if it would be possible to get this patch for kernel
>> 4.12.
>> Should I send a pull request? Or do you usually get the patch from the email
>> thread?
>
> Hi Helen,
>
> This looks good to me. I pulled this into a branch for the next merge
> window here:
Thanks :)
>
> http://git.infradead.org/nvme.git/shortlog/refs/heads/for-next
>
> There's only one other commit at the moment in addition to yours, so
> I'll just need to make sure we've got everything we want for 4.12 before
> submitting our next pull request.
>
^ permalink raw reply [flat|nested] 49+ messages in thread
* [PATCH v8] nvme: improve performance for virtual NVMe devices
@ 2017-04-17 23:20 ` Helen Koike
0 siblings, 0 replies; 49+ messages in thread
From: Helen Koike @ 2017-04-17 23:20 UTC (permalink / raw)
On 2017-04-17 08:01 PM, Keith Busch wrote:
> On Fri, Apr 14, 2017@03:10:30PM -0300, Helen Koike wrote:
>> + Add missing maintainers from scripts/get_maintainer.pl in the email thread
>>
>> Hi,
>>
>> I would like to know if it would be possible to get this patch for kernel
>> 4.12.
>> Should I send a pull request? Or do you usually get the patch from the email
>> thread?
>
> Hi Helen,
>
> This looks good to me. I pulled this into a branch for the next merge
> window here:
Thanks :)
>
> http://git.infradead.org/nvme.git/shortlog/refs/heads/for-next
>
> There's only one other commit at the moment in addition to yours, so
> I'll just need to make sure we've got everything we want for 4.12 before
> submitting our next pull request.
>
^ permalink raw reply [flat|nested] 49+ messages in thread
* Re: [PATCH v8] nvme: improve performance for virtual NVMe devices
2017-04-17 23:01 ` Keith Busch
@ 2017-04-20 10:22 ` Sagi Grimberg
-1 siblings, 0 replies; 49+ messages in thread
From: Sagi Grimberg @ 2017-04-20 10:22 UTC (permalink / raw)
To: Keith Busch, Helen Koike
Cc: Christoph Hellwig, fes, axboe, rlnelson,
open list : NVM EXPRESS DRIVER, mikew, digitaleric, monish,
james_p_freyensee @ linux . intel . com, tytso, mlin,
linux-kernel
>> + Add missing maintainers from scripts/get_maintainer.pl in the email thread
>>
>> Hi,
>>
>> I would like to know if it would be possible to get this patch for kernel
>> 4.12.
>> Should I send a pull request? Or do you usually get the patch from the email
>> thread?
>
> Hi Helen,
>
> This looks good to me. I pulled this into a branch for the next merge
> window here:
>
> http://git.infradead.org/nvme.git/shortlog/refs/heads/for-next
>
> There's only one other commit at the moment in addition to yours, so
> I'll just need to make sure we've got everything we want for 4.12 before
> submitting our next pull request.
I'm fine with this too.
^ permalink raw reply [flat|nested] 49+ messages in thread
* [PATCH v8] nvme: improve performance for virtual NVMe devices
@ 2017-04-20 10:22 ` Sagi Grimberg
0 siblings, 0 replies; 49+ messages in thread
From: Sagi Grimberg @ 2017-04-20 10:22 UTC (permalink / raw)
>> + Add missing maintainers from scripts/get_maintainer.pl in the email thread
>>
>> Hi,
>>
>> I would like to know if it would be possible to get this patch for kernel
>> 4.12.
>> Should I send a pull request? Or do you usually get the patch from the email
>> thread?
>
> Hi Helen,
>
> This looks good to me. I pulled this into a branch for the next merge
> window here:
>
> http://git.infradead.org/nvme.git/shortlog/refs/heads/for-next
>
> There's only one other commit at the moment in addition to yours, so
> I'll just need to make sure we've got everything we want for 4.12 before
> submitting our next pull request.
I'm fine with this too.
^ permalink raw reply [flat|nested] 49+ messages in thread