Thanks Joshua, I'll get this sorted out. In trying to incorporate all the feedback from you, Stephen, and AI suggestions at once, I ended up losing focus on the main scope. I'm going to break this work down into smaller, focused commits so it’s much easier to review and test.
On Wed, Oct 14, 2026 at 6:20 AM Joshua Washington <[email protected]> wrote: > > On Tue, Oct 13, 2026 at 3:44 AM Tathagat Priyadarshi > <[email protected]> wrote: > > > > The GVNIC DQO hardware restricts each packet (or MTU segment in TSO) > > to at most 10 data descriptors (GVE_TX_MAX_DATA_DESCS), beyond which > > the hardware drops traffic. In addition to this 10-descriptor limit, > > the hardware imposes strict packet sizing and header constraints. > > > > Previously, failing the descriptor limit or rte_mbuf_check() broke > > from the Tx burst loop without consuming the invalid mbuf. This caused > > tx_burst to return a short count, leading the application to retry the > > same rejected packet indefinitely and causing Head-of-Line (HOL) > > queue stalls. > > > > Enforce hardware validation and prevent HOL blocking in DPDK: > > - Validate packet and hardware constraints prior to ring space checks. > > - When packet validation or rte_mbuf_check() fails, drop and free the > > invalid mbuf with rte_pktmbuf_free(), update stats, and continue the > > burst to prevent Head-of-Line (HOL) blocking. > > - Exclude dropped packets from txq->stats.packets while returning the > > total consumed count to the caller. > > - Rename too_many_descs in struct gve_tx_stats to dropped_pkts to reflect > > all validation and malformed mbuf drop events. > > - Reject packets whose descriptor count exceeds the maximum capacity of > > the ring (accounting for reserved slots, cacheline overlap margin, and > > unreported descriptors), preventing permanent queue stalls. > > - Define GVE_TX_MIN_PKT_SIZE_DQO (17 bytes) and GVE_TX_MAX_PKT_SIZE_DQO > > (9728 bytes) to enforce wire packet boundaries for non-TSO packets > > and TSO MSS. > > - Define GVE_TX_MAX_TSO_LEN_DQO (262143 bytes) to enforce the hardware > > L4 payload limit in the 24-bit tso_total_len field. > > - In gve_tx_validate_descs(): > > * Enforce packet size in [17, 9728] for non-TSO packets alongside > > the 10-descriptor limit (nb_descs <= GVE_TX_MAX_DATA_DESCS). > > * Reject TSO packets missing an L4 header (l4_len == 0). > > * Reject packet with headers (L2-L4) not contiguous in first segment. > > * Reject TSO packets without payload (pkt_len <= headlen). > > * Enforce TSO total payload length limit (pkt_len - headlen <= 262143). > > * Enforce MSS limits [88, 9728] and header length limit <= 255. > > I agree with the AI code review on this patch description. > > 4. The commit message lists the diff line by line. Cut it to the > > problem (invalid packets stalled the queue; hardware limits were > > not enforced) and the fix (drop and continue; the limits enforced). > > Can we trim this message down a bit, and more succinctly explain the > error condition and how this patch fixes it? If it cannot be describe > succinctly, that might be a sign that the patch should be split up > into smaller logical pieces. > > > > > Fixes: 4022f9999f56 ("net/gve: support basic Tx data path for DQO") > > Fixes: f4a70b449bfa ("net/gve: add DQO Tx descriptor limit") > > Fixes: 403c671a46b6 ("net/gve: support TSO in DQO RDA") > > Cc: [email protected] > > > > Signed-off-by: Tathagat Priyadarshi <[email protected]> > > --- > > drivers/net/gve/base/gve_desc_dqo.h | 3 + > > drivers/net/gve/gve_ethdev.h | 2 +- > > drivers/net/gve/gve_tx_dqo.c | 92 +++++++++++++++++++++++------ > > 3 files changed, 79 insertions(+), 18 deletions(-) > > > > diff --git a/drivers/net/gve/base/gve_desc_dqo.h > > b/drivers/net/gve/base/gve_desc_dqo.h > > index c1534959c2..53e394f01c 100644 > > --- a/drivers/net/gve/base/gve_desc_dqo.h > > +++ b/drivers/net/gve/base/gve_desc_dqo.h > > @@ -12,6 +12,9 @@ > > > > #define GVE_TX_MAX_HDR_SIZE_DQO 255 > > #define GVE_TX_MIN_TSO_MSS_DQO 88 > > +#define GVE_TX_MIN_PKT_SIZE_DQO 17 > > +#define GVE_TX_MAX_PKT_SIZE_DQO 9728 > > +#define GVE_TX_MAX_TSO_LEN_DQO ((256 * 1024) - 1) > > > > /* Basic TX descriptor (DTYPE 0x0C) */ > > struct gve_tx_pkt_desc_dqo { > > diff --git a/drivers/net/gve/gve_ethdev.h b/drivers/net/gve/gve_ethdev.h > > index 2033f49444..d6d71c31bb 100644 > > --- a/drivers/net/gve/gve_ethdev.h > > +++ b/drivers/net/gve/gve_ethdev.h > > @@ -113,7 +113,7 @@ struct gve_tx_stats { > > uint64_t packets; > > uint64_t bytes; > > uint64_t errors; > > - uint64_t too_many_descs; > > + uint64_t dropped_pkts; > > Is there any reason to change this to a general "dropped_pkts" instead > of adding a different stat if there is a different error condition > that needs to be captured? These are extended stats, so if we can give > more specific drop conditions, I think we should aim to. However, > there is nothing wrong with also introducing a new aggregate > dropped_pkts stat. Not sure if adding new stats would be considered > scope creep, though. > > > }; > > > > struct gve_rx_stats { > > diff --git a/drivers/net/gve/gve_tx_dqo.c b/drivers/net/gve/gve_tx_dqo.c > > index eee1f10df1..94ea426961 100644 > > --- a/drivers/net/gve/gve_tx_dqo.c > > +++ b/drivers/net/gve/gve_tx_dqo.c > > @@ -126,20 +126,67 @@ gve_tx_clean_descs_dqo(struct gve_tx_queue *txq, > > uint16_t nb_descs) { > > gve_tx_clean_dqo(txq); > > } > > > > +/* Max descriptors a single packet may use. A larger packet can never pass > > + * gve_can_tx(): one slot is always reserved, the cacheline overlap margin > > + * must stay free, and up to GVE_TX_MIN_RE_INTERVAL - 1 descriptors posted > > + * after the last report_event are not reclaimed until more are posted. > > + */ > > +static inline int > > +gve_tx_max_pkt_descs(const struct gve_tx_queue *txq) > > +{ > > + return (int)txq->nb_tx_desc - 1 - > > GVE_TX_MIN_DESC_PREVENT_CACHE_OVERLAP - > > + (GVE_TX_MIN_RE_INTERVAL - 1); > > +} > > + > > /* GVE expects at most 10 data descriptors per mtu-sized segment. Beyond > > this, > > * the hardware will assume the driver is malicious and stop transmitting > > * packets altogether. Validate that a packet can be sent to avoid sending > > * posting descriptors for an invalid packet. > > */ > > static inline bool > > -gve_tx_validate_descs(struct rte_mbuf *tx_pkt, uint16_t nb_descs, bool > > is_tso) > > +gve_tx_validate_descs(const struct gve_tx_queue *txq, struct rte_mbuf > > *tx_pkt, > > + uint16_t nb_descs, bool is_tso) > > { > > - if (!is_tso) > > + int num_descs, seg_offset, mbuf_len; > > + int tso_segsz, headlen; > > + > > + /* Reject packets that can never fit in the ring */ > > + if (unlikely(nb_descs > gve_tx_max_pkt_descs(txq))) > > + return false; > > + > > + if (!is_tso) { > > + /* Reject packets that violate hardware length constraints > > */ > > + if (unlikely(tx_pkt->pkt_len < GVE_TX_MIN_PKT_SIZE_DQO || > > + tx_pkt->pkt_len > GVE_TX_MAX_PKT_SIZE_DQO)) > > + return false; > > return nb_descs <= GVE_TX_MAX_DATA_DESCS; > > + } > > > > - int tso_segsz = tx_pkt->tso_segsz; > > - int num_descs, seg_offset, mbuf_len; > > - int headlen = tx_pkt->l2_len + tx_pkt->l3_len + tx_pkt->l4_len; > > + headlen = tx_pkt->l2_len + tx_pkt->l3_len + tx_pkt->l4_len; > > + > > + /* Reject missing TCP header on TSO */ > > + if (unlikely(tx_pkt->l4_len == 0)) > > + return false; > > + > > + /* Reject packet with headers (L2-L4) not contiguous in first > > segment */ > > + if (unlikely(tx_pkt->data_len < headlen)) > > + return false; > > + > > + /* Reject TSO packet without payload */ > > + if (unlikely(tx_pkt->pkt_len <= (uint32_t)headlen)) > > + return false; > > + > > + /* Enforce GVE hardware TSO total length limit */ > > + if (unlikely(tx_pkt->pkt_len - headlen > GVE_TX_MAX_TSO_LEN_DQO)) > > + return false; > > + > > + /* Enforce GVE hardware MSS and header limits */ > > + if (unlikely(tx_pkt->tso_segsz < GVE_TX_MIN_TSO_MSS_DQO || > > + tx_pkt->tso_segsz > GVE_TX_MAX_PKT_SIZE_DQO || > > + headlen > GVE_TX_MAX_HDR_SIZE_DQO)) > > + return false; > > + > > + tso_segsz = tx_pkt->tso_segsz; > > > > /* Headers will be split into their own buffer. */ > > num_descs = 1; > > @@ -147,14 +194,18 @@ gve_tx_validate_descs(struct rte_mbuf *tx_pkt, > > uint16_t nb_descs, bool is_tso) > > mbuf_len = tx_pkt->data_len - headlen; > > > > while (tx_pkt) { > > + int seg_remain; > > + > > if (!mbuf_len) > > goto next_mbuf; > > > > - int seg_remain = tso_segsz - seg_offset; > > + seg_remain = tso_segsz - seg_offset; > > if (num_descs == GVE_TX_MAX_DATA_DESCS && seg_remain) > > return false; > > > > if (seg_remain < mbuf_len) { > > + int mbuf_remain = tx_pkt->data_len % > > GVE_TX_MAX_BUF_SIZE_DQO; > > + > > seg_offset = mbuf_len % tso_segsz; > > /* The MSS is bound from above by 9728B, so a > > * single TSO segment in the middle of an mbuf > > @@ -162,7 +213,6 @@ gve_tx_validate_descs(struct rte_mbuf *tx_pkt, uint16_t > > nb_descs, bool is_tso) > > * is not at risk of defying this limitation. > > * Thus, such segments are ignored. > > */ > > - int mbuf_remain = tx_pkt->data_len % > > GVE_TX_MAX_BUF_SIZE_DQO; > > > > /* For each TSO segment, HW will prepend > > * headers. The remaining bytes of this mbuf > > @@ -184,7 +234,6 @@ gve_tx_validate_descs(struct rte_mbuf *tx_pkt, uint16_t > > nb_descs, bool is_tso) > > mbuf_len = tx_pkt->data_len; > > } > > > > - > > return true; > > } > > > > @@ -256,6 +305,7 @@ gve_tx_burst_dqo(void *tx_queue, struct rte_mbuf > > **tx_pkts, uint16_t nb_pkts) > > struct rte_mbuf *tx_pkt; > > struct gve_tx_pkt *pkts; > > uint32_t last_desc_idx; > > + uint16_t nb_drop = 0; > > Minor nit: maybe we can capture the number of packet sent after we > write the descriptors instead? Then we can increment in one place > instead of two, and remove the math in getting the number of packets > sent. > > > uint16_t nb_tx = 0; > > uint64_t bytes = 0; > > const char *reason; > > @@ -281,9 +331,13 @@ gve_tx_burst_dqo(void *tx_queue, struct rte_mbuf > > **tx_pkts, uint16_t nb_pkts) > > txq->rs_thresh); > > > > > > - if (rte_mbuf_check(tx_pkt, true, &reason)) { > > + if (unlikely(rte_mbuf_check(tx_pkt, true, &reason))) { > > PMD_DRV_DP_LOG(DEBUG, "Invalid mbuf: %s", reason); > > - break; > > + txq->stats.dropped_pkts++; > > + txq->stats.errors++; > > + rte_pktmbuf_free(tx_pkt); > > + nb_drop++; > > + continue; > > } > > > > ol_flags = tx_pkt->ol_flags; > > @@ -292,6 +346,16 @@ gve_tx_burst_dqo(void *tx_queue, struct rte_mbuf > > **tx_pkts, uint16_t nb_pkts) > > > > nb_descs = gve_tx_pkt_nb_data_descs(tx_pkt); > > nb_descs += tso; > > + > > + /* Drop packet if it doesn't adhere to hardware limits. */ > > + if (unlikely(!gve_tx_validate_descs(txq, tx_pkt, nb_descs, > > tso))) { > > + txq->stats.dropped_pkts++; > > + txq->stats.errors++; > > + rte_pktmbuf_free(tx_pkt); > > + nb_drop++; > > + continue; > > + } > > + > > req_descs = nb_descs + > > GVE_TX_MIN_DESC_PREVENT_CACHE_OVERLAP; > > > > /* Clean if there aren't enough descriptors to send the > > packet > > @@ -306,12 +370,6 @@ gve_tx_burst_dqo(void *tx_queue, struct rte_mbuf > > **tx_pkts, uint16_t nb_pkts) > > break; > > } > > > > - /* Drop packet if it doesn't adhere to hardware limits. */ > > - if (!gve_tx_validate_descs(tx_pkt, nb_descs, tso)) { > > - txq->stats.too_many_descs++; > > - break; > > - } > > - > > if (!gve_free_compl_tags_pop(txq, &compl_tag)) > > break; > > > > @@ -357,7 +415,7 @@ gve_tx_burst_dqo(void *tx_queue, struct rte_mbuf > > **tx_pkts, uint16_t nb_pkts) > > } > > > > if (nb_tx > 0) { > > - txq->stats.packets += nb_tx; > > + txq->stats.packets += (nb_tx - nb_drop); > > txq->stats.bytes += bytes; > > txq->stats.errors += nb_pkts - nb_tx; > > } > > -- > > 2.43.0 > >
