Re: [PATCH for-next v2 1/4] RDMA/erdma: Support non-contiguous kernel QP buffers

[Cheng Xu <chengyou@linux.alibaba.com>]

On 9/18/26 8:47 PM, Leon Romanovsky wrote:
> On Fri, Sep 11, 2026 at 10:41:52AM +0800, Cheng Xu wrote:
>>
>>
>> On 9/10/26 11:33 PM, Leon Romanovsky wrote:
>>> On Wed, Sep 09, 2026 at 02:21:37PM +0800, Cheng Xu wrote:
>>>>
>>>>
>>>> On 9/3/26 8:38 PM, Cheng Xu wrote:
>>>>>
>>>>>
>>>>> On 9/3/26 5:10 PM, Leon Romanovsky wrote:
>>>>>> On Thu, Aug 27, 2026 at 04:25:20PM +0800, Cheng Xu wrote:
>>>>>>> A single coherent allocation for kernel QP queues can fail for large
>>>>>>> queues when memory is fragmented.
>>>>>>>
>>>>>>> Allocate page-sized coherent buffers and describe them with the existing
>>>>>>> MTT. Keep the userspace QP path unchanged.
>>>>>>>
>>>>>>> Signed-off-by: Cheng Xu <chengyou@linux.alibaba.com>
>>>>>>> ---
>>>>>>>  drivers/infiniband/hw/erdma/erdma_cq.c    |   4 +-
>>>>>>>  drivers/infiniband/hw/erdma/erdma_qp.c    |  38 +++--
>>>>>>>  drivers/infiniband/hw/erdma/erdma_verbs.c | 195 +++++++++++++---------
>>>>>>>  drivers/infiniband/hw/erdma/erdma_verbs.h |  44 ++++-
>>>>>>>  4 files changed, 179 insertions(+), 102 deletions(-)
>>>>>>
>>>>>> <...>
>>>>>>
>>>>
>>>> <...>
>>>>
>>>>>>
>>>>>>> +struct erdma_buf_list {
>>>>>>> +	void *buf;
>>>>>>> +	dma_addr_t dma_addr;
>>>>>>> +};
>>>>>>
>>>>>> This struct is very similar to scatter-gather list, why don't you use it
>>>>>> directly?
>>>>>
>>>>> Good idea. I will use struct scatterlist in the next revision.
>>>>
>>>> Hi Leon,
>>>>
>>>> I switched to scatterlist in v3, but Sashiko pointed out an issue with
>>>> using sg_set_buf() and sg_virt() on dma_alloc_coherent() memory [1].
>>>>
>>>> To handle this correctly, the driver would still need to retain the
>>>> original CPU addresses returned by dma_alloc_coherent(). Using scatterlist
>>>> does not simplify this implementation: we still need separate storage for
>>>> the CPU addresses, and the only scatterlist field we actually need is the
>>>> DMA address. A small structure holding both addresses would therefore be
>>>> simpler.
>>>
>>> Sashiko thinks that you are creating SG list to feed it to dma_map_sg()
>>> later which is not. You are using SG as simple database and you will get
>>> iterators for free.
>>>
>>> cpu_address = sg_page()
>>> dma_address = sg_dma_address()
>>>
>>
>> Hi Leon,
>>
>> Maybe I misunderstood something. My understanding of Sashiko's concern is
>> that the CPU address returned by dma_alloc_coherent() is not guaranteed to
>> be valid for virt_to_page(), as used by sg_set_buf(). Consequently, the
>> struct page later returned by sg_page() may be invalid.
>>
>> Although this works on the x86 platforms we tested, it may not be portable
>> to architectures where coherent memory is remapped outside the linear
>> mapping. From this perspective, would a small structure that explicitly
>> stores the CPU and DMA addresses, similar to those used by HNS and mlx5, be
>> more appropriate than scatterlist here?
> 
> Yes, let's use them directly, without SG. Sorry for the confusion in my
> previous comment.
> 

Thanks for confirming. v4 keeps the small private structure as discussed.

Thanks,
Cheng Xu

> Thanks
> 
>>
>> I collected some information related to this below [1][2][3].
>>
>> [1] include/linux/scatterlist.h:
>>
>> #ifdef CONFIG_DEBUG_SG
>> 	BUG_ON(!virt_addr_valid(buf));
>> #endif
>> sg_set_page(sg, virt_to_page(buf), buflen, offset_in_page(buf));
>>
>> [2] kernel/dma/mapping.c:
>>
>> /*
>>  * The whole dma_get_sgtable() idea is fundamentally unsafe ...
>>  * 1. Not all memory allocated via the coherent DMA APIs is backed by
>>  *    a struct page
>>  */
>>
>> [3] drivers/infiniband/hw/mthca/mthca_memfree.c:
>>
>> /* We use sg_set_buf for coherent allocs, which assumes low memory */
> 
> I wouldn't hold my breath about the validity of that comment. It was written
> a long time ago.
> 
>>
>> Thanks,
>> Cheng Xu
>>
>>
>>> Thanks
>>>
>>