Re: [PATCH v8] nvme: improve performance for virtual NVMe devices

* Re: [PATCH v8] nvme: improve performance for virtual NVMe devices
       [not found] ` <1491839467-11008-1-git-send-email-helen.koike@collabora.com>
@ 2017-04-14 18:10   ` Helen Koike
  2017-04-17 23:01     ` Keith Busch
  0 siblings, 1 reply; 4+ 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] 4+ messages in thread