diff mbox series

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

Message ID 20240211045511.1221879-1-kumaraparamesh92@gmail.com (mailing list archive)
State Changes Requested, archived
Delegated to: Ferruh Yigit
Headers
Series [v6] 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/github-robot: build success github build: passed
ci/iol-intel-Performance success Performance Testing PASS
ci/iol-intel-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-broadcom-Performance success Performance Testing PASS
ci/iol-broadcom-Functional success Functional Testing PASS
ci/iol-mellanox-Performance success Performance Testing PASS
ci/iol-sample-apps-testing success Testing PASS
ci/Intel-compilation success Compilation OK
ci/intel-Testing success Testing PASS
ci/intel-Functional success Functional PASS

Commit Message

Kumara Parameshwaran Feb. 11, 2024, 4:55 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

v6:
    Address review comments

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

Comments

Ferruh Yigit Feb. 12, 2024, 3:05 p.m. UTC | #1 
On 2/11/2024 4:55 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.
> 

Related to the patch title, 'gro: ' indicates a component for gro
library (lib/gro), but this is a testpmd patch, can you please update it as:
"app/testpmd: <verb> ..."

And can you please add a fixes tag [1], so this patch can be backported.

[1]
https://doc.dpdk.org/guides/contributing/patches.html#commit-messages-body


> 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
> 
>  app/test-pmd/csumonly.c | 14 ++++++++++----
>  1 file changed, 10 insertions(+), 4 deletions(-)
> 
> diff --git a/app/test-pmd/csumonly.c b/app/test-pmd/csumonly.c
> index c103e54111..21b45b4ba4 100644
> --- a/app/test-pmd/csumonly.c
> +++ b/app/test-pmd/csumonly.c
> @@ -863,16 +863,22 @@ 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))
> +			return false;
> +		if ((rte_gro_get_pkt_count(current_fwd_lcore()->gro_ctx) == 0))
> +			return false;
>

Why not "||" this condition to previous if block, instead of adding a
new if?


> +	}
> +#endif
>

Can you please put a comment in the GRO block that explains why these
checks are done and why we may not want to return although 'nb_rx == 0'?


Another option is make the block as below and keep the code below to get
'gro_enable':
```
if (unlikely(nb_rx == 0)) {
#ifndef RTE_LIB_GRO
	return false;
#else
	// Comment explaining what is done here
	gro_enable = gro_ports[fs->rx_port].enable;
	if (...)
		return false;
#endif
}
```

Above sample that keeps the #ifdef in a narrow scope ("nb_rx == 0"
block) looks tidier to me, but both works fine, no strong opinion from
me, what do you think?


>  	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..21b45b4ba4 100644
--- a/app/test-pmd/csumonly.c
+++ b/app/test-pmd/csumonly.c
@@ -863,16 +863,22 @@  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))
+			return false;
+		if ((rte_gro_get_pkt_count(current_fwd_lcore()->gro_ctx) == 0))
+			return false;
+	}
+#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;