diff mbox series

[V2,18/20] RDMA/siw: Only check attrs->cap.max_send_wr in siw_create_qp

Message ID 20231013020053.2120-19-guoqing.jiang@linux.dev (mailing list archive)
State Superseded
Headers show
Series Cleanup for siw | expand

Commit Message

Guoqing Jiang Oct. 13, 2023, 2 a.m. UTC 
We can just check max_send_wr here given both max_send_wr and
max_recv_wr are defined as u32 type, and we also need to ensure
num_sqe (derived from max_send_wr) shouldn't be zero.

Signed-off-by: Guoqing Jiang <guoqing.jiang@linux.dev>
---
 drivers/infiniband/sw/siw/siw_verbs.c | 18 +++++-------------
 1 file changed, 5 insertions(+), 13 deletions(-)

Comments

Bernard Metzler Oct. 25, 2023, 1:27 p.m. UTC | #1 
> -----Original Message-----
> From: Guoqing Jiang <guoqing.jiang@linux.dev>
> Sent: Friday, October 13, 2023 4:01 AM
> To: Bernard Metzler <BMT@zurich.ibm.com>; jgg@ziepe.ca; leon@kernel.org
> Cc: linux-rdma@vger.kernel.org
> Subject: [EXTERNAL] [PATCH V2 18/20] RDMA/siw: Only check attrs-
> >cap.max_send_wr in siw_create_qp
> 
> We can just check max_send_wr here given both max_send_wr and
> max_recv_wr are defined as u32 type, and we also need to ensure
> num_sqe (derived from max_send_wr) shouldn't be zero.
> 
> Signed-off-by: Guoqing Jiang <guoqing.jiang@linux.dev>
> ---
>  drivers/infiniband/sw/siw/siw_verbs.c | 18 +++++-------------
>  1 file changed, 5 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/infiniband/sw/siw/siw_verbs.c
> b/drivers/infiniband/sw/siw/siw_verbs.c
> index dcd69fc01176..ef149ed98946 100644
> --- a/drivers/infiniband/sw/siw/siw_verbs.c
> +++ b/drivers/infiniband/sw/siw/siw_verbs.c
> @@ -333,11 +333,10 @@ int siw_create_qp(struct ib_qp *ibqp, struct
> ib_qp_init_attr *attrs,
>  		goto err_atomic;
>  	}
>  	/*
> -	 * NOTE: we allow for zero element SQ and RQ WQE's SGL's
> -	 * but not for a QP unable to hold any WQE (SQ + RQ)
> +	 * NOTE: we don't allow for a QP unable to hold any SQ WQE
>  	 */
> -	if (attrs->cap.max_send_wr + attrs->cap.max_recv_wr == 0) {
> -		siw_dbg(base_dev, "QP must have send or receive queue\n");
> +	if (attrs->cap.max_send_wr == 0) {
> +		siw_dbg(base_dev, "QP must have send queue\n");
>  		rv = -EINVAL;
>  		goto err_atomic;
>  	}
> @@ -357,21 +356,14 @@ int siw_create_qp(struct ib_qp *ibqp, struct
> ib_qp_init_attr *attrs,
>  	if (rv)
>  		goto err_atomic;
> 
> -	num_sqe = attrs->cap.max_send_wr;
> -	num_rqe = attrs->cap.max_recv_wr;
> 
>  	/* All queue indices are derived from modulo operations
>  	 * on a free running 'get' (consumer) and 'put' (producer)
>  	 * unsigned counter. Having queue sizes at power of two
>  	 * avoids handling counter wrap around.
>  	 */
> -	if (num_sqe)
> -		num_sqe = roundup_pow_of_two(num_sqe);
> -	else {
> -		/* Zero sized SQ is not supported */
> -		rv = -EINVAL;
> -		goto err_out_xa;
> -	}
> +	num_sqe = roundup_pow_of_two(attrs->cap.max_send_wr);
> +	num_rqe = attrs->cap.max_recv_wr;
>  	if (num_rqe)
>  		num_rqe = roundup_pow_of_two(num_rqe);
> 
> --
> 2.35.3

No the original code allows for a QP which cannot send but
just receive or vice versa. A QP which cannot send should be allowed.
Only a QP which cannot do anything should be refused to be created.
Keep it as is.
Guoqing Jiang Oct. 26, 2023, 6:55 a.m. UTC | #2 
On 10/25/23 21:27, Bernard Metzler wrote:
>> -----Original Message-----
>> From: Guoqing Jiang<guoqing.jiang@linux.dev>
>> Sent: Friday, October 13, 2023 4:01 AM
>> To: Bernard Metzler<BMT@zurich.ibm.com>;jgg@ziepe.ca;leon@kernel.org
>> Cc:linux-rdma@vger.kernel.org
>> Subject: [EXTERNAL] [PATCH V2 18/20] RDMA/siw: Only check attrs-
>>> cap.max_send_wr in siw_create_qp
>> We can just check max_send_wr here given both max_send_wr and
>> max_recv_wr are defined as u32 type, and we also need to ensure
>> num_sqe (derived from max_send_wr) shouldn't be zero.
>>
>> Signed-off-by: Guoqing Jiang<guoqing.jiang@linux.dev>
>> ---
>>   drivers/infiniband/sw/siw/siw_verbs.c | 18 +++++-------------
>>   1 file changed, 5 insertions(+), 13 deletions(-)
>>
>> diff --git a/drivers/infiniband/sw/siw/siw_verbs.c
>> b/drivers/infiniband/sw/siw/siw_verbs.c
>> index dcd69fc01176..ef149ed98946 100644
>> --- a/drivers/infiniband/sw/siw/siw_verbs.c
>> +++ b/drivers/infiniband/sw/siw/siw_verbs.c
>> @@ -333,11 +333,10 @@ int siw_create_qp(struct ib_qp *ibqp, struct
>> ib_qp_init_attr *attrs,
>>   		goto err_atomic;
>>   	}
>>   	/*
>> -	 * NOTE: we allow for zero element SQ and RQ WQE's SGL's
>> -	 * but not for a QP unable to hold any WQE (SQ + RQ)
>> +	 * NOTE: we don't allow for a QP unable to hold any SQ WQE
>>   	 */
>> -	if (attrs->cap.max_send_wr + attrs->cap.max_recv_wr == 0) {
>> -		siw_dbg(base_dev, "QP must have send or receive queue\n");
>> +	if (attrs->cap.max_send_wr == 0) {
>> +		siw_dbg(base_dev, "QP must have send queue\n");
>>   		rv = -EINVAL;
>>   		goto err_atomic;
>>   	}
>> @@ -357,21 +356,14 @@ int siw_create_qp(struct ib_qp *ibqp, struct
>> ib_qp_init_attr *attrs,
>>   	if (rv)
>>   		goto err_atomic;
>>
>> -	num_sqe = attrs->cap.max_send_wr;
>> -	num_rqe = attrs->cap.max_recv_wr;
>>
>>   	/* All queue indices are derived from modulo operations
>>   	 * on a free running 'get' (consumer) and 'put' (producer)
>>   	 * unsigned counter. Having queue sizes at power of two
>>   	 * avoids handling counter wrap around.
>>   	 */
>> -	if (num_sqe)
>> -		num_sqe = roundup_pow_of_two(num_sqe);
>> -	else {
>> -		/* Zero sized SQ is not supported */
>> -		rv = -EINVAL;
>> -		goto err_out_xa;
>> -	}
>> +	num_sqe = roundup_pow_of_two(attrs->cap.max_send_wr);
>> +	num_rqe = attrs->cap.max_recv_wr;
>>   	if (num_rqe)
>>   		num_rqe = roundup_pow_of_two(num_rqe);
>>
>> --
>> 2.35.3
> No the original code allows for a QP which cannot send but
> just receive or vice versa. A QP which cannot send should be allowed.
> Only a QP which cannot do anything should be refused to be created.
> Keep it as is.

Seems it is not consistent with the original code, because Zero sized SQ
(num_seq = 0) is not supported, also num_seq is got from max_send_wr,
which means a QP without sq is not permitted here.

But I probably misunderstood something.

Thanks,
Guoqing
Bernard Metzler Oct. 26, 2023, 2 p.m. UTC | #3 
> -----Original Message-----
> From: Guoqing Jiang <guoqing.jiang@linux.dev>
> Sent: Thursday, October 26, 2023 8:55 AM
> To: Bernard Metzler <BMT@zurich.ibm.com>; jgg@ziepe.ca; leon@kernel.org
> Cc: linux-rdma@vger.kernel.org
> Subject: [EXTERNAL] Re: [PATCH V2 18/20] RDMA/siw: Only check attrs-
> >cap.max_send_wr in siw_create_qp
> 
> 
> 
> On 10/25/23 21:27, Bernard Metzler wrote:
> >> -----Original Message-----
> >> From: Guoqing Jiang<guoqing.jiang@linux.dev>
> >> Sent: Friday, October 13, 2023 4:01 AM
> >> To: Bernard Metzler<BMT@zurich.ibm.com>;jgg@ziepe.ca;leon@kernel.org
> >> Cc:linux-rdma@vger.kernel.org
> >> Subject: [EXTERNAL] [PATCH V2 18/20] RDMA/siw: Only check attrs-
> >>> cap.max_send_wr in siw_create_qp
> >> We can just check max_send_wr here given both max_send_wr and
> >> max_recv_wr are defined as u32 type, and we also need to ensure
> >> num_sqe (derived from max_send_wr) shouldn't be zero.
> >>
> >> Signed-off-by: Guoqing Jiang<guoqing.jiang@linux.dev>
> >> ---
> >>   drivers/infiniband/sw/siw/siw_verbs.c | 18 +++++-------------
> >>   1 file changed, 5 insertions(+), 13 deletions(-)
> >>
> >> diff --git a/drivers/infiniband/sw/siw/siw_verbs.c
> >> b/drivers/infiniband/sw/siw/siw_verbs.c
> >> index dcd69fc01176..ef149ed98946 100644
> >> --- a/drivers/infiniband/sw/siw/siw_verbs.c
> >> +++ b/drivers/infiniband/sw/siw/siw_verbs.c
> >> @@ -333,11 +333,10 @@ int siw_create_qp(struct ib_qp *ibqp, struct
> >> ib_qp_init_attr *attrs,
> >>   		goto err_atomic;
> >>   	}
> >>   	/*
> >> -	 * NOTE: we allow for zero element SQ and RQ WQE's SGL's
> >> -	 * but not for a QP unable to hold any WQE (SQ + RQ)
> >> +	 * NOTE: we don't allow for a QP unable to hold any SQ WQE
> >>   	 */
> >> -	if (attrs->cap.max_send_wr + attrs->cap.max_recv_wr == 0) {
> >> -		siw_dbg(base_dev, "QP must have send or receive queue\n");
> >> +	if (attrs->cap.max_send_wr == 0) {
> >> +		siw_dbg(base_dev, "QP must have send queue\n");
> >>   		rv = -EINVAL;
> >>   		goto err_atomic;
> >>   	}
> >> @@ -357,21 +356,14 @@ int siw_create_qp(struct ib_qp *ibqp, struct
> >> ib_qp_init_attr *attrs,
> >>   	if (rv)
> >>   		goto err_atomic;
> >>
> >> -	num_sqe = attrs->cap.max_send_wr;
> >> -	num_rqe = attrs->cap.max_recv_wr;
> >>
> >>   	/* All queue indices are derived from modulo operations
> >>   	 * on a free running 'get' (consumer) and 'put' (producer)
> >>   	 * unsigned counter. Having queue sizes at power of two
> >>   	 * avoids handling counter wrap around.
> >>   	 */
> >> -	if (num_sqe)
> >> -		num_sqe = roundup_pow_of_two(num_sqe);
> >> -	else {
> >> -		/* Zero sized SQ is not supported */
> >> -		rv = -EINVAL;
> >> -		goto err_out_xa;
> >> -	}
> >> +	num_sqe = roundup_pow_of_two(attrs->cap.max_send_wr);
> >> +	num_rqe = attrs->cap.max_recv_wr;
> >>   	if (num_rqe)
> >>   		num_rqe = roundup_pow_of_two(num_rqe);
> >>
> >> --
> >> 2.35.3
> > No the original code allows for a QP which cannot send but
> > just receive or vice versa. A QP which cannot send should be allowed.
> > Only a QP which cannot do anything should be refused to be created.
> > Keep it as is.
> 
> Seems it is not consistent with the original code, because Zero sized SQ
> (num_seq = 0) is not supported, also num_seq is got from max_send_wr,
> which means a QP without sq is not permitted here.
> 

You are right. The RQ can be zero especially if a shared receive queue
is provided. So your patch looks good.


> But I probably misunderstood something.
> 
> Thanks,
> Guoqing
diff mbox series

Patch

diff --git a/drivers/infiniband/sw/siw/siw_verbs.c b/drivers/infiniband/sw/siw/siw_verbs.c
index dcd69fc01176..ef149ed98946 100644
--- a/drivers/infiniband/sw/siw/siw_verbs.c
+++ b/drivers/infiniband/sw/siw/siw_verbs.c
@@ -333,11 +333,10 @@  int siw_create_qp(struct ib_qp *ibqp, struct ib_qp_init_attr *attrs,
 		goto err_atomic;
 	}
 	/*
-	 * NOTE: we allow for zero element SQ and RQ WQE's SGL's
-	 * but not for a QP unable to hold any WQE (SQ + RQ)
+	 * NOTE: we don't allow for a QP unable to hold any SQ WQE
 	 */
-	if (attrs->cap.max_send_wr + attrs->cap.max_recv_wr == 0) {
-		siw_dbg(base_dev, "QP must have send or receive queue\n");
+	if (attrs->cap.max_send_wr == 0) {
+		siw_dbg(base_dev, "QP must have send queue\n");
 		rv = -EINVAL;
 		goto err_atomic;
 	}
@@ -357,21 +356,14 @@  int siw_create_qp(struct ib_qp *ibqp, struct ib_qp_init_attr *attrs,
 	if (rv)
 		goto err_atomic;
 
-	num_sqe = attrs->cap.max_send_wr;
-	num_rqe = attrs->cap.max_recv_wr;
 
 	/* All queue indices are derived from modulo operations
 	 * on a free running 'get' (consumer) and 'put' (producer)
 	 * unsigned counter. Having queue sizes at power of two
 	 * avoids handling counter wrap around.
 	 */
-	if (num_sqe)
-		num_sqe = roundup_pow_of_two(num_sqe);
-	else {
-		/* Zero sized SQ is not supported */
-		rv = -EINVAL;
-		goto err_out_xa;
-	}
+	num_sqe = roundup_pow_of_two(attrs->cap.max_send_wr);
+	num_rqe = attrs->cap.max_recv_wr;
 	if (num_rqe)
 		num_rqe = roundup_pow_of_two(num_rqe);