On Wed, Feb 21, 2024 at 11:32 PM Ferruh Yigit <ferruh.yi...@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...@foxmail.com
> >>
> >> 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
> >>
> >> 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.yi...@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.
>
>
