diff mbox series

[v4,5/9] app/procinfo: add support for show tm

Message ID 20181106124912.40700-5-vipin.varghese@intel.com (mailing list archive)
State Superseded, archived
Delegated to: Thomas Monjalon
Headers show
Series [v4,1/9] app/procinfo: add usage for new debug | expand

Checks

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

Commit Message

Varghese, Vipin Nov. 6, 2018, 12:49 p.m. UTC
Function show_tm is used for displaying the tm PMD under the
primary process. This covers basic and per node|level details with
stats.

Signed-off-by: Vipin Varghese <vipin.varghese@intel.com>
---

V3:
 - memset for struct elements - Vipin Varghese
 - code cleanup for TM - Vipin Varghese
 - fetch for leaf nodes if node exist - Jasvinder Singh
 - display MARCO to function - Reshma Pathan & Stephen Hemminger

V2:
 - MACRO for display node|level - cap - Vipin Varghese
---
 app/proc-info/main.c | 276 ++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 275 insertions(+), 1 deletion(-)

Comments

Pattan, Reshma Nov. 21, 2018, 2:28 p.m. UTC | #1
> -----Original Message-----
> From: Varghese, Vipin
> Sent: Tuesday, November 6, 2018 12:49 PM
> To: dev@dpdk.org; thomas@monjalon.net; Pattan, Reshma
> <reshma.pattan@intel.com>; stephen@networkplumber.org; Mcnamara, John
> <john.mcnamara@intel.com>
> Cc: Byrne, Stephen1 <stephen1.byrne@intel.com>; Glynn, Michael J
> <michael.j.glynn@intel.com>; Patel, Amol <amol.patel@intel.com>; Varghese,
> Vipin <vipin.varghese@intel.com>
> Subject: [PATCH v4 5/9] app/procinfo: add support for show tm


> +			if ((ret) | (!is_leaf))
> +

Is the operator here should be || ?
Varghese, Vipin Nov. 22, 2018, 1:28 p.m. UTC | #2
Hi Reshma,

<snipped>

> 
> > +			if ((ret) | (!is_leaf))
> > +
> 
> Is the operator here should be || ?
> 
> 

Check is done for 'if either ret is not 0 or if it ret is 0 but not leaf' we skip leaf details print. If 'ret is 0 and is leaf' we skip continue to print leaf details.
Pattan, Reshma Nov. 23, 2018, 11:50 a.m. UTC | #3
> -----Original Message-----
> From: Varghese, Vipin
> Sent: Thursday, November 22, 2018 1:28 PM
> To: Pattan, Reshma <reshma.pattan@intel.com>; dev@dpdk.org;
> thomas@monjalon.net; stephen@networkplumber.org; Mcnamara, John
> <john.mcnamara@intel.com>
> Cc: Byrne, Stephen1 <stephen1.byrne@intel.com>; Glynn, Michael J
> <michael.j.glynn@intel.com>; Patel, Amol <amol.patel@intel.com>
> Subject: RE: [PATCH v4 5/9] app/procinfo: add support for show tm
> 
> Hi Reshma,
> 
> <snipped>
> 
> >
> > > +			if ((ret) | (!is_leaf))
> > > +
> >
> > Is the operator here should be || ?
> >
> >
> 
> Check is done for 'if either ret is not 0 or if it ret is 0 but not leaf' we skip leaf
> details print. If 'ret is 0 and is leaf' we skip continue to print leaf details.

IMO, using logical operator over bitwise operator is good here in if statement  . Like below.?

If (ret || (is_leaf == 0 ))
Varghese, Vipin Nov. 23, 2018, 1:29 p.m. UTC | #4
> -----Original Message-----
> From: Pattan, Reshma
> Sent: Friday, November 23, 2018 5:21 PM
> To: Varghese, Vipin <vipin.varghese@intel.com>; dev@dpdk.org;
> thomas@monjalon.net; stephen@networkplumber.org; Mcnamara, John
> <john.mcnamara@intel.com>
> Cc: Byrne, Stephen1 <stephen1.byrne@intel.com>; Glynn, Michael J
> <michael.j.glynn@intel.com>; Patel, Amol <amol.patel@intel.com>
> Subject: RE: [PATCH v4 5/9] app/procinfo: add support for show tm
> 
> 
> 
> > -----Original Message-----
> > From: Varghese, Vipin
> > Sent: Thursday, November 22, 2018 1:28 PM
> > To: Pattan, Reshma <reshma.pattan@intel.com>; dev@dpdk.org;
> > thomas@monjalon.net; stephen@networkplumber.org; Mcnamara, John
> > <john.mcnamara@intel.com>
> > Cc: Byrne, Stephen1 <stephen1.byrne@intel.com>; Glynn, Michael J
> > <michael.j.glynn@intel.com>; Patel, Amol <amol.patel@intel.com>
> > Subject: RE: [PATCH v4 5/9] app/procinfo: add support for show tm
> >
> > Hi Reshma,
> >
> > <snipped>
> >
> > >
> > > > +			if ((ret) | (!is_leaf))
> > > > +
> > >
> > > Is the operator here should be || ?
> > >
> > >
> >
> > Check is done for 'if either ret is not 0 or if it ret is 0 but not
> > leaf' we skip leaf details print. If 'ret is 0 and is leaf' we skip continue to print
> leaf details.
> 
> IMO, using logical operator over bitwise operator is good here in if statement  .
> Like below.?
> 
> If (ret || (is_leaf == 0 ))

