From b0cac37b070d52fd18f98919fb17736dd1a3f0eb Mon Sep 17 00:00:00 2001 From: Pavel Shirshov Date: Wed, 18 Apr 2018 12:11:54 -0700 Subject: [PATCH 1/7] Remove global mutex for the DB --- orchagent/main.cpp | 4 ---- orchagent/notifications.cpp | 6 ------ orchagent/orch.cpp | 6 ------ 3 files changed, 16 deletions(-) diff --git a/orchagent/main.cpp b/orchagent/main.cpp index 3a92a7ea8f..f88ad3589b 100644 --- a/orchagent/main.cpp +++ b/orchagent/main.cpp @@ -7,7 +7,6 @@ extern "C" { #include #include #include -#include #include #include #include @@ -47,9 +46,6 @@ bool gLogRotate = false; ofstream gRecordOfs; string gRecordFile; -/* Global database mutex */ -mutex gDbMutex; - void usage() { cout << "usage: orchagent [-h] [-r record_type] [-d record_location] [-b batch_size] [-m MAC]" << endl; diff --git a/orchagent/notifications.cpp b/orchagent/notifications.cpp index 7879827ffc..67b22bb1f5 100644 --- a/orchagent/notifications.cpp +++ b/orchagent/notifications.cpp @@ -1,5 +1,4 @@ #include -#include #include #include "portsorch.h" @@ -12,7 +11,6 @@ extern "C" { #include "logger.h" #include "notifications.h" -extern mutex gDbMutex; extern PortsOrch *gPortsOrch; extern FdbOrch *gFdbOrch; @@ -20,8 +18,6 @@ void on_fdb_event(uint32_t count, sai_fdb_event_notification_data_t *data) { SWSS_LOG_ENTER(); - lock_guard lock(gDbMutex); - if (!gFdbOrch) { SWSS_LOG_NOTICE("gFdbOrch is not initialized"); @@ -49,8 +45,6 @@ void on_port_state_change(uint32_t count, sai_port_oper_status_notification_t *d { SWSS_LOG_ENTER(); - lock_guard lock(gDbMutex); - if (!gPortsOrch) { SWSS_LOG_NOTICE("gPortsOrch is not initialized"); diff --git a/orchagent/orch.cpp b/orchagent/orch.cpp index 330ea32879..ce6258e543 100644 --- a/orchagent/orch.cpp +++ b/orchagent/orch.cpp @@ -1,6 +1,5 @@ #include #include -#include #include #include "timestamp.h" #include "orch.h" @@ -15,8 +14,6 @@ using namespace swss; extern int gBatchSize; -extern mutex gDbMutex; - extern bool gSwssRecord; extern ofstream gRecordOfs; extern bool gLogRotate; @@ -73,9 +70,6 @@ void Consumer::execute() { SWSS_LOG_ENTER(); - // TODO: remove DbMutex when there is only single thread - lock_guard lock(gDbMutex); - std::deque entries; getConsumerTable()->pops(entries); From f61cacba784cf3680314567b15bd47970414de19 Mon Sep 17 00:00:00 2001 From: Pavel Shirshov Date: Wed, 18 Apr 2018 12:18:02 -0700 Subject: [PATCH 2/7] Don't set event notifications on the switch --- orchagent/main.cpp | 20 ++++---------------- 1 file changed, 4 insertions(+), 16 deletions(-) diff --git a/orchagent/main.cpp b/orchagent/main.cpp index f88ad3589b..b17697a547 100644 --- a/orchagent/main.cpp +++ b/orchagent/main.cpp @@ -149,17 +149,6 @@ int main(int argc, char **argv) initSaiApi(); initSaiRedis(record_location); - sai_attribute_t attr; - vector attrs; - - attr.id = SAI_SWITCH_ATTR_INIT_SWITCH; - attr.value.booldata = true; - attrs.push_back(attr); - - attr.id = SAI_SWITCH_ATTR_FDB_EVENT_NOTIFY; - attr.value.ptr = (void *)on_fdb_event; - attrs.push_back(attr); - /* Disable/enable SwSS recording */ if (gSwssRecord) { @@ -173,12 +162,11 @@ int main(int argc, char **argv) gRecordOfs << getTimestamp() << "|recording started" << endl; } - attr.id = SAI_SWITCH_ATTR_PORT_STATE_CHANGE_NOTIFY; - attr.value.ptr = (void *)on_port_state_change; - attrs.push_back(attr); + sai_attribute_t attr; + vector attrs; - attr.id = SAI_SWITCH_ATTR_SHUTDOWN_REQUEST_NOTIFY; - attr.value.ptr = (void *)on_switch_shutdown_request; + attr.id = SAI_SWITCH_ATTR_INIT_SWITCH; + attr.value.booldata = true; attrs.push_back(attr); if (gMacAddress) From 94fcb19e591c5df678528bb74874c80087345dee Mon Sep 17 00:00:00 2001 From: Pavel Shirshov Date: Wed, 18 Apr 2018 15:23:32 -0700 Subject: [PATCH 3/7] Compile, but doesn't work' --- orchagent/fdborch.cpp | 86 ++++++++++++++++++++++++++----------- orchagent/fdborch.h | 2 +- orchagent/main.cpp | 4 ++ orchagent/notifications.cpp | 58 ------------------------- orchagent/notifications.h | 2 - orchagent/notifier.h | 9 ++-- orchagent/orch.h | 2 +- orchagent/pfcwdorch.cpp | 4 +- orchagent/pfcwdorch.h | 2 +- orchagent/portsorch.cpp | 55 ++++++++++++++++++++++++ orchagent/portsorch.h | 2 + 11 files changed, 134 insertions(+), 92 deletions(-) diff --git a/orchagent/fdborch.cpp b/orchagent/fdborch.cpp index 66e1d5af9b..e85942da2d 100644 --- a/orchagent/fdborch.cpp +++ b/orchagent/fdborch.cpp @@ -9,6 +9,7 @@ #include "fdborch.h" #include "crmorch.h" #include "notifier.h" +#include "sai_serialize.h" extern sai_fdb_api_t *sai_fdb_api; @@ -25,8 +26,13 @@ FdbOrch::FdbOrch(DBConnector *db, string tableName, PortsOrch *port) : { m_portsOrch->attach(this); auto consumer = new NotificationConsumer(db, "FLUSHFDBREQUEST"); - auto fdbNotification = new Notifier(consumer, this); + auto fdbNotification = new Notifier(consumer, this, "FLUSHFDBREQUEST"); Orch::addExecutor("", fdbNotification); + + DBConnector *notification_db = new DBConnector(ASIC_DB, DBConnector::DEFAULT_UNIXSOCKET, 0); + auto notification_consumer = new swss::NotificationConsumer(notification_db, "NOTIFICATIONS"); + auto fdb_notification = new Notifier(notification_consumer, this, "NOTIFICATIONS"); + Orch::addExecutor("FDB_NOTIFICATIONS", fdb_notification); } void FdbOrch::update(sai_fdb_event_t type, const sai_fdb_entry_t* entry, sai_object_id_t bridge_port_id) @@ -274,7 +280,7 @@ void FdbOrch::doTask(Consumer& consumer) } } -void FdbOrch::doTask(NotificationConsumer& consumer) +void FdbOrch::doTask(NotificationConsumer& consumer, const std::string &name) { SWSS_LOG_ENTER(); @@ -290,36 +296,68 @@ void FdbOrch::doTask(NotificationConsumer& consumer) consumer.pop(op, data, values); - if (op == "ALL") + if (name == "FLUSHFDBREQUEST") { - /* - * so far only support flush all the FDB entris - * flush per port and flush per vlan will be added later. - */ - status = sai_fdb_api->flush_fdb_entries(gSwitchId, 0, NULL); - if (status != SAI_STATUS_SUCCESS) + if (op == "ALL") { - SWSS_LOG_ERROR("Flush fdb failed, return code %x", status); - } + /* + * so far only support flush all the FDB entris + * flush per port and flush per vlan will be added later. + */ + status = sai_fdb_api->flush_fdb_entries(gSwitchId, 0, NULL); + if (status != SAI_STATUS_SUCCESS) + { + SWSS_LOG_ERROR("Flush fdb failed, return code %x", status); + } - return; - } - else if (op == "PORT") - { - /*place holder for flush port fdb*/ - SWSS_LOG_ERROR("Received unsupported flush port fdb request"); - return; + return; + } + else if (op == "PORT") + { + /*place holder for flush port fdb*/ + SWSS_LOG_ERROR("Received unsupported flush port fdb request"); + return; + } + else if (op == "VLAN") + { + /*place holder for flush vlan fdb*/ + SWSS_LOG_ERROR("Received unsupported flush vlan fdb request"); + return; + } + else + { + SWSS_LOG_ERROR("Received unknown flush fdb request"); + return; + } } - else if (op == "VLAN") + else if (name == "NOTIFICATIONS" && op == "fdb_event") { - /*place holder for flush vlan fdb*/ - SWSS_LOG_ERROR("Received unsupported flush vlan fdb request"); - return; + uint32_t count; + sai_fdb_event_notification_data_t *fdbevent = nullptr; + + sai_deserialize_fdb_event_ntf(data, count, &fdbevent); + + for (uint32_t i = 0; i < count; ++i) + { + sai_object_id_t oid = SAI_NULL_OBJECT_ID; + + for (uint32_t j = 0; j < fdbevent[i].attr_count; ++j) + { + if (fdbevent[i].attr[j].id == SAI_FDB_ENTRY_ATTR_BRIDGE_PORT_ID) + { + oid = fdbevent[i].attr[j].value.oid; + break; + } + } + + this->update(fdbevent[i].event_type, &fdbevent[i].fdb_entry, oid); + + sai_deserialize_free_fdb_event_ntf(count, fdbevent); + } } else { - SWSS_LOG_ERROR("Received unknown flush fdb request"); - return; + SWSS_LOG_ERROR("Wrong NotificationConsumer name: '%s'", name.c_str()); } } diff --git a/orchagent/fdborch.h b/orchagent/fdborch.h index eb36333184..1e538014ad 100644 --- a/orchagent/fdborch.h +++ b/orchagent/fdborch.h @@ -53,7 +53,7 @@ class FdbOrch: public Orch, public Subject, public Observer Table m_table; void doTask(Consumer& consumer); - void doTask(NotificationConsumer& consumer); + void doTask(NotificationConsumer& consumer, const std::string &name); void updateVlanMember(const VlanMemberUpdate&); bool addFdbEntry(const FdbEntry&, const string&, const string&); diff --git a/orchagent/main.cpp b/orchagent/main.cpp index b17697a547..68ea70cc06 100644 --- a/orchagent/main.cpp +++ b/orchagent/main.cpp @@ -169,6 +169,10 @@ int main(int argc, char **argv) attr.value.booldata = true; attrs.push_back(attr); + attr.id = SAI_SWITCH_ATTR_SHUTDOWN_REQUEST_NOTIFY; + attr.value.ptr = (void *)on_switch_shutdown_request; + attrs.push_back(attr); + if (gMacAddress) { attr.id = SAI_SWITCH_ATTR_SRC_MAC_ADDRESS; diff --git a/orchagent/notifications.cpp b/orchagent/notifications.cpp index 67b22bb1f5..8b444376a9 100644 --- a/orchagent/notifications.cpp +++ b/orchagent/notifications.cpp @@ -1,9 +1,3 @@ -#include -#include - -#include "portsorch.h" -#include "fdborch.h" - extern "C" { #include "sai.h" } @@ -11,58 +5,6 @@ extern "C" { #include "logger.h" #include "notifications.h" -extern PortsOrch *gPortsOrch; -extern FdbOrch *gFdbOrch; - -void on_fdb_event(uint32_t count, sai_fdb_event_notification_data_t *data) -{ - SWSS_LOG_ENTER(); - - if (!gFdbOrch) - { - SWSS_LOG_NOTICE("gFdbOrch is not initialized"); - return; - } - - for (uint32_t i = 0; i < count; ++i) - { - sai_object_id_t oid = SAI_NULL_OBJECT_ID; - - for (uint32_t j = 0; j < data[i].attr_count; ++j) - { - if (data[i].attr[j].id == SAI_FDB_ENTRY_ATTR_BRIDGE_PORT_ID) - { - oid = data[i].attr[j].value.oid; - break; - } - } - - gFdbOrch->update(data[i].event_type, &data[i].fdb_entry, oid); - } -} - -void on_port_state_change(uint32_t count, sai_port_oper_status_notification_t *data) -{ - SWSS_LOG_ENTER(); - - if (!gPortsOrch) - { - SWSS_LOG_NOTICE("gPortsOrch is not initialized"); - return; - } - - for (uint32_t i = 0; i < count; i++) - { - sai_object_id_t id = data[i].port_id; - sai_port_oper_status_t status = data[i].port_state; - - SWSS_LOG_NOTICE("Get port state change notification id:%lx status:%d", id, status); - - gPortsOrch->updateDbPortOperStatus(id, status); - gPortsOrch->setHostIntfsOperStatus(id, status == SAI_PORT_OPER_STATUS_UP); - } -} - void on_switch_shutdown_request() { SWSS_LOG_ENTER(); diff --git a/orchagent/notifications.h b/orchagent/notifications.h index 961e2eae9f..2aff91616c 100644 --- a/orchagent/notifications.h +++ b/orchagent/notifications.h @@ -2,6 +2,4 @@ extern "C" { #include "sai.h" } -void on_fdb_event(uint32_t count, sai_fdb_event_notification_data_t *data); -void on_port_state_change(uint32_t count, sai_port_oper_status_notification_t *data); void on_switch_shutdown_request(); diff --git a/orchagent/notifier.h b/orchagent/notifier.h index 6113c80527..9b329253f8 100644 --- a/orchagent/notifier.h +++ b/orchagent/notifier.h @@ -2,8 +2,8 @@ class Notifier : public Executor { public: - Notifier(NotificationConsumer *select, Orch *orch) - : Executor(select, orch) + Notifier(NotificationConsumer *select, Orch *orch, const std::string& notifier_name) + : Executor(select, orch), m_name(notifier_name) { } @@ -14,6 +14,9 @@ class Notifier : public Executor { void execute() { - m_orch->doTask(*getNotificationConsumer()); + m_orch->doTask(*getNotificationConsumer(), m_name); } + +private: + std::string m_name; }; diff --git a/orchagent/orch.h b/orchagent/orch.h index eddece4081..64a8a7df12 100644 --- a/orchagent/orch.h +++ b/orchagent/orch.h @@ -152,7 +152,7 @@ class Orch /* Run doTask against a specific executor */ virtual void doTask(Consumer &consumer) = 0; - virtual void doTask(NotificationConsumer &consumer) { } + virtual void doTask(NotificationConsumer &consumer, const std::string &name) { } virtual void doTask(SelectableTimer &timer) { } /* TODO: refactor recording */ diff --git a/orchagent/pfcwdorch.cpp b/orchagent/pfcwdorch.cpp index 8e5f3b7152..0efdb364b2 100644 --- a/orchagent/pfcwdorch.cpp +++ b/orchagent/pfcwdorch.cpp @@ -668,7 +668,7 @@ PfcWdSwOrch::PfcWdSwOrch( auto consumer = new swss::NotificationConsumer( PfcWdSwOrch::getCountersDb().get(), "PFC_WD"); - auto wdNotification = new Notifier(consumer, this); + auto wdNotification = new Notifier(consumer, this, "PFC_WD"); Orch::addExecutor("PFC_WD", wdNotification); } @@ -712,7 +712,7 @@ bool PfcWdSwOrch::stopWdOnPort(const Port& port) } template -void PfcWdSwOrch::doTask(swss::NotificationConsumer& wdNotification) +void PfcWdSwOrch::doTask(swss::NotificationConsumer& wdNotification, const std::string& name) { SWSS_LOG_ENTER(); diff --git a/orchagent/pfcwdorch.h b/orchagent/pfcwdorch.h index 74b709e46e..e329ddf127 100644 --- a/orchagent/pfcwdorch.h +++ b/orchagent/pfcwdorch.h @@ -93,7 +93,7 @@ class PfcWdSwOrch: public PfcWdOrch void registerInWdDb(const Port& port, uint32_t detectionTime, uint32_t restorationTime, PfcWdAction action); void unregisterFromWdDb(const Port& port); - void doTask(swss::NotificationConsumer &wdNotification); + void doTask(swss::NotificationConsumer &wdNotification, const std::string &name); string filterPfcCounters(string counters, set& losslessTc); string getFlexCounterTableKey(string s); diff --git a/orchagent/portsorch.cpp b/orchagent/portsorch.cpp index 2cf741695b..ceca970e9a 100644 --- a/orchagent/portsorch.cpp +++ b/orchagent/portsorch.cpp @@ -18,6 +18,7 @@ #include "sai_serialize.h" #include "crmorch.h" #include "countercheckorch.h" +#include "notifier.h" extern sai_switch_api_t *sai_switch_api; extern sai_bridge_api_t *sai_bridge_api; @@ -235,6 +236,12 @@ PortsOrch::PortsOrch(DBConnector *db, vector &tableNames) removeDefaultVlanMembers(); removeDefaultBridgePorts(); + + /* Add port oper status notification support */ + DBConnector *notification_db = new DBConnector(ASIC_DB, DBConnector::DEFAULT_UNIXSOCKET, 0); + auto notification_consumer = new swss::NotificationConsumer(notification_db, "NOTIFICATIONS"); + auto port_status_notification = new Notifier(notification_consumer, this, "NOTIFICATIONS"); + Orch::addExecutor("PORT_STATUS_NOTIFICATIONS", port_status_notification); } void PortsOrch::removeDefaultVlanMembers() @@ -2330,3 +2337,51 @@ bool PortsOrch::removeLagMember(Port &lag, Port &port) return true; } + +void PortsOrch::doTask(NotificationConsumer &consumer, const std::string &name) +{ + SWSS_LOG_ENTER(); + + /* Wait for all ports to be initialized */ + if (!isInitDone()) + { + return; + } + + std::string op; + std::string data; + std::vector values; + + consumer.pop(op, data, values); + + if (name != "NOTIFICATIONS") + { + SWSS_LOG_ERROR("Wrong name of notification provider: '%s'", name.c_str()); + return; + } + + if (op == "port_state_change") + { + uint32_t count; + sai_port_oper_status_notification_t *portoperstatus = nullptr; + + sai_deserialize_port_oper_status_ntf(data, count, &portoperstatus); + + for (uint32_t i = 0; i < count; i++) + { + sai_object_id_t id = portoperstatus[i].port_id; + sai_port_oper_status_t status = portoperstatus[i].port_state; + + SWSS_LOG_NOTICE("Get port state change notification id:%lx status:%d", id, status); + + this->updateDbPortOperStatus(id, status); + this->setHostIntfsOperStatus(id, status == SAI_PORT_OPER_STATUS_UP); + } + + sai_deserialize_free_port_oper_status_ntf(count, portoperstatus); + } + else + { + SWSS_LOG_ERROR("Wrong notification name in notification provider '%s': '%s'", name.c_str(), op.c_str()); + } +} diff --git a/orchagent/portsorch.h b/orchagent/portsorch.h index 2ea1bb874d..caac8f7a4b 100644 --- a/orchagent/portsorch.h +++ b/orchagent/portsorch.h @@ -92,6 +92,8 @@ class PortsOrch : public Orch, public Subject void doLagTask(Consumer &consumer); void doLagMemberTask(Consumer &consumer); + void doTask(NotificationConsumer &consumer, const std::string &name); + void removeDefaultVlanMembers(); void removeDefaultBridgePorts(); From e90839db796bcac21d080c81a5daddd26c4ef3bb Mon Sep 17 00:00:00 2001 From: Pavel Shirshov Date: Wed, 18 Apr 2018 16:08:34 -0700 Subject: [PATCH 4/7] Adding notification handlers enable notification in syncd --- orchagent/fdborch.cpp | 4 ---- orchagent/main.cpp | 18 +++++++++++++----- orchagent/notifications.cpp | 8 ++++++++ orchagent/notifications.h | 2 ++ orchagent/portsorch.cpp | 4 ---- 5 files changed, 23 insertions(+), 13 deletions(-) diff --git a/orchagent/fdborch.cpp b/orchagent/fdborch.cpp index e85942da2d..75a9b4ae21 100644 --- a/orchagent/fdborch.cpp +++ b/orchagent/fdborch.cpp @@ -355,10 +355,6 @@ void FdbOrch::doTask(NotificationConsumer& consumer, const std::string &name) sai_deserialize_free_fdb_event_ntf(count, fdbevent); } } - else - { - SWSS_LOG_ERROR("Wrong NotificationConsumer name: '%s'", name.c_str()); - } } void FdbOrch::updateVlanMember(const VlanMemberUpdate& update) diff --git a/orchagent/main.cpp b/orchagent/main.cpp index 68ea70cc06..f88ad3589b 100644 --- a/orchagent/main.cpp +++ b/orchagent/main.cpp @@ -149,6 +149,17 @@ int main(int argc, char **argv) initSaiApi(); initSaiRedis(record_location); + sai_attribute_t attr; + vector attrs; + + attr.id = SAI_SWITCH_ATTR_INIT_SWITCH; + attr.value.booldata = true; + attrs.push_back(attr); + + attr.id = SAI_SWITCH_ATTR_FDB_EVENT_NOTIFY; + attr.value.ptr = (void *)on_fdb_event; + attrs.push_back(attr); + /* Disable/enable SwSS recording */ if (gSwssRecord) { @@ -162,11 +173,8 @@ int main(int argc, char **argv) gRecordOfs << getTimestamp() << "|recording started" << endl; } - sai_attribute_t attr; - vector attrs; - - attr.id = SAI_SWITCH_ATTR_INIT_SWITCH; - attr.value.booldata = true; + attr.id = SAI_SWITCH_ATTR_PORT_STATE_CHANGE_NOTIFY; + attr.value.ptr = (void *)on_port_state_change; attrs.push_back(attr); attr.id = SAI_SWITCH_ATTR_SHUTDOWN_REQUEST_NOTIFY; diff --git a/orchagent/notifications.cpp b/orchagent/notifications.cpp index 8b444376a9..62b90e6d7f 100644 --- a/orchagent/notifications.cpp +++ b/orchagent/notifications.cpp @@ -5,6 +5,14 @@ extern "C" { #include "logger.h" #include "notifications.h" +void on_fdb_event(uint32_t count, sai_fdb_event_notification_data_t *data) +{ +} + +void on_port_state_change(uint32_t count, sai_port_oper_status_notification_t *data) +{ +} + void on_switch_shutdown_request() { SWSS_LOG_ENTER(); diff --git a/orchagent/notifications.h b/orchagent/notifications.h index 2aff91616c..961e2eae9f 100644 --- a/orchagent/notifications.h +++ b/orchagent/notifications.h @@ -2,4 +2,6 @@ extern "C" { #include "sai.h" } +void on_fdb_event(uint32_t count, sai_fdb_event_notification_data_t *data); +void on_port_state_change(uint32_t count, sai_port_oper_status_notification_t *data); void on_switch_shutdown_request(); diff --git a/orchagent/portsorch.cpp b/orchagent/portsorch.cpp index ceca970e9a..7ed9453fdf 100644 --- a/orchagent/portsorch.cpp +++ b/orchagent/portsorch.cpp @@ -2380,8 +2380,4 @@ void PortsOrch::doTask(NotificationConsumer &consumer, const std::string &name) sai_deserialize_free_port_oper_status_ntf(count, portoperstatus); } - else - { - SWSS_LOG_ERROR("Wrong notification name in notification provider '%s': '%s'", name.c_str(), op.c_str()); - } } From a8408c3e0ba06abd7d86c8977c3dc6bd46ed0e3e Mon Sep 17 00:00:00 2001 From: Pavel Shirshov Date: Wed, 18 Apr 2018 16:24:27 -0700 Subject: [PATCH 5/7] Add extra comments --- orchagent/fdborch.cpp | 1 + orchagent/notifications.cpp | 4 ++++ 2 files changed, 5 insertions(+) diff --git a/orchagent/fdborch.cpp b/orchagent/fdborch.cpp index 75a9b4ae21..f412973e42 100644 --- a/orchagent/fdborch.cpp +++ b/orchagent/fdborch.cpp @@ -29,6 +29,7 @@ FdbOrch::FdbOrch(DBConnector *db, string tableName, PortsOrch *port) : auto fdbNotification = new Notifier(consumer, this, "FLUSHFDBREQUEST"); Orch::addExecutor("", fdbNotification); + /* Add FDB notification support from ASIC */ DBConnector *notification_db = new DBConnector(ASIC_DB, DBConnector::DEFAULT_UNIXSOCKET, 0); auto notification_consumer = new swss::NotificationConsumer(notification_db, "NOTIFICATIONS"); auto fdb_notification = new Notifier(notification_consumer, this, "NOTIFICATIONS"); diff --git a/orchagent/notifications.cpp b/orchagent/notifications.cpp index 62b90e6d7f..209c03d83b 100644 --- a/orchagent/notifications.cpp +++ b/orchagent/notifications.cpp @@ -7,10 +7,14 @@ extern "C" { void on_fdb_event(uint32_t count, sai_fdb_event_notification_data_t *data) { + // don't use this event handler, because it runs by libsairedis in a separate thread + // which causes concurrency access to the DB } void on_port_state_change(uint32_t count, sai_port_oper_status_notification_t *data) { + // don't use this event handler, because it runs by libsairedis in a separate thread + // which causes concurrency access to the DB } void on_switch_shutdown_request() From e337326bc13cfb8e1df287afebf2063e628cbf0a Mon Sep 17 00:00:00 2001 From: Pavel Shirshov Date: Thu, 19 Apr 2018 12:40:06 -0700 Subject: [PATCH 6/7] Address PR comments --- orchagent/fdborch.cpp | 24 ++++++++++++------------ orchagent/fdborch.h | 2 +- orchagent/notifier.h | 8 ++++---- orchagent/orch.h | 4 +++- orchagent/pfcwdorch.cpp | 2 +- orchagent/pfcwdorch.h | 2 +- orchagent/portsorch.cpp | 6 +++--- orchagent/portsorch.h | 2 +- 8 files changed, 26 insertions(+), 24 deletions(-) diff --git a/orchagent/fdborch.cpp b/orchagent/fdborch.cpp index f412973e42..ed060c754b 100644 --- a/orchagent/fdborch.cpp +++ b/orchagent/fdborch.cpp @@ -25,15 +25,15 @@ FdbOrch::FdbOrch(DBConnector *db, string tableName, PortsOrch *port) : m_table(Table(db, tableName)) { m_portsOrch->attach(this); - auto consumer = new NotificationConsumer(db, "FLUSHFDBREQUEST"); - auto fdbNotification = new Notifier(consumer, this, "FLUSHFDBREQUEST"); - Orch::addExecutor("", fdbNotification); - - /* Add FDB notification support from ASIC */ - DBConnector *notification_db = new DBConnector(ASIC_DB, DBConnector::DEFAULT_UNIXSOCKET, 0); - auto notification_consumer = new swss::NotificationConsumer(notification_db, "NOTIFICATIONS"); - auto fdb_notification = new Notifier(notification_consumer, this, "NOTIFICATIONS"); - Orch::addExecutor("FDB_NOTIFICATIONS", fdb_notification); + auto flushNotificationsConsumer = new NotificationConsumer(db, "FLUSHFDBREQUEST"); + auto flushNotifier = new Notifier(flushNotificationsConsumer, this, "FLUSHFDBREQUEST"); + Orch::addExecutor("", flushNotifier); + + /* Add FDB notifications support from ASIC */ + DBConnector *notificationsDb = new DBConnector(ASIC_DB, DBConnector::DEFAULT_UNIXSOCKET, 0); + auto fdbNotificationConsumer = new swss::NotificationConsumer(notificationsDb, "NOTIFICATIONS"); + auto fdbNotifier = new Notifier(fdbNotificationConsumer, this, "NOTIFICATIONS"); + Orch::addExecutor("FDB_NOTIFICATIONS", fdbNotifier); } void FdbOrch::update(sai_fdb_event_t type, const sai_fdb_entry_t* entry, sai_object_id_t bridge_port_id) @@ -281,7 +281,7 @@ void FdbOrch::doTask(Consumer& consumer) } } -void FdbOrch::doTask(NotificationConsumer& consumer, const std::string &name) +void FdbOrch::doTask(NotificationConsumer& consumer, const std::string &consumer_name) { SWSS_LOG_ENTER(); @@ -297,7 +297,7 @@ void FdbOrch::doTask(NotificationConsumer& consumer, const std::string &name) consumer.pop(op, data, values); - if (name == "FLUSHFDBREQUEST") + if (consumer_name == "FLUSHFDBREQUEST") { if (op == "ALL") { @@ -331,7 +331,7 @@ void FdbOrch::doTask(NotificationConsumer& consumer, const std::string &name) return; } } - else if (name == "NOTIFICATIONS" && op == "fdb_event") + else if (consumer_name == "NOTIFICATIONS" && op == "fdb_event") { uint32_t count; sai_fdb_event_notification_data_t *fdbevent = nullptr; diff --git a/orchagent/fdborch.h b/orchagent/fdborch.h index 1e538014ad..13bcab1bc9 100644 --- a/orchagent/fdborch.h +++ b/orchagent/fdborch.h @@ -53,7 +53,7 @@ class FdbOrch: public Orch, public Subject, public Observer Table m_table; void doTask(Consumer& consumer); - void doTask(NotificationConsumer& consumer, const std::string &name); + void doTask(NotificationConsumer& consumer, const std::string &consumer_name); void updateVlanMember(const VlanMemberUpdate&); bool addFdbEntry(const FdbEntry&, const string&, const string&); diff --git a/orchagent/notifier.h b/orchagent/notifier.h index 9b329253f8..90579e23e7 100644 --- a/orchagent/notifier.h +++ b/orchagent/notifier.h @@ -2,8 +2,8 @@ class Notifier : public Executor { public: - Notifier(NotificationConsumer *select, Orch *orch, const std::string& notifier_name) - : Executor(select, orch), m_name(notifier_name) + Notifier(NotificationConsumer *select, Orch *orch, const std::string& notifier_channel) + : Executor(select, orch), m_channel(notifier_channel) { } @@ -14,9 +14,9 @@ class Notifier : public Executor { void execute() { - m_orch->doTask(*getNotificationConsumer(), m_name); + m_orch->doTask(*getNotificationConsumer(), m_channel); } private: - std::string m_name; + std::string m_channel; }; diff --git a/orchagent/orch.h b/orchagent/orch.h index 64a8a7df12..f52e7157b8 100644 --- a/orchagent/orch.h +++ b/orchagent/orch.h @@ -152,7 +152,9 @@ class Orch /* Run doTask against a specific executor */ virtual void doTask(Consumer &consumer) = 0; - virtual void doTask(NotificationConsumer &consumer, const std::string &name) { } + virtual void doTask(NotificationConsumer &consumer, const std::string &consumer_name) { } // consumer_name allows to distinct + // NotificationConsumer which causes + // this doTask executions virtual void doTask(SelectableTimer &timer) { } /* TODO: refactor recording */ diff --git a/orchagent/pfcwdorch.cpp b/orchagent/pfcwdorch.cpp index 0efdb364b2..02bedc2781 100644 --- a/orchagent/pfcwdorch.cpp +++ b/orchagent/pfcwdorch.cpp @@ -712,7 +712,7 @@ bool PfcWdSwOrch::stopWdOnPort(const Port& port) } template -void PfcWdSwOrch::doTask(swss::NotificationConsumer& wdNotification, const std::string& name) +void PfcWdSwOrch::doTask(swss::NotificationConsumer& wdNotification, const std::string& consumer_name) { SWSS_LOG_ENTER(); diff --git a/orchagent/pfcwdorch.h b/orchagent/pfcwdorch.h index e329ddf127..37e30b9d2e 100644 --- a/orchagent/pfcwdorch.h +++ b/orchagent/pfcwdorch.h @@ -93,7 +93,7 @@ class PfcWdSwOrch: public PfcWdOrch void registerInWdDb(const Port& port, uint32_t detectionTime, uint32_t restorationTime, PfcWdAction action); void unregisterFromWdDb(const Port& port); - void doTask(swss::NotificationConsumer &wdNotification, const std::string &name); + void doTask(swss::NotificationConsumer &wdNotification, const std::string &consumer_name); string filterPfcCounters(string counters, set& losslessTc); string getFlexCounterTableKey(string s); diff --git a/orchagent/portsorch.cpp b/orchagent/portsorch.cpp index 7ed9453fdf..b5cc426792 100644 --- a/orchagent/portsorch.cpp +++ b/orchagent/portsorch.cpp @@ -2338,7 +2338,7 @@ bool PortsOrch::removeLagMember(Port &lag, Port &port) return true; } -void PortsOrch::doTask(NotificationConsumer &consumer, const std::string &name) +void PortsOrch::doTask(NotificationConsumer &consumer, const std::string &consumer_name) { SWSS_LOG_ENTER(); @@ -2354,9 +2354,9 @@ void PortsOrch::doTask(NotificationConsumer &consumer, const std::string &name) consumer.pop(op, data, values); - if (name != "NOTIFICATIONS") + if (consumer_name != "NOTIFICATIONS") { - SWSS_LOG_ERROR("Wrong name of notification provider: '%s'", name.c_str()); + SWSS_LOG_ERROR("Wrong name of notification provider: '%s'", consumer_name.c_str()); return; } diff --git a/orchagent/portsorch.h b/orchagent/portsorch.h index caac8f7a4b..e7a3a30dd6 100644 --- a/orchagent/portsorch.h +++ b/orchagent/portsorch.h @@ -92,7 +92,7 @@ class PortsOrch : public Orch, public Subject void doLagTask(Consumer &consumer); void doLagMemberTask(Consumer &consumer); - void doTask(NotificationConsumer &consumer, const std::string &name); + void doTask(NotificationConsumer &consumer, const std::string &consumer_name); void removeDefaultVlanMembers(); void removeDefaultBridgePorts(); From b6f857b30ff242938fb0129cd812f802e3f0da38 Mon Sep 17 00:00:00 2001 From: Pavel Shirshov Date: Thu, 19 Apr 2018 14:14:53 -0700 Subject: [PATCH 7/7] Address comments --- .gitignore | 2 +- orchagent/fdborch.cpp | 20 ++++++++++---------- orchagent/fdborch.h | 4 +++- orchagent/notifier.h | 9 +++------ orchagent/orch.h | 4 +--- orchagent/pfcwdorch.cpp | 4 ++-- orchagent/pfcwdorch.h | 2 +- orchagent/portsorch.cpp | 13 ++++++------- orchagent/portsorch.h | 4 +++- 9 files changed, 30 insertions(+), 32 deletions(-) diff --git a/.gitignore b/.gitignore index 978e1f926d..34034cfe0c 100644 --- a/.gitignore +++ b/.gitignore @@ -47,6 +47,7 @@ fpmsyncd/fpmsyncd intfsyncd/intfsyncd cfgmgr/intfmgrd cfgmgr/vlanmgrd +cfgmgr/buffermanager neighsyncd/neighsyncd portsyncd/portsyncd orchagent/orchagent @@ -54,4 +55,3 @@ orchagent/routeresync swssconfig/swssconfig swssconfig/swssplayer tests/tests - diff --git a/orchagent/fdborch.cpp b/orchagent/fdborch.cpp index ed060c754b..cffdbdb548 100644 --- a/orchagent/fdborch.cpp +++ b/orchagent/fdborch.cpp @@ -25,14 +25,14 @@ FdbOrch::FdbOrch(DBConnector *db, string tableName, PortsOrch *port) : m_table(Table(db, tableName)) { m_portsOrch->attach(this); - auto flushNotificationsConsumer = new NotificationConsumer(db, "FLUSHFDBREQUEST"); - auto flushNotifier = new Notifier(flushNotificationsConsumer, this, "FLUSHFDBREQUEST"); + m_flushNotificationsConsumer = new NotificationConsumer(db, "FLUSHFDBREQUEST"); + auto flushNotifier = new Notifier(m_flushNotificationsConsumer, this); Orch::addExecutor("", flushNotifier); /* Add FDB notifications support from ASIC */ DBConnector *notificationsDb = new DBConnector(ASIC_DB, DBConnector::DEFAULT_UNIXSOCKET, 0); - auto fdbNotificationConsumer = new swss::NotificationConsumer(notificationsDb, "NOTIFICATIONS"); - auto fdbNotifier = new Notifier(fdbNotificationConsumer, this, "NOTIFICATIONS"); + m_fdbNotificationConsumer = new swss::NotificationConsumer(notificationsDb, "NOTIFICATIONS"); + auto fdbNotifier = new Notifier(m_fdbNotificationConsumer, this); Orch::addExecutor("FDB_NOTIFICATIONS", fdbNotifier); } @@ -281,7 +281,7 @@ void FdbOrch::doTask(Consumer& consumer) } } -void FdbOrch::doTask(NotificationConsumer& consumer, const std::string &consumer_name) +void FdbOrch::doTask(NotificationConsumer& consumer) { SWSS_LOG_ENTER(); @@ -297,7 +297,7 @@ void FdbOrch::doTask(NotificationConsumer& consumer, const std::string &consumer consumer.pop(op, data, values); - if (consumer_name == "FLUSHFDBREQUEST") + if (&consumer == m_flushNotificationsConsumer) { if (op == "ALL") { @@ -317,21 +317,21 @@ void FdbOrch::doTask(NotificationConsumer& consumer, const std::string &consumer { /*place holder for flush port fdb*/ SWSS_LOG_ERROR("Received unsupported flush port fdb request"); - return; + return; } else if (op == "VLAN") { /*place holder for flush vlan fdb*/ SWSS_LOG_ERROR("Received unsupported flush vlan fdb request"); - return; + return; } else { SWSS_LOG_ERROR("Received unknown flush fdb request"); - return; + return; } } - else if (consumer_name == "NOTIFICATIONS" && op == "fdb_event") + else if (&consumer == m_fdbNotificationConsumer && op == "fdb_event") { uint32_t count; sai_fdb_event_notification_data_t *fdbevent = nullptr; diff --git a/orchagent/fdborch.h b/orchagent/fdborch.h index 13bcab1bc9..4fd6b0d01e 100644 --- a/orchagent/fdborch.h +++ b/orchagent/fdborch.h @@ -51,9 +51,11 @@ class FdbOrch: public Orch, public Subject, public Observer set m_entries; fdb_entries_by_port_t saved_fdb_entries; Table m_table; + NotificationConsumer* m_flushNotificationsConsumer; + NotificationConsumer* m_fdbNotificationConsumer; void doTask(Consumer& consumer); - void doTask(NotificationConsumer& consumer, const std::string &consumer_name); + void doTask(NotificationConsumer& consumer); void updateVlanMember(const VlanMemberUpdate&); bool addFdbEntry(const FdbEntry&, const string&, const string&); diff --git a/orchagent/notifier.h b/orchagent/notifier.h index 90579e23e7..6113c80527 100644 --- a/orchagent/notifier.h +++ b/orchagent/notifier.h @@ -2,8 +2,8 @@ class Notifier : public Executor { public: - Notifier(NotificationConsumer *select, Orch *orch, const std::string& notifier_channel) - : Executor(select, orch), m_channel(notifier_channel) + Notifier(NotificationConsumer *select, Orch *orch) + : Executor(select, orch) { } @@ -14,9 +14,6 @@ class Notifier : public Executor { void execute() { - m_orch->doTask(*getNotificationConsumer(), m_channel); + m_orch->doTask(*getNotificationConsumer()); } - -private: - std::string m_channel; }; diff --git a/orchagent/orch.h b/orchagent/orch.h index f52e7157b8..eddece4081 100644 --- a/orchagent/orch.h +++ b/orchagent/orch.h @@ -152,9 +152,7 @@ class Orch /* Run doTask against a specific executor */ virtual void doTask(Consumer &consumer) = 0; - virtual void doTask(NotificationConsumer &consumer, const std::string &consumer_name) { } // consumer_name allows to distinct - // NotificationConsumer which causes - // this doTask executions + virtual void doTask(NotificationConsumer &consumer) { } virtual void doTask(SelectableTimer &timer) { } /* TODO: refactor recording */ diff --git a/orchagent/pfcwdorch.cpp b/orchagent/pfcwdorch.cpp index 02bedc2781..8e5f3b7152 100644 --- a/orchagent/pfcwdorch.cpp +++ b/orchagent/pfcwdorch.cpp @@ -668,7 +668,7 @@ PfcWdSwOrch::PfcWdSwOrch( auto consumer = new swss::NotificationConsumer( PfcWdSwOrch::getCountersDb().get(), "PFC_WD"); - auto wdNotification = new Notifier(consumer, this, "PFC_WD"); + auto wdNotification = new Notifier(consumer, this); Orch::addExecutor("PFC_WD", wdNotification); } @@ -712,7 +712,7 @@ bool PfcWdSwOrch::stopWdOnPort(const Port& port) } template -void PfcWdSwOrch::doTask(swss::NotificationConsumer& wdNotification, const std::string& consumer_name) +void PfcWdSwOrch::doTask(swss::NotificationConsumer& wdNotification) { SWSS_LOG_ENTER(); diff --git a/orchagent/pfcwdorch.h b/orchagent/pfcwdorch.h index 37e30b9d2e..74b709e46e 100644 --- a/orchagent/pfcwdorch.h +++ b/orchagent/pfcwdorch.h @@ -93,7 +93,7 @@ class PfcWdSwOrch: public PfcWdOrch void registerInWdDb(const Port& port, uint32_t detectionTime, uint32_t restorationTime, PfcWdAction action); void unregisterFromWdDb(const Port& port); - void doTask(swss::NotificationConsumer &wdNotification, const std::string &consumer_name); + void doTask(swss::NotificationConsumer &wdNotification); string filterPfcCounters(string counters, set& losslessTc); string getFlexCounterTableKey(string s); diff --git a/orchagent/portsorch.cpp b/orchagent/portsorch.cpp index b5cc426792..a169281e8f 100644 --- a/orchagent/portsorch.cpp +++ b/orchagent/portsorch.cpp @@ -238,10 +238,10 @@ PortsOrch::PortsOrch(DBConnector *db, vector &tableNames) removeDefaultBridgePorts(); /* Add port oper status notification support */ - DBConnector *notification_db = new DBConnector(ASIC_DB, DBConnector::DEFAULT_UNIXSOCKET, 0); - auto notification_consumer = new swss::NotificationConsumer(notification_db, "NOTIFICATIONS"); - auto port_status_notification = new Notifier(notification_consumer, this, "NOTIFICATIONS"); - Orch::addExecutor("PORT_STATUS_NOTIFICATIONS", port_status_notification); + DBConnector *notificationsDb = new DBConnector(ASIC_DB, DBConnector::DEFAULT_UNIXSOCKET, 0); + m_portStatusNotificationConsumer = new swss::NotificationConsumer(notificationsDb, "NOTIFICATIONS"); + auto portStatusNotificatier = new Notifier(m_portStatusNotificationConsumer, this); + Orch::addExecutor("PORT_STATUS_NOTIFICATIONS", portStatusNotificatier); } void PortsOrch::removeDefaultVlanMembers() @@ -2338,7 +2338,7 @@ bool PortsOrch::removeLagMember(Port &lag, Port &port) return true; } -void PortsOrch::doTask(NotificationConsumer &consumer, const std::string &consumer_name) +void PortsOrch::doTask(NotificationConsumer &consumer) { SWSS_LOG_ENTER(); @@ -2354,9 +2354,8 @@ void PortsOrch::doTask(NotificationConsumer &consumer, const std::string &consum consumer.pop(op, data, values); - if (consumer_name != "NOTIFICATIONS") + if (&consumer != m_portStatusNotificationConsumer) { - SWSS_LOG_ERROR("Wrong name of notification provider: '%s'", consumer_name.c_str()); return; } diff --git a/orchagent/portsorch.h b/orchagent/portsorch.h index e7a3a30dd6..f2bc8e41ae 100644 --- a/orchagent/portsorch.h +++ b/orchagent/portsorch.h @@ -85,6 +85,8 @@ class PortsOrch : public Orch, public Subject map, tuple> m_lanesAliasSpeedMap; map m_portList; + NotificationConsumer* m_portStatusNotificationConsumer; + void doTask(Consumer &consumer); void doPortTask(Consumer &consumer); void doVlanTask(Consumer &consumer); @@ -92,7 +94,7 @@ class PortsOrch : public Orch, public Subject void doLagTask(Consumer &consumer); void doLagMemberTask(Consumer &consumer); - void doTask(NotificationConsumer &consumer, const std::string &consumer_name); + void doTask(NotificationConsumer &consumer); void removeDefaultVlanMembers(); void removeDefaultBridgePorts();