[syncd]: Refactor mutexes (sonic-net#221)

syncd/syncd.cpp

@@ -7,7 +7,19 @@
 #include <iostream>
 #include <map>
 
-std::mutex g_db_mutex;
+/**
+ * @brief Global mutex for thread synchronization
+ *
+ * Purpose of this mutex is to synchronize multiple threads like main thread,
+ * counters and notifications as well as all operations which require multiple
+ * Redis DB access.
+ *
+ * For example: query DB for next VID id number, and then put map RID and VID
+ * to Redis. From syncd point of view this entire operation should be atomic
+ * and no other thread should access DB or make assumption on previous
+ * information until entire operation will finish.
+ */
+std::mutex g_mutex;
 
 std::shared_ptr<swss::RedisClient>          g_redisClient;
 std::shared_ptr<swss::ProducerTable>        getResponse;
@@ -41,7 +53,7 @@ std::map<sai_object_id_t, std::shared_ptr<SaiSwitch>> switches;
  * could be vlan members, bridge ports etc.
  *
  * We need this list to later on not put them back to temp view mode when doing
- * populate existing obejcts in apply view mode.
+ * populate existing objects in apply view mode.
  *
  * Object ids here a VIDs.
  */
@@ -327,8 +339,6 @@ sai_object_id_t translate_rid_to_vid(
         _In_ sai_object_id_t rid,
         _In_ sai_object_id_t switch_vid)
 {
-    std::lock_guard<std::mutex> lock(g_db_mutex);
-
     SWSS_LOG_ENTER();
 
     /*
@@ -516,7 +526,6 @@ void translate_rid_to_vid_list(
 sai_object_id_t translate_vid_to_rid(
         _In_ sai_object_id_t vid)
 {
-    std::lock_guard<std::mutex> lock(g_db_mutex);
 
     SWSS_LOG_ENTER();
 
@@ -679,7 +688,6 @@ void snoop_get_attr(
 
     SWSS_LOG_DEBUG("%s", key.c_str());
 
-    std::lock_guard<std::mutex> lock(g_db_mutex);
 
     g_redisClient->hset(key, attr_id, attr_value);
 }
@@ -1208,7 +1216,6 @@ sai_status_t handle_generic(
                      */
 
                     {
-                        std::lock_guard<std::mutex> lock(g_db_mutex);
 
                         g_redisClient->hset(VIDTORID, str_vid, str_rid);
                         g_redisClient->hset(RIDTOVID, str_rid, str_vid);
@@ -1246,7 +1253,6 @@ sai_status_t handle_generic(
                      */
 
                     {
-                        std::lock_guard<std::mutex> lock(g_db_mutex);
 
                         g_redisClient->hdel(VIDTORID, str_vid);
                         g_redisClient->hdel(RIDTOVID, str_rid);
@@ -1411,7 +1417,6 @@ void clearTempView()
      * We need to expose api to execute user lua script not only predefined.
      */
 
-    std::lock_guard<std::mutex> lock(g_db_mutex);
 
     for (const auto &key: g_redisClient->keys(pattern))
     {
@@ -2107,6 +2112,8 @@ sai_status_t processBulkEvent(
 sai_status_t processEvent(
         _In_ swss::ConsumerTable &consumer)
 {
+    std::lock_guard<std::mutex> lock(g_mutex);
+
     SWSS_LOG_ENTER();
 
     swss::KeyOpFieldsValuesTuple kco;
@@ -2613,7 +2620,6 @@ bool handleRestartQuery(swss::NotificationConsumer &restartQuery)
 
 bool isVeryFirstRun()
 {
-    std::lock_guard<std::mutex> lock(g_db_mutex);
 
     SWSS_LOG_ENTER();
 
@@ -2761,6 +2767,8 @@ void performWarmRestart()
 
 void onSyncdStart(bool warmStart)
 {
+    std::lock_guard<std::mutex> lock(g_mutex);
+
     /*
      * It may happen that after initialize we will receive some port
      * notifications with port'ids that are not in redis db yet, so after
@@ -2987,6 +2995,8 @@ int main(int argc, char **argv)
             startCountersThread(options.countersThreadIntervalInSeconds);
         }
 
+        startNotificationsProcessingThread();
+
         SWSS_LOG_NOTICE("syncd listening for events");
 
         swss::Select s;
@@ -3056,6 +3066,8 @@ int main(int argc, char **argv)
         SWSS_LOG_ERROR("failed to uninitialize api: %s", sai_serialize_status(status).c_str());
     }
 
+    stopNotificationsProcessingThread();
+
     SWSS_LOG_NOTICE("uninitialize finished");
 
     exit(EXIT_SUCCESS);

syncd/syncd.h

@@ -58,7 +58,7 @@ extern "C" {
 #define SWITCH_SAI_THRIFT_RPC_SERVER_PORT 9092
 #endif // SAITHRIFT
 
-extern std::mutex g_db_mutex;
+extern std::mutex g_mutex;
 
 extern std::map<sai_object_id_t, std::shared_ptr<SaiSwitch>> switches;
 
@@ -110,4 +110,7 @@ bool is_set_attribute_workaround(
         _In_ sai_attr_id_t attrid,
         _In_ sai_status_t status);
 
+void startNotificationsProcessingThread();
+void stopNotificationsProcessingThread();
+
 #endif // __SYNCD_H__

syncd/syncd_counters.cpp

@@ -29,8 +29,10 @@ void collectCountersThread(
 
         for (auto sw: switches)
         {
+            std::lock_guard<std::mutex> lock(g_mutex);
+
             /*
-             * Collect counters should be under mutex sice configuration can
+             * Collect counters should be under mutex since configuration can
              * change and we don't want that during counters collection.
              */