Thanks for the information, if the logic is correct do I need to change for v6
Pattan, Reshma Nov. 23, 2018, 1:33 p.m. UTC | #5
> -----Original Message-----
> From: Varghese, Vipin
> Sent: Friday, November 23, 2018 1:29 PM
> To: Pattan, Reshma <reshma.pattan@intel.com>; dev@dpdk.org;
> thomas@monjalon.net; stephen@networkplumber.org; Mcnamara, John
> <john.mcnamara@intel.com>
> Cc: Byrne, Stephen1 <stephen1.byrne@intel.com>; Glynn, Michael J
> <michael.j.glynn@intel.com>; Patel, Amol <amol.patel@intel.com>
> Subject: RE: [PATCH v4 5/9] app/procinfo: add support for show tm
> 
> 
> > -----Original Message-----
> > From: Pattan, Reshma
> > Sent: Friday, November 23, 2018 5:21 PM
> > To: Varghese, Vipin <vipin.varghese@intel.com>; dev@dpdk.org;
> > thomas@monjalon.net; stephen@networkplumber.org; Mcnamara, John
> > <john.mcnamara@intel.com>
> > Cc: Byrne, Stephen1 <stephen1.byrne@intel.com>; Glynn, Michael J
> > <michael.j.glynn@intel.com>; Patel, Amol <amol.patel@intel.com>
> > Subject: RE: [PATCH v4 5/9] app/procinfo: add support for show tm
> >
> >
> >
> > > -----Original Message-----
> > > From: Varghese, Vipin
> > > Sent: Thursday, November 22, 2018 1:28 PM
> > > To: Pattan, Reshma <reshma.pattan@intel.com>; dev@dpdk.org;
> > > thomas@monjalon.net; stephen@networkplumber.org; Mcnamara, John
> > > <john.mcnamara@intel.com>
> > > Cc: Byrne, Stephen1 <stephen1.byrne@intel.com>; Glynn, Michael J
> > > <michael.j.glynn@intel.com>; Patel, Amol <amol.patel@intel.com>
> > > Subject: RE: [PATCH v4 5/9] app/procinfo: add support for show tm
> > >
> > > Hi Reshma,
> > >
> > > <snipped>
> > >
> > > >
> > > > > +			if ((ret) | (!is_leaf))
> > > > > +
> > > >
> > > > Is the operator here should be || ?
> > > >
> > > >
> > >
> > > Check is done for 'if either ret is not 0 or if it ret is 0 but not
> > > leaf' we skip leaf details print. If 'ret is 0 and is leaf' we skip
> > > continue to print
> > leaf details.
> >
> > IMO, using logical operator over bitwise operator is good here in if statement
> .
> > Like below.?
> >
> > If (ret || (is_leaf == 0 ))
> 
> Thanks for the information, if the logic is correct do I need to change for v6
> 

OK in v6, but you can wait to hear more comments from others if any before sending v6 .
Varghese, Vipin Nov. 23, 2018, 1:55 p.m. UTC | #6
> -----Original Message-----
> From: Pattan, Reshma
> Sent: Friday, November 23, 2018 7:04 PM
> To: Varghese, Vipin <vipin.varghese@intel.com>; dev@dpdk.org;
> thomas@monjalon.net; stephen@networkplumber.org; Mcnamara, John
> <john.mcnamara@intel.com>
> Cc: Byrne, Stephen1 <stephen1.byrne@intel.com>; Glynn, Michael J
> <michael.j.glynn@intel.com>; Patel, Amol <amol.patel@intel.com>
> Subject: RE: [PATCH v4 5/9] app/procinfo: add support for show tm
> 
> 
> 
> > -----Original Message-----
> > From: Varghese, Vipin
> > Sent: Friday, November 23, 2018 1:29 PM
> > To: Pattan, Reshma <reshma.pattan@intel.com>; dev@dpdk.org;
> > thomas@monjalon.net; stephen@networkplumber.org; Mcnamara, John
> > <john.mcnamara@intel.com>
> > Cc: Byrne, Stephen1 <stephen1.byrne@intel.com>; Glynn, Michael J
> > <michael.j.glynn@intel.com>; Patel, Amol <amol.patel@intel.com>
> > Subject: RE: [PATCH v4 5/9] app/procinfo: add support for show tm
> >
> >
> > > -----Original Message-----
> > > From: Pattan, Reshma
> > > Sent: Friday, November 23, 2018 5:21 PM
> > > To: Varghese, Vipin <vipin.varghese@intel.com>; dev@dpdk.org;
> > > thomas@monjalon.net; stephen@networkplumber.org; Mcnamara, John
> > > <john.mcnamara@intel.com>
> > > Cc: Byrne, Stephen1 <stephen1.byrne@intel.com>; Glynn, Michael J
> > > <michael.j.glynn@intel.com>; Patel, Amol <amol.patel@intel.com>
> > > Subject: RE: [PATCH v4 5/9] app/procinfo: add support for show tm
> > >
> > >
> > >
> > > > -----Original Message-----
> > > > From: Varghese, Vipin
> > > > Sent: Thursday, November 22, 2018 1:28 PM
> > > > To: Pattan, Reshma <reshma.pattan@intel.com>; dev@dpdk.org;
> > > > thomas@monjalon.net; stephen@networkplumber.org; Mcnamara, John
> > > > <john.mcnamara@intel.com>
> > > > Cc: Byrne, Stephen1 <stephen1.byrne@intel.com>; Glynn, Michael J
> > > > <michael.j.glynn@intel.com>; Patel, Amol <amol.patel@intel.com>
> > > > Subject: RE: [PATCH v4 5/9] app/procinfo: add support for show tm
> > > >
> > > > Hi Reshma,
> > > >
> > > > <snipped>
> > > >
> > > > >
> > > > > > +			if ((ret) | (!is_leaf))
> > > > > > +
> > > > >
> > > > > Is the operator here should be || ?
> > > > >
> > > > >
> > > >
> > > > Check is done for 'if either ret is not 0 or if it ret is 0 but
> > > > not leaf' we skip leaf details print. If 'ret is 0 and is leaf' we
> > > > skip continue to print
> > > leaf details.
> > >
> > > IMO, using logical operator over bitwise operator is good here in if
> > > statement
> > .
> > > Like below.?
> > >
> > > If (ret || (is_leaf == 0 ))
> >
> > Thanks for the information, if the logic is correct do I need to
> > change for v6
> >
> 
> OK in v6, but you can wait to hear more comments from others if any before
> sending v6 .

