Re: [PATCH v4 09/14] nvme-pci: add SGL support for the dmabuf path

[Christoph Hellwig <hch@lst.de>]

On Tue, Jul 28, 2026 at 10:29:21PM +0100, Pavel Begunkov wrote:
> From: Anuj Gupta <anuj20.g@samsung.com>
> 
> Add SGL support in addition to PRP for dmabuf-backed requests,
> coalescing the mapping's sg_table into NVMe SGL data descriptors.
> 
> Signed-off-by: Anuj Gupta <anuj20.g@samsung.com>
> [pavel: rebased]
> Signed-off-by: Pavel Begunkov <asml.silence@gmail.com>
> ---
>  drivers/nvme/host/pci.c | 191 +++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 187 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/nvme/host/pci.c b/drivers/nvme/host/pci.c
> index f1b67c191892..cbb321fb7c50 100644
> --- a/drivers/nvme/host/pci.c
> +++ b/drivers/nvme/host/pci.c
> @@ -1287,12 +1287,18 @@ static blk_status_t nvme_pci_setup_data_prp(struct request *req,
>  	return BLK_STS_IOERR;
>  }
>  
> +static void nvme_pci_sgl_set_data_addr(struct nvme_sgl_desc *sge,
> +				       dma_addr_t addr, u32 len)
> +{
> +	sge->addr = cpu_to_le64(addr);
> +	sge->length = cpu_to_le32(len);
> +	sge->type = NVME_SGL_FMT_DATA_DESC << 4;
> +}
> +
>  static void nvme_pci_sgl_set_data(struct nvme_sgl_desc *sge,
>  		struct blk_dma_iter *iter)
>  {
> -	sge->addr = cpu_to_le64(iter->addr);
> -	sge->length = cpu_to_le32(iter->len);
> -	sge->type = NVME_SGL_FMT_DATA_DESC << 4;
> +	nvme_pci_sgl_set_data_addr(sge, iter->addr, iter->len);
>  }

The naming is a bit confusing (and me passing the iter to
nvme_pci_sgl_set_data is probably at faul for that).  So maybe
spin out a prep patch to rename the old nvme_pci_sgl_set_data
to nvme_pci_dma_iter_set_sgl or so, and then add the new one
as nvme_pci_sgl_set_data (as before the dma_iter conversion).

>
> +static unsigned int nvme_pci_dmabuf_sgl_nents(struct request *req,
> +					      dma_addr_t *first_dma,
> +					      u32 *first_len)

This is a really good example why the aligning to the opening braces
produces totally unreadble code..

But I also don't understand what the use case for this function
is to start with.  struct sg_table tells us how many segments
exist on the DMA side in the nents member, which should be just
fine for the SGL threshold calculation.

> +{
> +	struct nvme_iod *iod = blk_mq_rq_to_pdu(req);
> +	struct bio *bio = req->bio;
> +	struct nvme_dmabuf_map *map = to_nvme_dmabuf_map(bio->bi_dmabuf_map);
> +	size_t length = blk_rq_payload_bytes(req);
> +	struct nvme_sgl_desc *sg_list = NULL;
> +	dma_addr_t sgl_dma = 0, last_end = 0;
> +	unsigned int mapped = 0;
> +	unsigned long tmp;
> +	struct scatterlist *sg;
> +	size_t offset, remaining;
> +	bool have = false;
> +
> +	if (!entries)
> +		return BLK_STS_IOERR;
> +	if (entries > NVME_MAX_SEGS)
> +		return BLK_STS_AGAIN;

Given that the block layer enforced data in rw/command and the
max_segments limit, why do we need the extra check here?

> +			continue;
> +		}
> +
> +		addr += offset;
> +		sg_len -= offset;
> +		offset = 0;
> +
> +		while (sg_len && remaining) {

These can't be false on the first iteration, so maybe turn this into
a do {} while loop?

> +			u32 chunk = min_t(size_t, remaining, sg_len);
> +
> +			if (have && last_end == addr) {
> +				u32 old = le32_to_cpu(sg_list[mapped - 1].length);
> +
> +				sg_list[mapped - 1].length = cpu_to_le32(old + chunk);

Overly long line.

> +			} else {
> +				if (WARN_ON_ONCE(mapped == entries))
> +					goto err_free;
> +				nvme_pci_sgl_set_data_addr(&sg_list[mapped++],
> +							   addr, chunk);
> +			}

Why do we need this merging?  dma_map_sg should have already done
any interesting merging, or am I missing something?

> +		if (use_sgl != SGL_UNSUPPORTED) {
> +			dma_addr_t first_dma;
> +			u32 first_len;
> +			unsigned int entries;
> +
> +			entries = nvme_pci_dmabuf_sgl_nents(req, &first_dma,
> +							    &first_len);
> +
> +			if (use_sgl == SGL_FORCED) {
> +				ret = nvme_rq_setup_dmabuf_sgl(req, nvmeq,
> +								entries, first_dma, first_len);
> +				return ret == BLK_STS_AGAIN ? BLK_STS_IOERR : ret;
> +			}
> +
> +			if (sgl_threshold && entries &&
> +			    DIV_ROUND_UP(blk_rq_payload_bytes(req), entries) >=
> +			    sgl_threshold) {
> +				ret = nvme_rq_setup_dmabuf_sgl(req, nvmeq,
> +								entries, first_dma, first_len);
> +				if (ret != BLK_STS_AGAIN)
> +					return ret;
> +			}
> +		}

Various overly long lines.  Please factor out a helper for the
decisions to use sgl vs not instead of open coding it here.