diff mbox

[ovs-dev,06/12] cmap: Remove prefetching in cmap_find_batch().

Message ID 1475857062-55311-7-git-send-email-bhanuprakash.bodireddy@intel.com
State Changes Requested
Delegated to: Daniele Di Proietto
Headers show

Commit Message

Bodireddy, Bhanuprakash Oct. 7, 2016, 4:17 p.m. UTC 
prefetching the data in to the caches isn't improving the performance in
cmap_find_batch(). Moreover its found that there is slight improvement
in performance with out prefetching.

This patch removes prefetching from cmap_find_batch().

Signed-off-by: Bhanuprakash Bodireddy <bhanuprakash.bodireddy@intel.com>
Signed-off-by: Antonio Fischetti <antonio.fischetti@intel.com>
---
 lib/cmap.c | 4 ----
 1 file changed, 4 deletions(-)

Comments

Jarno Rajahalme Oct. 7, 2016, 9:10 p.m. UTC | #1 
> On Oct 7, 2016, at 9:17 AM, Bhanuprakash Bodireddy <bhanuprakash.bodireddy@intel.com> wrote:
> 
> prefetching the data in to the caches isn't improving the performance in
> cmap_find_batch(). Moreover its found that there is slight improvement
> in performance with out prefetching.
> 

I recall doing some performance testing for this earlier, but have no records of the system or other circumstances. Is it likely that this is at least somewhat system and test case dependent? Also the modified batch size may be a factor here. Anyway, I don’t currently have any proof to the contrary, so I have no problem removing the prefetching.

However, you should also update all the comments referring to prefetching.

  Jarno