Ok thanks Reshma, but can you tell me how the earlier logic fails and runs slow compared to logical or?
Pattan, Reshma Nov. 23, 2018, 2:57 p.m. UTC | #7
> -----Original Message-----
> From: Varghese, Vipin
> Sent: Friday, November 23, 2018 1:56 PM
> To: Pattan, Reshma <reshma.pattan@intel.com>; dev@dpdk.org;
> thomas@monjalon.net; stephen@networkplumber.org; Mcnamara, John
> <john.mcnamara@intel.com>
> Cc: Byrne, Stephen1 <stephen1.byrne@intel.com>; Glynn, Michael J
> <michael.j.glynn@intel.com>; Patel, Amol <amol.patel@intel.com>
> Subject: RE: [PATCH v4 5/9] app/procinfo: add support for show tm
> 
> 
> 
> > -----Original Message-----
> > From: Pattan, Reshma
> > Sent: Friday, November 23, 2018 7:04 PM
> > To: Varghese, Vipin <vipin.varghese@intel.com>; dev@dpdk.org;
> > thomas@monjalon.net; stephen@networkplumber.org; Mcnamara, John
> > <john.mcnamara@intel.com>
> > Cc: Byrne, Stephen1 <stephen1.byrne@intel.com>; Glynn, Michael J
> > <michael.j.glynn@intel.com>; Patel, Amol <amol.patel@intel.com>
> > Subject: RE: [PATCH v4 5/9] app/procinfo: add support for show tm
> >
> >
> >
> > > -----Original Message-----
> > > From: Varghese, Vipin
> > > Sent: Friday, November 23, 2018 1:29 PM
> > > To: Pattan, Reshma <reshma.pattan@intel.com>; dev@dpdk.org;
> > > thomas@monjalon.net; stephen@networkplumber.org; Mcnamara, John
> > > <john.mcnamara@intel.com>
> > > Cc: Byrne, Stephen1 <stephen1.byrne@intel.com>; Glynn, Michael J
> > > <michael.j.glynn@intel.com>; Patel, Amol <amol.patel@intel.com>
> > > Subject: RE: [PATCH v4 5/9] app/procinfo: add support for show tm
> > >
> > >
> > > > -----Original Message-----
> > > > From: Pattan, Reshma
> > > > Sent: Friday, November 23, 2018 5:21 PM
> > > > To: Varghese, Vipin <vipin.varghese@intel.com>; dev@dpdk.org;
> > > > thomas@monjalon.net; stephen@networkplumber.org; Mcnamara, John
> > > > <john.mcnamara@intel.com>
> > > > Cc: Byrne, Stephen1 <stephen1.byrne@intel.com>; Glynn, Michael J
> > > > <michael.j.glynn@intel.com>; Patel, Amol <amol.patel@intel.com>
> > > > Subject: RE: [PATCH v4 5/9] app/procinfo: add support for show tm
> > > >
> > > >
> > > >
> > > > > -----Original Message-----
> > > > > From: Varghese, Vipin
> > > > > Sent: Thursday, November 22, 2018 1:28 PM
> > > > > To: Pattan, Reshma <reshma.pattan@intel.com>; dev@dpdk.org;
> > > > > thomas@monjalon.net; stephen@networkplumber.org; Mcnamara, John
> > > > > <john.mcnamara@intel.com>
> > > > > Cc: Byrne, Stephen1 <stephen1.byrne@intel.com>; Glynn, Michael J
> > > > > <michael.j.glynn@intel.com>; Patel, Amol <amol.patel@intel.com>
> > > > > Subject: RE: [PATCH v4 5/9] app/procinfo: add support for show
> > > > > tm
> > > > >
> > > > > Hi Reshma,
> > > > >
> > > > > <snipped>
> > > > >
> > > > > >
> > > > > > > +			if ((ret) | (!is_leaf))
> > > > > > > +
> > > > > >
> > > > > > Is the operator here should be || ?
> > > > > >
> > > > > >
> > > > >
> > > > > Check is done for 'if either ret is not 0 or if it ret is 0 but
> > > > > not leaf' we skip leaf details print. If 'ret is 0 and is leaf'
> > > > > we skip continue to print
> > > > leaf details.
> > > >
> > > > IMO, using logical operator over bitwise operator is good here in
> > > > if statement
> > > .
> > > > Like below.?
> > > >
> > > > If (ret || (is_leaf == 0 ))
> > >
> > > Thanks for the information, if the logic is correct do I need to
> > > change for v6
> > >
> >
> > OK in v6, but you can wait to hear more comments from others if any
> > before sending v6 .
> 
> Ok thanks Reshma, but can you tell me how the earlier logic fails and runs slow
> compared to logical or?

Not about faster or slower. 

Logical operators are commonly used in decision making in C programming. 
Bitwise operators are used in C programming to perform bit-level operations.

Since , above if condition is for decision making here logical || operator will fit , so I am suggesting to use that. 

We  don't need to do any bitwise manipulation in if condition to make the decision, so bitwise | operator is not needed
Varghese, Vipin Nov. 23, 2018, 3:05 p.m. UTC | #8
> > > > > >
> > > > > > >
> > > > > > > > +			if ((ret) | (!is_leaf))
> > > > > > > > +
> > > > > > >
> > > > > > > Is the operator here should be || ?
> > > > > > >
> > > > > > >
> > > > > >
> > > > > > Check is done for 'if either ret is not 0 or if it ret is 0
> > > > > > but not leaf' we skip leaf details print. If 'ret is 0 and is leaf'
> > > > > > we skip continue to print
> > > > > leaf details.
> > > > >
> > > > > IMO, using logical operator over bitwise operator is good here
> > > > > in if statement
> > > > .
> > > > > Like below.?
> > > > >
> > > > > If (ret || (is_leaf == 0 ))
> > > >
> > > > Thanks for the information, if the logic is correct do I need to
> > > > change for v6
> > > >
> > >
> > > OK in v6, but you can wait to hear more comments from others if any
> > > before sending v6 .
> >
> > Ok thanks Reshma, but can you tell me how the earlier logic fails and
> > runs slow compared to logical or?
> 
> Not about faster or slower.

Now I see, I was wondering the suggestion was for improvement for performance.

> 
> Logical operators are commonly used in decision making in C programming.
> Bitwise operators are used in C programming to perform bit-level operations.
> 

Agreed

> Since , above if condition is for decision making here logical || operator will fit
> , so I am suggesting to use that.
> 

But bitwise OR is not wrong right?

