[PATCH v2] nvmet: protect sqhd update by a lock

[PATCH v2] nvmet: protect sqhd update by a lock

[James Smart]

In testing target io in read write mix, we did indeed get into cases
where sqhd didn't update properly and slowly missed enough updates to
shutdown the queue.

Protect the read/modify/set update of sqhd under a lock for coherency.

Signed-off-by: James Smart <james.smart at broadcom.com>

---
v2: move locks so around update and assignment

 drivers/nvme/target/core.c  | 5 +++++
 drivers/nvme/target/nvmet.h | 1 +
 2 files changed, 6 insertions(+)

diff --git a/drivers/nvme/target/core.c b/drivers/nvme/target/core.c
index cfc35cbb6fe2..28300e9bbef1 100644
--- a/drivers/nvme/target/core.c
+++ b/drivers/nvme/target/core.c
@@ -387,12 +387,16 @@ struct nvmet_ns *nvmet_ns_alloc(struct nvmet_subsys *subsys, u32 nsid)
 
 static void __nvmet_req_complete(struct nvmet_req *req, u16 status)
 {
+	unsigned long flags;
+
 	if (status)
 		nvmet_set_status(req, status);
 
+	spin_lock_irqsave(&req->sq->sqhd_lock, flags);
 	if (req->sq->size)
 		req->sq->sqhd = (req->sq->sqhd + 1) % req->sq->size;
 	req->rsp->sq_head = cpu_to_le16(req->sq->sqhd);
+	spin_unlock_irqrestore(&req->sq->sqhd_lock, flags);
 	req->rsp->sq_id = cpu_to_le16(req->sq->qid);
 	req->rsp->command_id = req->cmd->common.command_id;
 
@@ -425,6 +429,7 @@ void nvmet_sq_setup(struct nvmet_ctrl *ctrl, struct nvmet_sq *sq,
 	sq->size = size;
 
 	ctrl->sqs[qid] = sq;
+	spin_lock_init(&sq->sqhd_lock);
 }
 
 static void nvmet_confirm_sq(struct percpu_ref *ref)
diff --git a/drivers/nvme/target/nvmet.h b/drivers/nvme/target/nvmet.h
index ed38b44a7007..bdb904bdf676 100644
--- a/drivers/nvme/target/nvmet.h
+++ b/drivers/nvme/target/nvmet.h
@@ -77,6 +77,7 @@ struct nvmet_sq {
 	u16			sqhd;
 	struct completion	free_done;
 	struct completion	confirm_done;
+	spinlock_t		sqhd_lock;
 };
 
 /**
-- 
2.13.1

[PATCH v2] nvmet: protect sqhd update by a lock

[Johannes Thumshirn]

Still somewhat awkward to read, maybe we can fold in a comment on why the lock
only touches these two lines and isn't used outside of this function anymore.

Anyways,
Reviewed-by: Johannes Thumshirn <jthumshirn at suse.de>

-- 
Johannes Thumshirn                                          Storage
jthumshirn at suse.de                                +49 911 74053 689
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 N?rnberg
GF: Felix Imend?rffer, Jane Smithard, Graham Norton
HRB 21284 (AG N?rnberg)
Key fingerprint = EC38 9CAB C2C4 F25D 8600 D0D0 0393 969D 2D76 0850

[PATCH v2] nvmet: protect sqhd update by a lock

[Christoph Hellwig]

>  	if (status)
>  		nvmet_set_status(req, status);
>  
> +	spin_lock_irqsave(&req->sq->sqhd_lock, flags);
>  	if (req->sq->size)
>  		req->sq->sqhd = (req->sq->sqhd + 1) % req->sq->size;
>  	req->rsp->sq_head = cpu_to_le16(req->sq->sqhd);
> +	spin_unlock_irqrestore(&req->sq->sqhd_lock, flags);

What performance impact does this have?   I'm really reluctant to put
an irq disabling spinlock into a hot path for a feature that
theoretically is in the spec but ignored by every host.

I'd much rather play games with cmpxchg or similar here.

[PATCH v2] nvmet: protect sqhd update by a lock

[James Smart]

On 10/16/2017 11:43 PM, Christoph Hellwig wrote:
>>   	if (status)
>>   		nvmet_set_status(req, status);
>>   
>> +	spin_lock_irqsave(&req->sq->sqhd_lock, flags);
>>   	if (req->sq->size)
>>   		req->sq->sqhd = (req->sq->sqhd + 1) % req->sq->size;
>>   	req->rsp->sq_head = cpu_to_le16(req->sq->sqhd);
>> +	spin_unlock_irqrestore(&req->sq->sqhd_lock, flags);
> What performance impact does this have?   I'm really reluctant to put
> an irq disabling spinlock into a hot path for a feature that
> theoretically is in the spec but ignored by every host.
>
> I'd much rather play games with cmpxchg or similar here.

It's not ignored by every host. Its ignored by the linux host. There are 
other hosts out there that follow the spec and pay attention to sqhd - 
and the linux target would like to be used with them.

I'm open to other alternatives, but in the past, with pci adapter 
interfaces with similar "ring" index updates, the increment followed by 
modulo has always proven to be an issue for such tricks.

-- james

[PATCH v2] nvmet: protect sqhd update by a lock

[Christoph Hellwig]

On Tue, Oct 17, 2017@07:19:30AM -0700, James Smart wrote:
> On 10/16/2017 11:43 PM, Christoph Hellwig wrote:
> > >   	if (status)
> > >   		nvmet_set_status(req, status);
> > > +	spin_lock_irqsave(&req->sq->sqhd_lock, flags);
> > >   	if (req->sq->size)
> > >   		req->sq->sqhd = (req->sq->sqhd + 1) % req->sq->size;
> > >   	req->rsp->sq_head = cpu_to_le16(req->sq->sqhd);
> > > +	spin_unlock_irqrestore(&req->sq->sqhd_lock, flags);
> > What performance impact does this have?   I'm really reluctant to put
> > an irq disabling spinlock into a hot path for a feature that
> > theoretically is in the spec but ignored by every host.
> > 
> > I'd much rather play games with cmpxchg or similar here.
> 
> It's not ignored by every host. Its ignored by the linux host. There are
> other hosts out there that follow the spec and pay attention to sqhd - and
> the linux target would like to be used with them.
> 
> I'm open to other alternatives, but in the past, with pci adapter interfaces
> with similar "ring" index updates, the increment followed by modulo has
> always proven to be an issue for such tricks.

Something like:

	do {
		old_sqhd = req->sq->sqhd;
		new_sqhd = (old + 1) % req->sq->size;
	} while (cmpxchg(&req->sq->sqhd, old_sqhd, new_sqhd) != old_sqhd);

should do the job.

[PATCH v2] nvmet: protect sqhd update by a lock

[James Smart]

On 10/18/2017 12:23 AM, Christoph Hellwig wrote:
> do {
> 		old_sqhd = req->sq->sqhd;
> 		new_sqhd = (old + 1) % req->sq->size;
> 	} while (cmpxchg(&req->sq->sqhd, old_sqhd, new_sqhd) != old_sqhd);

sweet. does look good.

I'll repost as v3, but with an ammended title

-- james