[PATCH] vmw_pvrdma: Avoid rounding up of sge count to power of 2

[PATCH] vmw_pvrdma: Avoid rounding up of sge count to power of 2

[Adit Ranadive]

Creation of resources can fail if the rounding up exceeds
provider supported values.

Fixes: 4c8ed14eb6b7 ("vmw_pvrdma: Add SRQ support")
Reviewed-by: Bryan Tan <bryantan-pghWNbHTmq7QT0dZR+AlfA@public.gmane.org>
Reviewed-by: Nitish Bhat <bnitish-pghWNbHTmq7QT0dZR+AlfA@public.gmane.org>
Signed-off-by: Adit Ranadive <aditr-pghWNbHTmq7QT0dZR+AlfA@public.gmane.org>
Cc: stable-Xl5UnYtxxKxKUA01WzcqbQ@public.gmane.org
---
 providers/vmw_pvrdma/qp.c | 8 +++-----
 1 file changed, 3 insertions(+), 5 deletions(-)

diff --git a/providers/vmw_pvrdma/qp.c b/providers/vmw_pvrdma/qp.c
index efcc99b..4b9f897 100644
--- a/providers/vmw_pvrdma/qp.c
+++ b/providers/vmw_pvrdma/qp.c
@@ -113,7 +113,7 @@ struct ibv_srq *pvrdma_create_srq(struct ibv_pd *pd,
 	int ret;
 
 	attr->attr.max_wr = align_next_power2(max_t(uint32_t, 1U, attr->attr.max_wr));
-	attr->attr.max_sge = align_next_power2(max_t(uint32_t, 1U, attr->attr.max_sge));
+	attr->attr.max_sge = max_t(uint32_t, 1U, attr->attr.max_sge);
 
 	srq = malloc(sizeof(*srq));
 	if (!srq)
@@ -216,14 +216,12 @@ struct ibv_qp *pvrdma_create_qp(struct ibv_pd *pd,
 	int ret;
 	int is_srq = !!(attr->srq);
 
-	attr->cap.max_send_sge =
-		align_next_power2(max_t(uint32_t, 1U, attr->cap.max_send_sge));
+	attr->cap.max_send_sge = max_t(uint32_t, 1U, attr->cap.max_send_sge);
 	attr->cap.max_send_wr =
 		align_next_power2(max_t(uint32_t, 1U, attr->cap.max_send_wr));
 
 	if (!is_srq) {
-		attr->cap.max_recv_sge =
-			align_next_power2(max_t(uint32_t, 1U, attr->cap.max_recv_sge));
+		attr->cap.max_recv_sge = max_t(uint32_t, 1U, attr->cap.max_recv_sge);
 		attr->cap.max_recv_wr =
 			align_next_power2(max_t(uint32_t, 1U, attr->cap.max_recv_wr));
 	} else {
-- 
2.7.4

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] vmw_pvrdma: Avoid rounding up of sge count to power of 2

[Leon Romanovsky]

[-- Attachment #1: Type: text/plain, Size: 2292 bytes --]

On Thu, Feb 15, 2018 at 04:39:52PM -0800, Adit Ranadive wrote:
> Creation of resources can fail if the rounding up exceeds
> provider supported values.
>
> Fixes: 4c8ed14eb6b7 ("vmw_pvrdma: Add SRQ support")
> Reviewed-by: Bryan Tan <bryantan-pghWNbHTmq7QT0dZR+AlfA@public.gmane.org>
> Reviewed-by: Nitish Bhat <bnitish-pghWNbHTmq7QT0dZR+AlfA@public.gmane.org>
> Signed-off-by: Adit Ranadive <aditr-pghWNbHTmq7QT0dZR+AlfA@public.gmane.org>
> Cc: stable-Xl5UnYtxxKxKUA01WzcqbQ@public.gmane.org
> ---
>  providers/vmw_pvrdma/qp.c | 8 +++-----
>  1 file changed, 3 insertions(+), 5 deletions(-)
>
> diff --git a/providers/vmw_pvrdma/qp.c b/providers/vmw_pvrdma/qp.c
> index efcc99b..4b9f897 100644
> --- a/providers/vmw_pvrdma/qp.c
> +++ b/providers/vmw_pvrdma/qp.c
> @@ -113,7 +113,7 @@ struct ibv_srq *pvrdma_create_srq(struct ibv_pd *pd,
>  	int ret;
>
>  	attr->attr.max_wr = align_next_power2(max_t(uint32_t, 1U, attr->attr.max_wr));
> -	attr->attr.max_sge = align_next_power2(max_t(uint32_t, 1U, attr->attr.max_sge));
> +	attr->attr.max_sge = max_t(uint32_t, 1U, attr->attr.max_sge);

One of two: or your commit message is not correct and you should mention
that align_next_power2() is not needed, or your code is incorrect,
because user can provide large enough number and this call will fail
anyway.

>
>  	srq = malloc(sizeof(*srq));
>  	if (!srq)
> @@ -216,14 +216,12 @@ struct ibv_qp *pvrdma_create_qp(struct ibv_pd *pd,
>  	int ret;
>  	int is_srq = !!(attr->srq);
>
> -	attr->cap.max_send_sge =
> -		align_next_power2(max_t(uint32_t, 1U, attr->cap.max_send_sge));
> +	attr->cap.max_send_sge = max_t(uint32_t, 1U, attr->cap.max_send_sge);
>  	attr->cap.max_send_wr =
>  		align_next_power2(max_t(uint32_t, 1U, attr->cap.max_send_wr));
>
>  	if (!is_srq) {
> -		attr->cap.max_recv_sge =
> -			align_next_power2(max_t(uint32_t, 1U, attr->cap.max_recv_sge));
> +		attr->cap.max_recv_sge = max_t(uint32_t, 1U, attr->cap.max_recv_sge);
>  		attr->cap.max_recv_wr =
>  			align_next_power2(max_t(uint32_t, 1U, attr->cap.max_recv_wr));
>  	} else {
> --
> 2.7.4
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
> the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH] vmw_pvrdma: Avoid rounding up of sge count to power of 2

[Jason Gunthorpe]

On Thu, Feb 15, 2018 at 04:39:52PM -0800, Adit Ranadive wrote:
> Creation of resources can fail if the rounding up exceeds
> provider supported values.
> 
> Fixes: 4c8ed14eb6b7 ("vmw_pvrdma: Add SRQ support")
> Reviewed-by: Bryan Tan <bryantan-pghWNbHTmq7QT0dZR+AlfA@public.gmane.org>
> Reviewed-by: Nitish Bhat <bnitish-pghWNbHTmq7QT0dZR+AlfA@public.gmane.org>
> Signed-off-by: Adit Ranadive <aditr-pghWNbHTmq7QT0dZR+AlfA@public.gmane.org>
> Cc: stable-Xl5UnYtxxKxKUA01WzcqbQ@public.gmane.org
>  providers/vmw_pvrdma/qp.c | 8 +++-----
>  1 file changed, 3 insertions(+), 5 deletions(-)
> 
> diff --git a/providers/vmw_pvrdma/qp.c b/providers/vmw_pvrdma/qp.c
> index efcc99b..4b9f897 100644
> +++ b/providers/vmw_pvrdma/qp.c
> @@ -113,7 +113,7 @@ struct ibv_srq *pvrdma_create_srq(struct ibv_pd *pd,
>  	int ret;
>  
>  	attr->attr.max_wr = align_next_power2(max_t(uint32_t, 1U, attr->attr.max_wr));
> -	attr->attr.max_sge = align_next_power2(max_t(uint32_t, 1U, attr->attr.max_sge));
> +	attr->attr.max_sge = max_t(uint32_t, 1U, attr->attr.max_sge);

What is with all these max's? If the user provides <= 0 for these
values it should be EINVAL, not round up.

Jason
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] vmw_pvrdma: Avoid rounding up of sge count to power of 2

[Leon Romanovsky]

[-- Attachment #1: Type: text/plain, Size: 1754 bytes --]

On Fri, Feb 16, 2018 at 09:15:26AM -0700, Jason Gunthorpe wrote:
> On Thu, Feb 15, 2018 at 04:39:52PM -0800, Adit Ranadive wrote:
> > Creation of resources can fail if the rounding up exceeds
> > provider supported values.
> >
> > Fixes: 4c8ed14eb6b7 ("vmw_pvrdma: Add SRQ support")
> > Reviewed-by: Bryan Tan <bryantan-pghWNbHTmq7QT0dZR+AlfA@public.gmane.org>
> > Reviewed-by: Nitish Bhat <bnitish-pghWNbHTmq7QT0dZR+AlfA@public.gmane.org>
> > Signed-off-by: Adit Ranadive <aditr-pghWNbHTmq7QT0dZR+AlfA@public.gmane.org>
> > Cc: stable-Xl5UnYtxxKxKUA01WzcqbQ@public.gmane.org
> >  providers/vmw_pvrdma/qp.c | 8 +++-----
> >  1 file changed, 3 insertions(+), 5 deletions(-)
> >
> > diff --git a/providers/vmw_pvrdma/qp.c b/providers/vmw_pvrdma/qp.c
> > index efcc99b..4b9f897 100644
> > +++ b/providers/vmw_pvrdma/qp.c
> > @@ -113,7 +113,7 @@ struct ibv_srq *pvrdma_create_srq(struct ibv_pd *pd,
> >  	int ret;
> >
> >  	attr->attr.max_wr = align_next_power2(max_t(uint32_t, 1U, attr->attr.max_wr));
> > -	attr->attr.max_sge = align_next_power2(max_t(uint32_t, 1U, attr->attr.max_sge));
> > +	attr->attr.max_sge = max_t(uint32_t, 1U, attr->attr.max_sge);
>
> What is with all these max's? If the user provides <= 0 for these
> values it should be EINVAL, not round up.

SRQ attributes are uint32_t, user can't provide negative values.

 680 struct ibv_srq_attr {
 681         uint32_t                max_wr;
 682         uint32_t                max_sge;
 683         uint32_t                srq_limit;
 684 };

Thanks

>
> Jason
> --
> To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
> the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH] vmw_pvrdma: Avoid rounding up of sge count to power of 2

[Adit Ranadive]

On 2/15/18 11:22 PM, Leon Romanovsky wrote:
> On Thu, Feb 15, 2018 at 04:39:52PM -0800, Adit Ranadive wrote:
>> Creation of resources can fail if the rounding up exceeds
>> provider supported values.
>>
>> Fixes: 4c8ed14eb6b7 ("vmw_pvrdma: Add SRQ support")
>> Reviewed-by: Bryan Tan <bryantan-pghWNbHTmq7QT0dZR+AlfA@public.gmane.org>
>> Reviewed-by: Nitish Bhat <bnitish-pghWNbHTmq7QT0dZR+AlfA@public.gmane.org>
>> Signed-off-by: Adit Ranadive <aditr-pghWNbHTmq7QT0dZR+AlfA@public.gmane.org>
>> Cc: stable-Xl5UnYtxxKxKUA01WzcqbQ@public.gmane.org
>> ---
>>  providers/vmw_pvrdma/qp.c | 8 +++-----
>>  1 file changed, 3 insertions(+), 5 deletions(-)
>>
>> diff --git a/providers/vmw_pvrdma/qp.c b/providers/vmw_pvrdma/qp.c
>> index efcc99b..4b9f897 100644
>> --- a/providers/vmw_pvrdma/qp.c
>> +++ b/providers/vmw_pvrdma/qp.c
>> @@ -113,7 +113,7 @@ struct ibv_srq *pvrdma_create_srq(struct ibv_pd *pd,
>>  	int ret;
>>
>>  	attr->attr.max_wr = align_next_power2(max_t(uint32_t, 1U, attr->attr.max_wr));
>> -	attr->attr.max_sge = align_next_power2(max_t(uint32_t, 1U, attr->attr.max_sge));
>> +	attr->attr.max_sge = max_t(uint32_t, 1U, attr->attr.max_sge);
> 
> One of two: or your commit message is not correct and you should mention
> that align_next_power2() is not needed, or your code is incorrect,
> because user can provide large enough number and this call will fail
> anyway.

Sorry, that's a terrible commit message. Yeah the align_next_power2 is extraneous.
Will send v1 out with updated message.
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html