> We  don't need to do any bitwise manipulation in if condition to make the
> decision, so bitwise | operator is not needed

We can correct this in next patch set not v6 if this is only change for 'show tm'
Stephen Hemminger Nov. 23, 2018, 5:22 p.m. UTC | #9
On Fri, 23 Nov 2018 15:05:07 +0000
"Varghese, Vipin" <vipin.varghese@intel.com> wrote:

> > > > > > >  
> > > > > > > >  
> > > > > > > > > +			if ((ret) | (!is_leaf))
> > > > > > > > > +  
> > > > > > > >
> > > > > > > > Is the operator here should be || ?
> > > > > > > >
> > > > > > > >  
> > > > > > >
> > > > > > > Check is done for 'if either ret is not 0 or if it ret is 0
> > > > > > > but not leaf' we skip leaf details print. If 'ret is 0 and is leaf'
> > > > > > > we skip continue to print  
> > > > > > leaf details.
> > > > > >
> > > > > > IMO, using logical operator over bitwise operator is good here
> > > > > > in if statement  
> > > > > .  
> > > > > > Like below.?
> > > > > >
> > > > > > If (ret || (is_leaf == 0 ))  
> > > > >
> > > > > Thanks for the information, if the logic is correct do I need to
> > > > > change for v6
> > > > >  
> > > >
> > > > OK in v6, but you can wait to hear more comments from others if any
> > > > before sending v6 .  
> > >
> > > Ok thanks Reshma, but can you tell me how the earlier logic fails and
> > > runs slow compared to logical or?  
> > 
> > Not about faster or slower.  
> 
> Now I see, I was wondering the suggestion was for improvement for performance.
> 
> > 
> > Logical operators are commonly used in decision making in C programming.
> > Bitwise operators are used in C programming to perform bit-level operations.
> >   
> 
> Agreed
> 
> > Since , above if condition is for decision making here logical || operator will fit
> > , so I am suggesting to use that.
> >   
> 
> But bitwise OR is not wrong right?
> 
> > We  don't need to do any bitwise manipulation in if condition to make the
> > decision, so bitwise | operator is not needed  
> 
> We can correct this in next patch set not v6 if this is only change for 'show tm'

It could be that compiler might optimize logical into bitwise operation
to avoid cost of conditional branch (if there are no side effects).
Varghese, Vipin Nov. 26, 2018, 4:15 a.m. UTC | #10
Hi Stephen,

snipped

> 
> > > > > > > >
> > > > > > > > >
> > > > > > > > > > +			if ((ret) | (!is_leaf))
> > > > > > > > > > +
> > > > > > > > >
> > > > > > > > > Is the operator here should be || ?
> > > > > > > > >
> > > > > > > > >
> > > > > > > >
> > > > > > > > Check is done for 'if either ret is not 0 or if it ret is
> > > > > > > > 0 but not leaf' we skip leaf details print. If 'ret is 0 and is leaf'
> > > > > > > > we skip continue to print
> > > > > > > leaf details.
> > > > > > >
> > > > > > > IMO, using logical operator over bitwise operator is good
> > > > > > > here in if statement
> > > > > > .
> > > > > > > Like below.?
> > > > > > >
> > > > > > > If (ret || (is_leaf == 0 ))
> > > > > >
> > > > > > Thanks for the information, if the logic is correct do I need
> > > > > > to change for v6
> > > > > >
> > > > >
> > > > > OK in v6, but you can wait to hear more comments from others if
> > > > > any before sending v6 .
> > > >
> > > > Ok thanks Reshma, but can you tell me how the earlier logic fails
> > > > and runs slow compared to logical or?
> > >
> > > Not about faster or slower.
> >
> > Now I see, I was wondering the suggestion was for improvement for
> performance.
> >
> > >
> > > Logical operators are commonly used in decision making in C programming.
> > > Bitwise operators are used in C programming to perform bit-level operations.
> > >
> >
> > Agreed
> >
> > > Since , above if condition is for decision making here logical ||
> > > operator will fit , so I am suggesting to use that.
> > >
> >
> > But bitwise OR is not wrong right?
> >
> > > We  don't need to do any bitwise manipulation in if condition to
> > > make the decision, so bitwise | operator is not needed
> >
> > We can correct this in next patch set not v6 if this is only change for 'show tm'
> 
> It could be that compiler might optimize logical into bitwise operation to avoid
> cost of conditional branch (if there are no side effects).


Checking with 'EXTRA_CFLAGS=-g' we get the code generated as 

"""
                        if ((ret) | (!is_leaf))
   9a512:       8b 85 28 fd ff ff       mov    eax,DWORD PTR [rbp-0x2d8]
   9a518:       85 c0                   test   eax,eax
   9a51a:       0f 94 c0                sete   al
   9a51d:       0f b6 c0                movzx  eax,al
   9a520:       0b 85 3c fd ff ff       or     eax,DWORD PTR [rbp-0x2c4]
   9a526:       85 c0                   test   eax,eax
   9a528:       75 74                   jne    9a59e <show_tm+0x73e>
                                continue;
"""

Looks like the is_leaf is picked assembly 'test' is done then byte is set to 1 based on flags. This is then or with 'ret'. I think your observation is correct 'compiler is remapping to or'

