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 CE1A8D2FED9 for ; Tue, 27 Jan 2026 20:28:49 +0000 (UTC) Received: from mails.dpdk.org (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id 9911840F1A; Tue, 27 Jan 2026 21:28:48 +0100 (CET) Received: from mail-wm1-f54.google.com (mail-wm1-f54.google.com [209.85.128.54]) by mails.dpdk.org (Postfix) with ESMTP id 61C8940E21 for ; Tue, 27 Jan 2026 21:28:47 +0100 (CET) Received: by mail-wm1-f54.google.com with SMTP id 5b1f17b1804b1-47edffe5540so70368585e9.0 for ; Tue, 27 Jan 2026 12:28:47 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=networkplumber-org.20230601.gappssmtp.com; s=20230601; t=1769545727; x=1770150527; 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=Gm2HcbEsUxWYadQInnf3tCWYtDpCW2vqgz8dlCHzxmI=; b=o7NhL5ynAP67igPeES1UDgh52ai8cCjTvxS49f3p/KsiGY89qZuYDTFjIcAVEcF0Jh YNTaeSg12kwcGLZ/9sazBS0okFKRQ+KMOUkIH9MW7Oy+tmzzXVZ49k3/vGHActnPoiLh MThmcLE4dX/zck344CUx0WS1VYEQNut623hXWT5J6y2UgD81PbluQ3sm9HJqmZaOPJtR FSLMDs8xM3G4Pit5CWgxQCaHF+Afmv41PhkByfDbZQGv0ciUlaKH/oOCpneECNG5P4wW 0gW96ecB9JvKrosexE2ZpTTUEU+l5wuSndjrvqWmDDp8GWMKLt9M/g4fqYAc1NmDges9 KKUw== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1769545727; x=1770150527; 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=Gm2HcbEsUxWYadQInnf3tCWYtDpCW2vqgz8dlCHzxmI=; b=BG2NjMKng7P3QL2b9YsBpqPUMV7k9dLtcXvk6Q9ngNqr1bDOmB2IIaKkrcqrEVmeZt I2T3eIe0KAR+ulDk1JCHnvXh6sfhmzKlJ2Ee8vfoTaFaKmqfIRzL9CxFdwyqLsRobZT5 3zdR+27Clt32zbhIxTlO33rtJt0/DlgkfOaRJkzKn0w5Yy8ovhqZF8LIyJrH1QZBXGxJ hN121xbG3sk3lI0SiJi7ltqvdIJ5jnEZtA5EQ8eGUAOV0Ma7MgSY6PjWjwM1AaDowILO F/SRTi2/jLyBl3n4zpcudf+VB8MuQ+DE2UUG2ZKPNyNTN/JrrfPIevY9vNsIo6rkTk+e dwmw== X-Gm-Message-State: AOJu0Yy95AT9t+Dw+gJw0n9tzd9OsnqGpHImNwvGc1Nd+H/gyWF/rzll /Wc0vhWjivAfpJnZD3PZtOz4GFMSD08lX58jto2Gnf/BXh5yZelLuUeLG4YmHUeVqn2iUSSRLbQ JxBUZ X-Gm-Gg: AZuq6aK6Cpvzsb/Nfx/hpCuLKaFxNgjva3KfgtToIKLZr0htDxRatZ9Ct4TuKHOsZrX Xv7JEFG/fj6zQs+QCu3Vxe78GA221fAGmCwxEhr88buhrKrY5myWm4oJpJCZGnhSvokmWEWkKnb /P5kKncoVdhWXG5EqsPFrySPHek5ta9/6NKtsCZrEhaxnDZsxm72kWIosfmVc2YD0c1ZvcL+KIF YLl+3AyY9Gweswm1qko2a6ZdkXaKfNRH1FFUIEAR3LRZKADcUJNKFmZ8BhCoIvkZomS7NNL41bW CP6S3/Ym7K3uEG/yDwQnnxV1ZFQswiE0UNKe9v9UaRW0/qLHbVI0+QSJTdyqXuEoGmRylmQE3dX xBAI792iigoW9cJzP/slTV6/dl2UsKZJwtWANxrvVEfFC874GXKeSB8kXgK3uCr2s/oxSR2PLL0 Vx2iilS1idRmLeNmhE3AztdVx5MRp+B5u4o9fpE6yfUUIkJYjb5w== X-Received: by 2002:a05:600c:800f:b0:45d:d8d6:7fcc with SMTP id 5b1f17b1804b1-48069c6762cmr33651095e9.27.1769545726730; Tue, 27 Jan 2026 12:28:46 -0800 (PST) Received: from phoenix.lan (204-195-96-226.wavecable.com. [204.195.96.226]) by smtp.gmail.com with ESMTPSA id 5b1f17b1804b1-48066c42895sm85437225e9.14.2026.01.27.12.28.45 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Tue, 27 Jan 2026 12:28:46 -0800 (PST) From: Stephen Hemminger To: dev@dpdk.org Cc: Stephen Hemminger Subject: [REVIEW] eal: RTE_PTR_ADD/SUB API improvements Date: Tue, 27 Jan 2026 12:28:42 -0800 Message-ID: <20260127202843.20273-1-stephen@networkplumber.org> X-Mailer: git-send-email 2.51.0 In-Reply-To: <20260125223015.69190-1-scott.k.mitch1@gmail.com> References: <20260125223015.69190-1-scott.k.mitch1@gmail.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 ptr-add.mbox Reviewed using Claude (claude-opus-4-5-20251101) This is an automated review. Please verify all suggestions. --- ## DPDK Patch Review: `[PATCH v13] eal: RTE_PTR_ADD/SUB API improvements` ### Summary This patch refactors the `RTE_PTR_ADD` and `RTE_PTR_SUB` macros to preserve pointer provenance and type qualifiers, adds new `RTE_INT_PTR_*` variants for integer-as-pointer arithmetic, and updates all callers across the codebase. --- ### Errors 1. **Subject line exceeds 60 characters** - Current: `eal: RTE_PTR_ADD/SUB API improvements` (39 chars) - Actually OK - No error here. 2. **Missing `Cc: stable@dpdk.org`** - This patch changes API behavior (NULL to `RTE_PTR_ADD`/`RTE_PTR_SUB` is now undefined). If this is considered a fix for existing issues, it should include `Cc: stable@dpdk.org`. 3. **Commit body starts with "RTE_PTR_ADD"** - The body should provide context before diving into technical details. Consider starting with "The" or describing the problem first. --- ### Warnings 1. **Implicit comparison with NULL in several places** ```c // drivers/bus/cdx/cdx_vfio.c:374 if (msl->base_va == NULL) ``` This is correct style. Good. 2. **`__rte_auto_type` documentation could be clearer** - Line ~108 in `rte_common.h`: The comment should clarify this is a DPDK internal macro, not for general use. 3. **Missing explicit NULL checks before some `RTE_PTR_ADD` calls** - `lib/eal/common/eal_common_fbarray.c:1062`: Added `RTE_ASSERT(arr->data)` - good, but this is a debug-only check. Consider returning error in release builds. 4. **Diagnostic pragma usage in `malloc_elem.h`** ```c __rte_diagnostic_push __rte_diagnostic_ignored_array_bounds struct malloc_elem *elem = RTE_PTR_SUB(RTE_PTR_UNQUAL(data), MALLOC_ELEM_HEADER_LEN); ... return elem->state != ELEM_PAD ? elem : RTE_PTR_SUB(elem, elem->pad); __rte_diagnostic_pop ``` The `__rte_diagnostic_pop` should be on its own statement or the return should be before it. As written, the pop is unreachable after return. 5. **`volatile` qualifier handling in `idxd_pci.c`** ```c // Line 62 static volatile uint32_t * idxd_get_wq_cfg(struct idxd_pci_common *pci, uint8_t wq_idx) ``` Good fix, but the FIXME comment at line 398-400 indicates incomplete resolution: ```c /* FIXME: cast drops volatile propagation to idxd_dmadev.portal * See: https://bugs.dpdk.org/show_bug.cgi?id=1871 */ idxd.portal = RTE_PTR_ADD(RTE_PTR_UNQUAL(idxd.u.pci->portals), ``` This should be tracked in the commit message or release notes. 6. **`RTE_PTR_UNQUAL` macro is used but not defined in this patch** - The patch uses `RTE_PTR_UNQUAL` in multiple places but the macro definition is not visible in the diff. Ensure it exists or is added. 7. **Test file uses `alignas` from ``** ```c #include ... alignas(uint32_t) char buffer[TEST_BUFFER_SIZE]; ``` This is C11 but DPDK targets C11, so acceptable. However, verify MSVC compatibility. 8. **`drivers/dma/odm/odm_dmadev.c` changes remove `const`** ```c // Before: const uint32_t *base_addr = vq->cring_mz->addr; // After: uint32_t *base_addr = vq->cring_mz->addr; ``` This removes a useful const qualifier. If the intent is to allow writes, document why. If not, preserve const. 9. **Release notes format** - The API Changes section uses bullet points correctly but should reference the specific macros more prominently for searchability. --- ### Info 1. **Good practices observed:** - Proper use of `unit_test_suite_runner` in test file - `TEST_ASSERT` macros used correctly - Release notes included for API changes - NULL checks added where `RTE_PTR_ADD` is used with potentially NULL pointers - Comprehensive test coverage for both pointer and integer types 2. **Code organization in `rte_common.h`:** - The new macros are well-documented with Doxygen comments - The `__rte_auto_type` abstraction is clean for cross-compiler support 3. **Changelog in commit message:** - Detailed version history (v1-v13) is helpful for reviewers 4. **Consider adding to documentation:** - The behavioral change (NULL is now undefined for `RTE_PTR_ADD`/`RTE_PTR_SUB`) should be prominently noted in the programmer's guide, not just release notes. --- ### Specific Code Issues **`lib/eal/common/malloc_elem.h:312-322`:** ```c __rte_diagnostic_push __rte_diagnostic_ignored_array_bounds struct malloc_elem *elem = RTE_PTR_SUB(RTE_PTR_UNQUAL(data), MALLOC_ELEM_HEADER_LEN); if (!malloc_elem_cookies_ok(elem)) return NULL; return elem->state != ELEM_PAD ? elem : RTE_PTR_SUB(elem, elem->pad); __rte_diagnostic_pop ``` The `__rte_diagnostic_pop` is placed after return statements and will never execute. Move the pop before the final return or restructure: ```c __rte_diagnostic_push __rte_diagnostic_ignored_array_bounds struct malloc_elem *elem = RTE_PTR_SUB(RTE_PTR_UNQUAL(data), MALLOC_ELEM_HEADER_LEN); __rte_diagnostic_pop if (!malloc_elem_cookies_ok(elem)) return NULL; return elem->state != ELEM_PAD ? elem : RTE_PTR_SUB(elem, elem->pad); ``` **`lib/eal/common/eal_common_memory.c:1563`:** ```c struct malloc_elem *volatile elem; ``` The `volatile` on a local pointer variable is unusual. This appears to be preventing optimization, but the rationale should be documented. --- ### Verdict This is a well-structured patch with good documentation and test coverage. The main issues are: 1. The diagnostic pragma placement bug in `malloc_elem.h` 2. Missing definition of `RTE_PTR_UNQUAL` in the visible diff 3. The ODM driver const removal needs justification Recommend addressing the pragma placement issue before merge.