[v4,5/9] app/procinfo: add support for show tm
Checks
Commit Message
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
> -----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 || ?
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.
> -----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 ))
> -----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
> -----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 .
> -----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?
> -----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
> > > > > >
> > > > > > >
> > > > > > > > + 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'
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).
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
> -----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
> -----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.
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
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
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
@@ -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