Thanks
Vipin Varghese
Ananyev, Konstantin Nov. 26, 2018, 9:28 a.m. UTC | #11
> -----Original Message-----
> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Varghese, Vipin
> Sent: Monday, November 26, 2018 4:15 AM
> To: Stephen Hemminger <stephen@networkplumber.org>
> Cc: Pattan, Reshma <reshma.pattan@intel.com>; dev@dpdk.org; thomas@monjalon.net; Mcnamara, John <john.mcnamara@intel.com>;
> Byrne, Stephen1 <stephen1.byrne@intel.com>; Glynn, Michael J <michael.j.glynn@intel.com>; Patel, Amol <amol.patel@intel.com>
> Subject: Re: [dpdk-dev] [PATCH v4 5/9] app/procinfo: add support for show tm
> 
> Hi Stephen,
> 
> snipped
> 
> >
> > > > > > > > >
> > > > > > > > > >
> > > > > > > > > > > +			if ((ret) | (!is_leaf))
> > > > > > > > > > > +
> > > > > > > > > >
> > > > > > > > > > Is the operator here should be || ?
> > > > > > > > > >
> > > > > > > > > >
> > > > > > > > >
> > > > > > > > > Check is done for 'if either ret is not 0 or if it ret is
> > > > > > > > > 0 but not leaf' we skip leaf details print. If 'ret is 0 and is leaf'
> > > > > > > > > we skip continue to print
> > > > > > > > leaf details.
> > > > > > > >
> > > > > > > > IMO, using logical operator over bitwise operator is good
> > > > > > > > here in if statement
> > > > > > > .
> > > > > > > > Like below.?
> > > > > > > >
> > > > > > > > If (ret || (is_leaf == 0 ))
> > > > > > >
> > > > > > > Thanks for the information, if the logic is correct do I need
> > > > > > > to change for v6
> > > > > > >
> > > > > >
> > > > > > OK in v6, but you can wait to hear more comments from others if
> > > > > > any before sending v6 .
> > > > >
> > > > > Ok thanks Reshma, but can you tell me how the earlier logic fails
> > > > > and runs slow compared to logical or?
> > > >
> > > > Not about faster or slower.
> > >
> > > Now I see, I was wondering the suggestion was for improvement for
> > performance.
> > >
> > > >
> > > > Logical operators are commonly used in decision making in C programming.
> > > > Bitwise operators are used in C programming to perform bit-level operations.
> > > >
> > >
> > > Agreed
> > >
> > > > Since , above if condition is for decision making here logical ||
> > > > operator will fit , so I am suggesting to use that.
> > > >
> > >
> > > But bitwise OR is not wrong right?
> > >
> > > > We  don't need to do any bitwise manipulation in if condition to
> > > > make the decision, so bitwise | operator is not needed
> > >
> > > We can correct this in next patch set not v6 if this is only change for 'show tm'
> >
> > It could be that compiler might optimize logical into bitwise operation to avoid
> > cost of conditional branch (if there are no side effects).
> 
> 
> Checking with 'EXTRA_CFLAGS=-g' we get the code generated as
> 
> """
>                         if ((ret) | (!is_leaf))
>    9a512:       8b 85 28 fd ff ff       mov    eax,DWORD PTR [rbp-0x2d8]
>    9a518:       85 c0                   test   eax,eax
>    9a51a:       0f 94 c0                sete   al
>    9a51d:       0f b6 c0                movzx  eax,al
>    9a520:       0b 85 3c fd ff ff       or     eax,DWORD PTR [rbp-0x2c4]
>    9a526:       85 c0                   test   eax,eax
>    9a528:       75 74                   jne    9a59e <show_tm+0x73e>
>                                 continue;
> """
> 
> Looks like the is_leaf is picked assembly 'test' is done then byte is set to 1 based on flags. This is then or with 'ret'. I think your observation is
> correct 'compiler is remapping to or'

If the operator is '|' what else except of 'OR' you expect it to be remapped?
Konstantin
Ananyev, Konstantin Nov. 26, 2018, 9:33 a.m. UTC | #12
> -----Original Message-----
> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Ananyev, Konstantin
> Sent: Monday, November 26, 2018 9:28 AM
> To: Varghese, Vipin <vipin.varghese@intel.com>; Stephen Hemminger <stephen@networkplumber.org>
> Cc: Pattan, Reshma <reshma.pattan@intel.com>; dev@dpdk.org; thomas@monjalon.net; Mcnamara, John <john.mcnamara@intel.com>;
> Byrne, Stephen1 <stephen1.byrne@intel.com>; Glynn, Michael J <michael.j.glynn@intel.com>; Patel, Amol <amol.patel@intel.com>
> Subject: Re: [dpdk-dev] [PATCH v4 5/9] app/procinfo: add support for show tm
> 
> 
> 
> > -----Original Message-----
> > From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Varghese, Vipin
> > Sent: Monday, November 26, 2018 4:15 AM
> > To: Stephen Hemminger <stephen@networkplumber.org>
> > Cc: Pattan, Reshma <reshma.pattan@intel.com>; dev@dpdk.org; thomas@monjalon.net; Mcnamara, John
> <john.mcnamara@intel.com>;
> > Byrne, Stephen1 <stephen1.byrne@intel.com>; Glynn, Michael J <michael.j.glynn@intel.com>; Patel, Amol <amol.patel@intel.com>
> > Subject: Re: [dpdk-dev] [PATCH v4 5/9] app/procinfo: add support for show tm
> >
> > Hi Stephen,
> >
> > snipped
> >
> > >
> > > > > > > > > >
> > > > > > > > > > >
> > > > > > > > > > > > +			if ((ret) | (!is_leaf))
> > > > > > > > > > > > +
> > > > > > > > > > >
> > > > > > > > > > > Is the operator here should be || ?
> > > > > > > > > > >
> > > > > > > > > > >
> > > > > > > > > >
> > > > > > > > > > Check is done for 'if either ret is not 0 or if it ret is
> > > > > > > > > > 0 but not leaf' we skip leaf details print. If 'ret is 0 and is leaf'
> > > > > > > > > > we skip continue to print
> > > > > > > > > leaf details.
> > > > > > > > >
> > > > > > > > > IMO, using logical operator over bitwise operator is good
> > > > > > > > > here in if statement
> > > > > > > > .
> > > > > > > > > Like below.?
> > > > > > > > >
> > > > > > > > > If (ret || (is_leaf == 0 ))
> > > > > > > >
> > > > > > > > Thanks for the information, if the logic is correct do I need
> > > > > > > > to change for v6
> > > > > > > >
> > > > > > >
> > > > > > > OK in v6, but you can wait to hear more comments from others if
> > > > > > > any before sending v6 .
> > > > > >
> > > > > > Ok thanks Reshma, but can you tell me how the earlier logic fails
> > > > > > and runs slow compared to logical or?
> > > > >
> > > > > Not about faster or slower.
> > > >
> > > > Now I see, I was wondering the suggestion was for improvement for
> > > performance.
> > > >
> > > > >
> > > > > Logical operators are commonly used in decision making in C programming.
> > > > > Bitwise operators are used in C programming to perform bit-level operations.
> > > > >
> > > >
> > > > Agreed
> > > >
> > > > > Since , above if condition is for decision making here logical ||
> > > > > operator will fit , so I am suggesting to use that.
> > > > >
> > > >
> > > > But bitwise OR is not wrong right?
> > > >
> > > > > We  don't need to do any bitwise manipulation in if condition to
> > > > > make the decision, so bitwise | operator is not needed
> > > >
> > > > We can correct this in next patch set not v6 if this is only change for 'show tm'
> > >
> > > It could be that compiler might optimize logical into bitwise operation to avoid
> > > cost of conditional branch (if there are no side effects).
> >
> >
> > Checking with 'EXTRA_CFLAGS=-g' we get the code generated as
> >
> > """
> >                         if ((ret) | (!is_leaf))
> >    9a512:       8b 85 28 fd ff ff       mov    eax,DWORD PTR [rbp-0x2d8]
> >    9a518:       85 c0                   test   eax,eax
> >    9a51a:       0f 94 c0                sete   al
> >    9a51d:       0f b6 c0                movzx  eax,al
> >    9a520:       0b 85 3c fd ff ff       or     eax,DWORD PTR [rbp-0x2c4]
> >    9a526:       85 c0                   test   eax,eax
> >    9a528:       75 74                   jne    9a59e <show_tm+0x73e>
> >                                 continue;
> > """
> >
> > Looks like the is_leaf is picked assembly 'test' is done then byte is set to 1 based on flags. This is then or with 'ret'. I think your observation
> is
> > correct 'compiler is remapping to or'
> 
> If the operator is '|' what else except of 'OR' you expect it to be remapped?
BTW, I am agree with Reshma it has to be '||' here.
Varghese, Vipin Nov. 26, 2018, 9:46 a.m. UTC | #13
Hi Konstantin,

