Re: [PATCH for-next v2] RDMA/rxe: Remove redundant page presence check

[Zhu Yanjun <yanjun.zhu@linux.dev>]

在 2025/6/8 13:23, Zhu Yanjun 写道:
> 在 2025/6/8 11:59, Daisuke Matsuda 写道:
>> hmm_pfn_to_page() does not return NULL. ib_umem_odp_map_dma_and_lock()
>> should return an error in case the target pages cannot be mapped until
>> timeout, so these checks can safely be removed.
>>
>> Signed-off-by: Daisuke Matsuda <dskmtsd@gmail.com>
>> ---
>>   drivers/infiniband/sw/rxe/rxe_odp.c | 13 +------------
>>   1 file changed, 1 insertion(+), 12 deletions(-)
>>
>> diff --git a/drivers/infiniband/sw/rxe/rxe_odp.c b/drivers/infiniband/ 
>> sw/rxe/rxe_odp.c
>> index dbc5a5600eb7..02841346e30c 100644
>> --- a/drivers/infiniband/sw/rxe/rxe_odp.c
>> +++ b/drivers/infiniband/sw/rxe/rxe_odp.c
>> @@ -203,8 +203,6 @@ static int __rxe_odp_mr_copy(struct rxe_mr *mr, 
>> u64 iova, void *addr,
>>           page = hmm_pfn_to_page(umem_odp->map.pfn_list[idx]);
>>           user_va = kmap_local_page(page);
>> -        if (!user_va)
>> -            return -EFAULT;
>>           src = (dir == RXE_TO_MR_OBJ) ? addr : user_va;
>>           dest = (dir == RXE_TO_MR_OBJ) ? user_va : addr;
>> @@ -286,8 +284,6 @@ static enum resp_states 
>> rxe_odp_do_atomic_op(struct rxe_mr *mr, u64 iova,
>>       idx = rxe_odp_iova_to_index(umem_odp, iova);
>>       page_offset = rxe_odp_iova_to_page_offset(umem_odp, iova);
>>       page = hmm_pfn_to_page(umem_odp->map.pfn_list[idx]);
> 
> The function hmm_pfn_to_page will finally be "(mem_map + ((pfn) - 
> ARCH_PFN_OFFSET))"
> 
> The procedure is as below:
> 
> hmm_pfn_to_page -- > pfn_to_page -- > __pfn_to_page -- > (mem_map + 
> ((pfn) - ARCH_PFN_OFFSET))
> 
> Thus, I am fine with it.
> 
>> -    if (!page)
>> -        return RESPST_ERR_RKEY_VIOLATION;
>>       if (unlikely(page_offset & 0x7)) {
> 
> Normally page_offset error handler should be after this line 
> "page_offset = rxe_odp_iova_to_page_offset(umem_odp, iova);"
> 
> Why is this error handler after hmm_pfn_to_page?
> 
>>           rxe_dbg_mr(mr, "iova not aligned\n");
>> @@ -352,10 +348,6 @@ int rxe_odp_flush_pmem_iova(struct rxe_mr *mr, 
>> u64 iova,
>>           page_offset = rxe_odp_iova_to_page_offset(umem_odp, iova);
>>           page = hmm_pfn_to_page(umem_odp->map.pfn_list[index]);
>> -        if (!page) {
>> -            mutex_unlock(&umem_odp->umem_mutex);
>> -            return -EFAULT;
>> -        }
>>           bytes = min_t(unsigned int, length,
>>                     mr_page_size(mr) - page_offset);
>> @@ -398,10 +390,7 @@ enum resp_states rxe_odp_do_atomic_write(struct 
>> rxe_mr *mr, u64 iova, u64 value)
>>       page_offset = rxe_odp_iova_to_page_offset(umem_odp, iova);
>>       index = rxe_odp_iova_to_index(umem_odp, iova);
>>       page = hmm_pfn_to_page(umem_odp->map.pfn_list[index]);
>> -    if (!page) {
>> -        mutex_unlock(&umem_odp->umem_mutex);
>> -        return RESPST_ERR_RKEY_VIOLATION;
>> -    }
>> +
>>       /* See IBA A19.4.2 */
>>       if (unlikely(page_offset & 0x7)) {
> 
> Ditto, page_offset error handler is not after the line "page_offset = 
> rxe_odp_iova_to_page_offset(umem_odp, iova);" ?

Other than that, I am fine with this commit.
Thanks,

Reviewed-by: Zhu Yanjun <yanjun.zhu@linux.dev>

Zhu Yanjun

> 
> Thanks
> Yanjun.Zhu
> 
>>           mutex_unlock(&umem_odp->umem_mutex);
> 

-- 
Best Regards,
Yanjun.Zhu