[v8] app/testpmd : fix 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.
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(-)
Comments
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?
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.
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.
>
>
@@ -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,