diff mbox series

[v2,3/5] app/testpmd: enable burst stats for noisy vnf mode

Message ID 20200626220943.22526-3-honnappa.nagarahalli@arm.com (mailing list archive)
State Superseded, archived
Delegated to: Ferruh Yigit
Headers
Series [v2,1/5] app/testpmd: clock gettime call in throughput calculation |

Checks

Context Check Description
ci/checkpatch success coding style OK
ci/Intel-compilation success Compilation OK

Commit Message

Honnappa Nagarahalli June 26, 2020, 10:09 p.m. UTC 
  From: Phil Yang <phil.yang@arm.com>

Add burst stats for noisy vnf mode.

Fixes: 3c156061b938 ("app/testpmd: add noisy neighbour forwarding mode")
Cc: stable@dpdk.org
Cc: jfreimann@redhat.com

Signed-off-by: Phil Yang <phil.yang@arm.com>
Reviewed-by: Honnappa Nagarahalli <honnappa.nagarahalli@arm.com>
Reviewed-by: Ruifeng Wang <ruifeng.wang@arm.com>
Reviewed-by: Jens Freimann <jfreimann@redhat.com>
---
 app/test-pmd/noisy_vnf.c | 12 ++++++++++++
 1 file changed, 12 insertions(+)

Comments

Ferruh Yigit July 6, 2020, 4:08 p.m. UTC | #1 
On 6/26/2020 11:09 PM, Honnappa Nagarahalli wrote:
> From: Phil Yang <phil.yang@arm.com>
> 
> Add burst stats for noisy vnf mode.
> 
> Fixes: 3c156061b938 ("app/testpmd: add noisy neighbour forwarding mode")
> Cc: stable@dpdk.org
> Cc: jfreimann@redhat.com
> 
> Signed-off-by: Phil Yang <phil.yang@arm.com>
> Reviewed-by: Honnappa Nagarahalli <honnappa.nagarahalli@arm.com>
> Reviewed-by: Ruifeng Wang <ruifeng.wang@arm.com>
> Reviewed-by: Jens Freimann <jfreimann@redhat.com>
> ---
>  app/test-pmd/noisy_vnf.c | 12 ++++++++++++
>  1 file changed, 12 insertions(+)
> 
> diff --git a/app/test-pmd/noisy_vnf.c b/app/test-pmd/noisy_vnf.c
> index 58c4ee925..24f286da6 100644
> --- a/app/test-pmd/noisy_vnf.c
> +++ b/app/test-pmd/noisy_vnf.c
> @@ -154,6 +154,9 @@ pkt_burst_noisy_vnf(struct fwd_stream *fs)
>  
>  	nb_rx = rte_eth_rx_burst(fs->rx_port, fs->rx_queue,
>  			pkts_burst, nb_pkt_per_burst);
> +#ifdef RTE_TEST_PMD_RECORD_BURST_STATS
> +	fs->rx_burst_stats.pkt_burst_spread[nb_rx]++;
> +#endif
>  	if (unlikely(nb_rx == 0))
>  		goto flush;
>  	fs->rx_packets += nb_rx;
> @@ -164,6 +167,9 @@ pkt_burst_noisy_vnf(struct fwd_stream *fs)
>  				pkts_burst, nb_rx);
>  		if (unlikely(nb_tx < nb_rx) && fs->retry_enabled)
>  			nb_tx += do_retry(nb_rx, nb_tx, pkts_burst, fs);
> +#ifdef RTE_TEST_PMD_RECORD_BURST_STATS
> +		fs->tx_burst_stats.pkt_burst_spread[nb_tx]++;
> +#endif
>  		fs->tx_packets += nb_tx;
>  		fs->fwd_dropped += drop_pkts(pkts_burst, nb_rx, nb_tx);
>  		return;

No objection to add the burst stats to more forwarding engines, but this config
does not exist for the meson and make is going away.

We need to figure out how to support this option with meson before spreading it out.
Honnappa Nagarahalli July 6, 2020, 4:59 p.m. UTC | #2 
<snip>

