Re: [PATCH v8] app/testpmd : fix packets not getting flushed in heavy-weight mode API

[Ferruh Yigit <ferruh.yigit@amd.com>]

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?