[net-next,13/15] i40e/i40evf: use logical operators, not bitwise

From: Mitch Williams <mitch.a.williams@intel.com>

Mr. Spock would certainly raise an eyebrow to see us using bitwise
operators, when we should clearly be relying on logic. Fascinating.

Change-ID: Ie338010c016f93e9faa2002c07c90b15134b7477
Signed-off-by: Mitch Williams <mitch.a.williams@intel.com>
Tested-by: Andrew Bowers <andrewx.bowers@intel.com>
Signed-off-by: Jeff Kirsher <jeffrey.t.kirsher@intel.com>
---
 drivers/net/ethernet/intel/i40e/i40e_txrx.c   | 5 +++--
 drivers/net/ethernet/intel/i40evf/i40e_txrx.c | 5 +++--
 2 files changed, 6 insertions(+), 4 deletions(-)

On Wed, 2016-02-17 at 19:38 -0800, Jeff Kirsher wrote:
> From: Mitch Williams <mitch.a.williams@intel.com>
> 
> Mr. Spock would certainly raise an eyebrow to see us using bitwise
> operators, when we should clearly be relying on logic. Fascinating.

I think it read better before this change.

Spock might have looked at the type of the
variable before raising that eyebrow.

clean_complete is bool so it's not actually a
bitwise operation,

> diff --git a/drivers/net/ethernet/intel/i40e/i40e_txrx.c b/drivers/net/ethernet/intel/i40e/i40e_txrx.c
[]
> @@ -1996,7 +1996,8 @@ int i40e_napi_poll(struct napi_struct *napi, int budget)
>  	 * budget and be more aggressive about cleaning up the Tx descriptors.
>  	 */
>  	i40e_for_each_ring(ring, q_vector->tx) {
> -		clean_complete &= i40e_clean_tx_irq(ring, vsi->work_limit);
> +		clean_complete = clean_complete &&
> +				 i40e_clean_tx_irq(ring, vsi->work_limit);

etc...

From: Joe Perches
> Sent: 18 February 2016 04:59
> On Wed, 2016-02-17 at 19:38 -0800, Jeff Kirsher wrote:
> > From: Mitch Williams <mitch.a.williams@intel.com>
> >
> > Mr. Spock would certainly raise an eyebrow to see us using bitwise
> > operators, when we should clearly be relying on logic. Fascinating.
> 
> I think it read better before this change.
> 
> Spock might have looked at the type of the
> variable before raising that eyebrow.
> 
> clean_complete is bool so it's not actually a
> bitwise operation,

Except that, of course, you may well want a bitwise operation
in order to remove slow conditional instructions.

Hopefully gcc generates a bit-wise 'and' provided that i40e_clean_tx_irq()
returns a 'bool'.

	David

> > diff --git a/drivers/net/ethernet/intel/i40e/i40e_txrx.c
> b/drivers/net/ethernet/intel/i40e/i40e_txrx.c
> []
> > @@ -1996,7 +1996,8 @@ int i40e_napi_poll(struct napi_struct *napi, int budget)
> >  	 * budget and be more aggressive about cleaning up the Tx descriptors.
> >  	 */
> >  	i40e_for_each_ring(ring, q_vector->tx) {
> > -		clean_complete &= i40e_clean_tx_irq(ring, vsi->work_limit);
> > +		clean_complete = clean_complete &&
> > +				 i40e_clean_tx_irq(ring, vsi->work_limit);
> 
> etc...

On Wed, Feb 17, 2016 at 7:38 PM, Jeff Kirsher
<jeffrey.t.kirsher@intel.com> wrote:
> From: Mitch Williams <mitch.a.williams@intel.com>
>
> Mr. Spock would certainly raise an eyebrow to see us using bitwise
> operators, when we should clearly be relying on logic. Fascinating.
>
> Change-ID: Ie338010c016f93e9faa2002c07c90b15134b7477
> Signed-off-by: Mitch Williams <mitch.a.williams@intel.com>
> Tested-by: Andrew Bowers <andrewx.bowers@intel.com>
> Signed-off-by: Jeff Kirsher <jeffrey.t.kirsher@intel.com>

NAK

The bitwise operator lets us at least make a pass through all rings.
With this new code you stop as soon as one ring has not completed
cleaning.

> ---
>  drivers/net/ethernet/intel/i40e/i40e_txrx.c   | 5 +++--
>  drivers/net/ethernet/intel/i40evf/i40e_txrx.c | 5 +++--
>  2 files changed, 6 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/net/ethernet/intel/i40e/i40e_txrx.c b/drivers/net/ethernet/intel/i40e/i40e_txrx.c
> index 1abef01..0ffa9a8 100644
> --- a/drivers/net/ethernet/intel/i40e/i40e_txrx.c
> +++ b/drivers/net/ethernet/intel/i40e/i40e_txrx.c
> @@ -1996,7 +1996,8 @@ int i40e_napi_poll(struct napi_struct *napi, int budget)
>          * budget and be more aggressive about cleaning up the Tx descriptors.
>          */
>         i40e_for_each_ring(ring, q_vector->tx) {
> -               clean_complete &= i40e_clean_tx_irq(ring, vsi->work_limit);
> +               clean_complete = clean_complete &&
> +                                i40e_clean_tx_irq(ring, vsi->work_limit);
>                 arm_wb = arm_wb || ring->arm_wb;
>                 ring->arm_wb = false;
>         }

For example here.  You now will not call i40e_clean_tx_irq if
clean_complete is false.

> @@ -2020,7 +2021,7 @@ int i40e_napi_poll(struct napi_struct *napi, int budget)
>
>                 work_done += cleaned;
>                 /* if we didn't clean as many as budgeted, we must be done */
> -               clean_complete &= (budget_per_ring != cleaned);
> +               clean_complete = clean_complete && (budget_per_ring > cleaned);
>         }
>
>         /* If work not completed, return budget and polling will return */

This code is now slower after this change.

On Mon, 2016-02-22 at 08:33 -0800, Alexander Duyck wrote:
> On Wed, Feb 17, 2016 at 7:38 PM, Jeff Kirsher
> <jeffrey.t.kirsher@intel.com> wrote:
> > From: Mitch Williams <mitch.a.williams@intel.com>
> >
> > Mr. Spock would certainly raise an eyebrow to see us using bitwise
> > operators, when we should clearly be relying on logic. Fascinating.
> >
> > Change-ID: Ie338010c016f93e9faa2002c07c90b15134b7477
> > Signed-off-by: Mitch Williams <mitch.a.williams@intel.com>
> > Tested-by: Andrew Bowers <andrewx.bowers@intel.com>
> > Signed-off-by: Jeff Kirsher <jeffrey.t.kirsher@intel.com>
> 
> NAK
> 
> The bitwise operator lets us at least make a pass through all rings.
> With this new code you stop as soon as one ring has not completed
> cleaning.

I was hoping to hear from Mitch, but since I have not.  I will send out
a revert of this patch in my next i40e/i40evf update in the next day or
so.