> Subject: Re: [PATCH v2 3/5] app/testpmd: enable burst stats for noisy vnf
> mode
> 
> On 6/26/2020 11:09 PM, Honnappa Nagarahalli wrote:
> > From: Phil Yang <phil.yang@arm.com>
> >
> > Add burst stats for noisy vnf mode.
> >
> > Fixes: 3c156061b938 ("app/testpmd: add noisy neighbour forwarding
> > mode")
> > Cc: stable@dpdk.org
> > Cc: jfreimann@redhat.com
> >
> > Signed-off-by: Phil Yang <phil.yang@arm.com>
> > Reviewed-by: Honnappa Nagarahalli <honnappa.nagarahalli@arm.com>
> > Reviewed-by: Ruifeng Wang <ruifeng.wang@arm.com>
> > Reviewed-by: Jens Freimann <jfreimann@redhat.com>
> > ---
> >  app/test-pmd/noisy_vnf.c | 12 ++++++++++++
> >  1 file changed, 12 insertions(+)
> >
> > diff --git a/app/test-pmd/noisy_vnf.c b/app/test-pmd/noisy_vnf.c index
> > 58c4ee925..24f286da6 100644
> > --- a/app/test-pmd/noisy_vnf.c
> > +++ b/app/test-pmd/noisy_vnf.c
> > @@ -154,6 +154,9 @@ pkt_burst_noisy_vnf(struct fwd_stream *fs)
> >
> >  	nb_rx = rte_eth_rx_burst(fs->rx_port, fs->rx_queue,
> >  			pkts_burst, nb_pkt_per_burst);
> > +#ifdef RTE_TEST_PMD_RECORD_BURST_STATS
> > +	fs->rx_burst_stats.pkt_burst_spread[nb_rx]++;
> > +#endif
> >  	if (unlikely(nb_rx == 0))
> >  		goto flush;
> >  	fs->rx_packets += nb_rx;
> > @@ -164,6 +167,9 @@ pkt_burst_noisy_vnf(struct fwd_stream *fs)
> >  				pkts_burst, nb_rx);
> >  		if (unlikely(nb_tx < nb_rx) && fs->retry_enabled)
> >  			nb_tx += do_retry(nb_rx, nb_tx, pkts_burst, fs);
> > +#ifdef RTE_TEST_PMD_RECORD_BURST_STATS
> > +		fs->tx_burst_stats.pkt_burst_spread[nb_tx]++;
> > +#endif
> >  		fs->tx_packets += nb_tx;
> >  		fs->fwd_dropped += drop_pkts(pkts_burst, nb_rx, nb_tx);
> >  		return;
> 
> No objection to add the burst stats to more forwarding engines, but this
> config does not exist for the meson and make is going away.
> 
> We need to figure out how to support this option with meson before
> spreading it out.
Dharmik is working on making this a run time flag that can be enabled or disabled from command line (as was suggested in other emails). That would remove the requirement on meson.

This compile time flag existed before and this patch fixes a bug. IMO, we should separate the issue of how to enable this flag using meson from this bug fix.
Ferruh Yigit July 6, 2020, 5:11 p.m. UTC | #3 
On 7/6/2020 5:59 PM, Honnappa Nagarahalli wrote:
> <snip>
> 
>> Subject: Re: [PATCH v2 3/5] app/testpmd: enable burst stats for noisy vnf
>> mode
>>
>> On 6/26/2020 11:09 PM, Honnappa Nagarahalli wrote:
>>> From: Phil Yang <phil.yang@arm.com>
>>>
>>> Add burst stats for noisy vnf mode.
>>>
>>> Fixes: 3c156061b938 ("app/testpmd: add noisy neighbour forwarding
>>> mode")
>>> Cc: stable@dpdk.org
>>> Cc: jfreimann@redhat.com
>>>
>>> Signed-off-by: Phil Yang <phil.yang@arm.com>
>>> Reviewed-by: Honnappa Nagarahalli <honnappa.nagarahalli@arm.com>
>>> Reviewed-by: Ruifeng Wang <ruifeng.wang@arm.com>
>>> Reviewed-by: Jens Freimann <jfreimann@redhat.com>
>>> ---
>>>  app/test-pmd/noisy_vnf.c | 12 ++++++++++++
>>>  1 file changed, 12 insertions(+)
>>>
>>> diff --git a/app/test-pmd/noisy_vnf.c b/app/test-pmd/noisy_vnf.c index
>>> 58c4ee925..24f286da6 100644
>>> --- a/app/test-pmd/noisy_vnf.c
>>> +++ b/app/test-pmd/noisy_vnf.c
>>> @@ -154,6 +154,9 @@ pkt_burst_noisy_vnf(struct fwd_stream *fs)
>>>
>>>  	nb_rx = rte_eth_rx_burst(fs->rx_port, fs->rx_queue,
>>>  			pkts_burst, nb_pkt_per_burst);
>>> +#ifdef RTE_TEST_PMD_RECORD_BURST_STATS
>>> +	fs->rx_burst_stats.pkt_burst_spread[nb_rx]++;
>>> +#endif
>>>  	if (unlikely(nb_rx == 0))
>>>  		goto flush;
>>>  	fs->rx_packets += nb_rx;
>>> @@ -164,6 +167,9 @@ pkt_burst_noisy_vnf(struct fwd_stream *fs)
>>>  				pkts_burst, nb_rx);
>>>  		if (unlikely(nb_tx < nb_rx) && fs->retry_enabled)
>>>  			nb_tx += do_retry(nb_rx, nb_tx, pkts_burst, fs);
>>> +#ifdef RTE_TEST_PMD_RECORD_BURST_STATS
>>> +		fs->tx_burst_stats.pkt_burst_spread[nb_tx]++;
>>> +#endif
>>>  		fs->tx_packets += nb_tx;
>>>  		fs->fwd_dropped += drop_pkts(pkts_burst, nb_rx, nb_tx);
>>>  		return;
>>
>> No objection to add the burst stats to more forwarding engines, but this
>> config does not exist for the meson and make is going away.
>>
>> We need to figure out how to support this option with meson before
>> spreading it out.
> Dharmik is working on making this a run time flag that can be enabled or disabled from command line (as was suggested in other emails). That would remove the requirement on meson.

Cool, thanks.
Based on that work, shouldn't we need to update these lines again, instead why
not do the update after Dharmik's patch (or in that patch) based on what the new
way is?

> 
> This compile time flag existed before and this patch fixes a bug. IMO, we should separate the issue of how to enable this flag using meson from this bug fix.
> 

I know this flag exists before, but what is the bug this patch fixes? I thought
this patch enables burst stat for "noisy vnf" forwarding engine.
Honnappa Nagarahalli July 6, 2020, 5:41 p.m. UTC | #4 
<snip>

> >
> >> Subject: Re: [PATCH v2 3/5] app/testpmd: enable burst stats for noisy
> >> vnf mode
> >>
> >> On 6/26/2020 11:09 PM, Honnappa Nagarahalli wrote:
> >>> From: Phil Yang <phil.yang@arm.com>
> >>>
> >>> Add burst stats for noisy vnf mode.
> >>>
> >>> Fixes: 3c156061b938 ("app/testpmd: add noisy neighbour forwarding
> >>> mode")
> >>> Cc: stable@dpdk.org
> >>> Cc: jfreimann@redhat.com
> >>>
> >>> Signed-off-by: Phil Yang <phil.yang@arm.com>
> >>> Reviewed-by: Honnappa Nagarahalli <honnappa.nagarahalli@arm.com>
> >>> Reviewed-by: Ruifeng Wang <ruifeng.wang@arm.com>
> >>> Reviewed-by: Jens Freimann <jfreimann@redhat.com>
> >>> ---
> >>>  app/test-pmd/noisy_vnf.c | 12 ++++++++++++
> >>>  1 file changed, 12 insertions(+)
> >>>
> >>> diff --git a/app/test-pmd/noisy_vnf.c b/app/test-pmd/noisy_vnf.c
> >>> index
> >>> 58c4ee925..24f286da6 100644
> >>> --- a/app/test-pmd/noisy_vnf.c
> >>> +++ b/app/test-pmd/noisy_vnf.c
> >>> @@ -154,6 +154,9 @@ pkt_burst_noisy_vnf(struct fwd_stream *fs)
> >>>
> >>>  	nb_rx = rte_eth_rx_burst(fs->rx_port, fs->rx_queue,
> >>>  			pkts_burst, nb_pkt_per_burst);
> >>> +#ifdef RTE_TEST_PMD_RECORD_BURST_STATS
> >>> +	fs->rx_burst_stats.pkt_burst_spread[nb_rx]++;
> >>> +#endif
> >>>  	if (unlikely(nb_rx == 0))
> >>>  		goto flush;
> >>>  	fs->rx_packets += nb_rx;
> >>> @@ -164,6 +167,9 @@ pkt_burst_noisy_vnf(struct fwd_stream *fs)
> >>>  				pkts_burst, nb_rx);
> >>>  		if (unlikely(nb_tx < nb_rx) && fs->retry_enabled)
> >>>  			nb_tx += do_retry(nb_rx, nb_tx, pkts_burst, fs);
> >>> +#ifdef RTE_TEST_PMD_RECORD_BURST_STATS
> >>> +		fs->tx_burst_stats.pkt_burst_spread[nb_tx]++;
> >>> +#endif
> >>>  		fs->tx_packets += nb_tx;
> >>>  		fs->fwd_dropped += drop_pkts(pkts_burst, nb_rx, nb_tx);
> >>>  		return;
> >>
> >> No objection to add the burst stats to more forwarding engines, but
> >> this config does not exist for the meson and make is going away.
> >>
> >> We need to figure out how to support this option with meson before
> >> spreading it out.
> > Dharmik is working on making this a run time flag that can be enabled or
> disabled from command line (as was suggested in other emails). That would
> remove the requirement on meson.
> 
> Cool, thanks.
> Based on that work, shouldn't we need to update these lines again, instead
> why not do the update after Dharmik's patch (or in that patch) based on what
> the new way is?
Yes, agree, these lines need to be updated. I am fine to take this route as well.

> 
> >
> > This compile time flag existed before and this patch fixes a bug. IMO, we
> should separate the issue of how to enable this flag using meson from this
> bug fix.
> >
> 
> I know this flag exists before, but what is the bug this patch fixes? I thought
> this patch enables burst stat for "noisy vnf" forwarding engine.
If one enables 'RTE_TEST_PMD_RECORD_BURST_STATS' expecting the burst stats for 'noisy vnf' engine, it is not reported today.
diff mbox series

Patch

diff --git a/app/test-pmd/noisy_vnf.c b/app/test-pmd/noisy_vnf.c
index 58c4ee925..24f286da6 100644
--- a/app/test-pmd/noisy_vnf.c
+++ b/app/test-pmd/noisy_vnf.c
@@ -154,6 +154,9 @@  pkt_burst_noisy_vnf(struct fwd_stream *fs)
 
 	nb_rx = rte_eth_rx_burst(fs->rx_port, fs->rx_queue,
 			pkts_burst, nb_pkt_per_burst);
+#ifdef RTE_TEST_PMD_RECORD_BURST_STATS
+	fs->rx_burst_stats.pkt_burst_spread[nb_rx]++;
+#endif
 	if (unlikely(nb_rx == 0))
 		goto flush;
 	fs->rx_packets += nb_rx;
@@ -164,6 +167,9 @@  pkt_burst_noisy_vnf(struct fwd_stream *fs)
 				pkts_burst, nb_rx);
 		if (unlikely(nb_tx < nb_rx) && fs->retry_enabled)
 			nb_tx += do_retry(nb_rx, nb_tx, pkts_burst, fs);
+#ifdef RTE_TEST_PMD_RECORD_BURST_STATS
+		fs->tx_burst_stats.pkt_burst_spread[nb_tx]++;
+#endif
 		fs->tx_packets += nb_tx;
 		fs->fwd_dropped += drop_pkts(pkts_burst, nb_rx, nb_tx);
 		return;
@@ -187,6 +193,9 @@  pkt_burst_noisy_vnf(struct fwd_stream *fs)
 					nb_deqd);
 			if (unlikely(nb_tx < nb_rx) && fs->retry_enabled)
 				nb_tx += do_retry(nb_rx, nb_tx, tmp_pkts, fs);
+#ifdef RTE_TEST_PMD_RECORD_BURST_STATS
+			fs->tx_burst_stats.pkt_burst_spread[nb_tx]++;
+#endif
 			fs->fwd_dropped += drop_pkts(tmp_pkts, nb_deqd, nb_tx);
 		}
 	}
@@ -211,6 +220,9 @@  pkt_burst_noisy_vnf(struct fwd_stream *fs)
 					 tmp_pkts, nb_deqd);
 		if (unlikely(sent < nb_deqd) && fs->retry_enabled)
 			nb_tx += do_retry(nb_rx, nb_tx, tmp_pkts, fs);
+#ifdef RTE_TEST_PMD_RECORD_BURST_STATS
+		fs->tx_burst_stats.pkt_burst_spread[nb_tx]++;
+#endif
 		fs->fwd_dropped += drop_pkts(tmp_pkts, nb_deqd, sent);
 		ncf->prev_time = rte_get_timer_cycles();
 	}