> >
> > If the operator is '|' what else except of 'OR' you expect it to be remapped?

Thanks for your correction. I have rerun with logical or 

                        if ((ret) || (!is_leaf))
   9a512:       83 bd 3c fd ff ff 00    cmp    DWORD PTR [rbp-0x2c4],0x0
   9a519:       75 7e                   jne    9a599 <show_tm+0x739>
   9a51b:       8b 85 28 fd ff ff       mov    eax,DWORD PTR [rbp-0x2d8]
   9a521:       85 c0                   test   eax,eax
   9a523:       74 74                   je     9a599 <show_tm+0x739>
                                continue;

There is no conversion to 'Or' operation here. Thanks for 

> BTW, I am agree with Reshma it has to be '||' here.
Thanks for your opinion, can you share review comments for show_tm for debug?

Thanks
Vipin Varghese
Ananyev, Konstantin Nov. 26, 2018, 9:56 a.m. UTC | #14
Hi Vipin,

> 
> Hi Konstantin,
> 
> > >
> > > If the operator is '|' what else except of 'OR' you expect it to be remapped?
> 
> Thanks for your correction. I have rerun with logical or
> 
>                         if ((ret) || (!is_leaf))
>    9a512:       83 bd 3c fd ff ff 00    cmp    DWORD PTR [rbp-0x2c4],0x0
>    9a519:       75 7e                   jne    9a599 <show_tm+0x739>
>    9a51b:       8b 85 28 fd ff ff       mov    eax,DWORD PTR [rbp-0x2d8]
>    9a521:       85 c0                   test   eax,eax
>    9a523:       74 74                   je     9a599 <show_tm+0x739>
>                                 continue;
> 
> There is no conversion to 'Or' operation here. Thanks for
> 
> > BTW, I am agree with Reshma it has to be '||' here.
> Thanks for your opinion, can you share review comments for show_tm for debug?

Not sure I understand your last sentence...
Konstantin
Varghese, Vipin Nov. 26, 2018, 10:13 a.m. UTC | #15
Hi,

> Hi Vipin,
> 
> >
> > Hi Konstantin,
> >
> > > >
> > > > If the operator is '|' what else except of 'OR' you expect it to be remapped?
> >
> > Thanks for your correction. I have rerun with logical or
> >
> >                         if ((ret) || (!is_leaf))
> >    9a512:       83 bd 3c fd ff ff 00    cmp    DWORD PTR [rbp-0x2c4],0x0
> >    9a519:       75 7e                   jne    9a599 <show_tm+0x739>
> >    9a51b:       8b 85 28 fd ff ff       mov    eax,DWORD PTR [rbp-0x2d8]
> >    9a521:       85 c0                   test   eax,eax
> >    9a523:       74 74                   je     9a599 <show_tm+0x739>
> >                                 continue;
> >
> > There is no conversion to 'Or' operation here. Thanks for
> >
> > > BTW, I am agree with Reshma it has to be '||' here.
> > Thanks for your opinion, can you share review comments for show_tm for
> debug?
> 
> Not sure I understand your last sentence...
> Konstantin

As mentioned earlier email I am ready to spin v6 with 'logical or'. But if there any other comments for show_tm it will be nice to accommodate to v6 rather than v7. So, can you please share if there any other comments for show_tm?

Thanks
Vipin Varghese
diff mbox series

Patch

diff --git a/app/proc-info/main.c b/app/proc-info/main.c
index 48477097f..8ec2e9474 100644
--- a/app/proc-info/main.c
+++ b/app/proc-info/main.c
@@ -32,6 +32,7 @@ 
 #include <rte_cycles.h>
 #include <rte_security.h>
 #include <rte_cryptodev.h>
+#include <rte_tm.h>
 
 /* Maximum long option length for option parsing. */
 #define MAX_LONG_OPT_SZ 64
@@ -752,10 +753,283 @@  show_port(void)
 	STATS_BDR_STR(50, "");
 }
 
