Re: [PATCH v3 rdma-next 2/2] RDMA/qedr: Add EDPM max size to alloc ucontext response

From: Jason Gunthorpe <jgg@nvidia.com>
To: Michal Kalderon <mkalderon@marvell.com>
Cc: "dledford@redhat.com" <dledford@redhat.com>,
	Ariel Elior <aelior@marvell.com>,
	Yuval Basson <ybason@marvell.com>,
	"linux-rdma@vger.kernel.org" <linux-rdma@vger.kernel.org>
Subject: Re: [PATCH v3 rdma-next 2/2] RDMA/qedr: Add EDPM max size to alloc ucontext response
Date: Thu, 16 Jul 2020 15:57:31 -0300	[thread overview]
Message-ID: <20200716185731.GD2021234@nvidia.com> (raw)
In-Reply-To: <MN2PR18MB31824C9F96D7F0D511D9B261A17F0@MN2PR18MB3182.namprd18.prod.outlook.com>

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