| Message ID | 20200707063100.3811-3-michal.kalderon@marvell.com (mailing list archive) |
|---|---|
| State | Mainlined |
| Commit | eb7f84e379daad69b4c92538baeaf93bbf493c14 |
| Delegated to: | Jason Gunthorpe |
| Headers | show |
| Series | RDMA/qedr: Add EDPM kernel-user flags for feature compatibility | expand |
On Tue, Jul 07, 2020 at 09:31:00AM +0300, Michal Kalderon wrote: > User space should receive the maximum edpm size from kernel > driver, similar to other edpm/ldpm related limits. > Add an additional parameter to the alloc_ucontext_resp > structure for the edpm maximum size. > > In addition, pass an indication from user-space to kernel > (and not just kernel to user) that the DPM sizes are supported. > > This is for supporting backward-forward compatibility between driver and > lib for everything related to DPM transaction and limit sizes. > > This should have been part of commit mentioned in Fixes tag. > Fixes: 93a3d05f9d68 ("RDMA/qedr: Add kernel capability flags for dpm > enabled mode") > Signed-off-by: Ariel Elior <ariel.elior@marvell.com> > Signed-off-by: Michal Kalderon <michal.kalderon@marvell.com> > drivers/infiniband/hw/qedr/verbs.c | 9 ++++++--- > include/uapi/rdma/qedr-abi.h | 6 +++++- > 2 files changed, 11 insertions(+), 4 deletions(-) > > diff --git a/drivers/infiniband/hw/qedr/verbs.c b/drivers/infiniband/hw/qedr/verbs.c > index fbb0c66c7f2c..cfe4cd637f1c 100644 > +++ b/drivers/infiniband/hw/qedr/verbs.c > @@ -320,9 +320,12 @@ int qedr_alloc_ucontext(struct ib_ucontext *uctx, struct ib_udata *udata) > QEDR_DPM_TYPE_ROCE_LEGACY | > QEDR_DPM_TYPE_ROCE_EDPM_MODE; > > - uresp.dpm_flags |= QEDR_DPM_SIZES_SET; > - uresp.ldpm_limit_size = QEDR_LDPM_MAX_SIZE; > - uresp.edpm_trans_size = QEDR_EDPM_TRANS_SIZE; > + if (ureq.context_flags & QEDR_SUPPORT_DPM_SIZES) { Why does this need an input flag just to set some outputs? The usual truncate on not enough size should take care of it, right? Jason
> From: linux-rdma-owner@vger.kernel.org <linux-rdma- > owner@vger.kernel.org> On Behalf Of Jason Gunthorpe > > On Tue, Jul 07, 2020 at 09:31:00AM +0300, Michal Kalderon wrote: > > User space should receive the maximum edpm size from kernel driver, > > similar to other edpm/ldpm related limits. > > Add an additional parameter to the alloc_ucontext_resp structure for > > the edpm maximum size. > > > > In addition, pass an indication from user-space to kernel (and not > > just kernel to user) that the DPM sizes are supported. > > > > This is for supporting backward-forward compatibility between driver > > and lib for everything related to DPM transaction and limit sizes. > > > > This should have been part of commit mentioned in Fixes tag. > > Fixes: 93a3d05f9d68 ("RDMA/qedr: Add kernel capability flags for dpm > > enabled mode") > > Signed-off-by: Ariel Elior <ariel.elior@marvell.com> > > Signed-off-by: Michal Kalderon <michal.kalderon@marvell.com> > > drivers/infiniband/hw/qedr/verbs.c | 9 ++++++--- > > include/uapi/rdma/qedr-abi.h | 6 +++++- > > 2 files changed, 11 insertions(+), 4 deletions(-) > > > > diff --git a/drivers/infiniband/hw/qedr/verbs.c > > b/drivers/infiniband/hw/qedr/verbs.c > > index fbb0c66c7f2c..cfe4cd637f1c 100644 > > +++ b/drivers/infiniband/hw/qedr/verbs.c > > @@ -320,9 +320,12 @@ int qedr_alloc_ucontext(struct ib_ucontext *uctx, > struct ib_udata *udata) > > QEDR_DPM_TYPE_ROCE_LEGACY | > > QEDR_DPM_TYPE_ROCE_EDPM_MODE; > > > > - uresp.dpm_flags |= QEDR_DPM_SIZES_SET; > > - uresp.ldpm_limit_size = QEDR_LDPM_MAX_SIZE; > > - uresp.edpm_trans_size = QEDR_EDPM_TRANS_SIZE; > > + if (ureq.context_flags & QEDR_SUPPORT_DPM_SIZES) { > > Why does this need an input flag just to set some outputs? > > The usual truncate on not enough size should take care of it, right? At this point it just sets some output, but for future related changes around these sizes there will also be fw related configurations, we will need to know whether the lib supports accepting different sizes or not. This is for forward compatibility between libqedr and driver. > > Jason
On Thu, Jul 16, 2020 at 06:17:29PM +0000, Michal Kalderon wrote: > > From: linux-rdma-owner@vger.kernel.org <linux-rdma- > > owner@vger.kernel.org> On Behalf Of Jason Gunthorpe > > > > On Tue, Jul 07, 2020 at 09:31:00AM +0300, Michal Kalderon wrote: > > > User space should receive the maximum edpm size from kernel driver, > > > similar to other edpm/ldpm related limits. > > > Add an additional parameter to the alloc_ucontext_resp structure for > > > the edpm maximum size. > > > > > > In addition, pass an indication from user-space to kernel (and not > > > just kernel to user) that the DPM sizes are supported. > > > > > > This is for supporting backward-forward compatibility between driver > > > and lib for everything related to DPM transaction and limit sizes. > > > > > > This should have been part of commit mentioned in Fixes tag. > > > Fixes: 93a3d05f9d68 ("RDMA/qedr: Add kernel capability flags for dpm > > > enabled mode") > > > Signed-off-by: Ariel Elior <ariel.elior@marvell.com> > > > Signed-off-by: Michal Kalderon <michal.kalderon@marvell.com> > > > drivers/infiniband/hw/qedr/verbs.c | 9 ++++++--- > > > include/uapi/rdma/qedr-abi.h | 6 +++++- > > > 2 files changed, 11 insertions(+), 4 deletions(-) > > > > > > diff --git a/drivers/infiniband/hw/qedr/verbs.c > > > b/drivers/infiniband/hw/qedr/verbs.c > > > index fbb0c66c7f2c..cfe4cd637f1c 100644 > > > +++ b/drivers/infiniband/hw/qedr/verbs.c > > > @@ -320,9 +320,12 @@ int qedr_alloc_ucontext(struct ib_ucontext *uctx, > > struct ib_udata *udata) > > > QEDR_DPM_TYPE_ROCE_LEGACY | > > > QEDR_DPM_TYPE_ROCE_EDPM_MODE; > > > > > > - uresp.dpm_flags |= QEDR_DPM_SIZES_SET; > > > - uresp.ldpm_limit_size = QEDR_LDPM_MAX_SIZE; > > > - uresp.edpm_trans_size = QEDR_EDPM_TRANS_SIZE; > > > + if (ureq.context_flags & QEDR_SUPPORT_DPM_SIZES) { > > > > Why does this need an input flag just to set some outputs? > > > > The usual truncate on not enough size should take care of it, right? > At this point it just sets some output, but for future related changes around these sizes > there will also be fw related configurations, we will need to know whether the lib supports > accepting different sizes or not. This is for forward compatibility between libqedr and > driver. I would be happier to see this flag introduced when it actually had a purpose, as I really don't like the pattern of conditionally filling uresp, but OK. Please delete this if when you make use of it the flag properly. Jason
> From: Jason Gunthorpe <jgg@nvidia.com> > Sent: Thursday, July 16, 2020 9:58 PM > > External Email > > ---------------------------------------------------------------------- > On Thu, Jul 16, 2020 at 06:17:29PM +0000, Michal Kalderon wrote: > > > From: linux-rdma-owner@vger.kernel.org <linux-rdma- > > > owner@vger.kernel.org> On Behalf Of Jason Gunthorpe > > > > > > On Tue, Jul 07, 2020 at 09:31:00AM +0300, Michal Kalderon wrote: > > > > User space should receive the maximum edpm size from kernel > > > > driver, similar to other edpm/ldpm related limits. > > > > Add an additional parameter to the alloc_ucontext_resp structure > > > > for the edpm maximum size. > > > > > > > > In addition, pass an indication from user-space to kernel (and not > > > > just kernel to user) that the DPM sizes are supported. > > > > > > > > This is for supporting backward-forward compatibility between > > > > driver and lib for everything related to DPM transaction and limit sizes. > > > > > > > > This should have been part of commit mentioned in Fixes tag. > > > > Fixes: 93a3d05f9d68 ("RDMA/qedr: Add kernel capability flags for > > > > dpm enabled mode") > > > > Signed-off-by: Ariel Elior <ariel.elior@marvell.com> > > > > Signed-off-by: Michal Kalderon <michal.kalderon@marvell.com> > > > > drivers/infiniband/hw/qedr/verbs.c | 9 ++++++--- > > > > include/uapi/rdma/qedr-abi.h | 6 +++++- > > > > 2 files changed, 11 insertions(+), 4 deletions(-) > > > > > > > > diff --git a/drivers/infiniband/hw/qedr/verbs.c > > > > b/drivers/infiniband/hw/qedr/verbs.c > > > > index fbb0c66c7f2c..cfe4cd637f1c 100644 > > > > +++ b/drivers/infiniband/hw/qedr/verbs.c > > > > @@ -320,9 +320,12 @@ int qedr_alloc_ucontext(struct ib_ucontext > > > > *uctx, > > > struct ib_udata *udata) > > > > QEDR_DPM_TYPE_ROCE_LEGACY | > > > > QEDR_DPM_TYPE_ROCE_EDPM_MODE; > > > > > > > > - uresp.dpm_flags |= QEDR_DPM_SIZES_SET; > > > > - uresp.ldpm_limit_size = QEDR_LDPM_MAX_SIZE; > > > > - uresp.edpm_trans_size = QEDR_EDPM_TRANS_SIZE; > > > > + if (ureq.context_flags & QEDR_SUPPORT_DPM_SIZES) { > > > > > > Why does this need an input flag just to set some outputs? > > > > > > The usual truncate on not enough size should take care of it, right? > > At this point it just sets some output, but for future related changes > > around these sizes there will also be fw related configurations, we > > will need to know whether the lib supports accepting different sizes > > or not. This is for forward compatibility between libqedr and driver. > > I would be happier to see this flag introduced when it actually had a purpose, > as I really don't like the pattern of conditionally filling uresp, but OK. Please > delete this if when you make use of it the flag properly. Sure, thanks. > > Jason
diff --git a/drivers/infiniband/hw/qedr/verbs.c b/drivers/infiniband/hw/qedr/verbs.c index fbb0c66c7f2c..cfe4cd637f1c 100644 --- a/drivers/infiniband/hw/qedr/verbs.c +++ b/drivers/infiniband/hw/qedr/verbs.c @@ -320,9 +320,12 @@ int qedr_alloc_ucontext(struct ib_ucontext *uctx, struct ib_udata *udata) QEDR_DPM_TYPE_ROCE_LEGACY | QEDR_DPM_TYPE_ROCE_EDPM_MODE; - uresp.dpm_flags |= QEDR_DPM_SIZES_SET; - uresp.ldpm_limit_size = QEDR_LDPM_MAX_SIZE; - uresp.edpm_trans_size = QEDR_EDPM_TRANS_SIZE; + if (ureq.context_flags & QEDR_SUPPORT_DPM_SIZES) { + uresp.dpm_flags |= QEDR_DPM_SIZES_SET; + uresp.ldpm_limit_size = QEDR_LDPM_MAX_SIZE; + uresp.edpm_trans_size = QEDR_EDPM_TRANS_SIZE; + uresp.edpm_limit_size = QEDR_EDPM_MAX_SIZE; + } uresp.wids_enabled = 1; uresp.wid_count = oparams.wid_count; diff --git a/include/uapi/rdma/qedr-abi.h b/include/uapi/rdma/qedr-abi.h index b261c9fca07b..bf7333b2b5d7 100644 --- a/include/uapi/rdma/qedr-abi.h +++ b/include/uapi/rdma/qedr-abi.h @@ -40,7 +40,8 @@ /* user kernel communication data structures. */ enum qedr_alloc_ucontext_flags { QEDR_ALLOC_UCTX_EDPM_MODE = 1 << 0, - QEDR_ALLOC_UCTX_DB_REC = 1 << 1 + QEDR_ALLOC_UCTX_DB_REC = 1 << 1, + QEDR_SUPPORT_DPM_SIZES = 1 << 2, }; struct qedr_alloc_ucontext_req { @@ -50,6 +51,7 @@ struct qedr_alloc_ucontext_req { #define QEDR_LDPM_MAX_SIZE (8192) #define QEDR_EDPM_TRANS_SIZE (64) +#define QEDR_EDPM_MAX_SIZE (ROCE_REQ_MAX_INLINE_DATA_SIZE) enum qedr_rdma_dpm_type { QEDR_DPM_TYPE_NONE = 0, @@ -77,6 +79,8 @@ struct qedr_alloc_ucontext_resp { __u16 ldpm_limit_size; __u8 edpm_trans_size; __u8 reserved; + __u16 edpm_limit_size; + __u8 padding[6]; }; struct qedr_alloc_pd_ureq {
User space should receive the maximum edpm size from kernel driver, similar to other edpm/ldpm related limits. Add an additional parameter to the alloc_ucontext_resp structure for the edpm maximum size. In addition, pass an indication from user-space to kernel (and not just kernel to user) that the DPM sizes are supported. This is for supporting backward-forward compatibility between driver and lib for everything related to DPM transaction and limit sizes. This should have been part of commit mentioned in Fixes tag. Fixes: 93a3d05f9d68 ("RDMA/qedr: Add kernel capability flags for dpm enabled mode") Signed-off-by: Ariel Elior <ariel.elior@marvell.com> Signed-off-by: Michal Kalderon <michal.kalderon@marvell.com> --- drivers/infiniband/hw/qedr/verbs.c | 9 ++++++--- include/uapi/rdma/qedr-abi.h | 6 +++++- 2 files changed, 11 insertions(+), 4 deletions(-)