On Wed, Feb 21, 2024 at 11:32 PM Ferruh Yigit <ferruh.yigit@amd.com> wrote:

> 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: hujiayu.hu@foxmail.com
> >>
> >> Signed-off-by: Kumara Parameshwaran <kumaraparamesh92@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
> >>
> >> 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 <ferruh.yigit@amd.com>
>
> 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.
>
> 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.
> Thanks.
>
>