> This patch removes prefetching from cmap_find_batch().
> 
> Signed-off-by: Bhanuprakash Bodireddy <bhanuprakash.bodireddy@intel.com>
> Signed-off-by: Antonio Fischetti <antonio.fischetti@intel.com>
> ---
> lib/cmap.c | 4 ----
> 1 file changed, 4 deletions(-)
> 
> diff --git a/lib/cmap.c b/lib/cmap.c
> index 8c7312d..4c34bda 100644
> --- a/lib/cmap.c
> +++ b/lib/cmap.c
> @@ -397,7 +397,6 @@ cmap_find_batch(const struct cmap *cmap, unsigned long map,
>     ULLONG_FOR_EACH_1(i, map) {
>         h1s[i] = rehash(impl, hashes[i]);
>         b1s[i] = &impl->buckets[h1s[i] & impl->mask];
> -        OVS_PREFETCH(b1s[i]);
>     }
>     /* Lookups, Round 1. Only look up at the first bucket. */
>     ULLONG_FOR_EACH_1(i, map) {
> @@ -413,13 +412,11 @@ cmap_find_batch(const struct cmap *cmap, unsigned long map,
>         if (!node) {
>             /* Not found (yet); Prefetch the 2nd bucket. */
>             b2s[i] = &impl->buckets[other_hash(h1s[i]) & impl->mask];
> -            OVS_PREFETCH(b2s[i]);
>             c1s[i] = c1; /* We may need to check this after Round 2. */
>             continue;
>         }
>         /* Found. */
>         ULLONG_SET0(map, i); /* Ignore this on round 2. */
> -        OVS_PREFETCH(node);
>         nodes[i] = node;
>     }
>     /* Round 2. Look into the 2nd bucket, if needed. */
> @@ -453,7 +450,6 @@ cmap_find_batch(const struct cmap *cmap, unsigned long map,
>             continue;
>         }
> found:
> -        OVS_PREFETCH(node);
>         nodes[i] = node;
>     }
>     return result;
> -- 
> 2.4.11
> 
> _______________________________________________
> dev mailing list
> dev@openvswitch.org
> http://openvswitch.org/mailman/listinfo/dev
Bodireddy, Bhanuprakash Oct. 10, 2016, 3:50 p.m. UTC | #2 
>-----Original Message-----

>From: Jarno Rajahalme [mailto:jarno@ovn.org]

>Sent: Friday, October 7, 2016 10:10 PM

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

>Cc: dev@openvswitch.org

>Subject: Re: [ovs-dev] [PATCH 06/12] cmap: Remove prefetching in

>cmap_find_batch().

>

>

>> On Oct 7, 2016, at 9:17 AM, Bhanuprakash Bodireddy

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

>>

>> prefetching the data in to the caches isn't improving the performance

>> in cmap_find_batch(). Moreover its found that there is slight

>> improvement in performance with out prefetching.

>>

>

>I recall doing some performance testing for this earlier, but have no records of

>the system or other circumstances. Is it likely that this is at least somewhat

>system and test case dependent?

Agree with you,  we are testing this on Haswell CPUs. 

 Also the modified batch size may be a
>factor here. 

I verified this patch with batch size '16' and still could get better performance.
So the improvement may not be due to increasing the batch size to 32 as done in first patch of the series. 

Anyway, I don’t currently have any proof to the contrary, so I
>have no problem removing the prefetching.

>

>However, you should also update all the comments referring to prefetching.

Sure, I will do this in v2.

Regards,
Bhanu Prakash.

>

>  Jarno

>

>> This patch removes prefetching from cmap_find_batch().

>>

>> Signed-off-by: Bhanuprakash Bodireddy

>> <bhanuprakash.bodireddy@intel.com>

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

>> ---

>> lib/cmap.c | 4 ----

>> 1 file changed, 4 deletions(-)

>>

>> diff --git a/lib/cmap.c b/lib/cmap.c

>> index 8c7312d..4c34bda 100644

>> --- a/lib/cmap.c

>> +++ b/lib/cmap.c

>> @@ -397,7 +397,6 @@ cmap_find_batch(const struct cmap *cmap,

>unsigned long map,

>>     ULLONG_FOR_EACH_1(i, map) {

>>         h1s[i] = rehash(impl, hashes[i]);

>>         b1s[i] = &impl->buckets[h1s[i] & impl->mask];

>> -        OVS_PREFETCH(b1s[i]);

>>     }

>>     /* Lookups, Round 1. Only look up at the first bucket. */

>>     ULLONG_FOR_EACH_1(i, map) {

>> @@ -413,13 +412,11 @@ cmap_find_batch(const struct cmap *cmap,

>unsigned long map,

>>         if (!node) {

>>             /* Not found (yet); Prefetch the 2nd bucket. */

>>             b2s[i] = &impl->buckets[other_hash(h1s[i]) & impl->mask];

>> -            OVS_PREFETCH(b2s[i]);

>>             c1s[i] = c1; /* We may need to check this after Round 2. */

>>             continue;

>>         }

>>         /* Found. */

>>         ULLONG_SET0(map, i); /* Ignore this on round 2. */

>> -        OVS_PREFETCH(node);

>>         nodes[i] = node;

>>     }

>>     /* Round 2. Look into the 2nd bucket, if needed. */ @@ -453,7

>> +450,6 @@ cmap_find_batch(const struct cmap *cmap, unsigned long map,

>>             continue;

>>         }

>> found:

>> -        OVS_PREFETCH(node);

>>         nodes[i] = node;

>>     }

>>     return result;

>> --

>> 2.4.11

>>

>> _______________________________________________

>> dev mailing list

>> dev@openvswitch.org

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

Patch

diff --git a/lib/cmap.c b/lib/cmap.c
index 8c7312d..4c34bda 100644
--- a/lib/cmap.c
+++ b/lib/cmap.c
@@ -397,7 +397,6 @@  cmap_find_batch(const struct cmap *cmap, unsigned long map,
     ULLONG_FOR_EACH_1(i, map) {
         h1s[i] = rehash(impl, hashes[i]);
         b1s[i] = &impl->buckets[h1s[i] & impl->mask];
-        OVS_PREFETCH(b1s[i]);
     }
     /* Lookups, Round 1. Only look up at the first bucket. */
     ULLONG_FOR_EACH_1(i, map) {
@@ -413,13 +412,11 @@  cmap_find_batch(const struct cmap *cmap, unsigned long map,
         if (!node) {
             /* Not found (yet); Prefetch the 2nd bucket. */
             b2s[i] = &impl->buckets[other_hash(h1s[i]) & impl->mask];
-            OVS_PREFETCH(b2s[i]);
             c1s[i] = c1; /* We may need to check this after Round 2. */
             continue;
         }
         /* Found. */
         ULLONG_SET0(map, i); /* Ignore this on round 2. */
-        OVS_PREFETCH(node);
         nodes[i] = node;
     }
     /* Round 2. Look into the 2nd bucket, if needed. */
@@ -453,7 +450,6 @@  cmap_find_batch(const struct cmap *cmap, unsigned long map,
             continue;
         }
 found:
-        OVS_PREFETCH(node);
         nodes[i] = node;
     }
     return result;