[flex-counters] [202012] Delay flex counters stats init for faster boot time (#1804)

shlomibitton · web-flow · commit b0fb3924c47a · 2021-07-01T14:39:01.000-07:00
What I did
Update flex counters DB with counters stats only when counters are enabled.
As long as the polling counters are not enabled, flex counters information will stored internally on PortsOrch.

Why I did it
Creating flex counters objects on the DB will trigger 'SYNCD' to access the HW for query statistics capabilities.
This HW access takes time and will be better to finish boot before doing this (mainly for fast-reboot but good to have in general).
The flex counters are not crucial at boot time, we can delay it to the end of the boot process.

How I verified it
Reboot a switch and observer the flex counters DB populated after counters are enabled.
Signed-off-by: Shlomi Bitton &lt;shlomibi@nvidia.com&gt;
diff --git a/orchagent/flexcounterorch.cpp b/orchagent/flexcounterorch.cpp
@@ -16,6 +16,11 @@ extern IntfsOrch *gIntfsOrch;
 extern BufferOrch *gBufferOrch;
 
 #define BUFFER_POOL_WATERMARK_KEY   "BUFFER_POOL_WATERMARK"
+#define PORT_KEY                    "PORT"
+#define PORT_BUFFER_DROP_KEY        "PORT_BUFFER_DROP"
+#define QUEUE_KEY                   "QUEUE"
+#define PG_WATERMARK_KEY            "PG_WATERMARK"
+#define RIF_KEY                     "RIF"
 
 unordered_map<string, string> flexCounterGroupMap =
 {
@@ -95,17 +100,32 @@ void FlexCounterOrch::doTask(Consumer &consumer)
                     // which is automatically satisfied upon the creation of the orch object that requires
                     // the syncd flex counter polling service
                     // This postponement is introduced by design to accelerate the initialization process
-                    //
-                    // generateQueueMap() is called as long as a field "FLEX_COUNTER_STATUS" event is heard,
-                    // regardless of whether the key is "QUEUE" or whether the value is "enable" or "disable"
-                    // This can be because generateQueueMap() installs a fundamental list of queue stats
-                    // that need to be polled. So my doubt here is if queue watermark stats shall be piggybacked
-                    // into the same function as they may not be counted as fundamental
-                    gPortsOrch->generateQueueMap();
-                    gPortsOrch->generatePriorityGroupMap();
-                    gIntfsOrch->generateInterfaceMap();
-                    // Install COUNTER_ID_LIST/ATTR_ID_LIST only when hearing buffer pool watermark enable event
-                    if ((key == BUFFER_POOL_WATERMARK_KEY) && (value == "enable"))
+                    if(gPortsOrch && (value == "enable"))
+                    {
+                        if(key == PORT_KEY)
+                        {
+                            gPortsOrch->generatePortCounterMap();
+                            m_port_counter_enabled = true;
+                        }
+                        else if(key == PORT_BUFFER_DROP_KEY)
+                        {
+                            gPortsOrch->generatePortBufferDropCounterMap();
+                            m_port_buffer_drop_counter_enabled = true;
+                        }
+                        else if(key == QUEUE_KEY)
+                        {
+                            gPortsOrch->generateQueueMap();
+                        }
+                        else if(key == PG_WATERMARK_KEY)
+                        {
+                            gPortsOrch->generatePriorityGroupMap();
+                        }
+                    }
+                    if(gIntfsOrch && (key == RIF_KEY) && (value == "enable"))
+                    {
+                        gIntfsOrch->generateInterfaceMap();
+                    }
+                    if (gBufferOrch && (key == BUFFER_POOL_WATERMARK_KEY) && (value == "enable"))
                     {
                         gBufferOrch->generateBufferPoolWatermarkCounterIdList();
                     }
@@ -124,3 +144,13 @@ void FlexCounterOrch::doTask(Consumer &consumer)
         consumer.m_toSync.erase(it++);
     }
 }
