| Message ID | 20210801092050.6322-1-lizhijian@cn.fujitsu.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | RDMA/mlx5: return the EFAULT per ibv_advise_mr(3) | expand |
On Sun, Aug 01, 2021 at 05:20:50PM +0800, Li Zhijian wrote: > ibv_advise_mr(3) says: > EFAULT In one of the following: o When the range requested is out of the MR bounds, > or when parts of it are not part of the process address space. o One of the > lkeys provided in the scatter gather list is invalid or with wrong write access > > Actually get_prefetchable_mr() will return NULL if it see above conditions > > Signed-off-by: Li Zhijian <lizhijian@cn.fujitsu.com> > --- > drivers/infiniband/hw/mlx5/odp.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > Thanks, Reviewed-by: Leon Romanovsky <leonro@nvidia.com>
On Sun, Aug 01, 2021 at 05:20:50PM +0800, Li Zhijian wrote: > ibv_advise_mr(3) says: > EFAULT In one of the following: o When the range requested is out of the MR bounds, > or when parts of it are not part of the process address space. o One of the > lkeys provided in the scatter gather list is invalid or with wrong write access > > Actually get_prefetchable_mr() will return NULL if it see above conditions No, get_prefetchable_mr() returns NULL if the mkey is invalid The above is talking about the address, which is checked inside pagefault_mr() and does return EFAULT for all the cases I can see? Jason
On Tue, Aug 03, 2021 at 01:25:07PM -0300, Jason Gunthorpe wrote: > On Sun, Aug 01, 2021 at 05:20:50PM +0800, Li Zhijian wrote: > > ibv_advise_mr(3) says: > > EFAULT In one of the following: o When the range requested is out of the MR bounds, > > or when parts of it are not part of the process address space. o One of the > > lkeys provided in the scatter gather list is invalid or with wrong write access > > > > Actually get_prefetchable_mr() will return NULL if it see above conditions > > No, get_prefetchable_mr() returns NULL if the mkey is invalid And what is this? 1701 static struct mlx5_ib_mr * 1702 get_prefetchable_mr(struct ib_pd *pd, enum ib_uverbs_advise_mr_advice advice, 1703 u32 lkey) ... 1721 /* prefetch with write-access must be supported by the MR */ 1722 if (advice == IB_UVERBS_ADVISE_MR_ADVICE_PREFETCH_WRITE && 1723 !mr->umem->writable) { 1724 mr = NULL; 1725 goto end; 1726 } > > The above is talking about the address, which is checked inside > pagefault_mr() and does return EFAULT for all the cases I can see? > > Jason
On Tue, Aug 03, 2021 at 08:56:54PM +0300, Leon Romanovsky wrote: > On Tue, Aug 03, 2021 at 01:25:07PM -0300, Jason Gunthorpe wrote: > > On Sun, Aug 01, 2021 at 05:20:50PM +0800, Li Zhijian wrote: > > > ibv_advise_mr(3) says: > > > EFAULT In one of the following: o When the range requested is out of the MR bounds, > > > or when parts of it are not part of the process address space. o One of the > > > lkeys provided in the scatter gather list is invalid or with wrong write access > > > > > > Actually get_prefetchable_mr() will return NULL if it see above conditions > > > > No, get_prefetchable_mr() returns NULL if the mkey is invalid > > And what is this? > 1701 static struct mlx5_ib_mr * > 1702 get_prefetchable_mr(struct ib_pd *pd, enum ib_uverbs_advise_mr_advice advice, > 1703 u32 lkey) > > ... > > 1721 /* prefetch with write-access must be supported by the MR */ > 1722 if (advice == IB_UVERBS_ADVISE_MR_ADVICE_PREFETCH_WRITE && > 1723 !mr->umem->writable) { > 1724 mr = NULL; > 1725 goto end; > 1726 } I would say that is an invalid mkey Jason
On Tue, Aug 03, 2021 at 03:13:41PM -0300, Jason Gunthorpe wrote: > On Tue, Aug 03, 2021 at 08:56:54PM +0300, Leon Romanovsky wrote: > > On Tue, Aug 03, 2021 at 01:25:07PM -0300, Jason Gunthorpe wrote: > > > On Sun, Aug 01, 2021 at 05:20:50PM +0800, Li Zhijian wrote: > > > > ibv_advise_mr(3) says: > > > > EFAULT In one of the following: o When the range requested is out of the MR bounds, > > > > or when parts of it are not part of the process address space. o One of the > > > > lkeys provided in the scatter gather list is invalid or with wrong write access > > > > > > > > Actually get_prefetchable_mr() will return NULL if it see above conditions > > > > > > No, get_prefetchable_mr() returns NULL if the mkey is invalid > > > > And what is this? > > 1701 static struct mlx5_ib_mr * > > 1702 get_prefetchable_mr(struct ib_pd *pd, enum ib_uverbs_advise_mr_advice advice, > > 1703 u32 lkey) > > > > ... > > > > 1721 /* prefetch with write-access must be supported by the MR */ > > 1722 if (advice == IB_UVERBS_ADVISE_MR_ADVICE_PREFETCH_WRITE && > > 1723 !mr->umem->writable) { > > 1724 mr = NULL; > > 1725 goto end; > > 1726 } > > I would say that is an invalid mkey I see it is as wrong write access. Thanks > > Jason
On Wed, Aug 04, 2021 at 08:35:30AM +0300, Leon Romanovsky wrote: > On Tue, Aug 03, 2021 at 03:13:41PM -0300, Jason Gunthorpe wrote: > > On Tue, Aug 03, 2021 at 08:56:54PM +0300, Leon Romanovsky wrote: > > > On Tue, Aug 03, 2021 at 01:25:07PM -0300, Jason Gunthorpe wrote: > > > > On Sun, Aug 01, 2021 at 05:20:50PM +0800, Li Zhijian wrote: > > > > > ibv_advise_mr(3) says: > > > > > EFAULT In one of the following: o When the range requested is out of the MR bounds, > > > > > or when parts of it are not part of the process address space. o One of the > > > > > lkeys provided in the scatter gather list is invalid or with wrong write access > > > > > > > > > > Actually get_prefetchable_mr() will return NULL if it see above conditions > > > > > > > > No, get_prefetchable_mr() returns NULL if the mkey is invalid > > > > > > And what is this? > > > 1701 static struct mlx5_ib_mr * > > > 1702 get_prefetchable_mr(struct ib_pd *pd, enum ib_uverbs_advise_mr_advice advice, > > > 1703 u32 lkey) > > > > > > ... > > > > > > 1721 /* prefetch with write-access must be supported by the MR */ > > > 1722 if (advice == IB_UVERBS_ADVISE_MR_ADVICE_PREFETCH_WRITE && > > > 1723 !mr->umem->writable) { > > > 1724 mr = NULL; > > > 1725 goto end; > > > 1726 } > > > > I would say that is an invalid mkey > > I see it is as wrong write access. It just means the man page is wrong Jason
On Wed, Aug 04, 2021 at 03:50:22PM -0300, Jason Gunthorpe wrote: > On Wed, Aug 04, 2021 at 08:35:30AM +0300, Leon Romanovsky wrote: > > On Tue, Aug 03, 2021 at 03:13:41PM -0300, Jason Gunthorpe wrote: > > > On Tue, Aug 03, 2021 at 08:56:54PM +0300, Leon Romanovsky wrote: > > > > On Tue, Aug 03, 2021 at 01:25:07PM -0300, Jason Gunthorpe wrote: > > > > > On Sun, Aug 01, 2021 at 05:20:50PM +0800, Li Zhijian wrote: > > > > > > ibv_advise_mr(3) says: > > > > > > EFAULT In one of the following: o When the range requested is out of the MR bounds, > > > > > > or when parts of it are not part of the process address space. o One of the > > > > > > lkeys provided in the scatter gather list is invalid or with wrong write access > > > > > > > > > > > > Actually get_prefetchable_mr() will return NULL if it see above conditions > > > > > > > > > > No, get_prefetchable_mr() returns NULL if the mkey is invalid > > > > > > > > And what is this? > > > > 1701 static struct mlx5_ib_mr * > > > > 1702 get_prefetchable_mr(struct ib_pd *pd, enum ib_uverbs_advise_mr_advice advice, > > > > 1703 u32 lkey) > > > > > > > > ... > > > > > > > > 1721 /* prefetch with write-access must be supported by the MR */ > > > > 1722 if (advice == IB_UVERBS_ADVISE_MR_ADVICE_PREFETCH_WRITE && > > > > 1723 !mr->umem->writable) { > > > > 1724 mr = NULL; > > > > 1725 goto end; > > > > 1726 } > > > > > > I would say that is an invalid mkey > > > > I see it is as wrong write access. > > It just means the man page is wrong ok, it can be a solution too. Thanks > > Jason
On 05/08/2021 14:43, Leon Romanovsky wrote: > On Wed, Aug 04, 2021 at 03:50:22PM -0300, Jason Gunthorpe wrote: >> On Wed, Aug 04, 2021 at 08:35:30AM +0300, Leon Romanovsky wrote: >>> On Tue, Aug 03, 2021 at 03:13:41PM -0300, Jason Gunthorpe wrote: >>>> On Tue, Aug 03, 2021 at 08:56:54PM +0300, Leon Romanovsky wrote: >>>>> On Tue, Aug 03, 2021 at 01:25:07PM -0300, Jason Gunthorpe wrote: >>>>>> On Sun, Aug 01, 2021 at 05:20:50PM +0800, Li Zhijian wrote: >>>>>>> ibv_advise_mr(3) says: >>>>>>> EFAULT In one of the following: o When the range requested is out of the MR bounds, >>>>>>> or when parts of it are not part of the process address space. o One of the >>>>>>> lkeys provided in the scatter gather list is invalid or with wrong write access >>>>>>> >>>>>>> Actually get_prefetchable_mr() will return NULL if it see above conditions >>>>>> No, get_prefetchable_mr() returns NULL if the mkey is invalid >>>>> And what is this? >>>>> 1701 static struct mlx5_ib_mr * >>>>> 1702 get_prefetchable_mr(struct ib_pd *pd, enum ib_uverbs_advise_mr_advice advice, >>>>> 1703 u32 lkey) >>>>> >>>>> ... >>>>> >>>>> 1721 /* prefetch with write-access must be supported by the MR */ >>>>> 1722 if (advice == IB_UVERBS_ADVISE_MR_ADVICE_PREFETCH_WRITE && >>>>> 1723 !mr->umem->writable) { >>>>> 1724 mr = NULL; >>>>> 1725 goto end; >>>>> 1726 } >>>> I would say that is an invalid mkey >>> I see it is as wrong write access. >> It just means the man page is wrong > ok, it can be a solution too. It sounds good. I will try to update the manpage ibv_advise_mr(3) instead. Thanks Zhijian
convert to text and send again
Hi Jason & Leon
It reminds me that ibv_advise_mr doesn't mention ENOENT any more which value the API actually returns now.
the ENOENT cases/situations returned by kernel mlx5 implementation is most likely same with EINVALL as its manpage[1].
So shall we return EINVAL instead of ENOENT in kernel side when get_prefetchable_mr returns NULL?
1781 static int mlx5_ib_prefetch_sg_list(struct ib_pd *pd,
1782 enum ib_uverbs_advise_mr_advice advice,
1783 u32 pf_flags, struct ib_sge *sg_list,
1784 u32 num_sge)
1785 {
1786 u32 bytes_mapped = 0;
1787 int ret = 0;
1788 u32 i;
1789
1790 for (i = 0; i < num_sge; ++i) {
1791 struct mlx5_ib_mr *mr;
1792
1793 mr = get_prefetchable_mr(pd, advice, sg_list[i].lkey);
1794 if (!mr)
1795 return -ENOENT;
1796 ret = pagefault_mr(mr, sg_list[i].addr, sg_list[i].length,
1797 &bytes_mapped, pf_flags);
=============
RETURN VALUE
ibv_advise_mr() returns 0 when the call was successful, or the value of errno on failure (which indicates the failure reason).
EOPNOTSUPP
libibverbs or provider driver doesn’t support the ibv_advise_mr() verb (ENOSYS may sometimes be returned by old versions of libibverbs).
ENOTSUP
The advise operation isn’t supported.
EFAULT In one of the following: o When the range requested is out of the MR bounds, or when parts of it are not part of the process address space. o One of the lkeys provided in the scatter gather
list is invalid or with wrong write access.
EINVAL In one of the following: o The PD is invalid. o The flags are invalid.
[1]:https://github.com/linux-rdma/rdma-core/blob/master/libibverbs/man/ibv_advise_mr.3.md
Thanks
on 2021/8/16 10:59, Li, Zhijian wrote:
> On 05/08/2021 14:43, Leon Romanovsky wrote:
>> On Wed, Aug 04, 2021 at 03:50:22PM -0300, Jason Gunthorpe wrote:
>>> On Wed, Aug 04, 2021 at 08:35:30AM +0300, Leon Romanovsky wrote:
>>>> On Tue, Aug 03, 2021 at 03:13:41PM -0300, Jason Gunthorpe wrote:
>>>>> On Tue, Aug 03, 2021 at 08:56:54PM +0300, Leon Romanovsky wrote:
>>>>>> On Tue, Aug 03, 2021 at 01:25:07PM -0300, Jason Gunthorpe wrote:
>>>>>>> On Sun, Aug 01, 2021 at 05:20:50PM +0800, Li Zhijian wrote:
>>>>>>>> ibv_advise_mr(3) says:
>>>>>>>> EFAULT In one of the following: o When the range requested is out of the MR bounds,
>>>>>>>> or when parts of it are not part of the process address space. o One of the
>>>>>>>> lkeys provided in the scatter gather list is invalid or with wrong write access
>>>>>>>>
>>>>>>>> Actually get_prefetchable_mr() will return NULL if it see above conditions
>>>>>>> No, get_prefetchable_mr() returns NULL if the mkey is invalid
>>>>>> And what is this?
>>>>>> 1701 static struct mlx5_ib_mr *
>>>>>> 1702 get_prefetchable_mr(struct ib_pd *pd, enum ib_uverbs_advise_mr_advice advice,
>>>>>> 1703 u32 lkey)
>>>>>>
>>>>>> ...
>>>>>>
>>>>>> 1721 /* prefetch with write-access must be supported by the MR */
>>>>>> 1722 if (advice == IB_UVERBS_ADVISE_MR_ADVICE_PREFETCH_WRITE &&
>>>>>> 1723 !mr->umem->writable) {
>>>>>> 1724 mr = NULL;
>>>>>> 1725 goto end;
>>>>>> 1726 }
>>>>> I would say that is an invalid mkey
>>>> I see it is as wrong write access.
>>> It just means the man page is wrong
>> ok, it can be a solution too.
> It sounds good. I will try to update the manpage ibv_advise_mr(3) instead.
>
>
> Thanks
> Zhijian
On Sat, Aug 21, 2021 at 05:44:43PM +0800, Li, Zhijian wrote: > convert to text and send again > > > Hi Jason & Leon > > It reminds me that ibv_advise_mr doesn't mention ENOENT any more which value the API actually returns now. > the ENOENT cases/situations returned by kernel mlx5 implementation is most likely same with EINVALL as its manpage[1]. > > So shall we return EINVAL instead of ENOENT in kernel side when get_prefetchable_mr returns NULL? No, the man page should be fixed Jason
On 26/08/2021 01:28, Jason Gunthorpe wrote: > On Sat, Aug 21, 2021 at 05:44:43PM +0800, Li, Zhijian wrote: >> convert to text and send again >> >> >> Hi Jason & Leon >> >> It reminds me that ibv_advise_mr doesn't mention ENOENT any more which value the API actually returns now. >> the ENOENT cases/situations returned by kernel mlx5 implementation is most likely same with EINVALL as its manpage[1]. >> >> So shall we return EINVAL instead of ENOENT in kernel side when get_prefetchable_mr returns NULL? > No, the man page should be fixed thanks a lot, i have submitted a RP to rdma-core https://github.com/linux-rdma/rdma-core/pull/1048 Thanks Zhijian > > Jason > >
Just noticed that there is another code path i was missing will return EINVAL
when get_prefetchable_mr returns NULL
ENOENT:
mlx5_ib_advise_mr_prefetch()
-> mlx5_ib_prefetch_sg_list()
-> get_prefetchable_mr()
return -ENOENT;
EINVAL:
mlx5_ib_advise_mr_prefetch
->init_prefetch_work()
-> get_prefetchable_mr()
return -EINVAL;
where get_prefetchable_mr will check pd and write access & key
So which value we should return ?
Thanks
on 2021/8/26 9:18, Li, Zhijian wrote:
>
> On 26/08/2021 01:28, Jason Gunthorpe wrote:
>> On Sat, Aug 21, 2021 at 05:44:43PM +0800, Li, Zhijian wrote:
>>> convert to text and send again
>>>
>>>
>>> Hi Jason & Leon
>>>
>>> It reminds me that ibv_advise_mr doesn't mention ENOENT any more which value the API actually returns now.
>>> the ENOENT cases/situations returned by kernel mlx5 implementation is most likely same with EINVALL as its manpage[1].
>>>
>>> So shall we return EINVAL instead of ENOENT in kernel side when get_prefetchable_mr returns NULL?
>> No, the man page should be fixed
> thanks a lot, i have submitted a RP to rdma-core https://github.com/linux-rdma/rdma-core/pull/1048
>
> Thanks
> Zhijian
>
>> Jason
>>
>>
diff --git a/drivers/infiniband/hw/mlx5/odp.c b/drivers/infiniband/hw/mlx5/odp.c index d0d98e584ebc..8d2a626c87cf 100644 --- a/drivers/infiniband/hw/mlx5/odp.c +++ b/drivers/infiniband/hw/mlx5/odp.c @@ -1791,7 +1791,7 @@ static int mlx5_ib_prefetch_sg_list(struct ib_pd *pd, mr = get_prefetchable_mr(pd, advice, sg_list[i].lkey); if (!mr) - return -ENOENT; + return -EFAULT; ret = pagefault_mr(mr, sg_list[i].addr, sg_list[i].length, &bytes_mapped, pf_flags); if (ret < 0) {
ibv_advise_mr(3) says: EFAULT In one of the following: o When the range requested is out of the MR bounds, or when parts of it are not part of the process address space. o One of the lkeys provided in the scatter gather list is invalid or with wrong write access Actually get_prefetchable_mr() will return NULL if it see above conditions Signed-off-by: Li Zhijian <lizhijian@cn.fujitsu.com> --- drivers/infiniband/hw/mlx5/odp.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)