Re: [PATCH 0/9] Stability fixes for GVE

[Stephen Hemminger <stephen@networkplumber.org>]

On Wed, 1 Jul 2026 12:59:18 -0700
Joshua Washington <joshwash@google.com> wrote:

> On Wed, Jul 1, 2026 at 11:50 AM Stephen Hemminger
> <stephen@networkplumber.org> wrote:
> >
> > On Wed,  1 Jul 2026 11:35:18 -0700
> > Joshua Washington <joshwash@google.com> wrote:
> >  
> > > This patch series consists of mostly unrelated fixes in the GVE driver.
> > >
> > > Joshua Washington (9):
> > >   net/gve: clear out shared memory region for stats report
> > >   net/gve: delay adding mbuf head to software ring
> > >   net/gve: copy data to QPL buffer when mbuf read does not
> > >   net/gve: validate buf ID before processing Rx packet
> > >   net/gve: set mbuf to null in software ring after use
> > >   net/gve: free ctx mbuf if packet dropped after first segment
> > >   net/gve: increase range of DMA memzone ids to 64 bits
> > >   net/gve: don't reset ring size bounds to default on reset
> > >   net/gve: restrict max ring size in GQ QPL to 2K
> > >
> > >  drivers/net/gve/base/gve_adminq.c | 12 ++++++---
> > >  drivers/net/gve/base/gve_osdep.h  |  4 +--
> > >  drivers/net/gve/gve_ethdev.c      |  8 +++---
> > >  drivers/net/gve/gve_ethdev.h      |  1 +
> > >  drivers/net/gve/gve_rx.c          |  3 +++
> > >  drivers/net/gve/gve_rx_dqo.c      |  6 +++++
> > >  drivers/net/gve/gve_tx.c          | 42 +++++++++++++++++++------------
> > >  7 files changed, 52 insertions(+), 24 deletions(-)
> > >  
> >
> > *Build Failed #1:
> > OS: OpenAnolis8.10-64
> > Target: x86_64-native-linuxapp-gcc
> > FAILED: drivers/libtmp_rte_net_gve.a.p/net_gve_gve_tx.c.o
> > gcc -Idrivers/libtmp_rte_net_gve.a.p -Idrivers -I../drivers -Idrivers/net/gve -I../drivers/net/gve -I../drivers/net/gve/base -Ilib/ethdev -I../lib/ethdev -Ilib/eal/common -I../lib/eal/common -I. -I.. -Iconfig -I../config -Ilib/eal/include -I../lib/eal/include -Ilib/eal/linux/include -I../lib/eal/linux/include -Ilib/eal/x86/include -I../lib/eal/x86/include -I../kernel/linux -Ilib/eal -I../lib/eal -Ilib/kvargs -I../lib/kvargs -Ilib/log -I../lib/log -Ilib/metrics -I../lib/metrics -Ilib/telemetry -I../lib/telemetry -Ilib/argparse -I../lib/argparse -Ilib/net -I../lib/net -Ilib/mbuf -I../lib/mbuf -Ilib/mempool -I../lib/mempool -Ilib/ring -I../lib/ring -Ilib/meter -I../lib/meter -Idrivers/bus/pci -I../drivers/bus/pci -I../drivers/bus/pci/linux -Ilib/pci -I../lib/pci -Idrivers/bus/vdev -I../drivers/bus/vdev -fdiagnostics-color=always -D_FILE_OFFSET_BITS=64 -Wall -Winvalid-pch -Wextra -Werror -std=c11 -O3 -include rte_config.h -Wvla -Wcast-qual -Wdeprecated -Wformat -Wformat-nonliteral -Wformat-security -Wmissing-declarations -Wmissing-prototypes -Wnested-externs -Wold-style-definition -Wpointer-arith -Wshadow -Wsign-compare -Wstrict-prototypes -Wundef -Wwrite-strings -Wno-packed-not-aligned -Wno-missing-field-initializers -D_GNU_SOURCE -fPIC -march=native -mrtm -DALLOW_EXPERIMENTAL_API -DALLOW_INTERNAL_API -Wno-format-truncation -Wno-vla -DRTE_COMPONENT_CLASS=pmd_net -DRTE_COMPONENT_NAME=gve -DRTE_LOG_DEFAULT_LOGTYPE=pmd.net.gve -MD -MQ drivers/libtmp_rte_net_gve.a.p/net_gve_gve_tx.c.o -MF drivers/libtmp_rte_net_gve.a.p/net_gve_gve_tx.c.o.d -o drivers/libtmp_rte_net_gve.a.p/net_gve_gve_tx.c.o -c ../drivers/net/gve/gve_tx.c
> > ../drivers/net/gve/gve_tx.c: In function ‘gve_tx_burst_qpl’:
> > ../drivers/net/gve/gve_tx.c:258:21: error: variable ‘addr’ set but not used [-Werror=unused-but-set-variable]
> >   uint64_t ol_flags, addr, fifo_addr;
> >                      ^~~~
> > cc1: all warnings being treated as errors
> > [1640/3750] Compiling C object drivers/libtmp_rte_net_gve.a.p/net_gve_gve_rx.c.o
> > [1641/3750] Compiling C object drivers/libtmp_rte_net_hinic.a.p/net_hinic_base_hinic_pmd_cfg.c.o
> > [1642/3750] Compiling C object drivers/libtmp_rte_net_failsafe.a.p/net_failsafe_failsafe_ops.c.o
> > [1643/3750] Compiling C object drivers/libtmp_rte_net_hinic.a.p/net_hinic_base_hinic_pmd_api_cmd.c.o
> > [1644/3750] Compiling C object drivers/libtmp_rte_net_hinic.a.p/net_hinic_base_hinic_pmd_cmdq.c.o
> > [1645/3750] Compiling C object drivers/libtmp_rte_net_hinic.a.p/net_hinic_hinic_pmd_rx.c.o
> > [1646/3750] Compiling C object drivers/libtmp_rte_net_hinic.a.p/net_hinic_hinic_pmd_flow.c.o
> > [1647/3750] Compiling C object drivers/libtmp_rte_net_hinic.a.p/net_hinic_hinic_pmd_ethdev.c.o
> > [1648/3750] Compiling C object drivers/libtmp_rte_net_hinic.a.p/net_hinic_hinic_pmd_tx.c.o
> > ninja: build stopped  
> 
> Will fix in v2.