+
+bool FlexCounterOrch::getPortCountersState() const
+{
+    return m_port_counter_enabled;
+}
+
+bool FlexCounterOrch::getPortBufferDropCountersState() const
+{
+    return m_port_buffer_drop_counter_enabled;
+}
diff --git a/orchagent/flexcounterorch.h b/orchagent/flexcounterorch.h
@@ -15,10 +15,14 @@ class FlexCounterOrch: public Orch
     void doTask(Consumer &consumer);
     FlexCounterOrch(swss::DBConnector *db, std::vector<std::string> &tableNames);
     virtual ~FlexCounterOrch(void);
+    bool getPortCountersState() const;
+    bool getPortBufferDropCountersState() const;
  
 private:
     std::shared_ptr<swss::DBConnector> m_flexCounterDb = nullptr;
     std::shared_ptr<swss::ProducerTable> m_flexCounterGroupTable = nullptr;
+    bool m_port_counter_enabled = false;
+    bool m_port_buffer_drop_counter_enabled = false;
 };
 
 #endif
diff --git a/orchagent/orchdaemon.cpp b/orchagent/orchdaemon.cpp
@@ -330,7 +330,11 @@ bool OrchDaemon::init()
         CFG_FLEX_COUNTER_TABLE_NAME
     };
 
