| Message ID | 20210903084815.184320-1-lizhijian@cn.fujitsu.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Jason Gunthorpe |
| Headers | show |
| Series | IB/mlx5: unify return value to ENOENT | expand |
On Fri, Sep 03, 2021 at 04:48:15PM +0800, Li Zhijian wrote: > Previously, ENOENT or EINVAL will be returned by ibv_advise_mr() although > the errors all occur at get_prefetchable_mr(). What do you think about this instead? From b739920ed4869decb02a0dbc58256e6c72ec7061 Mon Sep 17 00:00:00 2001 From: Jason Gunthorpe <jgg@nvidia.com> Date: Fri, 3 Sep 2021 16:48:15 +0800 Subject: [PATCH] IB/mlx5: Flow through a more detailed return code from get_prefetchable_mr() The error returns for various cases detected by get_prefetchable_mr() get confused as it flows back to userspace. Properly label each error path and flow the error code properly back to the system call. Suggested-by: Li Zhijian <lizhijian@cn.fujitsu.com> Signed-off-by: Jason Gunthorpe <jgg@nvidia.com> --- drivers/infiniband/hw/mlx5/odp.c | 40 ++++++++++++++++++++------------ 1 file changed, 25 insertions(+), 15 deletions(-) diff --git a/drivers/infiniband/hw/mlx5/odp.c b/drivers/infiniband/hw/mlx5/odp.c index d0d98e584ebcc3..77890a85fc2dd3 100644 --- a/drivers/infiniband/hw/mlx5/odp.c +++ b/drivers/infiniband/hw/mlx5/odp.c @@ -1708,20 +1708,26 @@ get_prefetchable_mr(struct ib_pd *pd, enum ib_uverbs_advise_mr_advice advice, xa_lock(&dev->odp_mkeys); mmkey = xa_load(&dev->odp_mkeys, mlx5_base_mkey(lkey)); - if (!mmkey || mmkey->key != lkey || mmkey->type != MLX5_MKEY_MR) + if (!mmkey || mmkey->key != lkey) { + mr = ERR_PTR(-ENOENT); goto end; + } + if (mmkey->type != MLX5_MKEY_MR) { + mr = ERR_PTR(-EINVAL); + goto end; + } mr = container_of(mmkey, struct mlx5_ib_mr, mmkey); if (mr->ibmr.pd != pd) { - mr = NULL; + mr = ERR_PTR(-EPERM); goto end; } /* prefetch with write-access must be supported by the MR */ if (advice == IB_UVERBS_ADVISE_MR_ADVICE_PREFETCH_WRITE && !mr->umem->writable) { - mr = NULL; + mr = ERR_PTR(-EPERM); goto end; } @@ -1753,7 +1759,7 @@ static void mlx5_ib_prefetch_mr_work(struct work_struct *w) destroy_prefetch_work(work); } -static bool init_prefetch_work(struct ib_pd *pd, +static int init_prefetch_work(struct ib_pd *pd, enum ib_uverbs_advise_mr_advice advice, u32 pf_flags, struct prefetch_mr_work *work, struct ib_sge *sg_list, u32 num_sge) @@ -1764,17 +1770,19 @@ static bool init_prefetch_work(struct ib_pd *pd, work->pf_flags = pf_flags; for (i = 0; i < num_sge; ++i) { + struct mlx5_ib_mr *mr; + + mr = get_prefetchable_mr(pd, advice, sg_list[i].lkey); + if (IS_ERR(mr)) { + work->num_sge = i; + return PTR_ERR(mr); + } work->frags[i].io_virt = sg_list[i].addr; work->frags[i].length = sg_list[i].length; - work->frags[i].mr = - get_prefetchable_mr(pd, advice, sg_list[i].lkey); - if (!work->frags[i].mr) { - work->num_sge = i; - return false; - } + work->frags[i].mr = mr; } work->num_sge = num_sge; - return true; + return 0; } static int mlx5_ib_prefetch_sg_list(struct ib_pd *pd, @@ -1790,8 +1798,8 @@ static int mlx5_ib_prefetch_sg_list(struct ib_pd *pd, struct mlx5_ib_mr *mr; mr = get_prefetchable_mr(pd, advice, sg_list[i].lkey); - if (!mr) - return -ENOENT; + if (IS_ERR(mr)) + return PTR_ERR(mr); ret = pagefault_mr(mr, sg_list[i].addr, sg_list[i].length, &bytes_mapped, pf_flags); if (ret < 0) { @@ -1811,6 +1819,7 @@ int mlx5_ib_advise_mr_prefetch(struct ib_pd *pd, { u32 pf_flags = 0; struct prefetch_mr_work *work; + int rc; if (advice == IB_UVERBS_ADVISE_MR_ADVICE_PREFETCH) pf_flags |= MLX5_PF_FLAGS_DOWNGRADE; @@ -1826,9 +1835,10 @@ int mlx5_ib_advise_mr_prefetch(struct ib_pd *pd, if (!work) return -ENOMEM; - if (!init_prefetch_work(pd, advice, pf_flags, work, sg_list, num_sge)) { + rc = init_prefetch_work(pd, advice, pf_flags, work, sg_list, num_sge); + if (rc) { destroy_prefetch_work(work); - return -EINVAL; + return rc; } queue_work(system_unbound_wq, &work->work); return 0;
ping On 03/09/2021 16:48, Li Zhijian wrote: > Previously, ENOENT or EINVAL will be returned by ibv_advise_mr() although > the errors all occur at get_prefetchable_mr(). > > flags = IBV_ADVISE_MR_FLAG_FLUSH: > mlx5_ib_advise_mr_prefetch() > -> mlx5_ib_prefetch_sg_list() > -> get_prefetchable_mr() > return -ENOENT; > > flags = 0: > mlx5_ib_advise_mr_prefetch() > -> init_prefetch_work() > -> get_prefetchable_mr() > return -EINVAL; > > Signed-off-by: Li Zhijian <lizhijian@cn.fujitsu.com> > --- > drivers/infiniband/hw/mlx5/odp.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/infiniband/hw/mlx5/odp.c b/drivers/infiniband/hw/mlx5/odp.c > index d0d98e584ebc..52572e7ea6f6 100644 > --- a/drivers/infiniband/hw/mlx5/odp.c > +++ b/drivers/infiniband/hw/mlx5/odp.c > @@ -1828,7 +1828,7 @@ int mlx5_ib_advise_mr_prefetch(struct ib_pd *pd, > > if (!init_prefetch_work(pd, advice, pf_flags, work, sg_list, num_sge)) { > destroy_prefetch_work(work); > - return -EINVAL; > + return -ENOENT; > } > queue_work(system_unbound_wq, &work->work); > return 0;
So sorry, I missed this reply at previous ping On 29/09/2021 01:08, Jason Gunthorpe wrote: > On Fri, Sep 03, 2021 at 04:48:15PM +0800, Li Zhijian wrote: >> Previously, ENOENT or EINVAL will be returned by ibv_advise_mr() although >> the errors all occur at get_prefetchable_mr(). > What do you think about this instead? Thank you, it's much better. Thanks Zhijian > > From b739920ed4869decb02a0dbc58256e6c72ec7061 Mon Sep 17 00:00:00 2001 > From: Jason Gunthorpe <jgg@nvidia.com> > Date: Fri, 3 Sep 2021 16:48:15 +0800 > Subject: [PATCH] IB/mlx5: Flow through a more detailed return code from > get_prefetchable_mr() > > The error returns for various cases detected by get_prefetchable_mr() get > confused as it flows back to userspace. Properly label each error path and > flow the error code properly back to the system call. > > Suggested-by: Li Zhijian <lizhijian@cn.fujitsu.com> > Signed-off-by: Jason Gunthorpe <jgg@nvidia.com> > --- > drivers/infiniband/hw/mlx5/odp.c | 40 ++++++++++++++++++++------------ > 1 file changed, 25 insertions(+), 15 deletions(-) > > diff --git a/drivers/infiniband/hw/mlx5/odp.c b/drivers/infiniband/hw/mlx5/odp.c > index d0d98e584ebcc3..77890a85fc2dd3 100644 > --- a/drivers/infiniband/hw/mlx5/odp.c > +++ b/drivers/infiniband/hw/mlx5/odp.c > @@ -1708,20 +1708,26 @@ get_prefetchable_mr(struct ib_pd *pd, enum ib_uverbs_advise_mr_advice advice, > > xa_lock(&dev->odp_mkeys); > mmkey = xa_load(&dev->odp_mkeys, mlx5_base_mkey(lkey)); > - if (!mmkey || mmkey->key != lkey || mmkey->type != MLX5_MKEY_MR) > + if (!mmkey || mmkey->key != lkey) { > + mr = ERR_PTR(-ENOENT); > goto end; > + } > + if (mmkey->type != MLX5_MKEY_MR) { > + mr = ERR_PTR(-EINVAL); > + goto end; > + } > > mr = container_of(mmkey, struct mlx5_ib_mr, mmkey); > > if (mr->ibmr.pd != pd) { > - mr = NULL; > + mr = ERR_PTR(-EPERM); > goto end; > } > > /* prefetch with write-access must be supported by the MR */ > if (advice == IB_UVERBS_ADVISE_MR_ADVICE_PREFETCH_WRITE && > !mr->umem->writable) { > - mr = NULL; > + mr = ERR_PTR(-EPERM); > goto end; > } > > @@ -1753,7 +1759,7 @@ static void mlx5_ib_prefetch_mr_work(struct work_struct *w) > destroy_prefetch_work(work); > } > > -static bool init_prefetch_work(struct ib_pd *pd, > +static int init_prefetch_work(struct ib_pd *pd, > enum ib_uverbs_advise_mr_advice advice, > u32 pf_flags, struct prefetch_mr_work *work, > struct ib_sge *sg_list, u32 num_sge) > @@ -1764,17 +1770,19 @@ static bool init_prefetch_work(struct ib_pd *pd, > work->pf_flags = pf_flags; > > for (i = 0; i < num_sge; ++i) { > + struct mlx5_ib_mr *mr; > + > + mr = get_prefetchable_mr(pd, advice, sg_list[i].lkey); > + if (IS_ERR(mr)) { > + work->num_sge = i; > + return PTR_ERR(mr); > + } > work->frags[i].io_virt = sg_list[i].addr; > work->frags[i].length = sg_list[i].length; > - work->frags[i].mr = > - get_prefetchable_mr(pd, advice, sg_list[i].lkey); > - if (!work->frags[i].mr) { > - work->num_sge = i; > - return false; > - } > + work->frags[i].mr = mr; > } > work->num_sge = num_sge; > - return true; > + return 0; > } > > static int mlx5_ib_prefetch_sg_list(struct ib_pd *pd, > @@ -1790,8 +1798,8 @@ static int mlx5_ib_prefetch_sg_list(struct ib_pd *pd, > struct mlx5_ib_mr *mr; > > mr = get_prefetchable_mr(pd, advice, sg_list[i].lkey); > - if (!mr) > - return -ENOENT; > + if (IS_ERR(mr)) > + return PTR_ERR(mr); > ret = pagefault_mr(mr, sg_list[i].addr, sg_list[i].length, > &bytes_mapped, pf_flags); > if (ret < 0) { > @@ -1811,6 +1819,7 @@ int mlx5_ib_advise_mr_prefetch(struct ib_pd *pd, > { > u32 pf_flags = 0; > struct prefetch_mr_work *work; > + int rc; > > if (advice == IB_UVERBS_ADVISE_MR_ADVICE_PREFETCH) > pf_flags |= MLX5_PF_FLAGS_DOWNGRADE; > @@ -1826,9 +1835,10 @@ int mlx5_ib_advise_mr_prefetch(struct ib_pd *pd, > if (!work) > return -ENOMEM; > > - if (!init_prefetch_work(pd, advice, pf_flags, work, sg_list, num_sge)) { > + rc = init_prefetch_work(pd, advice, pf_flags, work, sg_list, num_sge); > + if (rc) { > destroy_prefetch_work(work); > - return -EINVAL; > + return rc; > } > queue_work(system_unbound_wq, &work->work); > return 0;
On Wed, Sep 29, 2021 at 04:37:22AM +0000, lizhijian@fujitsu.com wrote: > So sorry, I missed this reply at previous ping > > > On 29/09/2021 01:08, Jason Gunthorpe wrote: > > On Fri, Sep 03, 2021 at 04:48:15PM +0800, Li Zhijian wrote: > >> Previously, ENOENT or EINVAL will be returned by ibv_advise_mr() although > >> the errors all occur at get_prefetchable_mr(). > > What do you think about this instead? > Thank you, it's much better. Ok, applied to for-next Jason
Hi Jason When update the ibv_advise_mr man page, i have a few concerns: On 29/09/2021 01:08, Jason Gunthorpe wrote: > On Fri, Sep 03, 2021 at 04:48:15PM +0800, Li Zhijian wrote: >> Previously, ENOENT or EINVAL will be returned by ibv_advise_mr() although >> the errors all occur at get_prefetchable_mr(). > What do you think about this instead? > > From b739920ed4869decb02a0dbc58256e6c72ec7061 Mon Sep 17 00:00:00 2001 > From: Jason Gunthorpe <jgg@nvidia.com> > Date: Fri, 3 Sep 2021 16:48:15 +0800 > Subject: [PATCH] IB/mlx5: Flow through a more detailed return code from > get_prefetchable_mr() > > The error returns for various cases detected by get_prefetchable_mr() get > confused as it flows back to userspace. Properly label each error path and > flow the error code properly back to the system call. > > Suggested-by: Li Zhijian <lizhijian@cn.fujitsu.com> > Signed-off-by: Jason Gunthorpe <jgg@nvidia.com> > --- > drivers/infiniband/hw/mlx5/odp.c | 40 ++++++++++++++++++++------------ > 1 file changed, 25 insertions(+), 15 deletions(-) > > diff --git a/drivers/infiniband/hw/mlx5/odp.c b/drivers/infiniband/hw/mlx5/odp.c > index d0d98e584ebcc3..77890a85fc2dd3 100644 > --- a/drivers/infiniband/hw/mlx5/odp.c > +++ b/drivers/infiniband/hw/mlx5/odp.c > @@ -1708,20 +1708,26 @@ get_prefetchable_mr(struct ib_pd *pd, enum ib_uverbs_advise_mr_advice advice, > > xa_lock(&dev->odp_mkeys); > mmkey = xa_load(&dev->odp_mkeys, mlx5_base_mkey(lkey)); > - if (!mmkey || mmkey->key != lkey || mmkey->type != MLX5_MKEY_MR) > + if (!mmkey || mmkey->key != lkey) { > + mr = ERR_PTR(-ENOENT); > goto end; > + } > + if (mmkey->type != MLX5_MKEY_MR) { > + mr = ERR_PTR(-EINVAL); > + goto end; > + } Can we return EINVAL in both above 2 cases so that we can attribute them to *lkey is invalid* simply. Otherwise it's hard to describe 2nd case by man page since users/developers cannot link mmkey->type to the parameters of ibv_advise_mr(). > > mr = container_of(mmkey, struct mlx5_ib_mr, mmkey); > > if (mr->ibmr.pd != pd) { > - mr = NULL; > + mr = ERR_PTR(-EPERM); EINVAL is better for compatible ? since man page said EINVAL in this case before. Thanks > goto end; > } > > /* prefetch with write-access must be supported by the MR */ > if (advice == IB_UVERBS_ADVISE_MR_ADVICE_PREFETCH_WRITE && > !mr->umem->writable) { > - mr = NULL; > + mr = ERR_PTR(-EPERM); > goto end; > } > > @@ -1753,7 +1759,7 @@ static void mlx5_ib_prefetch_mr_work(struct work_struct *w) > destroy_prefetch_work(work); > } > > -static bool init_prefetch_work(struct ib_pd *pd, > +static int init_prefetch_work(struct ib_pd *pd, > enum ib_uverbs_advise_mr_advice advice, > u32 pf_flags, struct prefetch_mr_work *work, > struct ib_sge *sg_list, u32 num_sge) > @@ -1764,17 +1770,19 @@ static bool init_prefetch_work(struct ib_pd *pd, > work->pf_flags = pf_flags; > > for (i = 0; i < num_sge; ++i) { > + struct mlx5_ib_mr *mr; > + > + mr = get_prefetchable_mr(pd, advice, sg_list[i].lkey); > + if (IS_ERR(mr)) { > + work->num_sge = i; > + return PTR_ERR(mr); > + } > work->frags[i].io_virt = sg_list[i].addr; > work->frags[i].length = sg_list[i].length; > - work->frags[i].mr = > - get_prefetchable_mr(pd, advice, sg_list[i].lkey); > - if (!work->frags[i].mr) { > - work->num_sge = i; > - return false; > - } > + work->frags[i].mr = mr; > } > work->num_sge = num_sge; > - return true; > + return 0; > } > > static int mlx5_ib_prefetch_sg_list(struct ib_pd *pd, > @@ -1790,8 +1798,8 @@ static int mlx5_ib_prefetch_sg_list(struct ib_pd *pd, > struct mlx5_ib_mr *mr; > > mr = get_prefetchable_mr(pd, advice, sg_list[i].lkey); > - if (!mr) > - return -ENOENT; > + if (IS_ERR(mr)) > + return PTR_ERR(mr); > ret = pagefault_mr(mr, sg_list[i].addr, sg_list[i].length, > &bytes_mapped, pf_flags); > if (ret < 0) { > @@ -1811,6 +1819,7 @@ int mlx5_ib_advise_mr_prefetch(struct ib_pd *pd, > { > u32 pf_flags = 0; > struct prefetch_mr_work *work; > + int rc; > > if (advice == IB_UVERBS_ADVISE_MR_ADVICE_PREFETCH) > pf_flags |= MLX5_PF_FLAGS_DOWNGRADE; > @@ -1826,9 +1835,10 @@ int mlx5_ib_advise_mr_prefetch(struct ib_pd *pd, > if (!work) > return -ENOMEM; > > - if (!init_prefetch_work(pd, advice, pf_flags, work, sg_list, num_sge)) { > + rc = init_prefetch_work(pd, advice, pf_flags, work, sg_list, num_sge); > + if (rc) { > destroy_prefetch_work(work); > - return -EINVAL; > + return rc; > } > queue_work(system_unbound_wq, &work->work); > return 0;
On Wed, Oct 13, 2021 at 07:26:49AM +0000, lizhijian@fujitsu.com wrote: > Hi Jason > > When update the ibv_advise_mr man page, i have a few concerns: > > > On 29/09/2021 01:08, Jason Gunthorpe wrote: > > On Fri, Sep 03, 2021 at 04:48:15PM +0800, Li Zhijian wrote: > >> Previously, ENOENT or EINVAL will be returned by ibv_advise_mr() although > >> the errors all occur at get_prefetchable_mr(). > > What do you think about this instead? > > > > From b739920ed4869decb02a0dbc58256e6c72ec7061 Mon Sep 17 00:00:00 2001 > > From: Jason Gunthorpe <jgg@nvidia.com> > > Date: Fri, 3 Sep 2021 16:48:15 +0800 > > Subject: [PATCH] IB/mlx5: Flow through a more detailed return code from > > get_prefetchable_mr() > > > > The error returns for various cases detected by get_prefetchable_mr() get > > confused as it flows back to userspace. Properly label each error path and > > flow the error code properly back to the system call. > > > > Suggested-by: Li Zhijian <lizhijian@cn.fujitsu.com> > > Signed-off-by: Jason Gunthorpe <jgg@nvidia.com> > > drivers/infiniband/hw/mlx5/odp.c | 40 ++++++++++++++++++++------------ > > 1 file changed, 25 insertions(+), 15 deletions(-) > > > > diff --git a/drivers/infiniband/hw/mlx5/odp.c b/drivers/infiniband/hw/mlx5/odp.c > > index d0d98e584ebcc3..77890a85fc2dd3 100644 > > +++ b/drivers/infiniband/hw/mlx5/odp.c > > @@ -1708,20 +1708,26 @@ get_prefetchable_mr(struct ib_pd *pd, enum ib_uverbs_advise_mr_advice advice, > > > > xa_lock(&dev->odp_mkeys); > > mmkey = xa_load(&dev->odp_mkeys, mlx5_base_mkey(lkey)); > > - if (!mmkey || mmkey->key != lkey || mmkey->type != MLX5_MKEY_MR) > > + if (!mmkey || mmkey->key != lkey) { > > + mr = ERR_PTR(-ENOENT); > > goto end; > > + } > > + if (mmkey->type != MLX5_MKEY_MR) { > > + mr = ERR_PTR(-EINVAL); > > + goto end; > > + } > > > Can we return EINVAL in both above 2 cases so that we can attribute > them to *lkey is invalid* simply. Otherwise it's hard to describe > 2nd case by man page since users/developers cannot link mmkey->type > to the parameters of ibv_advise_mr(). kley is valid in the 2nd case, but points to the wrong kidn of object to prefetch, hence EIVNAL. Eg it is a MW or something. > > mr = container_of(mmkey, struct mlx5_ib_mr, mmkey); > > > > if (mr->ibmr.pd != pd) { > > - mr = NULL; > > + mr = ERR_PTR(-EPERM); > > EINVAL is better for compatible ? since man page said EINVAL in this case before. Referencing a valid lkey outside the caller's security scope should be EPERM. Jason
diff --git a/drivers/infiniband/hw/mlx5/odp.c b/drivers/infiniband/hw/mlx5/odp.c index d0d98e584ebc..52572e7ea6f6 100644 --- a/drivers/infiniband/hw/mlx5/odp.c +++ b/drivers/infiniband/hw/mlx5/odp.c @@ -1828,7 +1828,7 @@ int mlx5_ib_advise_mr_prefetch(struct ib_pd *pd, if (!init_prefetch_work(pd, advice, pf_flags, work, sg_list, num_sge)) { destroy_prefetch_work(work); - return -EINVAL; + return -ENOENT; } queue_work(system_unbound_wq, &work->work); return 0;
Previously, ENOENT or EINVAL will be returned by ibv_advise_mr() although the errors all occur at get_prefetchable_mr(). flags = IBV_ADVISE_MR_FLAG_FLUSH: mlx5_ib_advise_mr_prefetch() -> mlx5_ib_prefetch_sg_list() -> get_prefetchable_mr() return -ENOENT; flags = 0: mlx5_ib_advise_mr_prefetch() -> init_prefetch_work() -> get_prefetchable_mr() return -EINVAL; Signed-off-by: Li Zhijian <lizhijian@cn.fujitsu.com> --- drivers/infiniband/hw/mlx5/odp.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)