diff mbox

[ovs-dev,v3,03/12] flow: Skip invoking expensive count_1bits() with zero input.

Message ID 1476455835-77641-4-git-send-email-bhanuprakash.bodireddy@intel.com
State Superseded
Delegated to: Daniele Di Proietto
Headers show

Commit Message

Bodireddy, Bhanuprakash Oct. 14, 2016, 2:37 p.m. UTC 
This patch checks if trash is non-zero and only then resets the flowmap
bit and increment the pointer by set bits as found in trash.

Signed-off-by: Bhanuprakash Bodireddy <bhanuprakash.bodireddy@intel.com>
Co-authored-by: Antonio Fischetti <antonio.fischetti@intel.com>
Signed-off-by: Antonio Fischetti <antonio.fischetti@intel.com>
Acked-by: Jarno Rajahalme <jarno@ovn.org>
---
 lib/flow.h | 15 ++++++++++-----
 1 file changed, 10 insertions(+), 5 deletions(-)

Comments

Jarno Rajahalme Oct. 14, 2016, 4:02 p.m. UTC | #1 
> On Oct 14, 2016, at 7:37 AM, Bhanuprakash Bodireddy <bhanuprakash.bodireddy@intel.com> wrote:
> 
> This patch checks if trash is non-zero and only then resets the flowmap
> bit and increment the pointer by set bits as found in trash.
> 
> Signed-off-by: Bhanuprakash Bodireddy <bhanuprakash.bodireddy@intel.com>
> Co-authored-by: Antonio Fischetti <antonio.fischetti@intel.com>
> Signed-off-by: Antonio Fischetti <antonio.fischetti@intel.com>
> Acked-by: Jarno Rajahalme <jarno@ovn.org>
> ---
> lib/flow.h | 15 ++++++++++-----
> 1 file changed, 10 insertions(+), 5 deletions(-)
> 
> diff --git a/lib/flow.h b/lib/flow.h
> index 5a14941..74e75d6 100644
> --- a/lib/flow.h
> +++ b/lib/flow.h
> @@ -614,11 +614,16 @@ mf_get_next_in_map(struct mf_for_each_in_map_aux *aux,
>          * to ‘rm1bit’. */
>         map_t trash = *fmap & (rm1bit - 1);
> 
> -        *fmap -= trash;
> -        /* count_1bits() is fast for systems where speed matters (e.g.,
> -         * DPDK), so we don't try avoid using it.
> -         * Advance 'aux->values' to point to the value for 'rm1bit'. */
> -        aux->values += count_1bits(trash);
> +        /* Avoid resetting 'fmap' and calling count_1bits() when trash is
> +         * zero. */
> +        if (trash) {
> +            *fmap -= trash;
> +            /* count_1bits() is fast for systems where speed matters (e.g.,
> +             * DPDK), so we don't try avoid using it.

The comment above is still wrong as we now test ‘trash’ for non-zero above.

  Jarno

> +             * Advance 'aux->values' to point to the value for 'rm1bit' only.
> +             */
> +            aux->values += count_1bits(trash);
> +        }
> 
>         *value = *aux->values;
>     } else {
> -- 
> 2.4.11
> 
> _______________________________________________
> dev mailing list
> dev@openvswitch.org
> http://openvswitch.org/mailman/listinfo/dev
Fischetti, Antonio Oct. 17, 2016, 9:10 a.m. UTC | #2 
Thanks Jarno, one question below.

Antonio

> -----Original Message-----

> From: dev [mailto:dev-bounces@openvswitch.org] On Behalf Of Jarno

> Rajahalme

> Sent: Friday, October 14, 2016 5:03 PM

> To: Bodireddy, Bhanuprakash <bhanuprakash.bodireddy@intel.com>

> Cc: dev@openvswitch.org

> Subject: Re: [ovs-dev] [PATCH v3 03/12] flow: Skip invoking expensive

> count_1bits() with zero input.

> 

> 

> > On Oct 14, 2016, at 7:37 AM, Bhanuprakash Bodireddy

> <bhanuprakash.bodireddy@intel.com> wrote:

> >

> > This patch checks if trash is non-zero and only then resets the flowmap

> > bit and increment the pointer by set bits as found in trash.

> >

> > Signed-off-by: Bhanuprakash Bodireddy <bhanuprakash.bodireddy@intel.com>

> > Co-authored-by: Antonio Fischetti <antonio.fischetti@intel.com>

> > Signed-off-by: Antonio Fischetti <antonio.fischetti@intel.com>

> > Acked-by: Jarno Rajahalme <jarno@ovn.org>

> > ---

> > lib/flow.h | 15 ++++++++++-----

> > 1 file changed, 10 insertions(+), 5 deletions(-)

> >

> > diff --git a/lib/flow.h b/lib/flow.h

> > index 5a14941..74e75d6 100644

> > --- a/lib/flow.h

> > +++ b/lib/flow.h

> > @@ -614,11 +614,16 @@ mf_get_next_in_map(struct mf_for_each_in_map_aux

> *aux,

> >          * to ‘rm1bit’. */

> >         map_t trash = *fmap & (rm1bit - 1);

> >

> > -        *fmap -= trash;

> > -        /* count_1bits() is fast for systems where speed matters (e.g.,

> > -         * DPDK), so we don't try avoid using it.

> > -         * Advance 'aux->values' to point to the value for 'rm1bit'. */

> > -        aux->values += count_1bits(trash);

> > +        /* Avoid resetting 'fmap' and calling count_1bits() when trash

> is

> > +         * zero. */

> > +        if (trash) {

> > +            *fmap -= trash;

> > +            /* count_1bits() is fast for systems where speed matters

> (e.g.,

> > +             * DPDK), so we don't try avoid using it.

> 

> The comment above is still wrong as we now test ‘trash’ for non-zero

> above.

> 

>   Jarno

[Antonio F] Just to avoid misunderstanding, do you mean we can now entirely remove
the existing comment
    /* count_1bits() is fast for systems where speed....
right?
Because with the latest changes now we do try to avoid using count_1bits(). Is that ok?


> 

> > +             * Advance 'aux->values' to point to the value for 'rm1bit'

> only.

> > +             */

> > +            aux->values += count_1bits(trash);

> > +        }

> >

> >         *value = *aux->values;

> >     } else {

> > --

> > 2.4.11

> >

> > _______________________________________________

> > dev mailing list

> > dev@openvswitch.org

> > http://openvswitch.org/mailman/listinfo/dev

> 

> _______________________________________________

> dev mailing list

> dev@openvswitch.org

> http://openvswitch.org/mailman/listinfo/dev
Jarno Rajahalme Oct. 17, 2016, 8:09 p.m. UTC | #3 
> On Oct 17, 2016, at 2:10 AM, Fischetti, Antonio <antonio.fischetti@intel.com> wrote:
> 
> Thanks Jarno, one question below.
> 
> Antonio
> 
>> -----Original Message-----
>> From: dev [mailto:dev-bounces@openvswitch.org <mailto:dev-bounces@openvswitch.org>] On Behalf Of Jarno
>> Rajahalme
>> Sent: Friday, October 14, 2016 5:03 PM
>> To: Bodireddy, Bhanuprakash <bhanuprakash.bodireddy@intel.com <mailto:bhanuprakash.bodireddy@intel.com>>
>> Cc: dev@openvswitch.org <mailto:dev@openvswitch.org>
>> Subject: Re: [ovs-dev] [PATCH v3 03/12] flow: Skip invoking expensive
>> count_1bits() with zero input.
>> 
>> 
>>> On Oct 14, 2016, at 7:37 AM, Bhanuprakash Bodireddy
>> <bhanuprakash.bodireddy@intel.com> wrote:
>>> 
>>> This patch checks if trash is non-zero and only then resets the flowmap
>>> bit and increment the pointer by set bits as found in trash.
>>> 
>>> Signed-off-by: Bhanuprakash Bodireddy <bhanuprakash.bodireddy@intel.com>
>>> Co-authored-by: Antonio Fischetti <antonio.fischetti@intel.com>
>>> Signed-off-by: Antonio Fischetti <antonio.fischetti@intel.com>
>>> Acked-by: Jarno Rajahalme <jarno@ovn.org>
>>> ---
>>> lib/flow.h | 15 ++++++++++-----
>>> 1 file changed, 10 insertions(+), 5 deletions(-)
>>> 
>>> diff --git a/lib/flow.h b/lib/flow.h
>>> index 5a14941..74e75d6 100644
>>> --- a/lib/flow.h
>>> +++ b/lib/flow.h
>>> @@ -614,11 +614,16 @@ mf_get_next_in_map(struct mf_for_each_in_map_aux
>> *aux,
>>>         * to ‘rm1bit’. */
>>>        map_t trash = *fmap & (rm1bit - 1);
>>> 
>>> -        *fmap -= trash;
>>> -        /* count_1bits() is fast for systems where speed matters (e.g.,
>>> -         * DPDK), so we don't try avoid using it.
>>> -         * Advance 'aux->values' to point to the value for 'rm1bit'. */
>>> -        aux->values += count_1bits(trash);
>>> +        /* Avoid resetting 'fmap' and calling count_1bits() when trash
>> is
>>> +         * zero. */
>>> +        if (trash) {
>>> +            *fmap -= trash;
>>> +            /* count_1bits() is fast for systems where speed matters
>> (e.g.,
>>> +             * DPDK), so we don't try avoid using it.
>> 
>> The comment above is still wrong as we now test ‘trash’ for non-zero
>> above.
>> 
>>  Jarno
> [Antonio F] Just to avoid misunderstanding, do you mean we can now entirely remove
> the existing comment
>    /* count_1bits() is fast for systems where speed....
> right?
> Because with the latest changes now we do try to avoid using count_1bits(). Is that ok?
> 
> 

Yes.

  Jarno

>> 
>>> +             * Advance 'aux->values' to point to the value for 'rm1bit'
>> only.
>>> +             */
>>> +            aux->values += count_1bits(trash);
>>> +        }
>>> 
>>>        *value = *aux->values;
>>>    } else {
>>> --
>>> 2.4.11
>>> 
>>> _______________________________________________
>>> dev mailing list
>>> dev@openvswitch.org
>>> http://openvswitch.org/mailman/listinfo/dev
>> 
>> _______________________________________________
>> dev mailing list
>> dev@openvswitch.org <mailto:dev@openvswitch.org>
>> http://openvswitch.org/mailman/listinfo/dev <http://openvswitch.org/mailman/listinfo/dev>
diff mbox

Patch

diff --git a/lib/flow.h b/lib/flow.h
index 5a14941..74e75d6 100644
--- a/lib/flow.h
+++ b/lib/flow.h
@@ -614,11 +614,16 @@  mf_get_next_in_map(struct mf_for_each_in_map_aux *aux,
          * to ‘rm1bit’. */
         map_t trash = *fmap & (rm1bit - 1);
 
-        *fmap -= trash;
-        /* count_1bits() is fast for systems where speed matters (e.g.,
-         * DPDK), so we don't try avoid using it.
-         * Advance 'aux->values' to point to the value for 'rm1bit'. */
-        aux->values += count_1bits(trash);
+        /* Avoid resetting 'fmap' and calling count_1bits() when trash is
+         * zero. */
+        if (trash) {
+            *fmap -= trash;
+            /* count_1bits() is fast for systems where speed matters (e.g.,
+             * DPDK), so we don't try avoid using it.
+             * Advance 'aux->values' to point to the value for 'rm1bit' only.
+             */
+            aux->values += count_1bits(trash);
+        }
 
         *value = *aux->values;
     } else {