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

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

[Kumara Parameshwaran]

In heavy-weight mode GRO which is based on timer, the GRO packets will not be
flushed inspite 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.

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

diff --git a/app/test-pmd/csumonly.c b/app/test-pmd/csumonly.c
index c103e54111..42f105ac16 100644
--- a/app/test-pmd/csumonly.c
+++ b/app/test-pmd/csumonly.c
@@ -863,16 +863,24 @@ 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)) {
+#ifdef RTE_LIB_GRO
+		gro_enable = gro_ports[fs->rx_port].enable;
+		if (unlikely(gro_enable && (gro_flush_cycles != GRO_DEFAULT_FLUSH_CYCLES))) {
+			goto init;
+		} else {
+			return false;
+		}
+#else
 		return false;
+#endif
+	}
 
+init:
 	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;
-- 
2.25.1

Re: [PATCH v1] gro : packets not getting flushed in heavy-weight mode API

[kumaraparameshwaran rathinavel]

[-- Attachment #1: Type: text/plain, Size: 1926 bytes --]

On Thu, Jan 18, 2024 at 1:22 AM Kumara Parameshwaran <
kumaraparamesh92@gmail.com> wrote:

> In heavy-weight mode GRO which is based on timer, the GRO packets will not
> be
> flushed inspite 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.
>
>  app/test-pmd/csumonly.c | 16 ++++++++++++----
>  1 file changed, 12 insertions(+), 4 deletions(-)
>
> diff --git a/app/test-pmd/csumonly.c b/app/test-pmd/csumonly.c
> index c103e54111..42f105ac16 100644
> --- a/app/test-pmd/csumonly.c
> +++ b/app/test-pmd/csumonly.c
> @@ -863,16 +863,24 @@ 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)) {
> +#ifdef RTE_LIB_GRO
> +               gro_enable = gro_ports[fs->rx_port].enable;
> +               if (unlikely(gro_enable && (gro_flush_cycles !=
> GRO_DEFAULT_FLUSH_CYCLES))) {
> +                       goto init;
> +               } else {
> +                       return false;
> +               }
> +#else
>                 return false;
> +#endif
> +       }
>
> +init:
>         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;
> --
> 2.25.1
> >>Please ignore this patch, this has an issue will raise a new one.
>
>

[-- Attachment #2: Type: text/html, Size: 2565 bytes --]

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

[Kumara Parameshwaran]

In heavy-weight mode GRO which is based on timer, the GRO packets will not be
flushed inspite 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

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

diff --git a/app/test-pmd/csumonly.c b/app/test-pmd/csumonly.c
index c103e54111..c1fde50126 100644
--- a/app/test-pmd/csumonly.c
+++ b/app/test-pmd/csumonly.c
@@ -863,16 +863,25 @@ 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);
+#ifdef RTE_LIB_GRO
+	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;
+		}
+	}
+#else
 	if (unlikely(nb_rx == 0))
 		return false;
+#endif
 
+init:
 	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;
-- 
2.25.1

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

[Kumara Parameshwaran]

In heavy-weight mode GRO which is based on timer, the GRO packets will not be
flushed inspite 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

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

diff --git a/app/test-pmd/csumonly.c b/app/test-pmd/csumonly.c
index c103e54111..b50f1b01d0 100644
--- a/app/test-pmd/csumonly.c
+++ b/app/test-pmd/csumonly.c
@@ -863,16 +863,24 @@ 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);
+#ifdef RTE_LIB_GRO
+	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;
+	}
+#else
 	if (unlikely(nb_rx == 0))
 		return false;
+#endif
 
+init:
 	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;
-- 
2.25.1

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

[Kumara Parameshwaran]

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
 app/test-pmd/csumonly.c | 14 +++++++++++---
 1 file changed, 11 insertions(+), 3 deletions(-)

diff --git a/app/test-pmd/csumonly.c b/app/test-pmd/csumonly.c
index c103e54111..4265f98c15 100644
--- a/app/test-pmd/csumonly.c
+++ b/app/test-pmd/csumonly.c
@@ -863,16 +863,24 @@ 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);
+#ifdef RTE_LIB_GRO
+	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;
+	}
+#else
 	if (unlikely(nb_rx == 0))
 		return false;
+#endif
 
+init:
 	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;
-- 
2.25.1

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

[Kumara Parameshwaran]

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(-)

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;
-- 
2.25.1

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

[Ferruh Yigit]

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;

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

[Kumara Parameshwaran]

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(-)

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;
-- 
2.25.1

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

[Ferruh Yigit]

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;

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

[Kumara Parameshwaran]

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: jiayu.hu@intel.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

 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..637a46d92a 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 condidtions should be the same as gro_ctx is valid
+		 * only when gro_flush_cycles is not the GRO_DEFAULT_FLUSH_CYCLES which
+		 * indicates ligth weight mode GRO
+		 */
+		if (!gro_enable || (gro_flush_cycles == GRO_DEFAULT_FLUSH_CYCLES) ||
+			(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;
@@ -1103,6 +1116,7 @@ pkt_burst_checksum_forward(struct fwd_stream *fs)
 	}
 
 #ifdef RTE_LIB_GRO
+	gro_enable = gro_ports[fs->rx_port].enable;
 	if (unlikely(gro_enable)) {
 		if (gro_flush_cycles == GRO_DEFAULT_FLUSH_CYCLES) {
 			nb_rx = rte_gro_reassemble_burst(pkts_burst, nb_rx,
-- 
2.25.1

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

[Kumara Parameshwaran]

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
+		 */
+		if (!gro_enable || (gro_flush_cycles == GRO_DEFAULT_FLUSH_CYCLES) ||
+			(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;
@@ -1103,6 +1116,7 @@ pkt_burst_checksum_forward(struct fwd_stream *fs)
 	}
 
 #ifdef RTE_LIB_GRO
+	gro_enable = gro_ports[fs->rx_port].enable;
 	if (unlikely(gro_enable)) {
 		if (gro_flush_cycles == GRO_DEFAULT_FLUSH_CYCLES) {
 			nb_rx = rte_gro_reassemble_burst(pkts_burst, nb_rx,
-- 
2.25.1

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

[Stephen Hemminger]

On Fri, 16 Feb 2024 09:10:25 +0530
Kumara Parameshwaran <kumaraparamesh92@gmail.com> wrote:

> +#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 condidtions should be the same as gro_ctx is valid

Run spell check on the comments please

> +		 * only when gro_flush_cycles is not the GRO_DEFAULT_FLUSH_CYCLES which
> +		 * indicates ligth weight mode GRO
> +		 */
> +		if (!gro_enable || (gro_flush_cycles == GRO_DEFAULT_FLUSH_CYCLES) ||
> +			(rte_gro_get_pkt_count(current_fwd_lcore()->gro_ctx) == 0))
> +			return false;
> +#endif

FYI - Linux kernel GRO does a flush when ever NAPI finishes. Pretty much equivalent
to this but flushes more frequently.

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

[Ferruh Yigit]

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?

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

[Ferruh Yigit]

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.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?
> 
> 

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.yigit@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.

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

[kumaraparameshwaran rathinavel]

[-- Attachment #1: Type: text/plain, Size: 3849 bytes --]

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

[-- Attachment #2: Type: text/html, Size: 5260 bytes --]