From mboxrd@z Thu Jan  1 00:00:00 1970
From: hch@infradead.org (Christoph Hellwig)
Date: Wed, 14 Nov 2018 06:19:58 -0800
Subject: [PATCH v2 1/4] nvmet: support fabrics sq flow control
In-Reply-To: <20181003081323.7504-2-sagi@grimberg.me>
References: <20181003081323.7504-1-sagi@grimberg.me>
 <20181003081323.7504-2-sagi@grimberg.me>
Message-ID: <20181114141958.GA27204@infradead.org>

On Wed, Oct 03, 2018@01:13:19AM -0700, Sagi Grimberg wrote:
> Technical proposal 8005 "fabrics SQ flow control" introduces a mode
> where a host and controller agree to omit sq_head pointer updates
> when sending nvme completions.
> 
> In case the host indicated desire to operate in this mode (connect attribute)
> the controller will return back a connect completion with sq_head value
> of 0xffff as indication that it will omit sq_head pointer updates.
> 
> This mode saves us an atomic update in the I/O path.
> 
> Reviewed-by: Hannes Reinecke <hare at suse.com>
> Signed-off-by: Sagi Grimberg <sagi at grimberg.me>
> ---
>  drivers/nvme/target/core.c        | 34 ++++++++++++++++++++-----------
>  drivers/nvme/target/fabrics-cmd.c |  8 +++++++-
>  drivers/nvme/target/nvmet.h       |  3 ++-
>  include/linux/nvme.h              |  4 ++++
>  4 files changed, 35 insertions(+), 14 deletions(-)
> 
> diff --git a/drivers/nvme/target/core.c b/drivers/nvme/target/core.c
> index 7084704b468d..df2af5e3e4d6 100644
> --- a/drivers/nvme/target/core.c
> +++ b/drivers/nvme/target/core.c
> @@ -503,26 +503,35 @@ struct nvmet_ns *nvmet_ns_alloc(struct nvmet_subsys *subsys, u32 nsid)
>  	return ns;
>  }
>  
> -static void __nvmet_req_complete(struct nvmet_req *req, u16 status)
> +static void nvmet_update_sq_head(struct nvmet_req *req)
>  {
>  	u32 old_sqhd, new_sqhd;
>  	u16 sqhd;
>  
> +	if (!req->sq->sqhd_disabled) {
> +		if (req->sq->size) {

This should be (!req->sq->sqhd_disabled && req->sq->size))

> +			do {
> +				old_sqhd = req->sq->sqhd;
> +				new_sqhd = (old_sqhd + 1) % req->sq->size;
> +			} while (cmpxchg(&req->sq->sqhd, old_sqhd, new_sqhd) !=
> +						old_sqhd);
> +		}
>  	}
> +
>  	sqhd = req->sq->sqhd & 0x0000FFFF;
>  	req->rsp->sq_head = cpu_to_le16(sqhd);

sq_head is a const 0xffff if flow control is disabled isn't it?

Based on that I'd just move the sqhd_disabled check in the
caller and assign the constant value there.

> @@ -173,6 +175,8 @@ static void nvmet_execute_admin_connect(struct nvmet_req *req)
>  	kfree(d);
>  complete:
>  	nvmet_req_complete(req, status);
> +	if (req->sq->sqhd_disabled)
> +		req->sq->sqhd = 0;

Huh, why?