Re: [PATCH RFC V2 0/6] rxe_map_mr_sg() fix cleanup and refactor

[Greg Sword]

On Mon, Nov 6, 2023 at 4:01 PM Zhijian Li (Fujitsu)
<lizhijian@xxxxxxxxxxx> wrote:
>
>
>
> Very thanks for all your feedback.
>
> On 03/11/2023 17:55, Li Zhijian wrote:
> > I don't collect the Reviewed-by to the patch1-2 this time, since i
> > think we can make it better.
> >
> > Patch1-2: Fix kernel panic[1] and benifit to make srp work again.
> >            Almost nothing change from V1.
>
> Quote from Jason:
> "
> > The concept was that the xarray could store anything larger than
> > PAGE_SIZE and the entry would point at the first struct page of the
> > contiguous chunk
> >
> > That looks like it is right, or at least close to right, so lets try
> > to keep it
> "
>
>
> It seems it's okay to access address/memory across pages on RXE even though
> we only map the first page.

Do you really make tests in your test environment? Do you have test environment?
Do you really reproduce this problem in your test environment?
Your patches do not work actually. Please do not send these rubbish patches out.

>
> That also means PAGE_SIZE aligned MR is already supported, so only check
> `if (IS_ALIGNED(page_size, PAGE_SIZE))` is sufficient, right?
>
> diff --git a/drivers/infiniband/sw/rxe/rxe_mr.c b/drivers/infiniband/sw/rxe/rxe_mr.c
> index f54042e9aeb2..3755e530e6dc 100644
> --- a/drivers/infiniband/sw/rxe/rxe_mr.c
> +++ b/drivers/infiniband/sw/rxe/rxe_mr.c
> @@ -234,6 +234,12 @@ int rxe_map_mr_sg(struct ib_mr *ibmr, struct scatterlist *sgl,
>          struct rxe_mr *mr = to_rmr(ibmr);
>          unsigned int page_size = mr_page_size(mr);
>
> +       if (!IS_ALIGNED(page_size, PAGE_SIZE)) {
> +               rxe_err_mr(mr, "FIXME...\n")
> +               return -EINVAL;
> +       }
> +
>          mr->nbuf = 0;
>          mr->page_shift = ilog2(page_size);
>          mr->page_mask = ~((u64)page_size - 1);
> diff --git a/drivers/infiniband/sw/rxe/rxe_param.h b/drivers/infiniband/sw/rxe/rxe_param.h
> index d2f57ead78ad..b1cf1e1c0ce1 100644
> --- a/drivers/infiniband/sw/rxe/rxe_param.h
> +++ b/drivers/infiniband/sw/rxe/rxe_param.h
> @@ -38,7 +38,7 @@ static inline enum ib_mtu eth_mtu_int_to_enum(int mtu)
>   /* default/initial rxe device parameter settings */
>   enum rxe_device_param {
>          RXE_MAX_MR_SIZE                 = -1ull,
> -       RXE_PAGE_SIZE_CAP               = 0xfffff000,
> +       RXE_PAGE_SIZE_CAP               = 0xffffffff - (PAGE_SIZE - 1),
>          RXE_MAX_QP_WR                   = DEFAULT_MAX_VALUE,
>          RXE_DEVICE_CAP_FLAGS            = IB_DEVICE_BAD_PKEY_CNTR
>                                          | IB_DEVICE_BAD_QKEY_CNTR
>
>
> * minor cleanup will be done after this.
>
> Thanks
> Zhijian
>
> > Patch3-5: cleanups # newly add
> > Patch6: make RXE support PAGE_SIZE aligned mr # newly add, but not fully tested
> >
> > My bad arm64 mechine offten hangs when doing blktests even though i use the
> > default siw driver.
> >
> > - nvme and ULPs(rtrs, iser) always registers 4K mr still don't supported yet.
> >
> > [1] https://lore.kernel.org/all/CAHj4cs9XRqE25jyVw9rj9YugffLn5+f=1znaBEnu1usLOciD+g@xxxxxxxxxxxxxx/T/
> >
> > Li Zhijian (6):
> >    RDMA/rxe: RDMA/rxe: don't allow registering !PAGE_SIZE mr
> >    RDMA/rxe: set RXE_PAGE_SIZE_CAP to PAGE_SIZE
> >    RDMA/rxe: remove unused rxe_mr.page_shift
> >    RDMA/rxe: Use PAGE_SIZE and PAGE_SHIFT to extract address from
> >      page_list
> >    RDMA/rxe: cleanup rxe_mr.{page_size,page_shift}
> >    RDMA/rxe: Support PAGE_SIZE aligned MR
> >
> >   drivers/infiniband/sw/rxe/rxe_mr.c    | 80 ++++++++++++++++-----------
> >   drivers/infiniband/sw/rxe/rxe_param.h |  2 +-
> >   drivers/infiniband/sw/rxe/rxe_verbs.h |  9 ---
> >   3 files changed, 48 insertions(+), 43 deletions(-)
> >