[PATCH] nvme-pci: remove volatile cqes

[PATCH] nvme-pci: remove volatile cqes

[Keith Busch]

The completion queue entry is not volatile once the phase is confirmed.
Remove the volaitle keywords and check the phase using the appropriate
READ_ONCE() accessor, allowing the compiler to optimize the remaining
completion path.

Signed-off-by: Keith Busch <kbusch@kernel.org>
---
 drivers/nvme/host/pci.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/drivers/nvme/host/pci.c b/drivers/nvme/host/pci.c
index 8ebcf40f04fe..cf386c84588b 100644
--- a/drivers/nvme/host/pci.c
+++ b/drivers/nvme/host/pci.c
@@ -166,7 +166,7 @@ struct nvme_queue {
 	void *sq_cmds;
 	 /* only used for poll queues: */
 	spinlock_t cq_poll_lock ____cacheline_aligned_in_smp;
-	volatile struct nvme_completion *cqes;
+	struct nvme_completion *cqes;
 	dma_addr_t sq_dma_addr;
 	dma_addr_t cq_dma_addr;
 	u32 __iomem *q_db;
@@ -908,8 +908,9 @@ static void nvme_pci_complete_rq(struct request *req)
 /* We read the CQE phase first to check if the rest of the entry is valid */
 static inline bool nvme_cqe_pending(struct nvme_queue *nvmeq)
 {
-	return (le16_to_cpu(nvmeq->cqes[nvmeq->cq_head].status) & 1) ==
-			nvmeq->cq_phase;
+	return (le16_to_cpu(
+			READ_ONCE(nvmeq->cqes[nvmeq->cq_head].status)) & 1) ==
+				nvmeq->cq_phase;
 }
 
 static inline void nvme_ring_cq_doorbell(struct nvme_queue *nvmeq)
@@ -930,7 +931,7 @@ static inline struct blk_mq_tags *nvme_queue_tagset(struct nvme_queue *nvmeq)
 
 static inline void nvme_handle_cqe(struct nvme_queue *nvmeq, u16 idx)
 {
-	volatile struct nvme_completion *cqe = &nvmeq->cqes[idx];
+	struct nvme_completion *cqe = &nvmeq->cqes[idx];
 	struct request *req;
 
 	if (unlikely(cqe->command_id >= nvmeq->q_depth)) {
-- 
2.24.1


_______________________________________________
linux-nvme mailing list
linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme

Re: [PATCH] nvme-pci: remove volatile cqes

[Christoph Hellwig]

On Mon, Apr 27, 2020 at 12:14:46PM -0700, Keith Busch wrote:
> The completion queue entry is not volatile once the phase is confirmed.
> Remove the volaitle keywords and check the phase using the appropriate
> READ_ONCE() accessor, allowing the compiler to optimize the remaining
> completion path.
> 
> Signed-off-by: Keith Busch <kbusch@kernel.org>
> ---
>  drivers/nvme/host/pci.c | 9 +++++----
>  1 file changed, 5 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/nvme/host/pci.c b/drivers/nvme/host/pci.c
> index 8ebcf40f04fe..cf386c84588b 100644
> --- a/drivers/nvme/host/pci.c
> +++ b/drivers/nvme/host/pci.c
> @@ -166,7 +166,7 @@ struct nvme_queue {
>  	void *sq_cmds;
>  	 /* only used for poll queues: */
>  	spinlock_t cq_poll_lock ____cacheline_aligned_in_smp;
> -	volatile struct nvme_completion *cqes;
> +	struct nvme_completion *cqes;
>  	dma_addr_t sq_dma_addr;
>  	dma_addr_t cq_dma_addr;
>  	u32 __iomem *q_db;
> @@ -908,8 +908,9 @@ static void nvme_pci_complete_rq(struct request *req)
>  /* We read the CQE phase first to check if the rest of the entry is valid */
>  static inline bool nvme_cqe_pending(struct nvme_queue *nvmeq)
>  {
> -	return (le16_to_cpu(nvmeq->cqes[nvmeq->cq_head].status) & 1) ==
> -			nvmeq->cq_phase;
> +	return (le16_to_cpu(
> +			READ_ONCE(nvmeq->cqes[nvmeq->cq_head].status)) & 1) ==
> +				nvmeq->cq_phase;

Despite the sound logic this just visually looks horrible.  How about:

static inline bool nvme_cqe_pending(struct nvme_queue *nvmeq)
{
	struct nvme_completion *hcqe = &nvmeq->cqes[nvmeq->cq_head];

	return (le16_to_cpu(READ_ONCE(hcqe->status)) & 1) == nvmeq->cq_phase;
}

Otherwise looks fine:

Reviewed-by: Christoph Hellwig <hch@lst.de>

_______________________________________________
linux-nvme mailing list
linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme