diff mbox series

[v4,1/4] app/testpmd: add missing newline when showing statistics

Message ID 1553261824-1881-2-git-send-email-david.marchand@redhat.com (mailing list archive)
State Changes Requested, archived
Delegated to: Ferruh Yigit
Headers
Series display testpmd forwarding engine stats on the fly |

Checks

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

Commit Message

David Marchand March 22, 2019, 1:37 p.m. UTC 
  Having the standard stats and the rx burst stats on the same line gives a
really long line and is not consistent with the rest.

Before:
  RX-packets: 3542977        TX-packets: 3542971        TX-dropped: 6               RX-bursts : 499440 [24% of 2 pkts + 15% of 1 pkts + 61% of others]
  TX-bursts : 499440 [24% of 2 pkts + 15% of 1 pkts + 61% of others]

After:
  RX-packets: 4629969        TX-packets: 4629969        TX-dropped: 0
  RX-bursts : 663328 [19% of 2 pkts + 17% of 3 pkts + 64% of others]
  TX-bursts : 663328 [19% of 2 pkts + 17% of 3 pkts + 64% of others]

Fixes: af75078fece3 ("first public release")
Cc: stable@dpdk.org

Signed-off-by: David Marchand <david.marchand@redhat.com>
Reviewed-by: Rami Rosen <ramirose@gmail.com>
Reviewed-by: Andrew Rybchenko <arybchenko@solarflare.com>
---
Changelog since v2:
- Cc'd stable

---
 app/test-pmd/testpmd.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

Maxime Coquelin March 22, 2019, 5:03 p.m. UTC | #1 
On 3/22/19 2:37 PM, David Marchand wrote:
> Having the standard stats and the rx burst stats on the same line gives a
> really long line and is not consistent with the rest.
> 
> Before:
>    RX-packets: 3542977        TX-packets: 3542971        TX-dropped: 6               RX-bursts : 499440 [24% of 2 pkts + 15% of 1 pkts + 61% of others]
>    TX-bursts : 499440 [24% of 2 pkts + 15% of 1 pkts + 61% of others]
> 
> After:
>    RX-packets: 4629969        TX-packets: 4629969        TX-dropped: 0
>    RX-bursts : 663328 [19% of 2 pkts + 17% of 3 pkts + 64% of others]
>    TX-bursts : 663328 [19% of 2 pkts + 17% of 3 pkts + 64% of others]
> 
> Fixes: af75078fece3 ("first public release")
> Cc: stable@dpdk.org
> 
> Signed-off-by: David Marchand <david.marchand@redhat.com>
> Reviewed-by: Rami Rosen <ramirose@gmail.com>
> Reviewed-by: Andrew Rybchenko <arybchenko@solarflare.com>
> ---
> Changelog since v2:
> - Cc'd stable
> 
> ---
>   app/test-pmd/testpmd.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/app/test-pmd/testpmd.c b/app/test-pmd/testpmd.c
> index 216be47..40199c1 100644
> --- a/app/test-pmd/testpmd.c
> +++ b/app/test-pmd/testpmd.c
> @@ -1459,7 +1459,7 @@ struct extmem_param {
>   	       "TX Port=%2d/Queue=%2d %s\n",
>   	       fwd_top_stats_border, fs->rx_port, fs->rx_queue,
>   	       fs->tx_port, fs->tx_queue, fwd_top_stats_border);
> -	printf("  RX-packets: %-14u TX-packets: %-14u TX-dropped: %-14u",
> +	printf("  RX-packets: %-14u TX-packets: %-14u TX-dropped: %-14u\n",
>   	       fs->rx_packets, fs->tx_packets, fs->fwd_dropped);
>   
>   	/* if checksum mode */
> 

Reviewed-by: Maxime Coquelin <maxime.coquelin@redhat.com>
Maxime Coquelin March 22, 2019, 5:17 p.m. UTC | #2 
On 3/22/19 2:37 PM, David Marchand wrote:
> Having the standard stats and the rx burst stats on the same line gives a
> really long line and is not consistent with the rest.
> 
> Before:
>    RX-packets: 3542977        TX-packets: 3542971        TX-dropped: 6               RX-bursts : 499440 [24% of 2 pkts + 15% of 1 pkts + 61% of others]
>    TX-bursts : 499440 [24% of 2 pkts + 15% of 1 pkts + 61% of others]
> 
> After:
>    RX-packets: 4629969        TX-packets: 4629969        TX-dropped: 0
>    RX-bursts : 663328 [19% of 2 pkts + 17% of 3 pkts + 64% of others]
>    TX-bursts : 663328 [19% of 2 pkts + 17% of 3 pkts + 64% of others]
> 
> Fixes: af75078fece3 ("first public release")
> Cc:stable@dpdk.org

While the patch is good, I wonder whether we should backport it.
Indeed, it might break some scripts parsing testpmd output.

Any thoughts?

> Signed-off-by: David Marchand<david.marchand@redhat.com>
> Reviewed-by: Rami Rosen<ramirose@gmail.com>
> Reviewed-by: Andrew Rybchenko<arybchenko@solarflare.com>
> ---
> Changelog since v2:
> - Cc'd stable
> 
> ---
David Marchand March 22, 2019, 5:23 p.m. UTC | #3 
On Fri, Mar 22, 2019 at 6:17 PM Maxime Coquelin <maxime.coquelin@redhat.com>
wrote:

>
>
> On 3/22/19 2:37 PM, David Marchand wrote:
> > Having the standard stats and the rx burst stats on the same line gives a
> > really long line and is not consistent with the rest.
> >
> > Before:
> >    RX-packets: 3542977        TX-packets: 3542971        TX-dropped: 6
>              RX-bursts : 499440 [24% of 2 pkts + 15% of 1 pkts + 61% of
> others]
> >    TX-bursts : 499440 [24% of 2 pkts + 15% of 1 pkts + 61% of others]
> >
> > After:
> >    RX-packets: 4629969        TX-packets: 4629969        TX-dropped: 0
> >    RX-bursts : 663328 [19% of 2 pkts + 17% of 3 pkts + 64% of others]
> >    TX-bursts : 663328 [19% of 2 pkts + 17% of 3 pkts + 64% of others]
> >
> > Fixes: af75078fece3 ("first public release")
> > Cc:stable@dpdk.org
>
> While the patch is good, I wonder whether we should backport it.
> Indeed, it might break some scripts parsing testpmd output.
>
> Any thoughts?
>

It seems unlikely, this feature is disabled by default.
But yes, I would avoid backporting it.
Andrew Rybchenko March 22, 2019, 5:31 p.m. UTC | #4 
On 22.03.2019 20:23, David Marchand wrote:
>
>
> On Fri, Mar 22, 2019 at 6:17 PM Maxime Coquelin 
> <maxime.coquelin@redhat.com <mailto:maxime.coquelin@redhat.com>> wrote:
>
>
>
>     On 3/22/19 2:37 PM, David Marchand wrote:
>     > Having the standard stats and the rx burst stats on the same
>     line gives a
>     > really long line and is not consistent with the rest.
>     >
>     > Before:
>     >    RX-packets: 3542977        TX-packets: 3542971   TX-dropped:
>     6               RX-bursts : 499440 [24% of 2 pkts + 15% of 1 pkts
>     + 61% of others]
>     >    TX-bursts : 499440 [24% of 2 pkts + 15% of 1 pkts + 61% of
>     others]
>     >
>     > After:
>     >    RX-packets: 4629969        TX-packets: 4629969   TX-dropped: 0
>     >    RX-bursts : 663328 [19% of 2 pkts + 17% of 3 pkts + 64% of
>     others]
>     >    TX-bursts : 663328 [19% of 2 pkts + 17% of 3 pkts + 64% of
>     others]
>     >
>     > Fixes: af75078fece3 ("first public release")
>     > Cc:stable@dpdk.org <mailto:Cc%3Astable@dpdk.org>
>
>     While the patch is good, I wonder whether we should backport it.
>     Indeed, it might break some scripts parsing testpmd output.
>
>     Any thoughts?
>
>
> It seems unlikely, this feature is disabled by default.
> But yes, I would avoid backporting it.


For me it looks like a bug. That's why I think it should be backported, 
but the fix is not 100% correct in fact. I'll reply directly to the patch.
Andrew Rybchenko March 22, 2019, 5:35 p.m. UTC | #5 
On 22.03.2019 16:37, David Marchand wrote:
> Having the standard stats and the rx burst stats on the same line gives a
> really long line and is not consistent with the rest.
>
> Before:
>    RX-packets: 3542977        TX-packets: 3542971        TX-dropped: 6               RX-bursts : 499440 [24% of 2 pkts + 15% of 1 pkts + 61% of others]
>    TX-bursts : 499440 [24% of 2 pkts + 15% of 1 pkts + 61% of others]
>
> After:
>    RX-packets: 4629969        TX-packets: 4629969        TX-dropped: 0
>    RX-bursts : 663328 [19% of 2 pkts + 17% of 3 pkts + 64% of others]
>    TX-bursts : 663328 [19% of 2 pkts + 17% of 3 pkts + 64% of others]
>
> Fixes: af75078fece3 ("first public release")
> Cc: stable@dpdk.org
>
> Signed-off-by: David Marchand <david.marchand@redhat.com>
> Reviewed-by: Rami Rosen <ramirose@gmail.com>
> Reviewed-by: Andrew Rybchenko <arybchenko@solarflare.com>
> ---
> Changelog since v2:
> - Cc'd stable
>
> ---
>   app/test-pmd/testpmd.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/app/test-pmd/testpmd.c b/app/test-pmd/testpmd.c
> index 216be47..40199c1 100644
> --- a/app/test-pmd/testpmd.c
> +++ b/app/test-pmd/testpmd.c
> @@ -1459,7 +1459,7 @@ struct extmem_param {
>   	       "TX Port=%2d/Queue=%2d %s\n",
>   	       fwd_top_stats_border, fs->rx_port, fs->rx_queue,
>   	       fs->tx_port, fs->tx_queue, fwd_top_stats_border);
> -	printf("  RX-packets: %-14u TX-packets: %-14u TX-dropped: %-14u",
> +	printf("  RX-packets: %-14u TX-packets: %-14u TX-dropped: %-14u\n",
>   	       fs->rx_packets, fs->tx_packets, fs->fwd_dropped);
>   
>   	/* if checksum mode */


In fact bad Rx checksum counters follow and I think they should be in 
this line.

That's why there is no \n here from the very beginning.

My fix for the bug (local) just add \n in else branch below.


Andrew.
David Marchand March 22, 2019, 5:43 p.m. UTC | #6 
On Fri, Mar 22, 2019 at 6:35 PM Andrew Rybchenko <arybchenko@solarflare.com>
wrote:

> On 22.03.2019 16:37, David Marchand wrote:
> > Having the standard stats and the rx burst stats on the same line gives a
> > really long line and is not consistent with the rest.
> >
> > Before:
> >    RX-packets: 3542977        TX-packets: 3542971        TX-dropped: 6
>              RX-bursts : 499440 [24% of 2 pkts + 15% of 1 pkts + 61% of
> others]
> >    TX-bursts : 499440 [24% of 2 pkts + 15% of 1 pkts + 61% of others]
> >
> > After:
> >    RX-packets: 4629969        TX-packets: 4629969        TX-dropped: 0
> >    RX-bursts : 663328 [19% of 2 pkts + 17% of 3 pkts + 64% of others]
> >    TX-bursts : 663328 [19% of 2 pkts + 17% of 3 pkts + 64% of others]
> >
> > Fixes: af75078fece3 ("first public release")
> > Cc: stable@dpdk.org
> >
> > Signed-off-by: David Marchand <david.marchand@redhat.com>
> > Reviewed-by: Rami Rosen <ramirose@gmail.com>
> > Reviewed-by: Andrew Rybchenko <arybchenko@solarflare.com>
> > ---
> > Changelog since v2:
> > - Cc'd stable
> >
> > ---
> >   app/test-pmd/testpmd.c | 2 +-
> >   1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/app/test-pmd/testpmd.c b/app/test-pmd/testpmd.c
> > index 216be47..40199c1 100644
> > --- a/app/test-pmd/testpmd.c
> > +++ b/app/test-pmd/testpmd.c
> > @@ -1459,7 +1459,7 @@ struct extmem_param {
> >              "TX Port=%2d/Queue=%2d %s\n",
> >              fwd_top_stats_border, fs->rx_port, fs->rx_queue,
> >              fs->tx_port, fs->tx_queue, fwd_top_stats_border);
> > -     printf("  RX-packets: %-14u TX-packets: %-14u TX-dropped: %-14u",
> > +     printf("  RX-packets: %-14u TX-packets: %-14u TX-dropped: %-14u\n",
> >              fs->rx_packets, fs->tx_packets, fs->fwd_dropped);
> >
> >       /* if checksum mode */
>
>
> In fact bad Rx checksum counters follow and I think they should be in
> this line.
>
> That's why there is no \n here from the very beginning.
>
> My fix for the bug (local) just add \n in else branch below.
>


Mm, ok, I will look at this monday.
I might add the \n in pkt_burst_stats_display() and update other callers.
David Marchand March 23, 2019, 7:12 p.m. UTC | #7 
On Fri, Mar 22, 2019 at 6:43 PM David Marchand <david.marchand@redhat.com>
wrote:

> On Fri, Mar 22, 2019 at 6:35 PM Andrew Rybchenko <
> arybchenko@solarflare.com> wrote:
>
>>
>> In fact bad Rx checksum counters follow and I think they should be in
>> this line.
>>
>> That's why there is no \n here from the very beginning.
>>
>> My fix for the bug (local) just add \n in else branch below.
>>
>
Just to be clear.

origin/master:
- iofwd engine:
  ------- Forward Stats for RX Port= 0/Queue= 0 -> TX Port= 1/Queue= 0
-------
  RX-packets: 121811360      TX-packets: 121811392      TX-dropped:
0               RX-bursts : 3806605 [100% of 32 pkts]
  TX-bursts : 3806606 [100% of 32 pkts]

- csum engine:
  ------- Forward Stats for RX Port= 0/Queue= 0 -> TX Port= 1/Queue= 0
-------
  RX-packets: 5467488        TX-packets: 5467520        TX-dropped:
0               RX- bad IP checksum: 0               Rx- bad L4 checksum:
0              Rx- bad outer L4 checksum: 0
  RX-bursts : 170859 [100% of 32 pkts]
  TX-bursts : 170860 [100% of 32 pkts]


So, as suggested, I added a printf("\n") in the else for the csum engine
block:

- iofwd engine:
  ------- Forward Stats for RX Port= 0/Queue= 0 -> TX Port= 1/Queue= 0
-------
  RX-packets: 259770560      TX-packets: 259770592      TX-dropped:
0
  RX-bursts : 8117830 [100% of 32 pkts]
  TX-bursts : 8117831 [100% of 32 pkts]

- csum engine:
  ------- Forward Stats for RX Port= 0/Queue= 0 -> TX Port= 1/Queue= 0
-------
  RX-packets: 7834016        TX-packets: 7834048        TX-dropped:
0               RX- bad IP checksum: 0               Rx- bad L4 checksum:
0              Rx- bad outer L4 checksum: 0
  RX-bursts : 244813 [100% of 32 pkts]
  TX-bursts : 244814 [100% of 32 pkts]


deal ?
Andrew Rybchenko March 25, 2019, 6:34 a.m. UTC | #8 
On 3/23/19 10:12 PM, David Marchand wrote:
> On Fri, Mar 22, 2019 at 6:43 PM David Marchand <david.marchand@redhat.com>
> wrote:
>
>> On Fri, Mar 22, 2019 at 6:35 PM Andrew Rybchenko <
>> arybchenko@solarflare.com> wrote:
>>
>>> In fact bad Rx checksum counters follow and I think they should be in
>>> this line.
>>>
>>> That's why there is no \n here from the very beginning.
>>>
>>> My fix for the bug (local) just add \n in else branch below.
>>>
> Just to be clear.
>
> origin/master:
> - iofwd engine:
>    ------- Forward Stats for RX Port= 0/Queue= 0 -> TX Port= 1/Queue= 0
> -------
>    RX-packets: 121811360      TX-packets: 121811392      TX-dropped:
> 0               RX-bursts : 3806605 [100% of 32 pkts]
>    TX-bursts : 3806606 [100% of 32 pkts]
>
> - csum engine:
>    ------- Forward Stats for RX Port= 0/Queue= 0 -> TX Port= 1/Queue= 0
> -------
>    RX-packets: 5467488        TX-packets: 5467520        TX-dropped:
> 0               RX- bad IP checksum: 0               Rx- bad L4 checksum:
> 0              Rx- bad outer L4 checksum: 0
>    RX-bursts : 170859 [100% of 32 pkts]
>    TX-bursts : 170860 [100% of 32 pkts]
>
>
> So, as suggested, I added a printf("\n") in the else for the csum engine
> block:
>
> - iofwd engine:
>    ------- Forward Stats for RX Port= 0/Queue= 0 -> TX Port= 1/Queue= 0
> -------
>    RX-packets: 259770560      TX-packets: 259770592      TX-dropped:
> 0
>    RX-bursts : 8117830 [100% of 32 pkts]
>    TX-bursts : 8117831 [100% of 32 pkts]
>
> - csum engine:
>    ------- Forward Stats for RX Port= 0/Queue= 0 -> TX Port= 1/Queue= 0
> -------
>    RX-packets: 7834016        TX-packets: 7834048        TX-dropped:
> 0               RX- bad IP checksum: 0               Rx- bad L4 checksum:
> 0              Rx- bad outer L4 checksum: 0
>    RX-bursts : 244813 [100% of 32 pkts]
>    TX-bursts : 244814 [100% of 32 pkts]
>
>
> deal ?

Yes, thanks a lot.

Andrew.
diff mbox series

Patch

diff --git a/app/test-pmd/testpmd.c b/app/test-pmd/testpmd.c
index 216be47..40199c1 100644
--- a/app/test-pmd/testpmd.c
+++ b/app/test-pmd/testpmd.c
@@ -1459,7 +1459,7 @@  struct extmem_param {
 	       "TX Port=%2d/Queue=%2d %s\n",
 	       fwd_top_stats_border, fs->rx_port, fs->rx_queue,
 	       fs->tx_port, fs->tx_queue, fwd_top_stats_border);
-	printf("  RX-packets: %-14u TX-packets: %-14u TX-dropped: %-14u",
+	printf("  RX-packets: %-14u TX-packets: %-14u TX-dropped: %-14u\n",
 	       fs->rx_packets, fs->tx_packets, fs->fwd_dropped);
 
 	/* if checksum mode */