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

[Zhijian Li (Fujitsu)]

On 06/11/2023 17:35, Greg Sword wrote:
> 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?
I did the test, the kernel panic[1] is gone after patch1-patch2


Thanks
Zhijian


> 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(-)
>>>