From ec11131119502c72cd118a792494bd8da9cdbdc2 Mon Sep 17 00:00:00 2001 From: abanu-ms Date: Fri, 3 Dec 2021 08:30:27 +0200 Subject: [PATCH] [cbf] Fix max FC value (#2049) What I did Fixed the maximum FC value allowed by a switch. Why I did it There was a bit of confusion when I first wrote this, thinking the attribute should return the maximum FC value allowed by the switch, not the maximum number of FC classes. Because of this, the actual maximum FC value allowed is one less than the current one because we're counting from 0. As such, if the switch reports the maximum number of FCs is 255, the actual FC value must be in the 0-254 range. How I verified it Updated the existing UTs --- orchagent/cbf/cbfnhgorch.cpp | 2 +- orchagent/cbf/nhgmaporch.cpp | 24 ++++++++++++------------ orchagent/cbf/nhgmaporch.h | 4 ++-- orchagent/qosorch.cpp | 14 ++++++++------ tests/test_nhg.py | 2 +- tests/test_qos_map.py | 10 +++++----- 6 files changed, 29 insertions(+), 27 deletions(-) diff --git a/orchagent/cbf/cbfnhgorch.cpp b/orchagent/cbf/cbfnhgorch.cpp index b5a4263f15..76435ad12d 100644 --- a/orchagent/cbf/cbfnhgorch.cpp +++ b/orchagent/cbf/cbfnhgorch.cpp @@ -308,7 +308,7 @@ bool CbfNhg::sync() nhg_attr.value.u32 = static_cast(m_members.size()); nhg_attrs.push_back(move(nhg_attr)); - if (nhg_attr.value.u32 > gNhgMapOrch->getMaxFcVal()) + if (nhg_attr.value.u32 > gNhgMapOrch->getMaxNumFcs()) { /* If there are more members than FCs then this may be an error, as some members won't be used. */ SWSS_LOG_WARN("More CBF NHG members configured than supported Forwarding Classes"); diff --git a/orchagent/cbf/nhgmaporch.cpp b/orchagent/cbf/nhgmaporch.cpp index d765e3e90e..fd83fe4b12 100644 --- a/orchagent/cbf/nhgmaporch.cpp +++ b/orchagent/cbf/nhgmaporch.cpp @@ -294,34 +294,34 @@ void NhgMapOrch::decRefCount(const string &index) } /* - * Get the max FC value supported by the switch. + * Get the maximum number of FC classes supported by the switch. */ -sai_uint8_t NhgMapOrch::getMaxFcVal() +sai_uint8_t NhgMapOrch::getMaxNumFcs() { SWSS_LOG_ENTER(); - static int max_fc_val = -1; + static int max_num_fcs = -1; /* - * Get the maximum value allowed for FC if it wasn't already initialized. + * Get the maximum number of FC classes if it wasn't already initialized. */ - if (max_fc_val == -1) + if (max_num_fcs == -1) { sai_attribute_t attr; attr.id = SAI_SWITCH_ATTR_MAX_NUMBER_OF_FORWARDING_CLASSES; if (sai_switch_api->get_switch_attribute(gSwitchId, 1, &attr) == SAI_STATUS_SUCCESS) { - max_fc_val = attr.value.u8; + max_num_fcs = attr.value.u8; } else { SWSS_LOG_WARN("Switch does not support FCs"); - max_fc_val = 0; + max_num_fcs = 0; } } - return static_cast(max_fc_val); + return static_cast(max_num_fcs); } /* @@ -343,7 +343,7 @@ pair> NhgMapOrch::getMap(const ve } unordered_map fc_map; - sai_uint8_t max_fc_val = getMaxFcVal(); + sai_uint8_t max_num_fcs = getMaxNumFcs(); /* * Create the map while validating that the values are positive @@ -353,13 +353,13 @@ pair> NhgMapOrch::getMap(const ve try { /* - * Check the FC value is valid. + * Check the FC value is valid. FC value must be in range [0, max_num_fcs). */ auto fc = stoi(fvField(*it)); - if ((fc < 0) || (fc > max_fc_val)) + if ((fc < 0) || (fc >= max_num_fcs)) { - SWSS_LOG_ERROR("FC value %d is either negative or greater than max value %d", fc, max_fc_val); + SWSS_LOG_ERROR("FC value %d is either negative or greater than max value %d", fc, max_num_fcs - 1); success = false; break; } diff --git a/orchagent/cbf/nhgmaporch.h b/orchagent/cbf/nhgmaporch.h index c345e7d566..7d7317a1d6 100644 --- a/orchagent/cbf/nhgmaporch.h +++ b/orchagent/cbf/nhgmaporch.h @@ -43,9 +43,9 @@ class NhgMapOrch : public Orch void decRefCount(const string &key); /* - * Get the max FC value supported by the switch. + * Get the maximum number of FC classes supported by the switch. */ - static sai_uint8_t getMaxFcVal(); + static sai_uint8_t getMaxNumFcs(); private: /* diff --git a/orchagent/qosorch.cpp b/orchagent/qosorch.cpp index e51b0fcb73..5730952e82 100644 --- a/orchagent/qosorch.cpp +++ b/orchagent/qosorch.cpp @@ -808,7 +808,7 @@ bool DscpToFcMapHandler::convertFieldValuesToAttributes(KeyOpFieldsValuesTuple & { SWSS_LOG_ENTER(); - sai_uint8_t max_fc_val = NhgMapOrch::getMaxFcVal(); + sai_uint8_t max_num_fcs = NhgMapOrch::getMaxNumFcs(); sai_attribute_t list_attr; list_attr.id = SAI_QOS_MAP_ATTR_MAP_TO_VALUE_LIST; @@ -835,10 +835,11 @@ bool DscpToFcMapHandler::convertFieldValuesToAttributes(KeyOpFieldsValuesTuple & } list_attr.value.qosmap.list[ind].key.dscp = static_cast(value); + // FC value must be in range [0, max_num_fcs) value = stoi(fvValue(*i)); - if ((value < 0) || (value > max_fc_val)) + if ((value < 0) || (value >= max_num_fcs)) { - SWSS_LOG_ERROR("FC value %d is either negative, or bigger than max value %d", value, max_fc_val); + SWSS_LOG_ERROR("FC value %d is either negative, or bigger than max value %d", value, max_num_fcs - 1); delete[] list_attr.value.qosmap.list; return false; } @@ -901,7 +902,7 @@ bool ExpToFcMapHandler::convertFieldValuesToAttributes(KeyOpFieldsValuesTuple &t { SWSS_LOG_ENTER(); - sai_uint8_t max_fc_val = NhgMapOrch::getMaxFcVal(); + sai_uint8_t max_num_fcs = NhgMapOrch::getMaxNumFcs(); sai_attribute_t list_attr; list_attr.id = SAI_QOS_MAP_ATTR_MAP_TO_VALUE_LIST; @@ -928,10 +929,11 @@ bool ExpToFcMapHandler::convertFieldValuesToAttributes(KeyOpFieldsValuesTuple &t } list_attr.value.qosmap.list[ind].key.mpls_exp = static_cast(value); + // FC value must be in range [0, max_num_fcs) value = stoi(fvValue(*i)); - if ((value < 0) || (value > max_fc_val)) + if ((value < 0) || (value >= max_num_fcs)) { - SWSS_LOG_ERROR("FC value %d is either negative, or bigger than max value %hu", value, max_fc_val); + SWSS_LOG_ERROR("FC value %d is either negative, or bigger than max value %hu", value, max_num_fcs - 1); delete[] list_attr.value.qosmap.list; return false; } diff --git a/tests/test_nhg.py b/tests/test_nhg.py index da5ff171b1..c265e78b04 100644 --- a/tests/test_nhg.py +++ b/tests/test_nhg.py @@ -1980,7 +1980,7 @@ def data_validation_test(): # Test validation errors nhg_maps = [ ('-1', '0'), # negative FC - ('64', '0'), # greater than max FC value + ('63', '0'), # greater than max FC value ('a', '0'), # non-integer FC ('0', '-1'), # negative NH index ('0', 'a'), # non-integer NH index diff --git a/tests/test_qos_map.py b/tests/test_qos_map.py index 21a25742c9..301bd3c6d6 100644 --- a/tests/test_qos_map.py +++ b/tests/test_qos_map.py @@ -139,7 +139,7 @@ def test_dscp_to_fc(self, dvs): self.init_test(dvs) # Create a DSCP_TO_FC map - dscp_map = [(str(i), str(i)) for i in range(0, 64)] + dscp_map = [(str(i), str(i)) for i in range(0, 63)] self.dscp_ps.set("AZURE", swsscommon.FieldValuePairs(dscp_map)) self.asic_db.wait_for_n_keys(self.ASIC_QOS_MAP_STR, self.asic_qos_map_count + 1) @@ -153,7 +153,7 @@ def test_dscp_to_fc(self, dvs): assert(fvs.get("SAI_QOS_MAP_ATTR_TYPE") == "SAI_QOS_MAP_TYPE_DSCP_TO_FORWARDING_CLASS") # Modify the map - dscp_map = [(str(i), '0') for i in range(0, 64)] + dscp_map = [(str(i), '0') for i in range(0, 63)] self.dscp_ps.set("AZURE", swsscommon.FieldValuePairs(dscp_map)) time.sleep(1) @@ -174,7 +174,7 @@ def test_dscp_to_fc(self, dvs): ('-1', '0'), # negative DSCP ('64', '0'), # DSCP greater than max value ('0', '-1'), # negative FC - ('0', '64'), # FC greater than max value + ('0', '63'), # FC greater than max value ('a', '0'), # non-integer DSCP ('0', 'a'), # non-integet FC ] @@ -228,7 +228,7 @@ def test_exp_to_fc(self, dvs): ('-1', '0'), # negative EXP ('8', '0'), # EXP greater than max value ('0', '-1'), # negative FC - ('0', '64'), # FC greater than max value + ('0', '63'), # FC greater than max value ('a', '0'), # non-integer EXP ('0', 'a'), # non-integet FC ] @@ -258,7 +258,7 @@ def test_per_port_cbf_binding(self, dvs): self.init_test(dvs) # Create a DSCP_TO_FC map - dscp_map = [(str(i), str(i)) for i in range(0, 64)] + dscp_map = [(str(i), str(i)) for i in range(0, 63)] self.dscp_ps.set("AZURE", swsscommon.FieldValuePairs(dscp_map)) self.asic_db.wait_for_n_keys(self.ASIC_QOS_MAP_STR, self.asic_qos_map_count + 1) dscp_map_id = self.get_qos_id()