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

[Leon Romanovsky <leon@kernel.org>]

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

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