From efa0f01d61250475067354f971daa3bfb91279cf Mon Sep 17 00:00:00 2001 From: Stephen Sun <5379172+stephenxs@users.noreply.github.com> Date: Thu, 15 Sep 2022 00:33:59 +0800 Subject: [PATCH] [QoS] Enforce drop probability only for colors whose WRED are enabled (#2422) What I did Do not enforce drop probability for a color whose WRED is disabled. Signed-off-by: Stephen Sun stephens@nvidia.com Why I did it Currently, there is a logic to enforce the drop probability if it is not explicitly designated for a color. However, the drop probability is not a mandatory attribute. It can incur vendor SAI complaints to set it when the color is disabled. The logic was introduced from the very beginning (by PR #571) because no drop probability was defined in the QoS template at the time, which is no longer true. So we will enforce drop probability only if it is not configured and the color is enabled. How I verified it Unit test and manual test --- orchagent/qosorch.cpp | 48 ++++++++++++++---- tests/mock_tests/qosorch_ut.cpp | 90 +++++++++++++++++++++++++++++++++ 2 files changed, 128 insertions(+), 10 deletions(-) diff --git a/orchagent/qosorch.cpp b/orchagent/qosorch.cpp index c6d7bff842bb..2f8378d28417 100644 --- a/orchagent/qosorch.cpp +++ b/orchagent/qosorch.cpp @@ -46,6 +46,12 @@ enum { RED_DROP_PROBABILITY_SET = (1U << 2) }; +enum { + GREEN_WRED_ENABLED = (1U << 0), + YELLOW_WRED_ENABLED = (1U << 1), + RED_WRED_ENABLED = (1U << 2) +}; + // field_name is what is expected in CONFIG_DB PORT_QOS_MAP table map qos_to_attr_map = { {dscp_to_tc_field_name, SAI_PORT_ATTR_QOS_DSCP_TO_TC_MAP}, @@ -720,6 +726,7 @@ sai_object_id_t WredMapHandler::addQosItem(const vector &attrib sai_attribute_t attr; vector attrs; uint8_t drop_prob_set = 0; + uint8_t wred_enable_set = 0; attr.id = SAI_WRED_ATTR_WEIGHT; attr.value.s32 = 0; @@ -729,32 +736,53 @@ sai_object_id_t WredMapHandler::addQosItem(const vector &attrib { attrs.push_back(attrib); - if (attrib.id == SAI_WRED_ATTR_GREEN_DROP_PROBABILITY) + switch (attrib.id) { + case SAI_WRED_ATTR_GREEN_ENABLE: + if (attrib.value.booldata) + { + wred_enable_set |= GREEN_WRED_ENABLED; + } + break; + case SAI_WRED_ATTR_YELLOW_ENABLE: + if (attrib.value.booldata) + { + wred_enable_set |= YELLOW_WRED_ENABLED; + } + break; + case SAI_WRED_ATTR_RED_ENABLE: + if (attrib.value.booldata) + { + wred_enable_set |= RED_WRED_ENABLED; + } + break; + case SAI_WRED_ATTR_GREEN_DROP_PROBABILITY: drop_prob_set |= GREEN_DROP_PROBABILITY_SET; - } - else if (attrib.id == SAI_WRED_ATTR_YELLOW_DROP_PROBABILITY) - { + break; + case SAI_WRED_ATTR_YELLOW_DROP_PROBABILITY: drop_prob_set |= YELLOW_DROP_PROBABILITY_SET; - } - else if (attrib.id == SAI_WRED_ATTR_RED_DROP_PROBABILITY) - { + break; + case SAI_WRED_ATTR_RED_DROP_PROBABILITY: drop_prob_set |= RED_DROP_PROBABILITY_SET; + break; + default: + break; } } - if (!(drop_prob_set & GREEN_DROP_PROBABILITY_SET)) + + if (!(drop_prob_set & GREEN_DROP_PROBABILITY_SET) && (wred_enable_set & GREEN_WRED_ENABLED)) { attr.id = SAI_WRED_ATTR_GREEN_DROP_PROBABILITY; attr.value.s32 = 100; attrs.push_back(attr); } - if (!(drop_prob_set & YELLOW_DROP_PROBABILITY_SET)) + if (!(drop_prob_set & YELLOW_DROP_PROBABILITY_SET) && (wred_enable_set & YELLOW_WRED_ENABLED)) { attr.id = SAI_WRED_ATTR_YELLOW_DROP_PROBABILITY; attr.value.s32 = 100; attrs.push_back(attr); } - if (!(drop_prob_set & RED_DROP_PROBABILITY_SET)) + if (!(drop_prob_set & RED_DROP_PROBABILITY_SET) && (wred_enable_set & RED_WRED_ENABLED)) { attr.id = SAI_WRED_ATTR_RED_DROP_PROBABILITY; attr.value.s32 = 100; diff --git a/tests/mock_tests/qosorch_ut.cpp b/tests/mock_tests/qosorch_ut.cpp index e26b60c549db..c727aad70d54 100644 --- a/tests/mock_tests/qosorch_ut.cpp +++ b/tests/mock_tests/qosorch_ut.cpp @@ -37,6 +37,13 @@ namespace qosorch_test sai_set_switch_attribute_fn old_set_switch_attribute_fn; sai_switch_api_t ut_sai_switch_api, *pold_sai_switch_api; + typedef struct + { + sai_uint32_t green_max_drop_probability; + sai_uint32_t yellow_max_drop_probability; + sai_uint32_t red_max_drop_probability; + } qos_wred_max_drop_probability_t; + sai_status_t _ut_stub_sai_set_switch_attribute(sai_object_id_t switch_id, const sai_attribute_t *attr) { auto rc = old_set_switch_attribute_fn(switch_id, attr); @@ -55,6 +62,7 @@ namespace qosorch_test bool testing_wred_thresholds; WredMapHandler::qos_wred_thresholds_t saiThresholds; + qos_wred_max_drop_probability_t saiMaxDropProbabilities; void _ut_stub_sai_check_wred_attributes(const sai_attribute_t &attr) { if (!testing_wred_thresholds) @@ -88,6 +96,15 @@ namespace qosorch_test ASSERT_TRUE(!saiThresholds.red_max_threshold || saiThresholds.red_max_threshold > attr.value.u32); saiThresholds.red_min_threshold = attr.value.u32; break; + case SAI_WRED_ATTR_GREEN_DROP_PROBABILITY: + saiMaxDropProbabilities.green_max_drop_probability = attr.value.u32; + break; + case SAI_WRED_ATTR_YELLOW_DROP_PROBABILITY: + saiMaxDropProbabilities.yellow_max_drop_probability = attr.value.u32; + break; + case SAI_WRED_ATTR_RED_DROP_PROBABILITY: + saiMaxDropProbabilities.red_max_drop_probability = attr.value.u32; + break; default: break; } @@ -132,6 +149,23 @@ namespace qosorch_test ASSERT_TRUE(ts.empty()); } + void updateMaxDropProbabilityAndCheck(string name, vector &maxDropProbabilityVector, qos_wred_max_drop_probability_t &maxDropProbabilities) + { + std::deque entries; + vector ts; + entries.push_back({name, "SET", maxDropProbabilityVector}); + auto consumer = dynamic_cast(gQosOrch->getExecutor(CFG_WRED_PROFILE_TABLE_NAME)); + consumer->addToSync(entries); + entries.clear(); + saiMaxDropProbabilities.green_max_drop_probability = 0; + saiMaxDropProbabilities.yellow_max_drop_probability = 0; + saiMaxDropProbabilities.red_max_drop_probability = 0; + static_cast(gQosOrch)->doTask(); + ASSERT_EQ(saiMaxDropProbabilities.green_max_drop_probability, maxDropProbabilities.green_max_drop_probability); + ASSERT_EQ(saiMaxDropProbabilities.yellow_max_drop_probability, maxDropProbabilities.yellow_max_drop_probability); + ASSERT_EQ(saiMaxDropProbabilities.red_max_drop_probability, maxDropProbabilities.red_max_drop_probability); + } + sai_status_t _ut_stub_sai_create_wred( _Out_ sai_object_id_t *wred_id, _In_ sai_object_id_t switch_id, @@ -1338,6 +1372,62 @@ namespace qosorch_test testing_wred_thresholds = false; } + TEST_F(QosOrchTest, QosOrchTestWredDropProbability) + { + 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 greenProfile = { + {"wred_green_enable", "true"}, + {"wred_yellow_enable", "false"}, + }; + qos_wred_max_drop_probability_t greenProbabilities = { + 100, // green_max_drop_probability + 0, // yellow_max_drop_probability + 0 // red_max_drop_probability + }; + updateMaxDropProbabilityAndCheck("green_default", greenProfile, greenProbabilities); + + greenProfile.push_back({"green_drop_probability", "5"}); + greenProbabilities.green_max_drop_probability = 5; + updateMaxDropProbabilityAndCheck("green", greenProfile, greenProbabilities); + + vector yellowProfile = { + {"wred_yellow_enable", "true"}, + {"wred_red_enable", "false"}, + }; + qos_wred_max_drop_probability_t yellowProbabilities = { + 0, // green_max_drop_probability + 100, // yellow_max_drop_probability + 0 // red_max_drop_probability + }; + updateMaxDropProbabilityAndCheck("yellow_default", yellowProfile, yellowProbabilities); + + yellowProfile.push_back({"yellow_drop_probability", "5"}); + yellowProbabilities.yellow_max_drop_probability = 5; + updateMaxDropProbabilityAndCheck("yellow", yellowProfile, yellowProbabilities); + + vector redProfile = { + {"wred_green_enable", "false"}, + {"wred_red_enable", "true"}, + }; + qos_wred_max_drop_probability_t redProbabilities = { + 0, // green_max_drop_probability + 0, // yellow_max_drop_probability + 100 // red_max_drop_probability + }; + updateMaxDropProbabilityAndCheck("red_default", redProfile, redProbabilities); + + redProfile.push_back({"red_drop_probability", "5"}); + redProbabilities.red_max_drop_probability = 5; + updateMaxDropProbabilityAndCheck("red", redProfile, redProbabilities); + + testing_wred_thresholds = false; + } + + /* * Make sure empty fields won't cause orchagent crash */