[PATCH RFC 2/4] nvme-rdma: fix sqsize/hsqsize/hrqsize per spec

[J Freyensee]

On Thu, 2016-08-11 at 17:21 +0200, Christoph Hellwig wrote:
> I think having different notations for hsqsize vs hrqsize is highly
> confusing, and I've asked for a clarification of the hrqsize
> definition
> which is ambious at it's best at the moment.  But independent of
> that a few more comments:
> 
> > 
> > @@ -1907,7 +1920,7 @@ static struct nvme_ctrl
> > *nvme_rdma_create_ctrl(struct device *dev,
> >  	spin_lock_init(&ctrl->lock);
> >  
> >  	ctrl->queue_count = opts->nr_io_queues + 1; /* +1 for
> > admin queue */
> > -	ctrl->ctrl.sqsize = opts->queue_size;
> > +	ctrl->ctrl.sqsize = opts->queue_size-1;
> 
> We should keep our own sqsize in normal notation in core, and just
> convert to strange notations on the wire.  Especially as we use it
> to driver all the resource allocation as Sagi said.
> 
> 
> > 
> > +	if (priv.qid == 0) {
> > +		priv.hsqsize = cpu_to_le16(queue->ctrl-
> > >ctrl.admin_sqsize);
> > +		priv.hrqsize = cpu_to_le16(queue->ctrl-
> > >ctrl.admin_sqsize+1);
> 
> Based on that priv.hsqsize should have a - 1 here  and hrqsize should
> be kept as-is.  Also always use spaces around the operators.

Hmmm, not sure I understand.  In this current patch series
implementation, admin_sqsize will be NVMF_AQ_DEPTH-1 (31), so it
doesn't need a -1.

Then you are saying do NOT add hrqsize?  Thus, hsqsize == hrqsize??

> 
> And while we're at it - the fix to use the separate AQ values should
> go into the first patch.

It will get re-worked anyways based on the interest to not have a
variable at the moment at someone's disposal to create variable-sized
admin queue depths.