| Message ID | 1518741592-12723-1-git-send-email-aditr@vmware.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
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@vmware.com> > Reviewed-by: Nitish Bhat <bnitish@vmware.com> > Signed-off-by: Adit Ranadive <aditr@vmware.com> > Cc: stable@linux-rdma.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@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html
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@vmware.com> > Reviewed-by: Nitish Bhat <bnitish@vmware.com> > Signed-off-by: Adit Ranadive <aditr@vmware.com> > Cc: stable@linux-rdma.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@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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@vmware.com> > > Reviewed-by: Nitish Bhat <bnitish@vmware.com> > > Signed-off-by: Adit Ranadive <aditr@vmware.com> > > Cc: stable@linux-rdma.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@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html
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@vmware.com> >> Reviewed-by: Nitish Bhat <bnitish@vmware.com> >> Signed-off-by: Adit Ranadive <aditr@vmware.com> >> Cc: stable@linux-rdma.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@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 {
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@vmware.com> Reviewed-by: Nitish Bhat <bnitish@vmware.com> Signed-off-by: Adit Ranadive <aditr@vmware.com> Cc: stable@linux-rdma.org --- providers/vmw_pvrdma/qp.c | 8 +++----- 1 file changed, 3 insertions(+), 5 deletions(-)