[RFC PATCH V2 0/2] *** use rdma device capability to limit queue size ***

[RFC PATCH V2 0/2] *** use rdma device capability to limit queue size ***

[Guixin Liu]

Hi guys,

Currently, the queue size if nvme over rdma is limited with a depth
of 128, we can use rdma device capability to limit it.

Changes from v1 to v2:
- use nvmet_ctrl's port to find rdma's nvmet_rdma_queue without change
  get_max_queue_size's param.
- additional clarification that the queue depth will be controlled within
  1024, rather than being very large.

Guixin Liu (2):
  nvmet: rdma: utilize ib_device capability for setting max_queue_size
  nvme: rdma: use ib_device's max_qp_wr to limit sqsize

 drivers/nvme/host/rdma.c   | 14 ++++++++------
 drivers/nvme/target/rdma.c | 17 ++++++++++++++++-
 include/linux/nvme-rdma.h  |  2 +-
 3 files changed, 25 insertions(+), 8 deletions(-)

-- 
1.8.3.1

[RFC PATCH V2 1/2] nvmet: rdma: utilize ib_device capability for setting max_queue_size

[Guixin Liu]

Respond with the smaller value between 1024 and the ib_device's
max_qp_wr as the RDMA max queue size.

Signed-off-by: Guixin Liu <kanie@linux.alibaba.com>
---
 drivers/nvme/target/rdma.c | 17 ++++++++++++++++-
 include/linux/nvme-rdma.h  |  2 ++
 2 files changed, 18 insertions(+), 1 deletion(-)