+static void
+display_nodecap_info(int is_leaf, struct rte_tm_node_capabilities *cap)
+{
+	if (cap == NULL)
+		return;
+
+	if (!is_leaf) {
+		printf("\t  -- nonleaf sched max:\n"
+			"\t\t  + children (%u)\n"
+			"\t\t  + sp priorities (%u)\n"
+			"\t\t  + wfq children per group (%u)\n"
+			"\t\t  + wfq groups (%u)\n"
+			"\t\t  + wfq weight (%u)\n",
+			cap->nonleaf.sched_n_children_max,
+			cap->nonleaf.sched_sp_n_priorities_max,
+			cap->nonleaf.sched_wfq_n_children_per_group_max,
+			cap->nonleaf.sched_wfq_n_groups_max,
+			cap->nonleaf.sched_wfq_weight_max);
+	} else {
+		printf("\t  -- leaf cman support:\n"
+			"\t\t  + wred pkt mode (%d)\n"
+			"\t\t  + wred byte mode (%d)\n"
+			"\t\t  + head drop (%d)\n"
+			"\t\t  + wred context private (%d)\n"
+			"\t\t  + wred context shared (%u)\n",
+			cap->leaf.cman_wred_packet_mode_supported,
+			cap->leaf.cman_wred_byte_mode_supported,
+			cap->leaf.cman_head_drop_supported,
+			cap->leaf.cman_wred_context_private_supported,
+			cap->leaf.cman_wred_context_shared_n_max);
+	}
+}
+
+static void
+display_levelcap_info(int is_leaf, struct rte_tm_level_capabilities *cap)
+{
+	if (cap == NULL)
+		return;
+
+	if (!is_leaf) {
+		printf("\t  -- shaper private: (%d) dual rate (%d)\n",
+			cap->nonleaf.shaper_private_supported,
+			cap->nonleaf.shaper_private_dual_rate_supported);
+		printf("\t  -- shaper share: (%u)\n",
+			cap->nonleaf.shaper_shared_n_max);
+		printf("\t  -- non leaf sched MAX:\n"
+			"\t\t  + children (%u)\n"
+			"\t\t  + sp (%u)\n"
+			"\t\t  + wfq children per group (%u)\n"
+			"\t\t  + wfq groups (%u)\n"
+			"\t\t  + wfq weight (%u)\n",
+			cap->nonleaf.sched_n_children_max,
+			cap->nonleaf.sched_sp_n_priorities_max,
+			cap->nonleaf.sched_wfq_n_children_per_group_max,
+			cap->nonleaf.sched_wfq_n_groups_max,
+			cap->nonleaf.sched_wfq_weight_max);
+	} else {
+		printf("\t  -- shaper private: (%d) dual rate (%d)\n",
+			cap->leaf.shaper_private_supported,
+			cap->leaf.shaper_private_dual_rate_supported);
+		printf("\t  -- shaper share: (%u)\n",
+			cap->leaf.shaper_shared_n_max);
+		printf("  -- leaf cman support:\n"
+			"\t\t  + wred pkt mode (%d)\n"
+			"\t\t  + wred byte mode (%d)\n"
+			"\t\t  + head drop (%d)\n"
+			"\t\t  + wred context private (%d)\n"
+			"\t\t  + wred context shared (%u)\n",
+			cap->leaf.cman_wred_packet_mode_supported,
+			cap->leaf.cman_wred_byte_mode_supported,
+			cap->leaf.cman_head_drop_supported,
+			cap->leaf.cman_wred_context_private_supported,
+			cap->leaf.cman_wred_context_shared_n_max);
+	}
+}
+
 static void
 show_tm(void)
 {
-	printf(" tm\n");
+	int ret = 0, check_for_leaf = 0, is_leaf = 0;
+	unsigned int j, k;
+	uint16_t i = 0;
+
+	snprintf(bdr_str, MAX_STRING_LEN, " show - TM PMD %"PRIu64,
+			rte_get_tsc_hz());
+	STATS_BDR_STR(10, bdr_str);
+
+	RTE_ETH_FOREACH_DEV(i) {
+		struct rte_eth_dev_info dev_info;
+		struct rte_tm_capabilities cap;
+		struct rte_tm_error error;
+		struct rte_tm_node_capabilities capnode;
+		struct rte_tm_level_capabilities caplevel;
+		uint32_t n_leaf_nodes = 0;
+
+		memset(&dev_info, 0, sizeof(dev_info));
+		memset(&cap, 0, sizeof(cap));
+		memset(&error, 0, sizeof(error));
+
+		rte_eth_dev_info_get(i, &dev_info);
+		printf("  - Generic for port (%u)\n"
+			"\t  -- driver name %s\n"
+			"\t  -- max vf (%u)\n"
+			"\t  -- max tx queues (%u)\n"
+			"\t  -- number of tx queues (%u)\n",
+			i,
+			dev_info.driver_name,
+			dev_info.max_vfs,
+			dev_info.max_tx_queues,
+			dev_info.nb_tx_queues);
+
+		ret = rte_tm_capabilities_get(i, &cap, &error);
+		if (ret)
+			continue;
+
+		printf("  - MAX: nodes (%u) levels (%u) children (%u)\n",
+			cap.n_nodes_max,
+			cap.n_levels_max,
+			cap.sched_n_children_max);
+
+		printf("  - identical nodes: non leaf (%d) leaf (%d)\n",
+			cap.non_leaf_nodes_identical,
+			cap.leaf_nodes_identical);
+
+		printf("  - Shaper MAX:\n"
+			"\t  -- total (%u)\n"
+			"\t  -- private (%u) private dual (%d)\n"
+			"\t  -- shared (%u) shared dual (%u)\n",
+			cap.shaper_n_max,
+			cap.shaper_private_n_max,
+			cap.shaper_private_dual_rate_n_max,
+			cap.shaper_shared_n_max,
+			cap.shaper_shared_dual_rate_n_max);
+
+		printf("  - mark support:\n");
+		printf("\t  -- vlan dei: GREEN (%d) YELLOW (%d) RED (%d)\n",
+			cap.mark_vlan_dei_supported[RTE_TM_GREEN],
+			cap.mark_vlan_dei_supported[RTE_TM_YELLOW],
+			cap.mark_vlan_dei_supported[RTE_TM_RED]);
+		printf("\t  -- ip ecn tcp: GREEN (%d) YELLOW (%d) RED (%d)\n",
+			cap.mark_ip_ecn_tcp_supported[RTE_TM_GREEN],
+			cap.mark_ip_ecn_tcp_supported[RTE_TM_YELLOW],
+			cap.mark_ip_ecn_tcp_supported[RTE_TM_RED]);
+		printf("\t  -- ip ecn sctp: GREEN (%d) YELLOW (%d) RED (%d)\n",
+			cap.mark_ip_ecn_sctp_supported[RTE_TM_GREEN],
+			cap.mark_ip_ecn_sctp_supported[RTE_TM_YELLOW],
+			cap.mark_ip_ecn_sctp_supported[RTE_TM_RED]);
+		printf("\t  -- ip dscp: GREEN (%d) YELLOW (%d) RED (%d)\n",
+			cap.mark_ip_dscp_supported[RTE_TM_GREEN],
+			cap.mark_ip_dscp_supported[RTE_TM_YELLOW],
+			cap.mark_ip_dscp_supported[RTE_TM_RED]);
+
+		printf("  - mask stats (0x%"PRIx64")"
+			" dynamic update (0x%"PRIx64")\n",
+			cap.stats_mask,
+			cap.dynamic_update_mask);
+
+		printf("  - sched MAX:\n"
+			"\t  -- total (%u)\n"
+			"\t  -- sp levels (%u)\n"
+			"\t  -- wfq children per group (%u)\n"
+			"\t  -- wfq groups (%u)\n"
+			"\t  -- wfq weight (%u)\n",
+			cap.sched_sp_n_priorities_max,
+			cap.sched_sp_n_priorities_max,
+			cap.sched_wfq_n_children_per_group_max,
+			cap.sched_wfq_n_groups_max,
+			cap.sched_wfq_weight_max);
+
+		printf("  - CMAN support:\n"
+			"\t  -- WRED mode: pkt (%d) byte (%d)\n"
+			"\t  -- head drop (%d)\n",
+			cap.cman_wred_packet_mode_supported,
+			cap.cman_wred_byte_mode_supported,
+			cap.cman_head_drop_supported);
+		printf("\t  -- MAX WRED CONTEXT:"
+			" total (%u) private (%u) shared (%u)\n",
+			cap.cman_wred_context_n_max,
+			cap.cman_wred_context_private_n_max,
+			cap.cman_wred_context_shared_n_max);
+
+		for (j = 0; j < cap.n_nodes_max; j++) {
+			memset(&capnode, 0, sizeof(capnode));
+			ret = rte_tm_node_capabilities_get(i, j,
+				&capnode, &error);
+			if (ret)
+				continue;
+
+			check_for_leaf = 1;
+
+			printf("  NODE %u\n", j);
+			printf("\t  - shaper private: (%d) dual rate (%d)\n",
+				capnode.shaper_private_supported,
+				capnode.shaper_private_dual_rate_supported);
+			printf("\t  - shaper shared max: (%u)\n",
+				capnode.shaper_shared_n_max);
+			printf("\t  - stats mask %"PRIx64"\n",
+				capnode.stats_mask);
+
+			ret = rte_tm_node_type_get(i, j, &is_leaf, &error);
+			if (ret)
+				continue;
+
+			display_nodecap_info(is_leaf, &capnode);
+		}
+
+		for (j = 0; j < cap.n_levels_max; j++) {
+			memset(&caplevel, 0, sizeof(caplevel));
+			ret = rte_tm_level_capabilities_get(i, j,
+				&caplevel, &error);
+			if (ret)
+				continue;
+
+			printf("  - Level %u\n", j);
+			printf("\t  -- node MAX: %u non leaf %u leaf %u\n",
+				caplevel.n_nodes_max,
+				caplevel.n_nodes_nonleaf_max,
+				caplevel.n_nodes_leaf_max);
+			printf("\t  -- indetical: non leaf %u leaf %u\n",
+				caplevel.non_leaf_nodes_identical,
+				caplevel.leaf_nodes_identical);
+
+			for (k = 0; k < caplevel.n_nodes_max; k++) {
+				ret = rte_tm_node_type_get(i, k,
+					&is_leaf, &error);
+				if (ret)
+					continue;
+
+				display_levelcap_info(is_leaf, &caplevel);
+			}
+		}
+
+		if (check_for_leaf) {
+			ret = rte_tm_get_number_of_leaf_nodes(i,
+					&n_leaf_nodes, &error);
+			if (ret == 0)
+				printf("  - leaf nodes (%u)\n", n_leaf_nodes);
+		}
+
+		for (j = 0; j < n_leaf_nodes; j++) {
+			struct rte_tm_node_stats stats;
+			memset(&stats, 0, sizeof(stats));
+
+			ret = rte_tm_node_stats_read(i, j,
+				&stats, &cap.stats_mask, 0, &error);
+			if (ret)
+				continue;
+
+			printf("  - STATS for node (%u)\n", j);
+			printf("  -- pkts (%"PRIu64") bytes (%"PRIu64")\n",
+				stats.n_pkts, stats.n_bytes);
+
+			ret = rte_tm_node_type_get(i, j, &is_leaf, &error);
+			if ((ret) | (!is_leaf))
+				continue;
+
+			printf("  -- leaf queued:"
+				" pkts (%"PRIu64") bytes (%"PRIu64")\n",
+				stats.leaf.n_pkts_queued,
+				stats.leaf.n_bytes_queued);
+			printf("  - dropped:\n"
+				"\t  -- GREEN:"
+				" pkts (%"PRIu64") bytes (%"PRIu64")\n"
+				"\t  -- YELLOW:"
+				" pkts (%"PRIu64") bytes (%"PRIu64")\n"
+				"\t  -- RED:"
+				" pkts (%"PRIu64") bytes (%"PRIu64")\n",
+				stats.leaf.n_pkts_dropped[RTE_TM_GREEN],
+				stats.leaf.n_bytes_dropped[RTE_TM_GREEN],
+				stats.leaf.n_pkts_dropped[RTE_TM_YELLOW],
+				stats.leaf.n_bytes_dropped[RTE_TM_YELLOW],
+				stats.leaf.n_pkts_dropped[RTE_TM_RED],
+				stats.leaf.n_bytes_dropped[RTE_TM_RED]);
+		}
+	}
+
+	STATS_BDR_STR(50, "");
 }
 
 static void