-    m_orchList.push_back(new FlexCounterOrch(m_configDb, flex_counter_tables));
+    auto* flexCounterOrch = new FlexCounterOrch(m_configDb, flex_counter_tables);
+    m_orchList.push_back(flexCounterOrch);
+
+    gDirectory.set(flexCounterOrch);
+    gDirectory.set(gPortsOrch);
 
     vector<string> pfc_wd_tables = {
         CFG_PFC_WD_TABLE_NAME
diff --git a/orchagent/portsorch.cpp b/orchagent/portsorch.cpp
@@ -226,9 +226,9 @@ static char* hostif_vlan_tag[] = {
  */
 PortsOrch::PortsOrch(DBConnector *db, vector<table_name_with_pri_t> &tableNames) :
         Orch(db, tableNames),
-        port_stat_manager(PORT_STAT_COUNTER_FLEX_COUNTER_GROUP, StatsMode::READ, PORT_STAT_FLEX_COUNTER_POLLING_INTERVAL_MS, true),
-        port_buffer_drop_stat_manager(PORT_BUFFER_DROP_STAT_FLEX_COUNTER_GROUP, StatsMode::READ, PORT_BUFFER_DROP_STAT_POLLING_INTERVAL_MS, true),
-        queue_stat_manager(QUEUE_STAT_COUNTER_FLEX_COUNTER_GROUP, StatsMode::READ, QUEUE_STAT_FLEX_COUNTER_POLLING_INTERVAL_MS, true)
+        port_stat_manager(PORT_STAT_COUNTER_FLEX_COUNTER_GROUP, StatsMode::READ, PORT_STAT_FLEX_COUNTER_POLLING_INTERVAL_MS, false),
+        port_buffer_drop_stat_manager(PORT_BUFFER_DROP_STAT_FLEX_COUNTER_GROUP, StatsMode::READ, PORT_BUFFER_DROP_STAT_POLLING_INTERVAL_MS, false),
+        queue_stat_manager(QUEUE_STAT_COUNTER_FLEX_COUNTER_GROUP, StatsMode::READ, QUEUE_STAT_FLEX_COUNTER_POLLING_INTERVAL_MS, false)
 {
     SWSS_LOG_ENTER();
 
@@ -2012,19 +2012,21 @@ bool PortsOrch::initPort(const string &alias, const int index, const set<int> &l
                 vector<FieldValueTuple> fields;
                 fields.push_back(tuple);
                 m_counterTable->set("", fields);
+
                 // Install a flex counter for this port to track stats
-                std::unordered_set<std::string> counter_stats;
-                for (const auto& it: port_stat_ids)
+                auto flex_counters_orch = gDirectory.get<FlexCounterOrch*>();
+                /* Delay installing the counters if they are yet enabled
+                If they are enabled, install the counters immediately */
+                if (flex_counters_orch->getPortCountersState())
                 {
-                    counter_stats.emplace(sai_serialize_port_stat(it));
+                    auto port_counter_stats = generateCounterStats(PORT_STAT_COUNTER_FLEX_COUNTER_GROUP);
+                    port_stat_manager.setCounterIdList(p.m_port_id, CounterType::PORT, port_counter_stats);
                 }
-                port_stat_manager.setCounterIdList(p.m_port_id, CounterType::PORT, counter_stats);
-                std::unordered_set<std::string> port_buffer_drop_stats;
-                for (const auto& it: port_buffer_drop_stat_ids)
+                if (flex_counters_orch->getPortBufferDropCountersState())
                 {
-                    port_buffer_drop_stats.emplace(sai_serialize_port_stat(it));
+                    auto port_buffer_drop_stats = generateCounterStats(PORT_BUFFER_DROP_STAT_FLEX_COUNTER_GROUP);
+                    port_buffer_drop_stat_manager.setCounterIdList(p.m_port_id, CounterType::PORT, port_buffer_drop_stats);
                 }
-                port_buffer_drop_stat_manager.setCounterIdList(p.m_port_id, CounterType::PORT, port_buffer_drop_stats);
 
                 PortUpdate update = { p, true };
                 notify(SUBJECT_TYPE_PORT_CHANGE, static_cast<void *>(&update));
@@ -2057,8 +2059,11 @@ void PortsOrch::deInitPort(string alias, sai_object_id_t port_id)
     p.m_port_id = port_id;
 
     /* remove port from flex_counter_table for updating counters  */
-    port_stat_manager.clearCounterIdList(p.m_port_id);
-
+    auto flex_counters_orch = gDirectory.get<FlexCounterOrch*>();
+    if ((flex_counters_orch->getPortCountersState()))
+    {
+        port_stat_manager.clearCounterIdList(p.m_port_id);
+    }
     /* remove port name map from counter table */
     m_counter_db->hdel(COUNTERS_PORT_NAME_MAP, alias);
 
@@ -4375,6 +4380,48 @@ void PortsOrch::generatePriorityGroupMapPerPort(const Port& port)
     CounterCheckOrch::getInstance().addPort(port);
 }
 
+void PortsOrch::generatePortCounterMap()
+{
+    if (m_isPortCounterMapGenerated)
+    {
+        return;
+    }
+
+    auto port_counter_stats = generateCounterStats(PORT_STAT_COUNTER_FLEX_COUNTER_GROUP);
+    for (const auto& it: m_portList)
+    {
+        // Set counter stats only for PHY ports to ensure syncd will not try to query the counter statistics from the HW for non-PHY ports.
+        if (it.second.m_type != Port::Type::PHY)
+        {
+            continue;
+        }
+        port_stat_manager.setCounterIdList(it.second.m_port_id, CounterType::PORT, port_counter_stats);
+    }
+
+    m_isPortCounterMapGenerated = true;
+}
+
+void PortsOrch::generatePortBufferDropCounterMap()
+{
+    if (m_isPortBufferDropCounterMapGenerated)
+    {
+        return;
+    }
+
+    auto port_buffer_drop_stats = generateCounterStats(PORT_BUFFER_DROP_STAT_FLEX_COUNTER_GROUP);
+    for (const auto& it: m_portList)
+    {
+        // Set counter stats only for PHY ports to ensure syncd will not try to query the counter statistics from the HW for non-PHY ports.
+        if (it.second.m_type != Port::Type::PHY)
+        {
+            continue;
+        }
+        port_buffer_drop_stat_manager.setCounterIdList(it.second.m_port_id, CounterType::PORT, port_buffer_drop_stats);
+    }
+
+    m_isPortBufferDropCounterMapGenerated = true;
+}
+
 void PortsOrch::doTask(NotificationConsumer &consumer)
 {
     SWSS_LOG_ENTER();
@@ -4958,3 +5005,22 @@ bool PortsOrch::initGearboxPort(Port &port)
     return true;
 }
 
+std::unordered_set<std::string> PortsOrch::generateCounterStats(const string& type)
+{
+    std::unordered_set<std::string> counter_stats;
+    if (type == PORT_STAT_COUNTER_FLEX_COUNTER_GROUP)
+    {
+        for (const auto& it: port_stat_ids)
+        {
+            counter_stats.emplace(sai_serialize_port_stat(it));
+        }
+    }
+    else if (type == PORT_BUFFER_DROP_STAT_FLEX_COUNTER_GROUP)
+    {
+        for (const auto& it: port_buffer_drop_stat_ids)
+        {
+            counter_stats.emplace(sai_serialize_port_stat(it));
+        }
+    }
+    return counter_stats;
+}
diff --git a/orchagent/portsorch.h b/orchagent/portsorch.h
@@ -12,6 +12,7 @@
 #include "flex_counter_manager.h"
 #include "gearboxutils.h"
 #include "saihelper.h"
+#include "flexcounterorch.h"
 
 
 #define FCS_LEN 4
@@ -123,6 +124,8 @@ class PortsOrch : public Orch, public Subject
 
     void generateQueueMap();
     void generatePriorityGroupMap();
+    void generatePortCounterMap();
+    void generatePortBufferDropCounterMap();
 
     void refreshPortStatus();
     bool removeAclTableGroup(const Port &p);
@@ -277,6 +280,9 @@ class PortsOrch : public Orch, public Subject
     bool m_isPriorityGroupMapGenerated = false;
     void generatePriorityGroupMapPerPort(const Port& port);
 
+    bool m_isPortCounterMapGenerated = false;
+    bool m_isPortBufferDropCounterMapGenerated = false;
+
     bool setPortAutoNeg(sai_object_id_t id, int an);
     bool setPortFecMode(sai_object_id_t id, int fec);
 
@@ -296,6 +302,8 @@ class PortsOrch : public Orch, public Subject
     void initGearbox();
     bool initGearboxPort(Port &port);
     
+    std::unordered_set<std::string> generateCounterStats(const string& type);
+
 };
 #endif /* SWSS_PORTSORCH_H */
 
diff --git a/tests/mock_tests/mock_orchagent_main.h b/tests/mock_tests/mock_orchagent_main.h
@@ -15,6 +15,8 @@
 #include "vxlanorch.h"
 #include "policerorch.h"
 #include "fgnhgorch.h"
+#include "flexcounterorch.h"
+#include "directory.h"
 
 extern int gBatchSize;
 extern bool gSwssRecord;
@@ -42,6 +44,7 @@ extern FdbOrch *gFdbOrch;
 extern MirrorOrch *gMirrorOrch;
 extern BufferOrch *gBufferOrch;
 extern VRFOrch *gVrfOrch;
+extern Directory<Orch*> gDirectory;
 
 extern sai_acl_api_t *sai_acl_api;
 extern sai_switch_api_t *sai_switch_api;
diff --git a/tests/mock_tests/portsorch_ut.cpp b/tests/mock_tests/portsorch_ut.cpp
@@ -142,6 +142,14 @@ namespace portsorch_test
         };
 
         ASSERT_EQ(gPortsOrch, nullptr);
+
+        vector<string> flex_counter_tables = {
+            CFG_FLEX_COUNTER_TABLE_NAME
+        };
+        auto* flexCounterOrch = new FlexCounterOrch(m_config_db.get(), flex_counter_tables);
+
+        gDirectory.set(flexCounterOrch);
+
         gPortsOrch = new PortsOrch(m_app_db.get(), ports_tables);
         vector<string> buffer_tables = { APP_BUFFER_POOL_TABLE_NAME,
                                          APP_BUFFER_PROFILE_TABLE_NAME,
diff --git a/tests/test_flex_counters.py b/tests/test_flex_counters.py
diff --git a/tests/test_pg_drop_counter.py b/tests/test_pg_drop_counter.py
