From 9f2e27b4740ae5395d3691ce8a0199e5fb35119c Mon Sep 17 00:00:00 2001 From: Stephen Sun <5379172+stephenxs@users.noreply.github.com> Date: Tue, 9 Aug 2022 03:06:01 +0800 Subject: [PATCH] [QoS] Fix issue: the WRED profile can not be set if current min > new max or current max < new min (#2379) What I did Fix issue: Setting a WRED profile can fail in case the current min threshold is greater than the new max threshold or the current max threshold is less than the new min threshold for any color and at any time. Eg. Current min=1M, max=2M, new min=3M, new max=4M The min threshold is set first, so current max (2M) < new min (3M), which violates the condition This is because there can be only one attribute in each SAI SET operation, which means the vendor SAI does not understand the whole information of the new attributes and can only perform the sanity check against each SET operation. Signed-off-by: Stephen Sun stephens@nvidia.com Why I did it Fix the issue How I verified it Manual test and mock test. Details if related The fix The thresholds that have been applied to SAI will be stored in orchagent. The original logic is to handle each attribute to be set and append it to an attribute list. To resolve the issue, a deferred attribute list is introduced and will be appended to the original attribute list after all the attributes have been handled. In the new logic, each threshold to be set will be checked against the stored data. In case it violates the condition, the violating attribute will be deferred, done via putting it into the deferred attributes list. For any color, there can be only 1 threshold violating the condition. Otherwise, it means both new min > old max and new max > old min, which means either old max < old min or new max < new min, which means either old or new data is illegal. This can not happen because illegal data will not be applied and stored. By doing so, the other threshold will be applied first, which extends the threshold range and breaks the violating condition. A logic is also introduced to guarantee the min threshold is always less than the max threshold in the new profile to be set. For the above example, In the new logic, the min threshold will be deferred so the new max threshold will be applied first and then the new min is applied. In this flow, there is no violation at any time. min = 1M, max = 2M => min = 1M, max = 4M => min = 3M, max = 4M --- orchagent/qosorch.cpp | 141 +++++++++++-- orchagent/qosorch.h | 18 ++ tests/mock_tests/mock_orchagent_main.h | 2 +- tests/mock_tests/qosorch_ut.cpp | 267 +++++++++++++++++++++++++ 4 files changed, 409 insertions(+), 19 deletions(-) diff --git a/orchagent/qosorch.cpp b/orchagent/qosorch.cpp index 274003bd7219..14ca851e76a1 100644 --- a/orchagent/qosorch.cpp +++ b/orchagent/qosorch.cpp @@ -489,47 +489,140 @@ bool WredMapHandler::convertBool(string str, bool &val) return true; } +void WredMapHandler::appendThresholdToAttributeList(sai_attr_id_t type, + sai_uint32_t threshold, + bool needDefer, + vector &normalQueue, + vector &deferredQueue, + sai_uint32_t &newThreshold) +{ + sai_attribute_t attr; + + attr.id = type; + attr.value.u32 = threshold; + if (needDefer) + { + deferredQueue.push_back(attr); + } + else + { + normalQueue.push_back(attr); + } + newThreshold = threshold; +} + +WredMapHandler::qos_wred_thresholds_store_t WredMapHandler::m_wredProfiles; + bool WredMapHandler::convertFieldValuesToAttributes(KeyOpFieldsValuesTuple &tuple, vector &attribs) { SWSS_LOG_ENTER(); sai_attribute_t attr; + vector deferred_attributes; + auto &key = kfvKey(tuple); + auto &storedProfile = WredMapHandler::m_wredProfiles[key]; + qos_wred_thresholds_t currentProfile = storedProfile; + sai_uint32_t threshold; + + /* + * Setting WRED profile can fail in case + * - the current min threshold is greater than the new max threshold + * - or the current max threshold is less than the new min threshold + * for any color at any time, on some vendor's platforms. + * + * The root cause + * There can be only one attribute in each SAI SET operation, which means + * the vendor SAI do not have a big picture regarding what attributes are being set + * and can only perform the sanity check against each SET operation. + * In the above case, the sanity check will fail. + * + * The fix + * The thresholds that have been applied to SAI will be stored in orchagent. + * + * The original logic is to handle each attribute to be set and append it to an attribute list. + * To resolve the issue, a 2nd half attribute list is introduced and + * will be appended to the original attribute list after all the attributes have been handled. + * + * In the new logic, each threshold to be set will be checked against the stored data. + * In case it violates the condition, the violating attribute will be deferred, done via putting it into the 2nd half attributes list. + * + * For any color, there can be only 1 threshold violating the condition. + * Otherwise, it means both new min > old max and new max > old min, which means either old max < old min or new max < new min, + * which means either old or new data is illegal. + * This can not happen because illegal data can not be applied and stored. + * + * By doing so, the other threshold will be applied first, which extends the threshold range and breaks the violating condition. + * A logic is also introduced to guarantee the min threshold is always less than the max threshold in the new profile to be set. + * + * For example: + * Current min=1M, max=2M, new min=3M, new max=4M + * The min is set first, so current max (2M) < new min (3M), which violates the condition + * By the new logic, min threshold will be deferred so the new max will be applied first and then the new min is applied and no violating. + * min = 1M, max = 2M + * => min = 1M, max = 4M + * => min = 3M, max = 4M + */ + for (auto i = kfvFieldsValues(tuple).begin(); i != kfvFieldsValues(tuple).end(); i++) { if (fvField(*i) == yellow_max_threshold_field_name) { - attr.id = SAI_WRED_ATTR_YELLOW_MAX_THRESHOLD; - attr.value.s32 = stoi(fvValue(*i)); - attribs.push_back(attr); + threshold = stoi(fvValue(*i)); + appendThresholdToAttributeList(SAI_WRED_ATTR_YELLOW_MAX_THRESHOLD, + threshold, + (storedProfile.yellow_min_threshold > threshold), + attribs, + deferred_attributes, + currentProfile.yellow_max_threshold); } else if (fvField(*i) == yellow_min_threshold_field_name) { - attr.id = SAI_WRED_ATTR_YELLOW_MIN_THRESHOLD; - attr.value.s32 = stoi(fvValue(*i)); - attribs.push_back(attr); + threshold = stoi(fvValue(*i)); + appendThresholdToAttributeList(SAI_WRED_ATTR_YELLOW_MIN_THRESHOLD, + threshold, + (storedProfile.yellow_max_threshold < threshold), + attribs, + deferred_attributes, + currentProfile.yellow_min_threshold); } else if (fvField(*i) == green_max_threshold_field_name) { - attr.id = SAI_WRED_ATTR_GREEN_MAX_THRESHOLD; - attr.value.s32 = stoi(fvValue(*i)); - attribs.push_back(attr); + threshold = stoi(fvValue(*i)); + appendThresholdToAttributeList(SAI_WRED_ATTR_GREEN_MAX_THRESHOLD, + threshold, + (storedProfile.green_min_threshold > threshold), + attribs, + deferred_attributes, + currentProfile.green_max_threshold); } else if (fvField(*i) == green_min_threshold_field_name) { - attr.id = SAI_WRED_ATTR_GREEN_MIN_THRESHOLD; - attr.value.s32 = stoi(fvValue(*i)); - attribs.push_back(attr); + threshold = stoi(fvValue(*i)); + appendThresholdToAttributeList(SAI_WRED_ATTR_GREEN_MIN_THRESHOLD, + threshold, + (storedProfile.green_max_threshold < threshold), + attribs, + deferred_attributes, + currentProfile.green_min_threshold); } else if (fvField(*i) == red_max_threshold_field_name) { - attr.id = SAI_WRED_ATTR_RED_MAX_THRESHOLD; - attr.value.s32 = stoi(fvValue(*i)); - attribs.push_back(attr); + threshold = stoi(fvValue(*i)); + appendThresholdToAttributeList(SAI_WRED_ATTR_RED_MAX_THRESHOLD, + threshold, + (storedProfile.red_min_threshold > threshold), + attribs, + deferred_attributes, + currentProfile.red_max_threshold); } else if (fvField(*i) == red_min_threshold_field_name) { - attr.id = SAI_WRED_ATTR_RED_MIN_THRESHOLD; - attr.value.s32 = stoi(fvValue(*i)); - attribs.push_back(attr); + threshold = stoi(fvValue(*i)); + appendThresholdToAttributeList(SAI_WRED_ATTR_RED_MIN_THRESHOLD, + threshold, + (storedProfile.red_max_threshold < threshold), + attribs, + deferred_attributes, + currentProfile.red_min_threshold); } else if (fvField(*i) == green_drop_probability_field_name) { @@ -588,6 +681,18 @@ bool WredMapHandler::convertFieldValuesToAttributes(KeyOpFieldsValuesTuple &tupl return false; } } + + if ((currentProfile.green_min_threshold > currentProfile.green_max_threshold) + || (currentProfile.yellow_min_threshold > currentProfile.yellow_max_threshold) + || (currentProfile.red_min_threshold > currentProfile.red_max_threshold)) + { + SWSS_LOG_ERROR("Wrong wred profile: min threshold is greater than max threshold"); + return false; + } + + attribs.insert(attribs.end(), deferred_attributes.begin(), deferred_attributes.end()); + storedProfile = currentProfile; + return true; } diff --git a/orchagent/qosorch.h b/orchagent/qosorch.h index f8b97cd381cd..b5e2e1ad8666 100644 --- a/orchagent/qosorch.h +++ b/orchagent/qosorch.h @@ -112,6 +112,24 @@ class WredMapHandler : public QosMapHandler protected: bool convertEcnMode(string str, sai_ecn_mark_mode_t &ecn_val); bool convertBool(string str, bool &val); +private: + void appendThresholdToAttributeList(sai_attr_id_t type, + sai_uint32_t threshold, + bool needDefer, + vector &normalQueue, + vector &deferredQueue, + sai_uint32_t &newThreshold); + typedef struct { + sai_uint32_t green_max_threshold; + sai_uint32_t green_min_threshold; + sai_uint32_t yellow_max_threshold; + sai_uint32_t yellow_min_threshold; + sai_uint32_t red_max_threshold; + sai_uint32_t red_min_threshold; + } qos_wred_thresholds_t; + typedef map qos_wred_thresholds_store_t; + + static qos_wred_thresholds_store_t m_wredProfiles; }; diff --git a/tests/mock_tests/mock_orchagent_main.h b/tests/mock_tests/mock_orchagent_main.h index f41c5b29a588..0acba4ef1c20 100644 --- a/tests/mock_tests/mock_orchagent_main.h +++ b/tests/mock_tests/mock_orchagent_main.h @@ -12,8 +12,8 @@ #include "mirrororch.h" #define private public #include "bufferorch.h" -#undef private #include "qosorch.h" +#undef private #include "vrforch.h" #include "vnetorch.h" #include "vxlanorch.h" diff --git a/tests/mock_tests/qosorch_ut.cpp b/tests/mock_tests/qosorch_ut.cpp index 7461a29e3cad..13454cee56b5 100644 --- a/tests/mock_tests/qosorch_ut.cpp +++ b/tests/mock_tests/qosorch_ut.cpp @@ -23,11 +23,14 @@ namespace qosorch_test int sai_remove_qos_map_count; int sai_remove_wred_profile_count; int sai_remove_scheduler_count; + int sai_set_wred_attribute_count; sai_object_id_t switch_dscp_to_tc_map_id; sai_remove_scheduler_fn old_remove_scheduler; sai_scheduler_api_t ut_sai_scheduler_api, *pold_sai_scheduler_api; + sai_create_wred_fn old_create_wred; sai_remove_wred_fn old_remove_wred; + sai_set_wred_attribute_fn old_set_wred_attribute; sai_wred_api_t ut_sai_wred_api, *pold_sai_wred_api; sai_remove_qos_map_fn old_remove_qos_map; sai_qos_map_api_t ut_sai_qos_map_api, *pold_sai_qos_map_api; @@ -50,6 +53,102 @@ namespace qosorch_test return rc; } + bool testing_wred_thresholds; + WredMapHandler::qos_wred_thresholds_t saiThresholds; + void _ut_stub_sai_check_wred_attributes(const sai_attribute_t &attr) + { + if (!testing_wred_thresholds) + { + return; + } + + switch (attr.id) + { + case SAI_WRED_ATTR_GREEN_MAX_THRESHOLD: + ASSERT_TRUE(!saiThresholds.green_min_threshold || saiThresholds.green_min_threshold < attr.value.u32); + saiThresholds.green_max_threshold = attr.value.u32; + break; + case SAI_WRED_ATTR_GREEN_MIN_THRESHOLD: + ASSERT_TRUE(!saiThresholds.green_max_threshold || saiThresholds.green_max_threshold > attr.value.u32); + saiThresholds.green_min_threshold = attr.value.u32; + break; + case SAI_WRED_ATTR_YELLOW_MAX_THRESHOLD: + ASSERT_TRUE(!saiThresholds.yellow_min_threshold || saiThresholds.yellow_min_threshold < attr.value.u32); + saiThresholds.yellow_max_threshold = attr.value.u32; + break; + case SAI_WRED_ATTR_YELLOW_MIN_THRESHOLD: + ASSERT_TRUE(!saiThresholds.yellow_max_threshold || saiThresholds.yellow_max_threshold > attr.value.u32); + saiThresholds.yellow_min_threshold = attr.value.u32; + break; + case SAI_WRED_ATTR_RED_MAX_THRESHOLD: + ASSERT_TRUE(!saiThresholds.red_min_threshold || saiThresholds.red_min_threshold < attr.value.u32); + saiThresholds.red_max_threshold = attr.value.u32; + break; + case SAI_WRED_ATTR_RED_MIN_THRESHOLD: + ASSERT_TRUE(!saiThresholds.red_max_threshold || saiThresholds.red_max_threshold > attr.value.u32); + saiThresholds.red_min_threshold = attr.value.u32; + break; + default: + break; + } + } + + void checkWredProfileEqual(const string &name, WredMapHandler::qos_wred_thresholds_t &thresholds) + { + auto &oaThresholds = WredMapHandler::m_wredProfiles[name]; + + ASSERT_EQ(oaThresholds.green_min_threshold, thresholds.green_min_threshold); + ASSERT_EQ(oaThresholds.green_max_threshold, thresholds.green_max_threshold); + ASSERT_EQ(oaThresholds.yellow_min_threshold, thresholds.yellow_min_threshold); + ASSERT_EQ(oaThresholds.yellow_max_threshold, thresholds.yellow_max_threshold); + ASSERT_EQ(oaThresholds.red_min_threshold, thresholds.red_min_threshold); + ASSERT_EQ(oaThresholds.red_max_threshold, thresholds.red_max_threshold); + } + + void updateWredProfileAndCheck(vector &thresholdsVector, WredMapHandler::qos_wred_thresholds_t &thresholdsValue) + { + std::deque entries; + entries.push_back({"AZURE", "SET", thresholdsVector}); + auto consumer = dynamic_cast(gQosOrch->getExecutor(CFG_WRED_PROFILE_TABLE_NAME)); + consumer->addToSync(entries); + entries.clear(); + static_cast(gQosOrch)->doTask(); + checkWredProfileEqual("AZURE", saiThresholds); + checkWredProfileEqual("AZURE", thresholdsValue); + } + + void updateWrongWredProfileAndCheck(vector &thresholdsVector) + { + std::deque entries; + vector ts; + entries.push_back({"AZURE", "SET", thresholdsVector}); + auto consumer = dynamic_cast(gQosOrch->getExecutor(CFG_WRED_PROFILE_TABLE_NAME)); + consumer->addToSync(entries); + entries.clear(); + auto current_sai_wred_set_count = sai_set_wred_attribute_count; + static_cast(gQosOrch)->doTask(); + ASSERT_EQ(current_sai_wred_set_count, sai_set_wred_attribute_count); + static_cast(gQosOrch)->dumpPendingTasks(ts); + ASSERT_TRUE(ts.empty()); + } + + sai_status_t _ut_stub_sai_create_wred( + _Out_ sai_object_id_t *wred_id, + _In_ sai_object_id_t switch_id, + _In_ uint32_t attr_count, + _In_ const sai_attribute_t *attr_list) + { + auto rc = old_create_wred(wred_id, switch_id, attr_count, attr_list); + if (rc == SAI_STATUS_SUCCESS) + { + for (uint32_t i = 0; i < attr_count; i++) + { + _ut_stub_sai_check_wred_attributes(attr_list[i]); + } + } + return rc; + } + sai_status_t _ut_stub_sai_remove_wred(sai_object_id_t wred_id) { auto rc = old_remove_wred(wred_id); @@ -58,6 +157,19 @@ namespace qosorch_test return rc; } + sai_status_t _ut_stub_sai_set_wred_attribute( + _In_ sai_object_id_t wred_id, + _In_ const sai_attribute_t *attr) + { + auto rc = old_set_wred_attribute(wred_id, attr); + if (rc == SAI_STATUS_SUCCESS) + { + _ut_stub_sai_check_wred_attributes(*attr); + } + sai_set_wred_attribute_count++; + return rc; + } + sai_status_t _ut_stub_sai_remove_scheduler(sai_object_id_t scheduler_id) { auto rc = old_remove_scheduler(scheduler_id); @@ -133,6 +245,13 @@ namespace qosorch_test ReplaceSaiRemoveApi(sai_wred_api, ut_sai_wred_api, pold_sai_wred_api, _ut_stub_sai_remove_wred, sai_wred_api->remove_wred, old_remove_wred, ut_sai_wred_api.remove_wred); + // Mock other wred APIs + old_create_wred = pold_sai_wred_api->create_wred; + ut_sai_wred_api.create_wred = _ut_stub_sai_create_wred; + old_set_wred_attribute = pold_sai_wred_api->set_wred_attribute; + ut_sai_wred_api.set_wred_attribute = _ut_stub_sai_set_wred_attribute; + + // Mock switch API pold_sai_switch_api = sai_switch_api; ut_sai_switch_api = *pold_sai_switch_api; old_set_switch_attribute_fn = pold_sai_switch_api->set_switch_attribute; @@ -1070,4 +1189,152 @@ namespace qosorch_test ASSERT_EQ(ts.size(), 1); ts.clear(); } + + /* + * There are 4 ECN ranges + * ------------------------------------------------------------------------------- + * profile lower min=1M max=2M + * profile upper min=3M max=4M + * proile middle min=1.5M max=1.5M + * ------------------------------------------------------------------------------- + * Test step Test case + * 1. Initialize a wred profile with value lower set Wred profile intialization + * 2. Update the value to upper set The new min threshold is greater than the current max threshold + * 3. Update the value back to lower set The new max threshold is less than the current min threshold + * 4. Update the value to middle set Normal case to ensure nothing broken + * 5. Update the value back to lower set Normal case to ensure nothing broken + */ + TEST_F(QosOrchTest, QosOrchTestWredThresholdsTest) + { + testing_wred_thresholds = true; + + // The order of fields matters when the wred profile is updated from the upper set to the lower set + // It should be max, min for each color. In this order, the new max is less then the current min + // QoS orchagent should guarantee that the new min is configured first and then new max + vector lowerSetVector = { + {"ecn", "ecn_all"}, + {"green_drop_probability", "5"}, + {"green_max_threshold", "2097152"}, + {"green_min_threshold", "1048576"}, + {"wred_green_enable", "true"}, + {"yellow_drop_probability", "5"}, + {"yellow_max_threshold", "2097153"}, + {"yellow_min_threshold", "1048577"}, + {"wred_yellow_enable", "true"}, + {"red_drop_probability", "5"}, + {"red_max_threshold", "2097154"}, + {"red_min_threshold", "1048578"}, + {"wred_red_enable", "true"} + }; + WredMapHandler::qos_wred_thresholds_t lowerThresholds = { + 2097152, //green_max_threshold + 1048576, //green_min_threshold + 2097153, //yellow_max_threshold + 1048577, //yellow_min_threshold + 2097154, //red_max_threshold + 1048578 //red_min_threshold + }; + // The order of fields matters when the wred profile is updated from the lower set to the upper set + // It should be min, max for each color, in which the new min is larger then the current max + // QoS orchagent should guarantee that the new max is configured first and then new min + vector upperSetVector = { + {"ecn", "ecn_all"}, + {"green_drop_probability", "5"}, + {"green_min_threshold", "3145728"}, + {"green_max_threshold", "4194304"}, + {"wred_green_enable", "true"}, + {"yellow_drop_probability", "5"}, + {"yellow_min_threshold", "3145729"}, + {"yellow_max_threshold", "4194305"}, + {"wred_yellow_enable", "true"}, + {"red_drop_probability", "5"}, + {"red_min_threshold", "3145730"}, + {"red_max_threshold", "4194306"}, + {"wred_red_enable", "true"} + }; + WredMapHandler::qos_wred_thresholds_t upperThresholds = { + 4194304, //green_max_threshold + 3145728, //green_min_threshold + 4194305, //yellow_max_threshold + 3145729, //yellow_min_threshold + 4194306, //red_max_threshold + 3145730 //red_min_threshold + }; + // Order doesn't matter. + vector middleSetVector = { + {"ecn", "ecn_all"}, + {"green_drop_probability", "5"}, + {"green_min_threshold", "1572864"}, + {"green_max_threshold", "2621440"}, + {"wred_green_enable", "true"}, + {"yellow_drop_probability", "5"}, + {"yellow_min_threshold", "1572865"}, + {"yellow_max_threshold", "2621441"}, + {"wred_yellow_enable", "true"}, + {"red_drop_probability", "5"}, + {"red_min_threshold", "1572866"}, + {"red_max_threshold", "2621442"}, + {"wred_red_enable", "true"} + }; + WredMapHandler::qos_wred_thresholds_t middleThresholds = { + 2621440, //green_max_threshold + 1572864, //green_min_threshold + 2621441, //yellow_max_threshold + 1572865, //yellow_min_threshold + 2621442, //red_max_threshold + 1572866 //red_min_threshold + }; + + // Wrong profile + vector greenWrongVector = { + {"ecn", "ecn_green"}, + {"green_drop_probability", "5"}, + {"green_min_threshold", "2621440"}, + {"green_max_threshold", "1572864"}, + {"wred_green_enable", "true"} + }; + + vector yellowWrongVector = { + {"ecn", "ecn_yellow"}, + {"yellow_drop_probability", "5"}, + {"yellow_min_threshold", "2621441"}, + {"yellow_max_threshold", "1572865"}, + {"wred_yellow_enable", "true"} + }; + + vector redWrongVector = { + {"ecn", "ecn_red"}, + {"red_drop_probability", "5"}, + {"red_min_threshold", "2621442"}, + {"red_max_threshold", "1572866"}, + {"wred_red_enable", "true"} + }; + + std::deque entries; + // 1. Initialize + updateWredProfileAndCheck(lowerSetVector, lowerThresholds); + + // 2. Update the thresholds from the lower set to the upper set + updateWredProfileAndCheck(upperSetVector, upperThresholds); + + // 3. Update the thresholds from the upper set back to the lower set + updateWredProfileAndCheck(lowerSetVector, lowerThresholds); + + // 4. Update the thresholds from the lower set to the middle set + updateWredProfileAndCheck(middleSetVector, middleThresholds); + + // 5. Update the thresholds from the middle set back to the lower set + updateWredProfileAndCheck(lowerSetVector, lowerThresholds); + + // Wrong parameters + updateWrongWredProfileAndCheck(greenWrongVector); + updateWrongWredProfileAndCheck(yellowWrongVector); + updateWrongWredProfileAndCheck(redWrongVector); + + // Make sure the profiles in orchagent and SAI are not updated by the wrong profile + checkWredProfileEqual("AZURE", saiThresholds); + checkWredProfileEqual("AZURE", lowerThresholds); + + testing_wred_thresholds = false; + } }