On Fri, Oct 16, 2020 at 06:40:01AM +0000, Xiong, Jianxin wrote:
> > > + if (!mr)
> > > + return -EINVAL;
> > > +
> > > + return mlx5_ib_update_xlt(mr, 0, mr->npages, PAGE_SHIFT, flags); }
> > > +
> > > +static struct ib_umem_dmabuf_ops mlx5_ib_umem_dmabuf_ops = {
> > > + .init = mlx5_ib_umem_dmabuf_xlt_init,
> > > + .update = mlx5_ib_umem_dmabuf_xlt_update,
> > > + .invalidate = mlx5_ib_umem_dmabuf_xlt_invalidate,
> > > +};
> >
> > I'm not really convinced these should be ops, this is usually a bad design pattern.
> >
> > Why do I need so much code to extract the sgl from the dma_buf? I would prefer the dma_buf layer simplify this, not by adding a wrapper
> > around it in the IB core code...
> >
>
> We just need a way to call a device specific function to update the NIC's translation
> table. I considered three ways: (1) ops registered with ib_umem_get_dmabuf;
> (2) a single function pointer registered with ib_umem_get_dmabuf; (3) a method
> in 'struct ib_device'. Option (1) was chosen here with no strong reason. We could
> consolidate the three functions of the ops into one, but then we will need to
> define commands or flags for different update operations.
I'd rather the driver directly provide the dma_buf ops.. Inserting
layers that do nothing be call other layers is usually a bad idea. I
didn't look carefully yet at how that would be arranged.
> > > + ncont = npages;
> > > + order = ilog2(roundup_pow_of_two(ncont));
> >
> > We still need to deal with contiguity here, this ncont/npages is just obfuscation.
>
> Since the pages can move, we can't take advantage of contiguity here. This handling
> is similar to the ODP case. The variables 'ncont' and 'page_shift' here are not necessary.
> They are kept just for the sake of signifying the semantics of the following functions that
> use them.
Well, in this case we can manage it, and the performance boost is high
enough we need to. The work on mlx5 to do it is a bit inovlved though.
> > > + err = ib_umem_dmabuf_init_mapping(umem, mr);
> > > + if (err) {
> > > + dereg_mr(dev, mr);
> > > + return ERR_PTR(err);
> > > + }
> >
> > Did you test the page fault path at all? Looks like some xarray code is missing here, and this is also missing the related complex teardown
> > logic.
> >
> > Does this mean you didn't test the pagefault_dmabuf_mr() at all?
>
> Thanks for the hint. I was unable to get the test runs reaching the
> pagefault_dmabuf_mr() function. Now I have found the reason. Along
> the path of all the page fault handlers, the array "odp_mkeys" is checked
> against the mr key. Since the dmabuf mr keys are not in the list the
> handler is never called.
>
> On the other hand, it seems that pagefault_dmabuf_mr() is not needed at all.
> The pagefault is gracefully handled by retrying until the work thread finished
> programming the NIC.
This is a bug of some kind, pagefaults that can't find a mkey in the
xarray should cause completion with error.
Jason
