Repository navigation
[26.04_linux-nvidia] NVIDIA: SAUCE: nvme-rdma: size NVFS SGL for GPU pages - #634
sourabgupta3 wants to merge 2 commits into
Conversation
sourabgupta3
changed the title
BaseOS Kernel ReviewWarning
|
PR Validation ReportPatchscan ✅ No Missing FixesAll cherry-picked commits checked — no missing upstream fixes found. PR Lint ✅ All checks passedDetailsChecking 2 commits... Cherry-pick digest: ┌──────────────┬──────────────────────────────────────────────────────────────────┬────────────┬─────────┬───────────────────────────┐ │ Local │ Referenced upstream / Patch subject │ Patch-ID │ Subject │ SoB chain │ ├──────────────┼──────────────────────────────────────────────────────────────────┼────────────┼─────────┼───────────────────────────┤ │ c2b29e13246d │ [SAUCE] nvme-rdma: size nvfs sgl per bio │ N/A │ N/A │ sougupta │ ├──────────────┼──────────────────────────────────────────────────────────────────┼────────────┼─────────┼───────────────────────────┤ │ ca0c528404d0 │ [SAUCE] nvme-rdma: size nvfs sgl for gpu pages │ N/A │ N/A │ sougupta │ └──────────────┴──────────────────────────────────────────────────────────────────┴────────────┴─────────┴───────────────────────────┘ Lint: all checks passed. |
|
These are my codex findings: [P1] NV-Kernels/drivers/nvme/host/nvfs-rdma.h:69 First-page classification can misroute mixed CPU/GPU requests to the CPU DMA path. When the first bvec is not a GPU page, the code returns before calling the NVFS mapper. The block layer can merge adjacent bios without knowing this memory-type distinction, so a CPU-first request containing later GPU pages would skip NVFS validation/mapping and go through blk_rq_map_sg() plus ib_dma_map_sg(). That risks DMA against GPU proxy pages as if they were normal CPU memory. If mixed requests are intended to be impossible, this needs an explicit merge barrier or invariant; otherwise scan all request segments for any GPU page before deciding to bypass NVFS. |
|
In addition to the findings left by Carol (Codex) and the PR validation report: 1. NULL dereference on discard with nvidia-fs loaded (
2. CPU I/O gets the GPU-sized table when FT_GPU_PAGE is not advertised ( Without Minor: |
ccdf6f0 to
b086492
Compare
NVMe/RDMA sizes the request scatterlist from the block layer physical segment count. For GDS I/O, contiguous proxy pages do not guarantee that the corresponding 64K GPU pages are physically contiguous, so the NVFS mapper can require more entries than the block layer reports. Use the first request page only to select the allocation size. GPU requests receive enough entries for each 64K GPU page plus a possible boundary crossing, while CPU requests retain the normal physical segment count. All eligible requests still pass through the NVFS mapper so mixed CPU/GPU requests are rejected. Keep integrity, special-payload, and non-read/write requests on the normal block mapping path before inspecting request segments. This ensures data-less discard bios use their NVMe DSM special payload. Signed-off-by: Sourab Gupta <sougupta@nvidia.com>
b086492 to
ca0c528
Compare
|
Re-reviewed ca0c528. The discard and mixed CPU/GPU issues are fixed. Carol's first P1 is still open. |
|
Like Jamie said there still some pending. Sparse GPU SGL sizing: not addressed. PR head ca0c528 still sizes GPU-capable requests with DIV_ROUND_UP(blk_rq_payload_bytes(rq), 64K) + 1 in drivers/nvme/host/nvfs-rdma.h:95-97. That is still a dense-range bound, so the sparse case from the comment can still need more NVFS SGEs than allocated. The callback still gets only sglist, not a capacity, and the NVFS mapper can create a new SGE for each non-contiguous GPU physical page. |
|
Yes, that is valid feedback but the possibilty of such a scenario happening is very remote for GDS where a sparse file needs is read and the physical addresses on the GPU are also completely non-contiguous. In such scenarios, we if the SG list is underallocated, then we can run into this. Since we have never run into such a situation today as well with the buggy NVMeoF driver as well(current one that already underallocates), I think a conservative solution would be in nvidia-fs to just fail such IOs, rather than corrupting the memory(working on it currently). |
|
It does seem unlikely, but the result is serious: a silent memory corruption. Fixing it in nvidia-fs alone would need an interface change, because the mapper isn't given the table size. Claude suggests the following kernel-side change: +/*
+ * nvidia-fs issues each I/O from a single iovec, so every bio covers one
+ * contiguous range of a GPU buffer and maps to at most one entry per 64K
+ * GPU page it touches, plus one when it starts inside a GPU page. A request
+ * can merge bios from unrelated ranges, so bound each bio separately.
+ */
+static unsigned int nvme_rdma_nvfs_gpu_nr_sg(struct request *rq)
+{
+ unsigned int nr_sg = 0;
+ struct bio *bio;
+
+ __rq_for_each_bio(bio, rq)
+ nr_sg += DIV_ROUND_UP(bio->bi_iter.bi_size,
+ NVFS_GPU_PAGE_SIZE) + 1;
+
+ return nr_sg;
+}
+
static int nvme_rdma_nvfs_map_data(struct ib_device *ibdev,
struct request *rq, bool *is_nvfs_io,
bool *sg_allocated, int *count)
{
struct nvme_rdma_request *req = blk_mq_rq_to_pdu(rq);
enum dma_data_direction dma_dir = rq_dma_dir(rq);
- unsigned int nr_sg, gpu_nr_sg;
+ unsigned int nr_sg;
int ret = 0;
@@
/*
* The block layer may merge physically contiguous proxy pages
* into fewer segments. GPU physical pages need not have the
- * same contiguity. Allow one entry per 64K GPU page, plus one
- * for a request that starts at an offset within a GPU page.
+ * same contiguity.
*/
- gpu_nr_sg = DIV_ROUND_UP(blk_rq_payload_bytes(rq),
- NVFS_GPU_PAGE_SIZE);
- nr_sg = max_t(unsigned int, nr_sg, gpu_nr_sg + 1);
+ nr_sg = max(nr_sg, nvme_rdma_nvfs_gpu_nr_sg(rq));
}For a single-bio request this gives the same count as today, so the common path is unchanged. Build-tested on arm64 with W=1 only. |
nvidia-fs submits each I/O from one iovec, but the block layer can merge bios from separate GPU ranges into one request. A request-wide payload bound loses the independent 64K alignment of each bio and can underallocate the NVFS scatterlist. Compute the GPU-page upper bound for every bio and sum the results. Continue taking the maximum with blk_rq_nr_phys_segments(), preserving the existing CPU and single-bio paths. Signed-off-by: Sourab Gupta <sougupta@nvidia.com>
|
|
|
|
nirmoy
added
has_2_acks
and removed
help wanted
|
Carol, here is the PR for bos branch: #642 |
|
Applied to canonical-resolute
Closing. |
4 participants
NVMe/RDMA sizes the request scatterlist from the block layer physical segment count. For GDS I/O, contiguous proxy pages do not guarantee that the corresponding 64K GPU pages are physically contiguous, so the NVFS mapper can require more entries than the block layer reports.
Classify requests using the first page and, for GPU I/O, allocate enough entries for each 64K GPU page plus a possible boundary crossing. Track whether NVFS allocated the table so CPU requests continue to use the existing allocation and mapping path without a second allocation.
LP: https://bugs.launchpad.net/ubuntu/+source/linux-nvidia-bos/+bug/2170268