<div dir="ltr"><div dir="ltr"> </div> <div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Wed, Feb 21, 2024 at 11:32 PM Ferruh Yigit <<a href="mailto:ferruh.yigit@amd.com">ferruh.yigit@amd.com</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"><div><div>On 2/16/2024 1:56 PM, Ferruh Yigit wrote: > On 2/16/2024 3:47 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. >> >> Fixes: b7091f1dcfbc ("app/testpmd: enable the heavyweight mode TCP/IPv4 GRO") >> Cc: <a href="mailto:hujiayu.hu@foxmail.com" target="_blank">hujiayu.hu@foxmail.com</a> >> >> Signed-off-by: Kumara Parameshwaran <<a href="mailto:kumaraparamesh92@gmail.com" target="_blank">kumaraparamesh92@gmail.com</a>> >> --- >> 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 >> >> v7: >> Address review comments >> >> v8: >> Fix spell check warnings >> >> app/test-pmd/csumonly.c | 22 ++++++++++++++++++---- >> 1 file changed, 18 insertions(+), 4 deletions(-) >> >> diff --git a/app/test-pmd/csumonly.c b/app/test-pmd/csumonly.c >> index c103e54111..a922160f6d 100644 >> --- a/app/test-pmd/csumonly.c >> +++ b/app/test-pmd/csumonly.c >> @@ -863,16 +863,29 @@ 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); >> - if (unlikely(nb_rx == 0)) >> + if (unlikely(nb_rx == 0)) { >> +#ifndef RTE_LIB_GRO >> return false; >> +#else >> + gro_enable = gro_ports[fs->rx_port].enable; >> + /* >> + * Make sure that in case of Heavyweight mode GRO the packets in >> + * GRO cache should be flushed as the timer could have expired. >> + * >> + * The order of conditions should be the same as gro_ctx is valid >> + * only when gro_flush_cycles is not the GRO_DEFAULT_FLUSH_CYCLES which >> + * indicates light weight mode GRO >> + */ >> > > Updated comment as below to make it terse, what do you think: > /* > * Check if packets need to be flushed in the GRO context > * due to a timeout. > * > * Continue only in GRO heavyweight mode and if there are > * packets in the GRO context. > */ > > >> + if (!gro_enable || (gro_flush_cycles == GRO_DEFAULT_FLUSH_CYCLES) || >> + (rte_gro_get_pkt_count(current_fwd_lcore()->gro_ctx) == 0)) >> + return false; >> +#endif >> + } >> > > Another issue but also related to your patch, if there is no packet to > Tx after GRO block, should we add another zero packet check: > if (unlikely(nb_rx == 0)) > return false; > > To prevent executing GSO and Tx path code with zero packet, do you think > does this make sense? > > Patch looks good to me, with above comment update, but I am worried about side impacts of this patch that we might be missing, that is why I would like it to be in -rc1, so that it can be tested better. Hence, Reviewed-by: Ferruh Yigit <<a href="mailto:ferruh.yigit@amd.com" target="_blank">ferruh.yigit@amd.com</a>> Applied to dpdk-next-net/main, thanks. (Updated comment as suggested above while merging.) Lets continue to discuss return on "nb_rx == 0" case after GRO block, incremental to this patch. </div>I was not able to get to this. I will also take a look at the code to see if this can cause any issues. </div>Thanks. <div><div><div> </div></div></div></blockquote></div></div>