Thank you for these notes. I submitted a v3 with the following changes. 1. Fixed bit allocation in the flow rule bitmap using the suggestion given. 2. Changed rte_zmalloc/rte_free for struct gve_flow to calloc/free. In making this change, I realized that the driver currently would not support flow operations from secondary processes, so I made a note of that in gve.rst. 3. Changed both verbs to "added" to match the notes from other drivers.
On Tue, Mar 3, 2026 at 7:21 AM Stephen Hemminger <[email protected]> wrote: > On Tue, 3 Mar 2026 00:58:00 +0000 > "Jasper Tran O'Leary" <[email protected]> wrote: > > > This patch series adds flow steering support to the Google Virtual > > Ethernet (gve) driver. This functionality allows traffic to be directed > > to specific receive queues based on user-specified flow patterns. > > > > The series includes foundational support for extended admin queue > > commands needed to handle flow rules, the specific adminqueue commands > > for flow rule management, and the integration with the DPDK rte_flow > > API. The series adds support flow matching on the following protocols: > > IPv4, IPv6, TCP, UDP, SCTP, ESP, and AH. > > > > Patch Overview: > > > > 1. "net/gve: add flow steering device option" checks for and enables > > the flow steering capability in the device options during > > initialization. > > 2. "net/gve: introduce extended adminq command" adds infrastructure > > for sending extended admin queue commands. These commands use a > > flexible buffer descriptor format required for flow rule management. > > 3. "net/gve: add adminq commands for flow steering" implements the > > specific admin queue commands to add and remove flow rules on the > > device, including handling of rule IDs and parameters. > > 4. "net/gve: add rte flow API integration" exposes the flow steering > > functionality via the DPDK rte_flow API. This includes strict > > pattern validation, rule parsing, and lifecycle management (create, > > destroy, flush). It ensures thread-safe access to the flow subsystem > > and proper resource cleanup during device reset. > > > > Jasper Tran O'Leary (2): > > net/gve: add adminq commands for flow steering > > net/gve: add rte flow API integration > > > > Vee Agarwal (2): > > net/gve: add flow steering device option > > net/gve: introduce extended adminq command > > > > doc/guides/nics/features/gve.ini | 12 + > > doc/guides/nics/gve.rst | 26 + > > doc/guides/rel_notes/release_26_03.rst | 1 + > > drivers/net/gve/base/gve.h | 3 +- > > drivers/net/gve/base/gve_adminq.c | 118 ++++- > > drivers/net/gve/base/gve_adminq.h | 57 +++ > > drivers/net/gve/gve_ethdev.c | 83 +++- > > drivers/net/gve/gve_ethdev.h | 46 ++ > > drivers/net/gve/gve_flow_rule.c | 656 +++++++++++++++++++++++++ > > drivers/net/gve/gve_flow_rule.h | 65 +++ > > drivers/net/gve/meson.build | 1 + > > 11 files changed, 1063 insertions(+), 5 deletions(-) > > create mode 100644 dpdk/drivers/net/gve/gve_flow_rule.c > > create mode 100644 dpdk/drivers/net/gve/gve_flow_rule.h > > > > Automated review spotted a few things: > > 1. rte_bitmap_scan usage is incorrect. > 2. Using rte_malloc where not necessary > 3. Grammar in the release note. > > I can fix the last one when merging, the second one is not a big issue > but would be good to have. > > --- > > > Error: Incorrect rte_bitmap_scan usage — wrong rule ID allocation (~85% > confidence) > > In gve_create_flow_rule(): > c > > uint64_t bmp_slab __rte_unused; > ... > if (rte_bitmap_scan(priv->avail_flow_rule_bmp, &flow->rule_id, > &bmp_slab) == 1) { > rte_bitmap_clear(priv->avail_flow_rule_bmp, flow->rule_id); > } > > rte_bitmap_scan() writes the slab base position to pos and the slab bit > pattern to slab. The actual bit position of the first available rule is pos > + __builtin_ctzll(slab), not pos alone. The __rte_unused annotation on > bmp_slab confirms the slab value is being ignored entirely. > > After the first rule allocation clears bit 0, a subsequent scan returns > the same slab base (pos=0) with bit 0 now clear in the slab. The code would > again set rule_id=0 and attempt to clear an already-clear bit, then try to > create a duplicate rule on the device. > > Suggested fix: > c > > uint64_t bmp_slab; > uint32_t pos; > ... > if (rte_bitmap_scan(priv->avail_flow_rule_bmp, &pos, > &bmp_slab) == 1) { > flow->rule_id = pos + __builtin_ctzll(bmp_slab); > rte_bitmap_clear(priv->avail_flow_rule_bmp, flow->rule_id); > } > > Warning: rte_zmalloc used for ordinary control structures > > Both avail_flow_rule_bmp_mem and individual gve_flow structures are > allocated with rte_zmalloc, but they don't require DMA access, NUMA > placement, or multi-process shared memory. Standard calloc/malloc would be > more appropriate and wouldn't consume limited hugepage resources. > > Warning: Release notes tense inconsistency > rst > > * Added application-initiated device reset. > + * Add support for receive flow steering. > > "Added" (past tense) vs "Add" (imperative). Both entries should use the > same tense for consistency. The DPDK convention is imperative mood. > >
