On 2/11/2024 4:55 AM, Kumara Parameshwaran wrote: > In heavy-weight mode GRO which is based on timer, the GRO packets > will not be flushed in spite of timer expiry if there is no packet > in the current poll. If timer mode GRO is enabled the > rte_gro_timeout_flush API should be invoked. >
Related to the patch title, 'gro: ' indicates a component for gro library (lib/gro), but this is a testpmd patch, can you please update it as: "app/testpmd: <verb> ..." And can you please add a fixes tag [1], so this patch can be backported. [1] https://doc.dpdk.org/guides/contributing/patches.html#commit-messages-body > Signed-off-by: Kumara Parameshwaran <kumaraparames...@gmail.com> > --- > v1: > Changes to make sure that the GRO flush API is invoked if there are no > packets in > current poll and timer expiry. > > v2: > Fix code organisation issue > > v3: > Fix warnings > > v4: > Fix error and warnings > > v5: > Fix compilation issue when GRO is not defined > > v6: > Address review comments > > app/test-pmd/csumonly.c | 14 ++++++++++---- > 1 file changed, 10 insertions(+), 4 deletions(-) > > diff --git a/app/test-pmd/csumonly.c b/app/test-pmd/csumonly.c > index c103e54111..21b45b4ba4 100644 > --- a/app/test-pmd/csumonly.c > +++ b/app/test-pmd/csumonly.c > @@ -863,16 +863,22 @@ pkt_burst_checksum_forward(struct fwd_stream *fs) > > /* receive a burst of packet */ > nb_rx = common_fwd_stream_receive(fs, pkts_burst, nb_pkt_per_burst); > +#ifndef RTE_LIB_GRO > if (unlikely(nb_rx == 0)) > return false; > - > +#else > + gro_enable = gro_ports[fs->rx_port].enable; > + if (unlikely(nb_rx == 0)) { > + if (!gro_enable || (gro_flush_cycles == > GRO_DEFAULT_FLUSH_CYCLES)) > + return false; > + if ((rte_gro_get_pkt_count(current_fwd_lcore()->gro_ctx) == 0)) > + return false; > Why not "||" this condition to previous if block, instead of adding a new if? > + } > +#endif > Can you please put a comment in the GRO block that explains why these checks are done and why we may not want to return although 'nb_rx == 0'? Another option is make the block as below and keep the code below to get 'gro_enable': ``` if (unlikely(nb_rx == 0)) { #ifndef RTE_LIB_GRO return false; #else // Comment explaining what is done here gro_enable = gro_ports[fs->rx_port].enable; if (...) return false; #endif } ``` Above sample that keeps the #ifdef in a narrow scope ("nb_rx == 0" block) looks tidier to me, but both works fine, no strong opinion from me, what do you think? > rx_bad_ip_csum = 0; > rx_bad_l4_csum = 0; > rx_bad_outer_l4_csum = 0; > rx_bad_outer_ip_csum = 0; > -#ifdef RTE_LIB_GRO > - gro_enable = gro_ports[fs->rx_port].enable; > -#endif > > txp = &ports[fs->tx_port]; > tx_offloads = txp->dev_conf.txmode.offloads;
