diff mbox series

[v5] gro : packets not getting flushed in heavy-weight mode API

Message ID 20240118083626.432048-1-kumaraparamesh92@gmail.com (mailing list archive)
State Changes Requested, archived
Delegated to: Ferruh Yigit
Headers
Series [v5] gro : packets not getting flushed in heavy-weight mode API |

Checks

Context Check Description
ci/checkpatch success coding style OK
ci/loongarch-compilation success Compilation OK
ci/loongarch-unit-testing success Unit Testing PASS
ci/Intel-compilation success Compilation OK
ci/intel-Testing success Testing PASS
ci/iol-intel-Performance success Performance Testing PASS
ci/intel-Functional success Functional PASS
ci/iol-broadcom-Performance success Performance Testing PASS
ci/iol-intel-Functional success Functional Testing PASS
ci/iol-broadcom-Functional success Functional Testing PASS
ci/iol-abi-testing success Testing PASS
ci/iol-compile-amd64-testing success Testing PASS
ci/iol-unit-amd64-testing success Testing PASS
ci/iol-unit-arm64-testing success Testing PASS
ci/iol-compile-arm64-testing success Testing PASS
ci/iol-mellanox-Performance success Performance Testing PASS
ci/iol-sample-apps-testing success Testing PASS

Commit Message

Kumara Parameshwaran Jan. 18, 2024, 8:36 a.m. UTC 
  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.

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

 app/test-pmd/csumonly.c | 15 +++++++++++----
 1 file changed, 11 insertions(+), 4 deletions(-)

Comments

Ferruh Yigit Feb. 7, 2024, 10:57 p.m. UTC | #1 
On 1/18/2024 8:36 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.
> 

Agree on the problem, need a way to flush existing packets in GRO
context when no more packet receiving.


> 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
> 
>  app/test-pmd/csumonly.c | 15 +++++++++++----
>  1 file changed, 11 insertions(+), 4 deletions(-)
> 
> diff --git a/app/test-pmd/csumonly.c b/app/test-pmd/csumonly.c
> index c103e54111..6d9ce99500 100644
> --- a/app/test-pmd/csumonly.c
> +++ b/app/test-pmd/csumonly.c
> @@ -863,16 +863,23 @@ 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))
> +			goto init;
>

Rest of the function expects 'nb_rx' non zero, so I am concerned about
unexpected side effects, like:

- GRO reassembles packets and flush, how reassembles behaves with zero
nb_rx?

- If there is no packet in GRO context, what happens to call with zero
nb_rx?


To address above issue, what about add 'rte_gro_get_pkt_count()' check
and only continue if it return not zero.
Also in below GRO block, call 'rte_gro_reassemble()' only when 'nb_rx'
is not zero.

> +		else
> +			return false;
>

'goto' can be prevented by changing the condition in if block.


> +	}
> +init:
>

Label name 'init' is not a good name.

> +#endif
>  	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;
diff mbox series

Patch

diff --git a/app/test-pmd/csumonly.c b/app/test-pmd/csumonly.c
index c103e54111..6d9ce99500 100644
--- a/app/test-pmd/csumonly.c
+++ b/app/test-pmd/csumonly.c
@@ -863,16 +863,23 @@  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))
+			goto init;
+		else
+			return false;
+	}
+init:
+#endif
 	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;