From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from mails.dpdk.org (mails.dpdk.org [217.70.189.124]) by smtp.lore.kernel.org (Postfix) with ESMTP id D348AD2FEDB for ; Tue, 27 Jan 2026 19:21:52 +0000 (UTC) Received: from mails.dpdk.org (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id 0DFF040F1A; Tue, 27 Jan 2026 20:21:52 +0100 (CET) Received: from mail-wm1-f41.google.com (mail-wm1-f41.google.com [209.85.128.41]) by mails.dpdk.org (Postfix) with ESMTP id 6C68740E21 for ; Tue, 27 Jan 2026 20:21:50 +0100 (CET) Received: by mail-wm1-f41.google.com with SMTP id 5b1f17b1804b1-4806ce0f97bso1953055e9.0 for ; Tue, 27 Jan 2026 11:21:50 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=networkplumber-org.20230601.gappssmtp.com; s=20230601; t=1769541710; x=1770146510; darn=dpdk.org; h=content-transfer-encoding:mime-version:references:in-reply-to :message-id:date:subject:cc:to:from:from:to:cc:subject:date :message-id:reply-to; bh=OPAinidqnLzTK6vNOAK2AdkijO7D5R6RMDqjWjz8tJw=; b=U5V/bQq/LHVe1IptKzad+H/Qhrml0H8aNFz7pb7Wl5M8ZpBdobDD+tVz8TPZ6j9SJm 7Nx9zDS8fHeAllmTiIRN1E4NPGnshw1PjOQ6B9qKToLR5mqO248Z2Q7GQNXecY8nMSAF CWVNuBQsqSMrof59n+zncn2FUXgJKrocTi8XUpxGDRhccx7RP62GyZel86vpGEH4rhLI g/f4cZ3AVM9RqywX9aIMpnYEfdII0qBD2Gnn3pTTa+ZAUvAGUYW0pS3xMkdgegYf6U5p dq2qOgCAtVMmz9NsFq1VycZ/Onnl4SUdu9TaR7+6fR3kuSbLFXanpY1IjCdeQSqhodVF RNFQ== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1769541710; x=1770146510; h=content-transfer-encoding:mime-version:references:in-reply-to :message-id:date:subject:cc:to:from:x-gm-gg:x-gm-message-state:from :to:cc:subject:date:message-id:reply-to; bh=OPAinidqnLzTK6vNOAK2AdkijO7D5R6RMDqjWjz8tJw=; b=FLebZYD9uoSqgUuOnONSNjMkKxqw9/ESeUHi5i18vHxHLDEBbSQKTqySul2WiCMII6 JUkpHXusfGQYg9c/NgVkae7WXf+kZOVBJfKs757352++1V4j/sBvM0BWQ4Kh6ECwpahL upz6RuMZlPbZ0yW+xWRO9t1AFHw/twQ6ZI/5wS8o9IJ0zPEsE0jBnhWs5wuwY6NpbFqR xrZy20dfZxIiuOyE7N3y1WWxDGL9a3Z+0whkK+Y0/hlIBPP02yJ2GHlOXZnevN5AFEbP 28tTqi5fZYAHLluLF34DpT8LBVwXNhBmiOU5LBWSdpVjrLFRPI8KUVwkDwlapQjqqPpw jkBw== X-Gm-Message-State: AOJu0Yw/K4Ltzrsex0XqDr0X0RO9EEF/nR5feFTuDAbt/zFagumQ7ORC BspLUxSMEaqnrYPXOzX0L81PA/RdAV5zQYLj1APK8WTkqcvztzzcqDn0V6C/SkBu8/8X0yh7Cxv 59poE X-Gm-Gg: AZuq6aKCNYIYOG+11MOSUPqGTuxVhkqeuYTgUwx+GlVMN7o8SK4wbjUxZntq3XXwosY zsvFf0LUJ1wQ3HMe/S3y3PC2jz7/zlvydUct3+FbnGPMDH8nCTFN5cSsstEteg6ieatfXjDOKID 9My5olbvY5Ab8nkCrEzdPbyTeNTAstlgSPHpwzMqB4lfUgJP6qsM8BozZ7DsqO6aMUxPUdYM4Ee M7KBPVQBJ7eyBESjImpy4c6XrrhQUpXcfros47tIQx87js6hrM25EeZH2GikegEL/pDv5qLZgC7 UmJuWbzh5jiJ9OAcwWKD9qLHYWAE1xO54Enhtyxa+MiShJ/0xrUIt0CpKI5cyxVknO1uTYy/Abq aoi1FhlN7Vg3s4iXH38wAYGNdQKSSEQP8Fb5u0JNURSmSpe5beSta5AkC30qkhtNUABOuhmv33F t80oXWOx0bK+jp0FxRKvrckJnyPWNuTneavsPFc1tNnszdEhDrhQ== X-Received: by 2002:a05:600c:34d0:b0:480:683f:76e8 with SMTP id 5b1f17b1804b1-48069c7c265mr31849845e9.26.1769541709739; Tue, 27 Jan 2026 11:21:49 -0800 (PST) Received: from phoenix.lan (204-195-96-226.wavecable.com. [204.195.96.226]) by smtp.gmail.com with ESMTPSA id 5b1f17b1804b1-4806cddffc0sm14584515e9.5.2026.01.27.11.21.48 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Tue, 27 Jan 2026 11:21:49 -0800 (PST) From: Stephen Hemminger To: dev@dpdk.org Cc: Stephen Hemminger Subject: [REVIEW] common/mlx5: support dmabuf Date: Tue, 27 Jan 2026 11:21:46 -0800 Message-ID: <20260127192146.169967-1-stephen@networkplumber.org> X-Mailer: git-send-email 2.51.0 In-Reply-To: <20260127174429.1504288-3-cburdick@nvidia.com> References: <20260127174429.1504288-3-cburdick@nvidia.com> Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org AI-generated review of bundle-1701-dmabuf.mbox Reviewed using Claude (claude-opus-4-5-20251101) This is an automated review. Please verify all suggestions. --- # DPDK Patch Review: dmabuf Support ## Summary This patch series adds dmabuf (DMA buffer) support to DPDK EAL and the MLX5 driver, enabling DMA transfers between drivers without proprietary kernel modules. --- ## Patch 1/2: eal: support dmabuf ### Commit Message Issues **Warning: Subject line format** - Subject "eal: support dmabuf" is acceptable but could be more descriptive - Consider: "eal: add dmabuf external memory registration support" **Warning: Body contains questions to reviewers** The commit message contains design questions that should be resolved before submission: ``` Which option is preferred? ``` Remove these questions and state the chosen design approach clearly. **Info: Body line length** Some lines in the body exceed 75 characters but are within acceptable range. ### Code Issues **Error: Double blank line** ```c } dmabuf_info[RTE_MAX_MEMSEG_LISTS] = { [0 ... RTE_MAX_MEMSEG_LISTS - 1] = { .fd = -1, .offset = 0 } }; #define MAX_MMAP_WITH_DEFINED_ADDR_TRIES 5 ``` Remove the extra blank line after the struct initialization. **Warning: Inconsistent indentation in struct** ```c static struct { int fd; /**< dmabuf fd, -1 if not dmabuf backed */ uint64_t offset; /**< offset within dmabuf */ } dmabuf_info[RTE_MAX_MEMSEG_LISTS] = { ``` The struct members are double-indented with tabs. Should use single tab for consistency: ```c static struct { int fd; /**< dmabuf fd, -1 if not dmabuf backed */ uint64_t offset; /**< offset within dmabuf */ } dmabuf_info[RTE_MAX_MEMSEG_LISTS] = { ``` **Error: New public APIs missing `__rte_experimental`** All new public API functions in `rte_memory.h` must be marked as experimental: - `rte_memseg_list_get_dmabuf_fd()` - `rte_memseg_list_get_dmabuf_fd_thread_unsafe()` - `rte_memseg_list_get_dmabuf_offset()` - `rte_memseg_list_get_dmabuf_offset_thread_unsafe()` - `rte_extmem_register_dmabuf()` Add `__rte_experimental` on the line before each function declaration in the header: ```c __rte_experimental int rte_memseg_list_get_dmabuf_fd(const struct rte_memseg_list *msl); ``` **Warning: Missing release notes** New API functions require documentation in `doc/guides/rel_notes/release_XX_YY.rst`. **Warning: Missing version.map updates** New exported symbols need to be added to `lib/eal/version.map` under the `EXPERIMENTAL` section. **Warning: Missing testpmd hooks and functional tests** New APIs should have tests in `app/test/` and hooks in `app/testpmd` per guidelines. **Info: Doxygen style** The Doxygen comments use `dma-buf` inconsistently (sometimes `dmabuf`, sometimes `dma-buf`). Consider standardizing to one form throughout. **Warning: Variable `n` unused context** ```c n = len / page_sz; if (malloc_heap_create_external_seg_dmabuf(va_addr, iova_addrs, n, ``` The variable `n` is computed but the result from `len / page_sz` could be used directly since it's only used once. --- ## Patch 2/2: common/mlx5: support dmabuf ### Commit Message Issues **Info: Subject is acceptable** "common/mlx5: support dmabuf" follows the format guidelines. ### Code Issues **Warning: Long lines exceed 100 characters** Several lines in `mlx5_common.c` and `mlx5_common_mr.c` exceed the 100-character limit: ```c mr = mlx5_create_mr_ext_dmabuf(dev->pd, (uintptr_t)addr, len, SOCKET_ID_ANY, dmabuf_fd, dmabuf_off, dev->mr_scache.reg_dmabuf_mr_cb); ``` Line 1149: `mr = mlx5_create_mr_ext_dmabuf(dev->pd, (uintptr_t)addr, len,` - OK But the continuation could be better formatted. In `mlx5_common_mr.c`: ```c int dmabuf_fd = rte_memseg_list_get_dmabuf_fd_thread_unsafe(msl); ``` This line likely exceeds 100 characters. Break it up: ```c int dmabuf_fd; dmabuf_fd = rte_memseg_list_get_dmabuf_fd_thread_unsafe(msl); ``` **Warning: Missing `__rte_internal` attribute** The function `mlx5_create_mr_ext_dmabuf()` is declared in `mlx5_common_mr.h` but lacks `__rte_internal`: ```c struct mlx5_mr * mlx5_create_mr_ext_dmabuf(void *pd, uintptr_t addr, size_t len, int socket_id, int dmabuf_fd, uint64_t dmabuf_offset, mlx5_reg_dmabuf_mr_t reg_dmabuf_mr_cb); ``` If this is internal to the driver, add `__rte_internal` on the line before. **Error: Missing RTE_EXPORT_INTERNAL_SYMBOL for mlx5_create_mr_ext_dmabuf** The function `mlx5_create_mr_ext_dmabuf()` in `mlx5_common_mr.c` is missing the export macro if it's meant to be used by other mlx5 components. **Warning: Brace style inconsistency** ```c if (dmabuf_fd >= 0) { uint64_t dmabuf_off; ``` This is fine, but ensure consistency throughout. **Info: Code duplication** The dmabuf detection and registration logic is duplicated between `mlx5_common_dev_dma_map()` and `mlx5_mr_mempool_register_primary()`. Consider extracting to a helper function. **Warning: Potential NULL dereference** In `mlx5_common.c`: ```c msl = rte_mem_virt2memseg_list(addr); if (msl != NULL && msl->external) { int dmabuf_fd = rte_memseg_list_get_dmabuf_fd_thread_unsafe(msl); ``` The `rte_memseg_list_get_dmabuf_fd_thread_unsafe()` call happens without holding the memory lock, but the comment in the header says it's only safe within memory-related callbacks. This may be a race condition if memory configuration changes. **Warning: Missing error check** ```c rte_memseg_list_get_dmabuf_offset_thread_unsafe(msl, &dmabuf_off); ``` The return value is not checked. If the function fails, `dmabuf_off` may be uninitialized. **Info: Windows stub** ```c *reg_dmabuf_mr_cb = NULL; /* dma-buf not supported on Windows */ ``` Good practice to document platform limitations. --- ## General Issues **Warning: Missing documentation** - No updates to programmer's guide documentation explaining dmabuf usage - No updates to release notes **Warning: ABI considerations** The patch mentions avoiding ABI changes by using a side-table for dmabuf info. This design decision should be documented more clearly, and the commit message questions should be resolved. **Info: Linux-only feature** The dmabuf feature is Linux-specific. Consider adding explicit documentation about platform support. --- ## Checklist Summary | Item | Status | |------|--------| | SPDX license | ✓ OK (existing files) | | Signed-off-by | ✓ Present | | Subject line length | ✓ OK | | Subject format | ✓ OK | | Body line wrap | ⚠ Some lines long | | `__rte_experimental` on new APIs | ✗ Missing | | Release notes | ✗ Missing | | version.map updates | ✗ Missing | | Tests | ✗ Missing | | Documentation | ✗ Missing | | Code style | ⚠ Minor issues |