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 7E564D2FEE1 for ; Tue, 27 Jan 2026 20:45:28 +0000 (UTC) Received: from mails.dpdk.org (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id 6E5BD40F1A; Tue, 27 Jan 2026 21:45:27 +0100 (CET) Received: from mail-wr1-f48.google.com (mail-wr1-f48.google.com [209.85.221.48]) by mails.dpdk.org (Postfix) with ESMTP id 41EFB40E21 for ; Tue, 27 Jan 2026 21:45:25 +0100 (CET) Received: by mail-wr1-f48.google.com with SMTP id ffacd0b85a97d-43246af170aso150113f8f.0 for ; Tue, 27 Jan 2026 12:45:25 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=networkplumber-org.20230601.gappssmtp.com; s=20230601; t=1769546725; x=1770151525; 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=u/qCCKuWHCaflmmrNdfj/1emNur5+rdaUFsOT4AOd1Q=; b=XXhmVwBKmn0Xi6bDqM3he+PKxGdX211BcK6OGDeRldLE6kywVptzgj6kWfdrMj8qrq wuX1t6vjpFY7eqM9sa9nZzVx2WWqjKZIL4ib8DdXZ7041fm08utK9Y8wIwNfzijJnDnR 67QwG3VbR4CcnD0vDSjiLtpTtrjFrivffD8EjD3Uh3Q5V2fCi82j1NUr/yBP+B8ZDVQ3 ic1CgbR243CuMarjKQ5OHzoAbpG4xH97RkcJHmybtxuvSHHiQeSHNps6XGjxG48gzH4B 90ZfGufzaYXmneqlWGJ/f8m3+hBskDfL5P9j3b/6Qe7g4kXUVMECbzP81PvV0FWmsV83 9rlA== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1769546725; x=1770151525; 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=u/qCCKuWHCaflmmrNdfj/1emNur5+rdaUFsOT4AOd1Q=; b=t2wXahLStgBFX5LjVEE59ga7pbwCxeE9FsOIu6nIpnYyMVFiBlzNT3s1n3fXHiTJEf erNHVFzIYhXk/0VdzmIp8Nl2irHIP4OFo3GTZCL0OXZJc53sty5BAXF3ELoLjGIIYX4T 3UfvIDEz6/l17EKUZ/liDZX+pUGQfxGevCIpE6pB4nKoYaWpK3PvWOAdjrnFas0fA+qS FnV6T83+15uHZtt4n+MFMWg35/rn0+7GD92t+as4Qn7XM0lE4muWNPqrORkqEX0URJ4i iT5+Fmw1EeP8Wa/srvFiieQ1OlXCjqal+ro9GHQx/vUZLSo4fRQqyVLMwjm/B7/gnlYw T6zg== X-Gm-Message-State: AOJu0Yytue6JXXIkLQ7tK3L8B8544aBfTmRB6N8UU/UZx0zylshp6CjH BjU/2qQMCsNzUqSXhinVMDng7q75T+h1hSiuxxsSPkqcY/AWMw1bTyMwN66VEbclQr98sLXh8M1 OJatG X-Gm-Gg: AZuq6aJRIcxcZjFCXZFziiFyQCin7wTZaUsaBgs3XjvWQqFNddvkXw9chmc9UyodMWs BsSCg7AO9BQ94Z236zgsc4X02IYi4HqG7aTD7ZhkiUuErf40eNjdSLeUOsMj+BlFfaes6AVkCsm j5WkJ6wsBeE7NCkHeMdiOFdvpsTre7hRTJI8CR8zkrwJCjKd+5AUfrO32hVwxWE7JS5hGQRsigA MdLHCVTEovo1HypDPnberICGFZr+AK+AtEq/KEFu5/XE58CIJU0tAJljprnR7n3wF8s5u5QbICL kQJ0qsOdP7kVLxpXxlwj0rl1nxFVJ7D2lymehyXHsDmrIFKlOcQBpr+DHuRKbnfx44JkBfH1rid egyI/8cBLlNVgNvq/fNWBLzTsGLUns8R7TyV2BQELFcd1ff1EVNEdOgDiUb/bHGe2GOS+7R7lkP UnAqZ5bwzdFF3wKr/BofLN1ZGJTrkO8MzQ7dRXUpXaoK7xxkYmDQ== X-Received: by 2002:a5d:42cc:0:b0:435:e07d:47ac with SMTP id ffacd0b85a97d-435e07d47c5mr1665997f8f.28.1769546724248; Tue, 27 Jan 2026 12:45:24 -0800 (PST) Received: from phoenix.lan (204-195-96-226.wavecable.com. [204.195.96.226]) by smtp.gmail.com with ESMTPSA id 5b1f17b1804b1-4806d25838csm3565345e9.0.2026.01.27.12.45.22 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Tue, 27 Jan 2026 12:45:23 -0800 (PST) From: Stephen Hemminger To: dev@dpdk.org Cc: Stephen Hemminger Subject: [REVIEW] net/af_packet: software checksum and tx poll control Date: Tue, 27 Jan 2026 12:45:20 -0800 Message-ID: <20260127204520.21426-1-stephen@networkplumber.org> X-Mailer: git-send-email 2.51.0 In-Reply-To: <20260127181355.98437-4-scott.k.mitch1@gmail.com> References: <20260127181355.98437-4-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 bundle-1702-af-packet.mbox Reviewed using Claude (claude-opus-4-5-20251101) This is an automated review. Please verify all suggestions. --- # DPDK Patch Review: AF_PACKET Thread Safety and Frame Calculations ## Patch 1/3: net/af_packet: fix thread safety and frame calculations ### Commit Message Issues **Error: Subject line exceeds 60 characters** - Current: `net/af_packet: fix thread safety and frame calculations` (59 characters) - This is actually within the limit, my mistake on initial count. **Info: Subject is acceptable** - Format is correct: `net/af_packet:` prefix with lowercase description - No trailing period, imperative mood **Info: Body is well-structured** - Good explanation of the problems and solutions - Proper wrapping within 75 characters for most lines - Required tags present (Fixes, Cc: stable@dpdk.org, Signed-off-by) ### Code Review **Warning: Missing space after comma in void* cast** ```c rte_memcpy(pbuf, rte_pktmbuf_mtod(tmp_mbuf, void*), data_len); ``` Should be: ```c rte_memcpy(pbuf, rte_pktmbuf_mtod(tmp_mbuf, void *), data_len); ``` This appears in multiple locations. **Warning: Variable declaration in middle of block (C99 style)** ```c for (i = 0; i < nb_pkts; i++) { /* point at the next incoming frame */ ppd = (struct tpacket2_hdr *) pkt_q->rd[framenum].iov_base; uint32_t tp_status = rte_atomic_load_explicit(&ppd->tp_status, ``` While C99 style is acceptable per AGENTS.md, consistency within the function is preferred. The original code declares variables at the start of the block. **Warning: Double blank line in eth_stats_get()** ```c uint64_t rx_nombuf = rte_atomic_load_explicit(&internal->rx_queue[i].rx_nombuf, rte_memory_order_relaxed); uint64_t tx_pkts = rte_atomic_load_explicit(&internal->tx_queue[i].tx_pkts, ``` Remove one blank line. **Info: Atomic operations usage is correct** - Proper use of `rte_atomic_load_explicit()` with `rte_memory_order_acquire` for tp_status reads - Proper use of `rte_atomic_store_explicit()` with `rte_memory_order_release` for tp_status writes - Relaxed ordering for statistics is appropriate **Warning: Static const at file scope should use uppercase naming** ```c static const uint16_t ETH_AF_PACKET_FRAME_SIZE_MAX = RTE_IPV4_MAX_PKT_LEN; ``` This is correct - uppercase for constants. However, the type is `uint16_t` but `RTE_IPV4_MAX_PKT_LEN` is typically larger. Verify this doesn't truncate. **Info: Frame calculation fix appears correct** The block-based addressing fix aligns with kernel's `packet_lookup_frame()` behavior. --- ## Patch 2/3: net/af_packet: RX/TX rte_memcpy, bulk free, prefetch ### Commit Message Issues **Warning: Subject uses terms that should be expanded** - `RX/TX` should be `Rx/Tx` per DPDK naming conventions in words-case.txt **Error: Missing Cc: stable@dpdk.org if this is a bug fix** If this patch contains performance fixes that should be backported, add `Cc: stable@dpdk.org`. ### Code Review **Warning: rte_memcpy() may not be appropriate here** Per AGENTS.md: "rte_memcpy() is optimized for bulk data transfer in the fast path. For general use, standard memcpy() is preferred." The packet data copy is indeed in the fast path, so `rte_memcpy()` usage is justified here. However, the commit message should clarify this is intentional for fast-path optimization. **Warning: rte_pktmbuf_free_bulk() called on potentially modified array** ```c rte_pktmbuf_free_bulk(&bufs[0], i); ``` The VLAN insertion path calls `rte_vlan_insert(&mbuf)` which may reallocate the mbuf. If the original mbuf pointer in `bufs[i]` is not updated, this could free the wrong mbuf or double-free. The current code uses a local `mbuf` variable but doesn't update `bufs[i]`. **Error: Potential memory leak on skipped packets** ```c if (unlikely(mbuf->pkt_len > pkt_q->frame_data_size || ((mbuf->ol_flags & RTE_MBUF_F_TX_VLAN) != 0 && rte_vlan_insert(&mbuf) != 0))) { continue; } ``` When a packet is skipped (oversized or VLAN insert fails), it's not freed. The bulk free at the end uses `i` which includes skipped packets, but those packets were never processed. This needs careful review - the original code freed packets individually including on error paths. **Warning: Missing space after comma** ```c rte_memcpy(pbuf, rte_pktmbuf_mtod(tmp_mbuf, void*), data_len); ``` **Info: Prefetch optimization looks reasonable** The prefetch of next frame/mbuf while processing current packet is a valid optimization. --- ## Patch 3/3: net/af_packet: software checksum and tx poll control ### Commit Message Issues **Warning: Subject could be more specific** Consider: `net/af_packet: add software checksum and tx poll control` ### Code Review **Error: Missing header include** The code uses `struct rte_udp_hdr` and `struct rte_tcp_hdr` but only includes ``. Need: ```c #include #include ``` **Warning: Including `` is unnecessary** DPDK's `rte_common.h` (included via other headers) provides `bool` type. However, explicit inclusion isn't harmful. **Warning: New devarg not documented** The new `txpollnotrdy` argument should be documented in the driver documentation (`doc/guides/nics/af_packet.rst`). **Warning: Inline function in .c file should use `static inline`** ```c static inline void af_packet_sw_cksum(struct rte_mbuf *mbuf) ``` This is correct. **Warning: Magic check for minimum header sizes** ```c if (unlikely(mbuf_len < l4_offset)) return; ``` This silently returns without computing checksum. Consider logging a debug message for diagnosability. **Info: Software checksum implementation looks correct** - Properly handles IPv4/IPv6 - Properly handles UDP/TCP - Uses mbuf-aware checksum functions for multi-segment packets **Warning: Boolean parameter parsing could be clearer** ```c txpollnotrdy = atoi(pair->value) != 0; ``` Consider using a more explicit boolean parsing that accepts "true"/"false" or "yes"/"no" in addition to 0/1. **Warning: New capability advertised but may not match hardware** Adding `RTE_ETH_TX_OFFLOAD_IPV4_CKSUM | RTE_ETH_TX_OFFLOAD_UDP_CKSUM | RTE_ETH_TX_OFFLOAD_TCP_CKSUM` to capabilities is correct for software implementation, but documentation should clarify this is software-based. --- ## Summary ### Errors (Must Fix) 1. Patch 2: Potential memory leak on skipped packets - packets that fail size check or VLAN insert are not freed 2. Patch 3: Missing includes for `rte_udp.h` and `rte_tcp.h` ### Warnings (Should Fix) 1. All patches: Missing space after comma in `void*` casts 2. Patch 1: Double blank line in `eth_stats_get()` 3. Patch 2: Subject should use `Rx/Tx` not `RX/TX` 4. Patch 2: Verify `rte_pktmbuf_free_bulk()` handles VLAN-modified mbufs correctly 5. Patch 3: New devarg `txpollnotrdy` needs documentation update ### Info (Consider) 1. The atomic operations and memory ordering appear correct for the kernel synchronization protocol 2. Frame calculation fix aligns with kernel behavior 3. Prefetch optimizations are reasonable 4. Software checksum implementation is correct