From a4197a7e2b48539295c68c9cb954cc033c43c6c4 Mon Sep 17 00:00:00 2001 From: Saikrishna Arcot Date: Mon, 9 Dec 2024 17:55:23 -0800 Subject: [PATCH 1/3] Fix codeql following Bookworm upgrade (#3416) What I did Fix the codeql checker following the Bookworm and libnl 3.7.0 upgrade. --- .github/workflows/codeql-analysis.yml | 19 ++++++++++--------- 1 file changed, 10 insertions(+), 9 deletions(-) diff --git a/.github/workflows/codeql-analysis.yml b/.github/workflows/codeql-analysis.yml index 24957fede5..2bdb3e9933 100644 --- a/.github/workflows/codeql-analysis.yml +++ b/.github/workflows/codeql-analysis.yml @@ -127,20 +127,21 @@ jobs: git config --local --unset user.name git config --local --unset user.email ln -s ../debian debian - dpkg-buildpackage -rfakeroot -us -uc -b -j$(nproc) + DPKG_GENSYMBOLS_CHECK_LEVEL=0 dpkg-buildpackage -rfakeroot -us -uc -b -j$(nproc) popd - dpkg-deb -x libnl-3-200_${LIBNL3_VER}-${LIBNL3_REV}_amd64.deb $(dirname $GITHUB_WORKSPACE) - dpkg-deb -x libnl-3-dev_${LIBNL3_VER}-${LIBNL3_REV}_amd64.deb $(dirname $GITHUB_WORKSPACE) - dpkg-deb -x libnl-genl-3-200_${LIBNL3_VER}-${LIBNL3_REV}_amd64.deb $(dirname $GITHUB_WORKSPACE) - dpkg-deb -x libnl-genl-3-dev_${LIBNL3_VER}-${LIBNL3_REV}_amd64.deb $(dirname $GITHUB_WORKSPACE) - dpkg-deb -x libnl-route-3-200_${LIBNL3_VER}-${LIBNL3_REV}_amd64.deb $(dirname $GITHUB_WORKSPACE) - dpkg-deb -x libnl-route-3-dev_${LIBNL3_VER}-${LIBNL3_REV}_amd64.deb $(dirname $GITHUB_WORKSPACE) - dpkg-deb -x libnl-nf-3-200_${LIBNL3_VER}-${LIBNL3_REV}_amd64.deb $(dirname $GITHUB_WORKSPACE) - dpkg-deb -x libnl-nf-3-dev_${LIBNL3_VER}-${LIBNL3_REV}_amd64.deb $(dirname $GITHUB_WORKSPACE) + dpkg-deb -x libnl-3-200_${LIBNL3_VER}-${LIBNL3_REV_SONIC}_amd64.deb $(dirname $GITHUB_WORKSPACE) + dpkg-deb -x libnl-3-dev_${LIBNL3_VER}-${LIBNL3_REV_SONIC}_amd64.deb $(dirname $GITHUB_WORKSPACE) + dpkg-deb -x libnl-genl-3-200_${LIBNL3_VER}-${LIBNL3_REV_SONIC}_amd64.deb $(dirname $GITHUB_WORKSPACE) + dpkg-deb -x libnl-genl-3-dev_${LIBNL3_VER}-${LIBNL3_REV_SONIC}_amd64.deb $(dirname $GITHUB_WORKSPACE) + dpkg-deb -x libnl-route-3-200_${LIBNL3_VER}-${LIBNL3_REV_SONIC}_amd64.deb $(dirname $GITHUB_WORKSPACE) + dpkg-deb -x libnl-route-3-dev_${LIBNL3_VER}-${LIBNL3_REV_SONIC}_amd64.deb $(dirname $GITHUB_WORKSPACE) + dpkg-deb -x libnl-nf-3-200_${LIBNL3_VER}-${LIBNL3_REV_SONIC}_amd64.deb $(dirname $GITHUB_WORKSPACE) + dpkg-deb -x libnl-nf-3-dev_${LIBNL3_VER}-${LIBNL3_REV_SONIC}_amd64.deb $(dirname $GITHUB_WORKSPACE) popd env: LIBNL3_VER: "3.7.0" LIBNL3_REV: "0.2" + LIBNL3_REV_SONIC: "0.2+b1sonic1" - if: matrix.language == 'cpp' name: Build repository From 90612d2969ad21eecf43e15ef589f81f02d5f14e Mon Sep 17 00:00:00 2001 From: Sudharsan Dhamal Gopalarathnam Date: Tue, 10 Dec 2024 09:48:45 -0800 Subject: [PATCH 2/3] Fixing fpmsyncd to handle scaled nexthops (#3361) What I did Fixing fpmsyncd to handle scaled nexthops(e.g. 256 nexthops). In this scenario the max message len is increased to accommodate zebra message size. In addition initialized nlmsg_set_default_size to allow netlink to process the increased message size as well without which libnl would crash. Why I did it To support increased nexthops. --- fpmsyncd/fpm/fpm.h | 2 +- fpmsyncd/fpmsyncd.cpp | 1 + fpmsyncd/routesync.cpp | 13 +++++++- tests/mock_tests/fake_netlink.cpp | 33 ++++++++++++++++++++ tests/mock_tests/fpmsyncd/test_routesync.cpp | 17 ++++++++++ 5 files changed, 64 insertions(+), 2 deletions(-) diff --git a/fpmsyncd/fpm/fpm.h b/fpmsyncd/fpm/fpm.h index 8af9b30ae9..11f13b168a 100644 --- a/fpmsyncd/fpm/fpm.h +++ b/fpmsyncd/fpm/fpm.h @@ -92,7 +92,7 @@ /* * Largest message that can be sent to or received from the FPM. */ -#define FPM_MAX_MSG_LEN 4096 +#define FPM_MAX_MSG_LEN 16384 /* * Header that precedes each fpm message to/from the FPM. diff --git a/fpmsyncd/fpmsyncd.cpp b/fpmsyncd/fpmsyncd.cpp index c3479efc84..f01bebe203 100644 --- a/fpmsyncd/fpmsyncd.cpp +++ b/fpmsyncd/fpmsyncd.cpp @@ -98,6 +98,7 @@ int main(int argc, char **argv) NetDispatcher::getInstance().registerMessageHandler(RTM_DELLINK, &sync); rtnl_route_read_protocol_names(DefaultRtProtoPath); + nlmsg_set_default_size(FPM_MAX_MSG_LEN); std::string suppressionEnabledStr; deviceMetadataTable.hget("localhost", "suppress-fib-pending", suppressionEnabledStr); diff --git a/fpmsyncd/routesync.cpp b/fpmsyncd/routesync.cpp index 4d6aa708ab..cea63dc42f 100644 --- a/fpmsyncd/routesync.cpp +++ b/fpmsyncd/routesync.cpp @@ -2235,12 +2235,23 @@ bool RouteSync::sendOffloadReply(struct nlmsghdr* hdr) bool RouteSync::sendOffloadReply(struct rtnl_route* route_obj) { SWSS_LOG_ENTER(); + int ret = 0; nl_msg* msg{}; - rtnl_route_build_add_request(route_obj, NLM_F_CREATE, &msg); + ret = rtnl_route_build_add_request(route_obj, NLM_F_CREATE, &msg); + if (ret !=0) + { + SWSS_LOG_ERROR("Route build add returned %d", ret); + return false; + } auto nlMsg = makeUniqueWithDestructor(msg, nlmsg_free); + if (nlMsg.get() == NULL) + { + SWSS_LOG_ERROR("Error in allocation for sending offload reply"); + return false; + } return sendOffloadReply(nlmsg_hdr(nlMsg.get())); } diff --git a/tests/mock_tests/fake_netlink.cpp b/tests/mock_tests/fake_netlink.cpp index 2370e13129..54eb197493 100644 --- a/tests/mock_tests/fake_netlink.cpp +++ b/tests/mock_tests/fake_netlink.cpp @@ -1,5 +1,6 @@ #include #include +#include static rtnl_link* g_fakeLink = [](){ auto fakeLink = rtnl_link_alloc(); @@ -7,6 +8,8 @@ static rtnl_link* g_fakeLink = [](){ return fakeLink; }(); +extern int rt_build_ret; +extern bool nlmsg_alloc_ret; extern "C" { @@ -15,4 +18,34 @@ struct rtnl_link* rtnl_link_get_by_name(struct nl_cache *cache, const char *name return g_fakeLink; } +static int build_route_msg(struct rtnl_route *tmpl, int cmd, int flags, + struct nl_msg **result) +{ + struct nl_msg *msg; + int err; + if (!(msg = nlmsg_alloc_simple(cmd, flags))) + return -NLE_NOMEM; + if ((err = rtnl_route_build_msg(msg, tmpl)) < 0) { + nlmsg_free(msg); + return err; + } + *result = msg; + return 0; +} + +int rtnl_route_build_add_request(struct rtnl_route *tmpl, int flags, + struct nl_msg **result) +{ + if (rt_build_ret != 0) + { + return rt_build_ret; + } + else if (!nlmsg_alloc_ret) + { + *result = NULL; + return 0; + } + return build_route_msg(tmpl, RTM_NEWROUTE, NLM_F_CREATE | flags, + result); +} } diff --git a/tests/mock_tests/fpmsyncd/test_routesync.cpp b/tests/mock_tests/fpmsyncd/test_routesync.cpp index a8de78859f..b1c23aca85 100644 --- a/tests/mock_tests/fpmsyncd/test_routesync.cpp +++ b/tests/mock_tests/fpmsyncd/test_routesync.cpp @@ -12,6 +12,8 @@ using namespace swss; using ::testing::_; +int rt_build_ret = 0; +bool nlmsg_alloc_ret = true; class MockRouteSync : public RouteSync { public: @@ -232,3 +234,18 @@ TEST_F(FpmSyncdResponseTest, testEvpn) ASSERT_EQ(value.get(), "0xc8"); } + +TEST_F(FpmSyncdResponseTest, testSendOffloadReply) +{ + + rt_build_ret = 1; + rtnl_route* routeObject{}; + + + ASSERT_EQ(m_routeSync.sendOffloadReply(routeObject), false); + rt_build_ret = 0; + nlmsg_alloc_ret = false; + ASSERT_EQ(m_routeSync.sendOffloadReply(routeObject), false); + nlmsg_alloc_ret = true; + +} From fa2b5304420ea11a7191210c2689631007335d60 Mon Sep 17 00:00:00 2001 From: Stephen Sun <5379172+stephenxs@users.noreply.github.com> Date: Wed, 11 Dec 2024 02:01:31 +0800 Subject: [PATCH 3/3] Do not start counter polling for queues with zero buffer profiles if create_only_config_db_buffers is enabled (#3360) What I did Do not start counter-polling for queues with zero buffer profiles if create_only_config_db_buffers is enabled. Why I did it If create_only_config_db_buffers is enabled, counters will be polled only on queues with a buffer configured based on the hypothesis that a customer must configure a buffer profile before using the queue. The system performance can be optimized by reducing the number of counter polled. However, zero buffer profiles are configured on queues of inactive ports to reclaim unused buffers. Those queues are not used by customers. Hence, we do not need to start counter polling for queues with such buffer profiles configured. --- orchagent/bufferorch.cpp | 60 +++++++++++++++++++++++++++++------ orchagent/bufferorch.h | 1 + orchagent/flexcounterorch.cpp | 8 ++--- 3 files changed, 55 insertions(+), 14 deletions(-) diff --git a/orchagent/bufferorch.cpp b/orchagent/bufferorch.cpp index ccbc8e9b25..fb38cfe447 100644 --- a/orchagent/bufferorch.cpp +++ b/orchagent/bufferorch.cpp @@ -360,6 +360,25 @@ const object_reference_map &BufferOrch::getBufferPoolNameOidMap(void) return *m_buffer_type_maps[APP_BUFFER_POOL_TABLE_NAME]; } +void BufferOrch::getBufferObjectsWithNonZeroProfile(vector &nonZeroQueues, const string &table) +{ + for (auto &&queueRef: (*m_buffer_type_maps[table])) + { + for (auto &&profileRef: queueRef.second.m_objsReferencingByMe) + { + if (profileRef.second.find("_zero_") == std::string::npos) + { + SWSS_LOG_INFO("Selected key %s with profile %s", queueRef.first.c_str(), profileRef.second.c_str()); + nonZeroQueues.push_back(queueRef.first); + } + else + { + SWSS_LOG_INFO("Skipped key %s with profile %s", queueRef.first.c_str(), profileRef.second.c_str()); + } + } + } +} + task_process_status BufferOrch::processBufferPool(KeyOpFieldsValuesTuple &tuple) { SWSS_LOG_ENTER(); @@ -797,6 +816,9 @@ task_process_status BufferOrch::processQueue(KeyOpFieldsValuesTuple &tuple) sai_uint32_t range_low, range_high; bool need_update_sai = true; bool local_port = false; + bool counter_was_added = false; + bool counter_needs_to_add = false; + string old_buffer_profile_name; string local_port_name; SWSS_LOG_DEBUG("Processing:%s", key.c_str()); @@ -856,7 +878,6 @@ task_process_status BufferOrch::processQueue(KeyOpFieldsValuesTuple &tuple) return task_process_status::task_failed; } - string old_buffer_profile_name; if (doesObjectExist(m_buffer_type_maps, APP_BUFFER_QUEUE_TABLE_NAME, key, buffer_profile_field_name, old_buffer_profile_name) && (old_buffer_profile_name == buffer_profile_name)) { @@ -874,11 +895,14 @@ task_process_status BufferOrch::processQueue(KeyOpFieldsValuesTuple &tuple) SWSS_LOG_NOTICE("Set buffer queue %s to %s", key.c_str(), buffer_profile_name.c_str()); setObjectReference(m_buffer_type_maps, APP_BUFFER_QUEUE_TABLE_NAME, key, buffer_profile_field_name, buffer_profile_name); + + // Counter operation + counter_needs_to_add = buffer_profile_name.find("_zero_") == std::string::npos; + SWSS_LOG_INFO("%s to create counter for %s with new profile %s", counter_needs_to_add ? "Need" : "No need", key.c_str(), buffer_profile_name.c_str()); } else if (op == DEL_COMMAND) { - auto &typemap = (*m_buffer_type_maps[APP_BUFFER_QUEUE_TABLE_NAME]); - if (typemap.find(key) == typemap.end()) + if (!doesObjectExist(m_buffer_type_maps, APP_BUFFER_QUEUE_TABLE_NAME, key, buffer_profile_field_name, old_buffer_profile_name)) { SWSS_LOG_INFO("%s doesn't not exist, don't need to notfiy SAI", key.c_str()); need_update_sai = false; @@ -887,6 +911,7 @@ task_process_status BufferOrch::processQueue(KeyOpFieldsValuesTuple &tuple) SWSS_LOG_NOTICE("Remove buffer queue %s", key.c_str()); removeObject(m_buffer_type_maps, APP_BUFFER_QUEUE_TABLE_NAME, key); m_partiallyAppliedQueues.erase(key); + counter_needs_to_add = false; } else { @@ -894,6 +919,9 @@ task_process_status BufferOrch::processQueue(KeyOpFieldsValuesTuple &tuple) return task_process_status::task_invalid_entry; } + counter_was_added = !old_buffer_profile_name.empty() && old_buffer_profile_name.find("_zero_") == std::string::npos; + SWSS_LOG_INFO("%s to remove counter for %s with old profile %s", counter_was_added ? "Need" : "No need", key.c_str(), old_buffer_profile_name.c_str()); + sai_attribute_t attr; attr.id = SAI_QUEUE_ATTR_BUFFER_PROFILE_ID; attr.value.oid = sai_buffer_profile; @@ -963,14 +991,16 @@ task_process_status BufferOrch::processQueue(KeyOpFieldsValuesTuple &tuple) { auto flexCounterOrch = gDirectory.get(); auto queues = tokens[1]; - if (op == SET_COMMAND && + if (!counter_was_added && counter_needs_to_add && (flexCounterOrch->getQueueCountersState() || flexCounterOrch->getQueueWatermarkCountersState())) { + SWSS_LOG_INFO("Creating counters for %s %zd", port_name.c_str(), ind); gPortsOrch->createPortBufferQueueCounters(port, queues); } - else if (op == DEL_COMMAND && + else if (counter_was_added && !counter_needs_to_add && (flexCounterOrch->getQueueCountersState() || flexCounterOrch->getQueueWatermarkCountersState())) { + SWSS_LOG_INFO("Removing counters for %s %zd", port_name.c_str(), ind); gPortsOrch->removePortBufferQueueCounters(port, queues); } } @@ -1057,6 +1087,9 @@ task_process_status BufferOrch::processPriorityGroup(KeyOpFieldsValuesTuple &tup vector tokens; sai_uint32_t range_low, range_high; bool need_update_sai = true; + bool counter_was_added = false; + bool counter_needs_to_add = false; + string old_buffer_profile_name; SWSS_LOG_DEBUG("processing:%s", key.c_str()); tokens = tokenize(key, delimiter); @@ -1089,7 +1122,6 @@ task_process_status BufferOrch::processPriorityGroup(KeyOpFieldsValuesTuple &tup return task_process_status::task_failed; } - string old_buffer_profile_name; if (doesObjectExist(m_buffer_type_maps, APP_BUFFER_PG_TABLE_NAME, key, buffer_profile_field_name, old_buffer_profile_name) && (old_buffer_profile_name == buffer_profile_name)) { @@ -1100,11 +1132,14 @@ task_process_status BufferOrch::processPriorityGroup(KeyOpFieldsValuesTuple &tup SWSS_LOG_NOTICE("Set buffer PG %s to %s", key.c_str(), buffer_profile_name.c_str()); setObjectReference(m_buffer_type_maps, APP_BUFFER_PG_TABLE_NAME, key, buffer_profile_field_name, buffer_profile_name); + + // Counter operation + counter_needs_to_add = buffer_profile_name.find("_zero_") == std::string::npos; + SWSS_LOG_INFO("%s to create counter for priority group %s with new profile %s", counter_needs_to_add ? "Need" : "No need", key.c_str(), buffer_profile_name.c_str()); } else if (op == DEL_COMMAND) { - auto &typemap = (*m_buffer_type_maps[APP_BUFFER_PG_TABLE_NAME]); - if (typemap.find(key) == typemap.end()) + if (!doesObjectExist(m_buffer_type_maps, APP_BUFFER_PG_TABLE_NAME, key, buffer_profile_field_name, old_buffer_profile_name)) { SWSS_LOG_INFO("%s doesn't not exist, don't need to notfiy SAI", key.c_str()); need_update_sai = false; @@ -1119,6 +1154,9 @@ task_process_status BufferOrch::processPriorityGroup(KeyOpFieldsValuesTuple &tup return task_process_status::task_invalid_entry; } + counter_was_added = !old_buffer_profile_name.empty() && old_buffer_profile_name.find("_zero_") == std::string::npos; + SWSS_LOG_INFO("%s to remove counter for priority group %s with old profile %s", counter_was_added ? "Need" : "No need", key.c_str(), old_buffer_profile_name.c_str()); + sai_attribute_t attr; attr.id = SAI_INGRESS_PRIORITY_GROUP_ATTR_BUFFER_PROFILE; attr.value.oid = sai_buffer_profile; @@ -1161,14 +1199,16 @@ task_process_status BufferOrch::processPriorityGroup(KeyOpFieldsValuesTuple &tup { auto flexCounterOrch = gDirectory.get(); auto pgs = tokens[1]; - if (op == SET_COMMAND && + if (!counter_was_added && counter_needs_to_add && (flexCounterOrch->getPgCountersState() || flexCounterOrch->getPgWatermarkCountersState())) { + SWSS_LOG_INFO("Creating counters for priority group %s %zd", port_name.c_str(), ind); gPortsOrch->createPortBufferPgCounters(port, pgs); } - else if (op == DEL_COMMAND && + else if (counter_was_added && !counter_needs_to_add && (flexCounterOrch->getPgCountersState() || flexCounterOrch->getPgWatermarkCountersState())) { + SWSS_LOG_INFO("Removing counters for priority group %s %zd", port_name.c_str(), ind); gPortsOrch->removePortBufferPgCounters(port, pgs); } } diff --git a/orchagent/bufferorch.h b/orchagent/bufferorch.h index f78fe05318..3d255b87dd 100644 --- a/orchagent/bufferorch.h +++ b/orchagent/bufferorch.h @@ -38,6 +38,7 @@ class BufferOrch : public Orch static type_map m_buffer_type_maps; void generateBufferPoolWatermarkCounterIdList(void); const object_reference_map &getBufferPoolNameOidMap(void); + void getBufferObjectsWithNonZeroProfile(vector &nonZeroQueues, const string &table); private: typedef task_process_status (BufferOrch::*buffer_table_handler)(KeyOpFieldsValuesTuple &tuple); diff --git a/orchagent/flexcounterorch.cpp b/orchagent/flexcounterorch.cpp index b8918d0ac9..2832f0bd12 100644 --- a/orchagent/flexcounterorch.cpp +++ b/orchagent/flexcounterorch.cpp @@ -370,11 +370,11 @@ map FlexCounterOrch::getQueueConfigurations() } std::vector portQueueKeys; - m_bufferQueueConfigTable.getKeys(portQueueKeys); + gBufferOrch->getBufferObjectsWithNonZeroProfile(portQueueKeys, APP_BUFFER_QUEUE_TABLE_NAME); for (const auto& portQueueKey : portQueueKeys) { - auto toks = tokenize(portQueueKey, '|'); + auto toks = tokenize(portQueueKey, ':'); if (toks.size() != 2) { SWSS_LOG_ERROR("Invalid BUFFER_QUEUE key: [%s]", portQueueKey.c_str()); @@ -439,11 +439,11 @@ map FlexCounterOrch::getPgConfigurations() } std::vector portPgKeys; - m_bufferPgConfigTable.getKeys(portPgKeys); + gBufferOrch->getBufferObjectsWithNonZeroProfile(portPgKeys, APP_BUFFER_PG_TABLE_NAME); for (const auto& portPgKey : portPgKeys) { - auto toks = tokenize(portPgKey, '|'); + auto toks = tokenize(portPgKey, ':'); if (toks.size() != 2) { SWSS_LOG_ERROR("Invalid BUFFER_PG key: [%s]", portPgKey.c_str());