Doubling the number of atomic ops was expected to cause a severe performance
regression. This hack demonstrates that the test code wasn't responsible. A
bit of extra suspicion in clib_bihash_search_inline_2_with_hash() will
likely solve the problem without the huge performance regression. 

D.  

-----Original Message-----
From: vpp-dev@lists.fd.io <vpp-dev@lists.fd.io> On Behalf Of Hao Tian
Sent: Tuesday, March 14, 2023 9:47 PM
To: vpp-dev@lists.fd.io
Subject: Re: [vpp-dev] Race condition between bihash deletion and searching
- misuse or bug?

Hi,

I've done the change you asked, and the "suspicious value" warnings are all
gone. Here is the diff:

diff --git a/src/vppinfra/bihash_template.h b/src/vppinfra/bihash_template.h
index c4e120e4a..b9f658db3 100644
--- a/src/vppinfra/bihash_template.h
+++ b/src/vppinfra/bihash_template.h
@@ -532,12 +532,7 @@ static inline int BV
(clib_bihash_search_inline_2_with_hash)
   if (PREDICT_FALSE (BV (clib_bihash_bucket_is_empty) (b)))
     return -1;
 
-  if (PREDICT_FALSE (b->lock))
-    {
-      volatile BVT (clib_bihash_bucket) * bv = b;
-      while (bv->lock)
-       CLIB_PAUSE ();
-    }
+  BV (clib_bihash_lock_bucket) (b);
 
   v = BV (clib_bihash_get_value) (h, b->offset);
 
@@ -557,9 +552,11 @@ static inline int BV
(clib_bihash_search_inline_2_with_hash)
       if (BV (clib_bihash_key_compare) (v->kvp[i].key, search_key->key))
        {
          *valuep = v->kvp[i];
+         BV (clib_bihash_unlock_bucket) (b);
          return 0;
        }
     }
+  BV (clib_bihash_unlock_bucket) (b);
   return -1;
 }
 

Some questions regarding the change:

1. There is similar logic in clib_bihash_search_inline_with_hash. I suppose
that this change needs to be applied there as well. Am I right?

2. This patch does eliminate the race condition, but appears to introduce a
huge performance regression. The clocks in `show runtime` will double after
the change (with the 100000-ops limit removed, so the test node runs
indefinitely).

Regards,
Hao Tian

-=-=-=-=-=-=-=-=-=-=-=-
Links: You receive all messages sent to this group.
View/Reply Online (#22708): https://lists.fd.io/g/vpp-dev/message/22708
Mute This Topic: https://lists.fd.io/mt/97599770/21656
Group Owner: vpp-dev+ow...@lists.fd.io
Unsubscribe: https://lists.fd.io/g/vpp-dev/leave/1480452/21656/631435203/xyzzy 
[arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

Reply via email to