On Mon, Feb 04, 2019 at 11:45:20PM +0000, Jason Gunthorpe wrote:
> On Mon, Feb 04, 2019 at 10:13:48AM +0200, Leon Romanovsky wrote:
> > From: Moni Shoua <monis@xxxxxxxxxxxx>
> >
> > When prefetching odp mr it is required to verify that pd of the mr is
> > identical to the pd for which the advise_mr request arrived with.
> >
> > This check was missing from synchronous flow and is added now.
> >
> > Fixes: 813e90b1aeaa ("IB/mlx5: Add advise_mr() support")
> > Reported-by: Parav Pandit <parav@xxxxxxxxxxxx>
> > Signed-off-by: Moni Shoua <monis@xxxxxxxxxxxx>
> > Signed-off-by: Leon Romanovsky <leonro@xxxxxxxxxxxx>
> > drivers/infiniband/hw/mlx5/odp.c | 43 +++++++++++++++++++-------------
> > 1 file changed, 26 insertions(+), 17 deletions(-)
> >
> > diff --git a/drivers/infiniband/hw/mlx5/odp.c b/drivers/infiniband/hw/mlx5/odp.c
> > index 977d8f5d1e0f..b520178272bd 100644
> > +++ b/drivers/infiniband/hw/mlx5/odp.c
> > @@ -737,7 +737,8 @@ static int get_indirect_num_descs(struct mlx5_core_mkey *mmkey)
> > * -EFAULT when there's an error mapping the requested pages. The caller will
> > * abort the page fault handling.
> > */
> > -static int pagefault_single_data_segment(struct mlx5_ib_dev *dev, u32 key,
> > +static int pagefault_single_data_segment(struct mlx5_ib_dev *dev,
> > + struct ib_pd *pd, u32 key,
> > u64 io_virt, size_t bcnt,
> > u32 *bytes_committed,
> > u32 *bytes_mapped, u32 flags)
> > @@ -787,9 +788,15 @@ static int pagefault_single_data_segment(struct mlx5_ib_dev *dev, u32 key,
> > goto srcu_unlock;
> > }
> >
> > - if (prefetch && !is_odp_mr(mr)) {
> > - ret = -EINVAL;
> > - goto srcu_unlock;
> > + if (prefetch) {
> > + if (!is_odp_mr(mr) ||
> > + mr->ibmr.pd != pd) {
> > + mlx5_ib_dbg(dev, "Invalid prefetch request: %s\n",
> > + is_odp_mr(mr) ? "MR is not ODP" :
> > + "PD is not of the MR");
> > + ret = -EINVAL;
> > + goto srcu_unlock;
> > + }
> > }
> >
> > if (!is_odp_mr(mr)) {
> > @@ -972,7 +979,8 @@ static int pagefault_data_segments(struct mlx5_ib_dev *dev,
> > continue;
> > }
> >
> > - ret = pagefault_single_data_segment(dev, key, io_virt, bcnt,
> > + ret = pagefault_single_data_segment(dev, NULL, key,
> > + io_virt, bcnt,
> > &pfault->bytes_committed,
> > bytes_mapped, 0);
> > if (ret < 0)
> > @@ -1339,7 +1347,7 @@ static void mlx5_ib_mr_rdma_pfault_handler(struct mlx5_ib_dev *dev,
> > prefetch_len = min(MAX_PREFETCH_LEN, prefetch_len);
> > }
> >
> > - ret = pagefault_single_data_segment(dev, rkey, address, length,
> > + ret = pagefault_single_data_segment(dev, NULL, rkey, address, length,
> > &pfault->bytes_committed, NULL,
> > 0);
> > if (ret == -EAGAIN) {
> > @@ -1366,7 +1374,7 @@ static void mlx5_ib_mr_rdma_pfault_handler(struct mlx5_ib_dev *dev,
> > if (prefetch_activated) {
> > u32 bytes_committed = 0;
> >
> > - ret = pagefault_single_data_segment(dev, rkey, address,
> > + ret = pagefault_single_data_segment(dev, NULL, rkey, address,
> > prefetch_len,
> > &bytes_committed, NULL,
> > 0);
> > @@ -1663,23 +1671,24 @@ int mlx5_ib_odp_init(void)
> >
> > struct prefetch_mr_work {
> > struct work_struct work;
> > - struct mlx5_ib_dev *dev;
> > + struct ib_pd *pd;
> > u32 pf_flags;
> > u32 num_sge;
> > struct ib_sge sg_list[0];
> > };
> >
> > -static int mlx5_ib_prefetch_sg_list(struct mlx5_ib_dev *dev, u32 pf_flags,
> > +static int mlx5_ib_prefetch_sg_list(struct ib_pd *pd, u32 pf_flags,
> > struct ib_sge *sg_list, u32 num_sge)
> > {
> > int i;
> > int ret = 0;
> > + struct mlx5_ib_dev *dev = to_mdev(pd->device);
> >
> > for (i = 0; i < num_sge; ++i) {
> > struct ib_sge *sg = &sg_list[i];
> > int bytes_committed = 0;
> >
> > - ret = pagefault_single_data_segment(dev, sg->lkey, sg->addr,
> > + ret = pagefault_single_data_segment(dev, pd, sg->lkey, sg->addr,
> > sg->length,
> > &bytes_committed, NULL,
> > pf_flags);
> > @@ -1715,9 +1724,9 @@ static void mlx5_ib_prefetch_mr_work(struct work_struct *work)
> > struct prefetch_mr_work *w =
> > container_of(work, struct prefetch_mr_work, work);
> >
> > - if (w->dev->ib_dev.reg_state == IB_DEV_REGISTERED)
> > - mlx5_ib_prefetch_sg_list(w->dev, w->pf_flags, w->sg_list,
> > - w->num_sge);
> > + if (w->pd->device->reg_state == IB_DEV_REGISTERED)
> > + mlx5_ib_prefetch_sg_list(w->pd, w->pf_flags,
> > + w->sg_list, w->num_sge);
> >
> > kfree(w);
> > }
> > @@ -1738,10 +1747,10 @@ int mlx5_ib_advise_mr_prefetch(struct ib_pd *pd,
> > pf_flags |= MLX5_PF_FLAGS_DOWNGRADE;
> >
> > if (flags & IB_UVERBS_ADVISE_MR_FLAG_FLUSH)
> > - return mlx5_ib_prefetch_sg_list(dev, pf_flags, sg_list,
> > - num_sge);
> > + return mlx5_ib_prefetch_sg_list(pd, pf_flags,
> > + sg_list, num_sge);
> >
> > - if (dev->ib_dev.reg_state != IB_DEV_REGISTERED)
> > + if (pd->device->reg_state != IB_DEV_REGISTERED)
> > return -ENODEV;
>
> This needs to be rebased onto a rc5 merge'd tree.
I see that you updated rdma/wip/jgg-for-next to be based on -rc5, which
wasn't the case during submission. I'll resend.
>
> > work = kvzalloc(struct_size(work, sg_list, num_sge), GFP_KERNEL);
> > @@ -1750,7 +1759,7 @@ int mlx5_ib_advise_mr_prefetch(struct ib_pd *pd,
> >
> > memcpy(work->sg_list, sg_list, num_sge * sizeof(struct ib_sge));
> >
> > - work->dev = dev;
> > + work->pd = pd;
>
> This deserves a comment that it is safe to have this pointer because
> MR destruction flushes the WQ, and the PD can't be destroyed while it
> has MRs
Sure
>
> Jason
Attachment:
signature.asc
Description: PGP signature
