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

[Yishai Hadas <yishaih@dev.mellanox.co.il>]

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.

Yishai