Re: [PATCH rdma-core 10/13] mlx5: Implement the import/unimport MR verbs

[Jason Gunthorpe <jgg@nvidia.com>]

On Wed, Jul 01, 2020 at 03:28:24PM +0300, Yishai Hadas wrote:
> On 6/23/2020 8:33 PM, Jason Gunthorpe wrote:
> > On Sun, Jun 21, 2020 at 11:44:52AM +0300, Yishai Hadas wrote:
> > > On 6/19/2020 3:50 PM, Jason Gunthorpe wrote:
> > > > On Wed, Jun 17, 2020 at 10:45:53AM +0300, Yishai Hadas wrote:
> > > > > Implement the import/unimport MR verbs based on their definition as
> > > > > described in the man page.
> > > > > 
> > > > > It uses the query MR KABI to retrieve the original MR properties based
> > > > > on its given handle.
> > > > > 
> > > > > Signed-off-by: Yishai Hadas <yishaih@mellanox.com>
> > > > >    libibverbs/cmd_mr.c          | 35 +++++++++++++++++++++++++++++++++++
> > > > >    libibverbs/driver.h          |  3 +++
> > > > >    libibverbs/libibverbs.map.in |  1 +
> > > > >    providers/mlx5/mlx5.c        |  2 ++
> > > > >    providers/mlx5/mlx5.h        |  3 +++
> > > > >    providers/mlx5/verbs.c       | 24 ++++++++++++++++++++++++
> > > > >    6 files changed, 68 insertions(+)
> > > > > 
> > > > > diff --git a/libibverbs/cmd_mr.c b/libibverbs/cmd_mr.c
> > > > > index cb729b6..6984948 100644
> > > > > +++ b/libibverbs/cmd_mr.c
> > > > > @@ -85,3 +85,38 @@ int ibv_cmd_dereg_mr(struct verbs_mr *vmr)
> > > > >    		return ret;
> > > > >    	return 0;
> > > > >    }
> > > > > +
> > > > > +int ibv_cmd_query_mr(struct ibv_pd *pd, struct verbs_mr *vmr,
> > > > > +		     uint32_t mr_handle)
> > > > > +{
> > > > > +	DECLARE_FBCMD_BUFFER(cmd, UVERBS_OBJECT_MR,
> > > > > +			     UVERBS_METHOD_QUERY_MR,
> > > > > +			     5, NULL);
> > > > > +	struct ibv_mr *mr = &vmr->ibv_mr;
> > > > > +	uint64_t iova;
> > > > > +	int ret;
> > > > > +
> > > > > +	fill_attr_in_obj(cmd, UVERBS_ATTR_QUERY_MR_HANDLE, mr_handle);
> > > > > +	fill_attr_out_ptr(cmd, UVERBS_ATTR_QUERY_MR_RESP_LKEY,
> > > > > +			  &mr->lkey);
> > > > > +	fill_attr_out_ptr(cmd, UVERBS_ATTR_QUERY_MR_RESP_RKEY,
> > > > > +			  &mr->rkey);
> > > > > +	fill_attr_out_ptr(cmd, UVERBS_ATTR_QUERY_MR_RESP_LENGTH,
> > > > > +			  &mr->length);
> > > > > +	/* The iova may be used down the road, let's have it ready from kernel */
> > > > > +	fill_attr_out_ptr(cmd, UVERBS_ATTR_QUERY_MR_RESP_IOVA,
> > > > > +			  &iova);
> > > > 
> > > > There isn't much reason to fill the attribute here..
> > > > 
> > > 
> > > We have defined this attribute from kernel perspective to be mandatory from
> > > day one as of other attributes above.
> > > Are you suggesting to change in kernel to let this attribute be optional and
> > > not fill it here ? other alternative ?
> > 
> > I'm not sure output attributes should be marked as mandatory?
> > 
> 
> OK, this attribute was changed to be optional in V1 kernel series, the PR
> was updated to not fill this attribute at all, thanks.

The kernel series looks OK, someone will send the v1?

Jason