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

syncd/syncd.cpp

@@ -5,7 +5,19 @@
 #include "swss/tokenize.h"
 #include <limits.h>
 
-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;
 
 swss::RedisClient *g_redisClient = NULL;
 
@@ -148,7 +160,6 @@ void remove_rid_and_vid_from_local(
 sai_object_id_t translate_rid_to_vid(
         _In_ sai_object_id_t rid)
 {
-    std::lock_guard<std::mutex> lock(g_db_mutex);
 
     SWSS_LOG_ENTER();
 
@@ -277,7 +288,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();
 
@@ -401,7 +411,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);
 }
@@ -589,7 +598,6 @@ void internal_syncd_get_send(
     // object type and object id, only get status is required
     // get response will not put any data to table only queue is used
 
-    std::lock_guard<std::mutex> lock(g_db_mutex);
 
     getResponse->set(key, entry, "getresponse");
 
@@ -707,7 +715,6 @@ sai_status_t handle_generic(
                     std::string str_vid = sai_serialize_object_id(object_id);
                     std::string str_rid = sai_serialize_object_id(real_object_id);
 
-                    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);
@@ -741,7 +748,6 @@ sai_status_t handle_generic(
                 std::string str_vid = sai_serialize_object_id(object_id);
                 std::string str_rid = sai_serialize_object_id(rid);
 
-                std::lock_guard<std::mutex> lock(g_db_mutex);
 
                 g_redisClient->hdel(VIDTORID, str_vid);
                 g_redisClient->hdel(RIDTOVID, str_rid);
@@ -984,7 +990,6 @@ void sendResponse(sai_status_t status)
 
     SWSS_LOG_NOTICE("sending response: %s", str_status.c_str());
 
-    std::lock_guard<std::mutex> lock(g_db_mutex);
 
     getResponse->set(str_status, entry, "notify");
 }
@@ -1001,7 +1006,6 @@ void clearTempView()
 
     // TODO optimize with lua script (this takes ~0.2s now)
 
-    std::lock_guard<std::mutex> lock(g_db_mutex);
 
     for (const auto &key: g_redisClient->keys(pattern))
     {
@@ -1396,6 +1400,8 @@ sai_status_t processBulkEvent(
 
 sai_status_t processEvent(swss::ConsumerTable &consumer)
 {
+    std::lock_guard<std::mutex> lock(g_mutex);
+
     SWSS_LOG_ENTER();
 
     swss::KeyOpFieldsValuesTuple kco;
@@ -1799,8 +1805,6 @@ bool handleRestartQuery(swss::NotificationConsumer &restartQuery)
 
 bool isVeryFirstRun()
 {
-    std::lock_guard<std::mutex> lock(g_db_mutex);
-
     SWSS_LOG_ENTER();
 
     // if lane map is not defined in redis db then
@@ -1977,6 +1981,8 @@ int main(int argc, char **argv)
             startCountersThread(options.countersThreadIntervalInSeconds);
         }
 
+        startNotificationsProcessingThread();
+
         SWSS_LOG_NOTICE("syncd listening for events");
 
         swss::Select s;
@@ -2039,5 +2045,7 @@ int main(int argc, char **argv)
 
     stop_cli();
 
+    stopNotificationsProcessingThread();
+
     return EXIT_SUCCESS;
 }

syncd/syncd.h

@@ -62,7 +62,7 @@ extern "C" {
 
 extern void exit_and_notify(int status) __attribute__ ((__noreturn__));
 
-extern std::mutex g_db_mutex;
+extern std::mutex g_mutex;
 extern std::set<sai_object_id_t> g_defaultPriorityGroupsRids;
 extern std::set<sai_object_id_t> g_defaultSchedulerGroupsRids;
 extern std::set<sai_object_id_t> g_defaultQueuesRids;
@@ -167,4 +167,7 @@ void stop_cli();
 sai_status_t applyViewTransition();
 sai_status_t syncdApplyView();
 
+void startNotificationsProcessingThread();
+void stopNotificationsProcessingThread();
+
 #endif // __SYNCD_H__

syncd/syncd_counters.cpp

@@ -6,9 +6,11 @@ void collectCounters(swss::Table &countersTable,
                      const std::vector<sai_port_stat_counter_t> &supportedCounters)
 {
     // collect counters should be under mutex
-    // sice configuration can change and we
+    // since configuration can change and we
     // don't want that during counters collection
 
+    std::lock_guard<std::mutex> lock(g_mutex);
+
     SWSS_LOG_ENTER();
 
     uint32_t countersSize = (uint32_t)supportedCounters.size();
@@ -50,8 +52,6 @@ void collectCounters(swss::Table &countersTable,
             values.push_back(fvt);
         }
 
-        std::lock_guard<std::mutex> lock(g_db_mutex);
-
         countersTable.set(strPortId, values, "");
     }
 }