<div dir="ltr">Thank you for these notes. I submitted a v3 with the following changes.<div> </div><div>1. Fixed bit allocation in the flow rule bitmap using the suggestion given.</div><div>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.</div><div>3. Changed both verbs to "added" to match the notes from other drivers. </div></div> <div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Tue, Mar 3, 2026 at 7:21 AM Stephen Hemminger <<a href="mailto:stephen@networkplumber.org" target="_blank">stephen@networkplumber.org</a>> wrote: </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">On Tue, 3 Mar 2026 00:58:00 +0000 "Jasper Tran O'Leary" <<a href="mailto:jtranoleary@google.com" target="_blank">jtranoleary@google.com</a>> 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. </blockquote></div>