[PATCH 2/2] nvme-rdma: sqsize/hsqsize/hrqsize is 0-based val

[J Freyensee]

On Mon, 2016-08-08 at 18:14 +0000, Minturn, Dave B wrote:
> > 
> > > 
> > > -----Original Message-----
> > > From: Linux-nvme [mailto:linux-nvme-bounces at lists.infradead.org]
> > > On
> > > Behalf Of Verkamp, Daniel
> > > Sent: Monday, August 08, 2016 9:30 AM
> > > To: hch at lst.de; sagi at grimberg.me
> > > Cc: james_p_freyensee at linux.intel.com; linux-nvme at lists.infradead
> > > .org
> > > Subject: Re: [PATCH 2/2] nvme-rdma: sqsize/hsqsize/hrqsize is 0-
> > > based val
> > > 
> > > On Mon, 2016-08-08 at 09:06 +0200, Christoph Hellwig wrote:
> > > > 
> > > > On Sun, Aug 07, 2016 at 10:29:08AM +0300, Sagi Grimberg wrote:
> > > > > 
> > > > > 
> > > > > Did we really decide that? The spec actually explicitly says
> > > > > that a 0 sqsize will be failed:
> > > > > 
> > > > > "If the size is 0h or larger than the controller supports,
> > > > > then a
> > > > > status value of Connect Invalid Parameters shall be
> > > > > returned."
> > > > > 
> > > > > And then it says: "This is a 0’s based value"
> > > > > 
> > > > > Confusing....
> > > > 
> > > > Yeah, I think we'll need an errate in the TWG.  And I think the
> > > > current Linux behavior is the sensible one.  Jay, do you want
> > > > to drive this in the WG or I should I bring it up?
> > > 
> > > I would be happy if all of the 0's based values in the spec were
> > > removed, but I think it's a little too late for that...
> > > 
> > > In this particular case, I'm pretty sure this is not a mistake;
> > > the
> > > relevant text is copied directly from the NVMe base spec Create
> > > I/O
> > > Submission Queue command's QSIZE parameter.
> > > 
> 
> Daniel is correct, this is not a mistake in the specification.
> 
> > 
> > > 
> > > What does need to be clarified is the RDMA transport definition.
> > >  The
> > > Connect private data contains two queue size-related fields:
> > > 
> > > - HSQSIZE, which is supposed to be the RDMA QP send depth, but
> > > also says
> > > "shall be set to the Submission Queue Size (SQSIZE)", while not
> > > saying
> > > whether it is 0's based.
> 
> The spec. states that SQSIZE is 0-based and given HSQSIZE must be
> equal to SQSIZE, the value is going to be 0's based.
> We can suggest adding some additional clarification the spec. such as
> a reminder that SQSIZE is 0-based.   
> 
> 
> > 
> > > 
> > > - HRQSIZE, which is the QP receive depth, which has no equivalent
> > > like
> > > SQSIZE in the Connect command, and also does not mention being
> > > 0's
> > > based.

(replying back to the list)

So with the comment Dave made, this code in the rdma.c host:


-               priv.hrqsize = cpu_to_le16(queue->queue_size);
-               priv.hsqsize = cpu_to_le16(queue->queue_size);
+               priv.hrqsize = cpu_to_le16(queue->ctrl->ctrl.sqsize);

priv.hrqsize is wrong according to the NVMe fabrics spec.  At minimum
it should be implemented as:

priv.hrqsize = cpu_to_le16(queue->ctrl->ctrl.sqsize + 1);


+               priv.hsqsize = cpu_to_le16(queue->ctrl->ctrl.sqsize);


and this line remains the same because sqsize is 0 based and the spec
clearly says hsqsize shall be set to sqsize.


I see the confusion for hrqsize and I think we should try and get it
cleared up in errata, but I think sqsize and hsqsize the way it's
stated in the spec is pretty clear.