Here is AI review if it helps
Review of "net/gve" 9-patch series (bundle 1995)

Applied cleanly on top of 26.07-rc2 and built with
  meson -Dbuildtype=minsize -Ddefault_library=static -Dwerror=true

The series is a solid set of datapath and reset-path bug fixes. One patch
breaks the build on its own, which blocks the series until fixed.


Patch 3/9 (net/gve: copy data to QPL buffer when mbuf read does not)

Error: build failure. Removing the two rte_memcpy() calls that consumed
'addr' leaves 'addr' set but never read, so the driver no longer compiles
under -Werror:

  gve_tx.c: In function 'gve_tx_burst_qpl':
  gve_tx.c:258:28: error: variable 'addr' set but not used
                          [-Werror=unused-but-set-variable]

Both remaining assignments are now dead:

  addr = (uint64_t)(tx_pkt->buf_addr) + tx_pkt->data_off;
  ...
  addr = (uint64_t)(tx_pkt->buf_addr) + tx_pkt->data_off + hlen;

Drop 'addr' from the declaration and delete both assignments. This also
means each commit does not build independently, breaking git bisect.

The fix itself is correct: rte_pktmbuf_read() returns a pointer into the
mbuf when the range is contiguous and does not touch the destination, so
the guarded rte_memcpy() is needed for both the header and the TSO
payload copy.


Patch 9/9 (net/gve: restrict max ring size in GQ QPL to 2K)

Info: the new include uses quotes,

  #include "rte_common.h"

but every other DPDK header in this driver (including base/gve_osdep.h)
uses angle brackets. Prefer:

  #include <rte_common.h>

The GQ-QPL RTE_MIN() cap and the DQO override refactor are correct; the
initial device-value assignment that DQO overwrites is an intentional
default, not a dead store.


Patches 1, 2, 4, 5, 6, 7, 8: no issues found.

- 2/9: sw_ring record loop advances sw_id by nb_segs exactly as before;
  the added !tx_pkt guard is a safe bound.
- 4/9: buf_id bound check sits after the completion descriptor is
  consumed (rx_id / nb_rx_hold / generation advanced), so a bad id is
  dropped without stalling the ring.
- 5/9 + 6/9: nulling sw_ring after handing the mbuf to the application
  and freeing ctx->mbuf_head on drop are consistent - the head chain is
  removed from sw_ring before being freed, so no double free, and refill
  overwrites the slots with freshly allocated mbufs.

No Reviewed-by given: patch 3 does not build. Once the unused 'addr' is
removed I'm happy to ack the series.