diff --git a/drivers/nvme/target/rdma.c b/drivers/nvme/target/rdma.c
index 4597bca..d546203 100644
--- a/drivers/nvme/target/rdma.c
+++ b/drivers/nvme/target/rdma.c
@@ -2002,7 +2002,22 @@ static u8 nvmet_rdma_get_mdts(const struct nvmet_ctrl *ctrl)
 
 static u16 nvmet_rdma_get_max_queue_size(const struct nvmet_ctrl *ctrl)
 {
-	return NVME_RDMA_MAX_QUEUE_SIZE;
+	struct nvmet_rdma_queue *queue;
+	struct nvmet_port *nport = ctrl->port;
+	int max_qp_wr = 0;
+
+	/* Here nvmet_ctrl's port is already set. */
+	mutex_lock(&nvmet_rdma_queue_mutex);
+	list_for_each_entry(queue, &nvmet_rdma_queue_list, queue_list) {
+		if (queue->port == nport) {
+			max_qp_wr = queue->dev->device->attrs.max_qp_wr;
+			break;
+		}
+	}
+	mutex_unlock(&nvmet_rdma_queue_mutex);
+
+	return (u16)min_t(int, NVMET_QUEUE_SIZE,
+			  max_qp_wr / (NVME_RDMA_SEND_WR_FACTOR + 1));
 }
 
 static const struct nvmet_fabrics_ops nvmet_rdma_ops = {
diff --git a/include/linux/nvme-rdma.h b/include/linux/nvme-rdma.h
index 4dd7e6f..c19858b 100644
--- a/include/linux/nvme-rdma.h
+++ b/include/linux/nvme-rdma.h
@@ -8,6 +8,8 @@
 
 #define NVME_RDMA_MAX_QUEUE_SIZE	128
 
+#define NVME_RDMA_SEND_WR_FACTOR 3  /* MR, SEND, INV */
+
 enum nvme_rdma_cm_fmt {
 	NVME_RDMA_CM_FMT_1_0 = 0x0,
 };
-- 
1.8.3.1

[RFC PATCH V2 2/2] nvme: rdma: use ib_device's max_qp_wr to limit sqsize

[Guixin Liu]

Currently, the host is limited to creating queues with a depth of
128. To enable larger queue sizes, constrain the sqsize based on
the ib_device's max_qp_wr capability.
In addition, the queue size is restricted between 16 and 1024 in
nvmf_parse_options(), so the final value of queue depth will not
biger than 1024.

And also remove unused NVME_RDMA_MAX_QUEUE_SIZE macro.

Signed-off-by: Guixin Liu <kanie@linux.alibaba.com>
---
 drivers/nvme/host/rdma.c  | 14 ++++++++------
 include/linux/nvme-rdma.h |  2 --
 2 files changed, 8 insertions(+), 8 deletions(-)

diff --git a/drivers/nvme/host/rdma.c b/drivers/nvme/host/rdma.c
index 81e2621..982f3e4 100644
--- a/drivers/nvme/host/rdma.c
+++ b/drivers/nvme/host/rdma.c
@@ -489,8 +489,7 @@ static int nvme_rdma_create_cq(struct ib_device *ibdev,
 static int nvme_rdma_create_queue_ib(struct nvme_rdma_queue *queue)
 {
 	struct ib_device *ibdev;
-	const int send_wr_factor = 3;			/* MR, SEND, INV */
-	const int cq_factor = send_wr_factor + 1;	/* + RECV */
+	const int cq_factor = NVME_RDMA_SEND_WR_FACTOR + 1;	/* + RECV */
 	int ret, pages_per_mr;
 
 	queue->device = nvme_rdma_find_get_device(queue->cm_id);
@@ -508,7 +507,7 @@ static int nvme_rdma_create_queue_ib(struct nvme_rdma_queue *queue)
 	if (ret)
 		goto out_put_dev;
 
-	ret = nvme_rdma_create_qp(queue, send_wr_factor);
+	ret = nvme_rdma_create_qp(queue, NVME_RDMA_SEND_WR_FACTOR);
 	if (ret)
 		goto out_destroy_ib_cq;
 
@@ -1006,6 +1005,7 @@ static int nvme_rdma_setup_ctrl(struct nvme_rdma_ctrl *ctrl, bool new)
 {
 	int ret;
 	bool changed;
+	int ib_max_qsize;
 
 	ret = nvme_rdma_configure_admin_queue(ctrl, new);
 	if (ret)
@@ -1030,11 +1030,13 @@ static int nvme_rdma_setup_ctrl(struct nvme_rdma_ctrl *ctrl, bool new)
 			ctrl->ctrl.opts->queue_size, ctrl->ctrl.sqsize + 1);
 	}
 
-	if (ctrl->ctrl.sqsize + 1 > NVME_RDMA_MAX_QUEUE_SIZE) {
+	ib_max_qsize = ctrl->device->dev->attrs.max_qp_wr /
+			(NVME_RDMA_SEND_WR_FACTOR + 1);
+	if (ctrl->ctrl.sqsize + 1 > ib_max_qsize) {
 		dev_warn(ctrl->ctrl.device,
 			"ctrl sqsize %u > max queue size %u, clamping down\n",
-			ctrl->ctrl.sqsize + 1, NVME_RDMA_MAX_QUEUE_SIZE);
-		ctrl->ctrl.sqsize = NVME_RDMA_MAX_QUEUE_SIZE - 1;
+			ctrl->ctrl.sqsize + 1, ib_max_qsize);
+		ctrl->ctrl.sqsize = ib_max_qsize - 1;
 	}
 
 	if (ctrl->ctrl.sqsize + 1 > ctrl->ctrl.maxcmd) {
diff --git a/include/linux/nvme-rdma.h b/include/linux/nvme-rdma.h
index c19858b..67ee770 100644
--- a/include/linux/nvme-rdma.h
+++ b/include/linux/nvme-rdma.h
@@ -6,8 +6,6 @@
 #ifndef _LINUX_NVME_RDMA_H
 #define _LINUX_NVME_RDMA_H
 
-#define NVME_RDMA_MAX_QUEUE_SIZE	128
-
 #define NVME_RDMA_SEND_WR_FACTOR 3  /* MR, SEND, INV */
 
 enum nvme_rdma_cm_fmt {
-- 
1.8.3.1

Re: [RFC PATCH V2 2/2] nvme: rdma: use ib_device's max_qp_wr to limit sqsize

[Sagi Grimberg]

On 12/19/23 09:32, Guixin Liu wrote:
> Currently, the host is limited to creating queues with a depth of
> 128. To enable larger queue sizes, constrain the sqsize based on
> the ib_device's max_qp_wr capability.
> In addition, the queue size is restricted between 16 and 1024 in
> nvmf_parse_options(), so the final value of queue depth will not
> biger than 1024.
> 
> And also remove unused NVME_RDMA_MAX_QUEUE_SIZE macro.
> 
> Signed-off-by: Guixin Liu <kanie@linux.alibaba.com>
> ---
>   drivers/nvme/host/rdma.c  | 14 ++++++++------
>   include/linux/nvme-rdma.h |  2 --
>   2 files changed, 8 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/nvme/host/rdma.c b/drivers/nvme/host/rdma.c
> index 81e2621..982f3e4 100644
> --- a/drivers/nvme/host/rdma.c
> +++ b/drivers/nvme/host/rdma.c
> @@ -489,8 +489,7 @@ static int nvme_rdma_create_cq(struct ib_device *ibdev,
>   static int nvme_rdma_create_queue_ib(struct nvme_rdma_queue *queue)
>   {
>   	struct ib_device *ibdev;
> -	const int send_wr_factor = 3;			/* MR, SEND, INV */
> -	const int cq_factor = send_wr_factor + 1;	/* + RECV */
> +	const int cq_factor = NVME_RDMA_SEND_WR_FACTOR + 1;	/* + RECV */
>   	int ret, pages_per_mr;
>   
>   	queue->device = nvme_rdma_find_get_device(queue->cm_id);
> @@ -508,7 +507,7 @@ static int nvme_rdma_create_queue_ib(struct nvme_rdma_queue *queue)
>   	if (ret)
>   		goto out_put_dev;
>   
> -	ret = nvme_rdma_create_qp(queue, send_wr_factor);
> +	ret = nvme_rdma_create_qp(queue, NVME_RDMA_SEND_WR_FACTOR);
>   	if (ret)
>   		goto out_destroy_ib_cq;
>   
> @@ -1006,6 +1005,7 @@ static int nvme_rdma_setup_ctrl(struct nvme_rdma_ctrl *ctrl, bool new)
>   {
>   	int ret;
>   	bool changed;
> +	int ib_max_qsize;
>   
>   	ret = nvme_rdma_configure_admin_queue(ctrl, new);
>   	if (ret)
> @@ -1030,11 +1030,13 @@ static int nvme_rdma_setup_ctrl(struct nvme_rdma_ctrl *ctrl, bool new)
>   			ctrl->ctrl.opts->queue_size, ctrl->ctrl.sqsize + 1);
>   	}
>   
> -	if (ctrl->ctrl.sqsize + 1 > NVME_RDMA_MAX_QUEUE_SIZE) {
> +	ib_max_qsize = ctrl->device->dev->attrs.max_qp_wr /
> +			(NVME_RDMA_SEND_WR_FACTOR + 1);

rdma_dev_max_qsize is a better name.

Also, you can drop the RFC for the next submission.

Re: [RFC PATCH V2 2/2] nvme: rdma: use ib_device's max_qp_wr to limit sqsize

[Max Gurtovoy]

Hi Guixin Liu,

On 20/12/2023 11:17, Sagi Grimberg wrote:
> 
> 
> On 12/19/23 09:32, Guixin Liu wrote:
>> Currently, the host is limited to creating queues with a depth of
>> 128. To enable larger queue sizes, constrain the sqsize based on
>> the ib_device's max_qp_wr capability.
>> In addition, the queue size is restricted between 16 and 1024 in
>> nvmf_parse_options(), so the final value of queue depth will not
>> biger than 1024.

can you please explain why this patch series is needed ?
what is the added value you see with it ?

>>
>> And also remove unused NVME_RDMA_MAX_QUEUE_SIZE macro.
>>
>> Signed-off-by: Guixin Liu <kanie@linux.alibaba.com>
>> ---
>>   drivers/nvme/host/rdma.c  | 14 ++++++++------
>>   include/linux/nvme-rdma.h |  2 --
>>   2 files changed, 8 insertions(+), 8 deletions(-)
>>
>> diff --git a/drivers/nvme/host/rdma.c b/drivers/nvme/host/rdma.c
>> index 81e2621..982f3e4 100644
>> --- a/drivers/nvme/host/rdma.c
>> +++ b/drivers/nvme/host/rdma.c
>> @@ -489,8 +489,7 @@ static int nvme_rdma_create_cq(struct ib_device 
>> *ibdev,
>>   static int nvme_rdma_create_queue_ib(struct nvme_rdma_queue *queue)
>>   {
>>       struct ib_device *ibdev;
>> -    const int send_wr_factor = 3;            /* MR, SEND, INV */
>> -    const int cq_factor = send_wr_factor + 1;    /* + RECV */
>> +    const int cq_factor = NVME_RDMA_SEND_WR_FACTOR + 1;    /* + RECV */
>>       int ret, pages_per_mr;
>>       queue->device = nvme_rdma_find_get_device(queue->cm_id);
>> @@ -508,7 +507,7 @@ static int nvme_rdma_create_queue_ib(struct 
>> nvme_rdma_queue *queue)
>>       if (ret)
>>           goto out_put_dev;
>> -    ret = nvme_rdma_create_qp(queue, send_wr_factor);
>> +    ret = nvme_rdma_create_qp(queue, NVME_RDMA_SEND_WR_FACTOR);
>>       if (ret)
>>           goto out_destroy_ib_cq;
>> @@ -1006,6 +1005,7 @@ static int nvme_rdma_setup_ctrl(struct 
>> nvme_rdma_ctrl *ctrl, bool new)
>>   {
>>       int ret;
>>       bool changed;
>> +    int ib_max_qsize;
>>       ret = nvme_rdma_configure_admin_queue(ctrl, new);
>>       if (ret)
>> @@ -1030,11 +1030,13 @@ static int nvme_rdma_setup_ctrl(struct 
>> nvme_rdma_ctrl *ctrl, bool new)
>>               ctrl->ctrl.opts->queue_size, ctrl->ctrl.sqsize + 1);
>>       }
>> -    if (ctrl->ctrl.sqsize + 1 > NVME_RDMA_MAX_QUEUE_SIZE) {
>> +    ib_max_qsize = ctrl->device->dev->attrs.max_qp_wr /
>> +            (NVME_RDMA_SEND_WR_FACTOR + 1);
> 
> rdma_dev_max_qsize is a better name.
> 
> Also, you can drop the RFC for the next submission.
> 

Sagi,
I don't feel comfortable with these patches.
First I would like to understand the need for it.
Second, the QP WR can be constructed from one or more WQEs and the WQEs 
can be constructed from one or more WQEBBs. The max_qp_wr doesn't take 
it into account.

Re: [RFC PATCH V2 2/2] nvme: rdma: use ib_device's max_qp_wr to limit sqsize

[Sagi Grimberg]

>>> @@ -1030,11 +1030,13 @@ static int nvme_rdma_setup_ctrl(struct 
>>> nvme_rdma_ctrl *ctrl, bool new)
>>>               ctrl->ctrl.opts->queue_size, ctrl->ctrl.sqsize + 1);
>>>       }
>>> -    if (ctrl->ctrl.sqsize + 1 > NVME_RDMA_MAX_QUEUE_SIZE) {
>>> +    ib_max_qsize = ctrl->device->dev->attrs.max_qp_wr /
>>> +            (NVME_RDMA_SEND_WR_FACTOR + 1);
>>
>> rdma_dev_max_qsize is a better name.
>>
>> Also, you can drop the RFC for the next submission.
>>
> 
> Sagi,
> I don't feel comfortable with these patches.

Well, good that you're speaking up then ;)

> First I would like to understand the need for it.

I assumed that he stumbled on a device that did not support the
existing max of 128 nvme commands (which is 384 rdma wrs for the qp).

> Second, the QP WR can be constructed from one or more WQEs and the WQEs 
> can be constructed from one or more WQEBBs. The max_qp_wr doesn't take 
> it into account.

Well, it is not taken into account now either with the existing magic
limit in nvmet. The rdma limits reporting mechanism was and still is
unusable.

I would expect a device that has different size for different work
items to report max_qp_wr accounting for the largest work element that
the device supports, so it is universally correct.

The fact that max_qp_wr means the maximum number of slots is a qp and
at the same time different work requests can arbitrarily use any number
of slots without anyone ever knowing, makes it pretty much impossible to
use reliably.

Maybe rdma device attributes need a new attribute called
universal_max_qp_wr that is going to actually be reliable and not
guess-work?

Re: [RFC PATCH V2 2/2] nvme: rdma: use ib_device's max_qp_wr to limit sqsize

[Guixin Liu]

在 2023/12/21 03:27, Sagi Grimberg 写道:
>
>>>> @@ -1030,11 +1030,13 @@ static int nvme_rdma_setup_ctrl(struct 
>>>> nvme_rdma_ctrl *ctrl, bool new)
>>>>               ctrl->ctrl.opts->queue_size, ctrl->ctrl.sqsize + 1);
>>>>       }
>>>> -    if (ctrl->ctrl.sqsize + 1 > NVME_RDMA_MAX_QUEUE_SIZE) {
>>>> +    ib_max_qsize = ctrl->device->dev->attrs.max_qp_wr /
>>>> +            (NVME_RDMA_SEND_WR_FACTOR + 1);
>>>
>>> rdma_dev_max_qsize is a better name.
>>>
>>> Also, you can drop the RFC for the next submission.
>>>
>>
>> Sagi,
>> I don't feel comfortable with these patches.
>
> Well, good that you're speaking up then ;)
>
>> First I would like to understand the need for it.
>
> I assumed that he stumbled on a device that did not support the
> existing max of 128 nvme commands (which is 384 rdma wrs for the qp).
>
The situation is that I need a queue depth greater than 128.
>> Second, the QP WR can be constructed from one or more WQEs and the 
>> WQEs can be constructed from one or more WQEBBs. The max_qp_wr 
>> doesn't take it into account.
>
> Well, it is not taken into account now either with the existing magic
> limit in nvmet. The rdma limits reporting mechanism was and still is
> unusable.
>
> I would expect a device that has different size for different work
> items to report max_qp_wr accounting for the largest work element that
> the device supports, so it is universally correct.
>
> The fact that max_qp_wr means the maximum number of slots is a qp and
> at the same time different work requests can arbitrarily use any number
> of slots without anyone ever knowing, makes it pretty much impossible to
> use reliably.
>
> Maybe rdma device attributes need a new attribute called
> universal_max_qp_wr that is going to actually be reliable and not
> guess-work?

I see, the max_qp_wr is not as reliable as I imagined. Is there any 
another way to get a queue depth grater than 128

instead of changing NVME_RDMA_MAX_QUEUE_SIZE?

Re: [RFC PATCH V2 2/2] nvme: rdma: use ib_device's max_qp_wr to limit sqsize

[Max Gurtovoy]

On 22/12/2023 8:58, Guixin Liu wrote:
> 
> 在 2023/12/21 03:27, Sagi Grimberg 写道:
>>
>>>>> @@ -1030,11 +1030,13 @@ static int nvme_rdma_setup_ctrl(struct 
>>>>> nvme_rdma_ctrl *ctrl, bool new)
>>>>>               ctrl->ctrl.opts->queue_size, ctrl->ctrl.sqsize + 1);
>>>>>       }
>>>>> -    if (ctrl->ctrl.sqsize + 1 > NVME_RDMA_MAX_QUEUE_SIZE) {
>>>>> +    ib_max_qsize = ctrl->device->dev->attrs.max_qp_wr /
>>>>> +            (NVME_RDMA_SEND_WR_FACTOR + 1);
>>>>
>>>> rdma_dev_max_qsize is a better name.
>>>>
>>>> Also, you can drop the RFC for the next submission.
>>>>
>>>
>>> Sagi,
>>> I don't feel comfortable with these patches.
>>
>> Well, good that you're speaking up then ;)
>>
>>> First I would like to understand the need for it.
>>
>> I assumed that he stumbled on a device that did not support the
>> existing max of 128 nvme commands (which is 384 rdma wrs for the qp).
>>
> The situation is that I need a queue depth greater than 128.
>>> Second, the QP WR can be constructed from one or more WQEs and the 
>>> WQEs can be constructed from one or more WQEBBs. The max_qp_wr 
>>> doesn't take it into account.
>>
>> Well, it is not taken into account now either with the existing magic
>> limit in nvmet. The rdma limits reporting mechanism was and still is
>> unusable.
>>
>> I would expect a device that has different size for different work
>> items to report max_qp_wr accounting for the largest work element that
>> the device supports, so it is universally correct.
>>
>> The fact that max_qp_wr means the maximum number of slots is a qp and
>> at the same time different work requests can arbitrarily use any number
>> of slots without anyone ever knowing, makes it pretty much impossible to
>> use reliably.
>>
>> Maybe rdma device attributes need a new attribute called
>> universal_max_qp_wr that is going to actually be reliable and not
>> guess-work?
> 
> I see, the max_qp_wr is not as reliable as I imagined. Is there any 
> another way to get a queue depth grater than 128
> 
> instead of changing NVME_RDMA_MAX_QUEUE_SIZE?
> 

When I added this limit to RDMA transports it was to avoid a situation 
that a QP will fail to be created if one will ask a large queue.

I choose 128 since it was supported for all the RDMA adapters I've 
tested in my lab (mostly Mellanox adapters).
For this queue depth we found that the performance is good enough and it 
will not be improved if we will increase the depth.

Are you saying that you have a device that can provide better 
performance with qdepth > 128 ?
What is the tested qdepth and what are the numbers you see with this 
qdepth ?

Re: [RFC PATCH V2 2/2] nvme: rdma: use ib_device's max_qp_wr to limit sqsize

[Guixin Liu]

在 2023/12/24 09:37, Max Gurtovoy 写道:
>
>
> On 22/12/2023 8:58, Guixin Liu wrote:
>>
>> 在 2023/12/21 03:27, Sagi Grimberg 写道:
>>>
>>>>>> @@ -1030,11 +1030,13 @@ static int nvme_rdma_setup_ctrl(struct 
>>>>>> nvme_rdma_ctrl *ctrl, bool new)
>>>>>>               ctrl->ctrl.opts->queue_size, ctrl->ctrl.sqsize + 1);
>>>>>>       }
>>>>>> -    if (ctrl->ctrl.sqsize + 1 > NVME_RDMA_MAX_QUEUE_SIZE) {
>>>>>> +    ib_max_qsize = ctrl->device->dev->attrs.max_qp_wr /
>>>>>> +            (NVME_RDMA_SEND_WR_FACTOR + 1);
>>>>>
>>>>> rdma_dev_max_qsize is a better name.
>>>>>
>>>>> Also, you can drop the RFC for the next submission.
>>>>>
>>>>
>>>> Sagi,
>>>> I don't feel comfortable with these patches.
>>>
>>> Well, good that you're speaking up then ;)
>>>
>>>> First I would like to understand the need for it.
>>>
>>> I assumed that he stumbled on a device that did not support the
>>> existing max of 128 nvme commands (which is 384 rdma wrs for the qp).
>>>
>> The situation is that I need a queue depth greater than 128.
>>>> Second, the QP WR can be constructed from one or more WQEs and the 
>>>> WQEs can be constructed from one or more WQEBBs. The max_qp_wr 
>>>> doesn't take it into account.
>>>
>>> Well, it is not taken into account now either with the existing magic
>>> limit in nvmet. The rdma limits reporting mechanism was and still is
>>> unusable.
>>>
>>> I would expect a device that has different size for different work
>>> items to report max_qp_wr accounting for the largest work element that
>>> the device supports, so it is universally correct.
>>>
>>> The fact that max_qp_wr means the maximum number of slots is a qp and
>>> at the same time different work requests can arbitrarily use any number
>>> of slots without anyone ever knowing, makes it pretty much 
>>> impossible to
>>> use reliably.
>>>
>>> Maybe rdma device attributes need a new attribute called
>>> universal_max_qp_wr that is going to actually be reliable and not
>>> guess-work?
>>
>> I see, the max_qp_wr is not as reliable as I imagined. Is there any 
>> another way to get a queue depth grater than 128
>>
>> instead of changing NVME_RDMA_MAX_QUEUE_SIZE?
>>
>
> When I added this limit to RDMA transports it was to avoid a situation 
> that a QP will fail to be created if one will ask a large queue.
>
> I choose 128 since it was supported for all the RDMA adapters I've 
> tested in my lab (mostly Mellanox adapters).
> For this queue depth we found that the performance is good enough and 
> it will not be improved if we will increase the depth.
>
> Are you saying that you have a device that can provide better 
> performance with qdepth > 128 ?
> What is the tested qdepth and what are the numbers you see with this 
> qdepth ?

Yeah, you are right, the improvement is small(about %1~2%), I do this 
only for better benchmark,

I still consist that using the capabilities of RDMA device to determine 
the size of queue is a better choice, but now I change the

NVME_RDMA_MAX_QUEUE_SIZE to 256 for bidding.

Best regards,

Guixin Liu

Re: [RFC PATCH V2 2/2] nvme: rdma: use ib_device's max_qp_wr to limit sqsize

[Sagi Grimberg]

>>>>>>> @@ -1030,11 +1030,13 @@ static int nvme_rdma_setup_ctrl(struct 
>>>>>>> nvme_rdma_ctrl *ctrl, bool new)
>>>>>>>               ctrl->ctrl.opts->queue_size, ctrl->ctrl.sqsize + 1);
>>>>>>>       }
>>>>>>> -    if (ctrl->ctrl.sqsize + 1 > NVME_RDMA_MAX_QUEUE_SIZE) {
>>>>>>> +    ib_max_qsize = ctrl->device->dev->attrs.max_qp_wr /
>>>>>>> +            (NVME_RDMA_SEND_WR_FACTOR + 1);
>>>>>>
>>>>>> rdma_dev_max_qsize is a better name.
>>>>>>
>>>>>> Also, you can drop the RFC for the next submission.
>>>>>>
>>>>>
>>>>> Sagi,
>>>>> I don't feel comfortable with these patches.
>>>>
>>>> Well, good that you're speaking up then ;)
>>>>
>>>>> First I would like to understand the need for it.
>>>>
>>>> I assumed that he stumbled on a device that did not support the
>>>> existing max of 128 nvme commands (which is 384 rdma wrs for the qp).
>>>>
>>> The situation is that I need a queue depth greater than 128.
>>>>> Second, the QP WR can be constructed from one or more WQEs and the 
>>>>> WQEs can be constructed from one or more WQEBBs. The max_qp_wr 
>>>>> doesn't take it into account.
>>>>
>>>> Well, it is not taken into account now either with the existing magic
>>>> limit in nvmet. The rdma limits reporting mechanism was and still is
>>>> unusable.
>>>>
>>>> I would expect a device that has different size for different work
>>>> items to report max_qp_wr accounting for the largest work element that
>>>> the device supports, so it is universally correct.
>>>>
>>>> The fact that max_qp_wr means the maximum number of slots is a qp and
>>>> at the same time different work requests can arbitrarily use any number
>>>> of slots without anyone ever knowing, makes it pretty much 
>>>> impossible to
>>>> use reliably.
>>>>
>>>> Maybe rdma device attributes need a new attribute called
>>>> universal_max_qp_wr that is going to actually be reliable and not
>>>> guess-work?
>>>
>>> I see, the max_qp_wr is not as reliable as I imagined. Is there any 
>>> another way to get a queue depth grater than 128
>>>
>>> instead of changing NVME_RDMA_MAX_QUEUE_SIZE?
>>>
>>
>> When I added this limit to RDMA transports it was to avoid a situation 
>> that a QP will fail to be created if one will ask a large queue.
>>
>> I choose 128 since it was supported for all the RDMA adapters I've 
>> tested in my lab (mostly Mellanox adapters).
>> For this queue depth we found that the performance is good enough and 
>> it will not be improved if we will increase the depth.
>>
>> Are you saying that you have a device that can provide better 
>> performance with qdepth > 128 ?
>> What is the tested qdepth and what are the numbers you see with this 
>> qdepth ?
> 
> Yeah, you are right, the improvement is small(about %1~2%), I do this 
> only for better benchmark,

Well, it doesn't come for free, you are essentially doubling the queue
depth. I'm also assuming that you tested a single initiator and a
single queue?

> I still consist that using the capabilities of RDMA device to determine 
> the size of queue is a better choice, but now I change the
> 
> NVME_RDMA_MAX_QUEUE_SIZE to 256 for bidding.

Still doesn't change the fact that its a pure guess-work if it is
supported by the device or not.

Are you even able to create that queue depth with DIF workloads?

Max, what is the maximum effective depth with DIF enabled?

Re: [RFC PATCH V2 2/2] nvme: rdma: use ib_device's max_qp_wr to limit sqsize

[Max Gurtovoy]

On 25/12/2023 10:59, Sagi Grimberg wrote:
> 
>>>>>>>> @@ -1030,11 +1030,13 @@ static int nvme_rdma_setup_ctrl(struct 
>>>>>>>> nvme_rdma_ctrl *ctrl, bool new)
>>>>>>>>               ctrl->ctrl.opts->queue_size, ctrl->ctrl.sqsize + 1);
>>>>>>>>       }
>>>>>>>> -    if (ctrl->ctrl.sqsize + 1 > NVME_RDMA_MAX_QUEUE_SIZE) {
>>>>>>>> +    ib_max_qsize = ctrl->device->dev->attrs.max_qp_wr /
>>>>>>>> +            (NVME_RDMA_SEND_WR_FACTOR + 1);
>>>>>>>
>>>>>>> rdma_dev_max_qsize is a better name.
>>>>>>>
>>>>>>> Also, you can drop the RFC for the next submission.
>>>>>>>
>>>>>>
>>>>>> Sagi,
>>>>>> I don't feel comfortable with these patches.
>>>>>
>>>>> Well, good that you're speaking up then ;)
>>>>>
>>>>>> First I would like to understand the need for it.
>>>>>
>>>>> I assumed that he stumbled on a device that did not support the
>>>>> existing max of 128 nvme commands (which is 384 rdma wrs for the qp).
>>>>>
>>>> The situation is that I need a queue depth greater than 128.
>>>>>> Second, the QP WR can be constructed from one or more WQEs and the 
>>>>>> WQEs can be constructed from one or more WQEBBs. The max_qp_wr 
>>>>>> doesn't take it into account.
>>>>>
>>>>> Well, it is not taken into account now either with the existing magic
>>>>> limit in nvmet. The rdma limits reporting mechanism was and still is
>>>>> unusable.
>>>>>
>>>>> I would expect a device that has different size for different work
>>>>> items to report max_qp_wr accounting for the largest work element that
>>>>> the device supports, so it is universally correct.
>>>>>
>>>>> The fact that max_qp_wr means the maximum number of slots is a qp and
>>>>> at the same time different work requests can arbitrarily use any 
>>>>> number
>>>>> of slots without anyone ever knowing, makes it pretty much 
>>>>> impossible to
>>>>> use reliably.
>>>>>
>>>>> Maybe rdma device attributes need a new attribute called
>>>>> universal_max_qp_wr that is going to actually be reliable and not
>>>>> guess-work?
>>>>
>>>> I see, the max_qp_wr is not as reliable as I imagined. Is there any 
>>>> another way to get a queue depth grater than 128
>>>>
>>>> instead of changing NVME_RDMA_MAX_QUEUE_SIZE?
>>>>
>>>
>>> When I added this limit to RDMA transports it was to avoid a 
>>> situation that a QP will fail to be created if one will ask a large 
>>> queue.
>>>
>>> I choose 128 since it was supported for all the RDMA adapters I've 
>>> tested in my lab (mostly Mellanox adapters).
>>> For this queue depth we found that the performance is good enough and 
>>> it will not be improved if we will increase the depth.
>>>
>>> Are you saying that you have a device that can provide better 
>>> performance with qdepth > 128 ?
>>> What is the tested qdepth and what are the numbers you see with this 
>>> qdepth ?
>>
>> Yeah, you are right, the improvement is small(about %1~2%), I do this 
>> only for better benchmark,
> 
> Well, it doesn't come for free, you are essentially doubling the queue
> depth. I'm also assuming that you tested a single initiator and a
> single queue?
> 
>> I still consist that using the capabilities of RDMA device to 
>> determine the size of queue is a better choice, but now I change the
>>
>> NVME_RDMA_MAX_QUEUE_SIZE to 256 for bidding.
> 
> Still doesn't change the fact that its a pure guess-work if it is
> supported by the device or not.
> 
> Are you even able to create that queue depth with DIF workloads?
> 
> Max, what is the maximum effective depth with DIF enabled?

I'll need to check it.

I'll prepare some patches to allow RDMA queue_size to be 256 for non-pi 
controllers anyway.
I also would like to add another configfs entry to determine the max 
queue size of a target port.
hope to merge it for upcoming merge window.