Re: [dpdk-dev] [PATCH v4 5/9] app/procinfo: add support for show tm

["Ananyev, Konstantin" <konstantin.ananyev@intel.com>]

> -----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.