[v6] gro : packets not getting flushed in heavy-weight mode API
Checks
Commit Message
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
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;
@@ -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;