Skip to content

Commit

Permalink
[cbf] Fix max FC value (sonic-net#2049)
Browse files Browse the repository at this point in the history
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
  • Loading branch information
abanu-ms authored Dec 3, 2021
1 parent b1b5b29 commit 45e446d
Show file tree
Hide file tree
Showing 6 changed files with 30 additions and 28 deletions.
2 changes: 1 addition & 1 deletion orchagent/cbf/cbfnhgorch.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -308,7 +308,7 @@ bool CbfNhg::sync()
nhg_attr.value.u32 = static_cast<sai_uint32_t>(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");
Expand Down
24 changes: 12 additions & 12 deletions orchagent/cbf/nhgmaporch.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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<sai_uint8_t>(max_fc_val);
return static_cast<sai_uint8_t>(max_num_fcs);
}

/*
Expand All @@ -343,7 +343,7 @@ pair<bool, unordered_map<sai_uint32_t, sai_int32_t>> NhgMapOrch::getMap(const ve
}

unordered_map<sai_uint32_t, sai_int32_t> 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
Expand All @@ -353,13 +353,13 @@ pair<bool, unordered_map<sai_uint32_t, sai_int32_t>> 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;
}
Expand Down
4 changes: 2 additions & 2 deletions orchagent/cbf/nhgmaporch.h
Original file line number Diff line number Diff line change
Expand Up @@ -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:
/*
Expand Down
16 changes: 9 additions & 7 deletions orchagent/qosorch.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -840,7 +840,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;
Expand All @@ -867,10 +867,11 @@ bool DscpToFcMapHandler::convertFieldValuesToAttributes(KeyOpFieldsValuesTuple &
}
list_attr.value.qosmap.list[ind].key.dscp = static_cast<sai_uint8_t>(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;
}
Expand Down Expand Up @@ -933,7 +934,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;
Expand All @@ -960,10 +961,11 @@ bool ExpToFcMapHandler::convertFieldValuesToAttributes(KeyOpFieldsValuesTuple &t
}
list_attr.value.qosmap.list[ind].key.mpls_exp = static_cast<sai_uint8_t>(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;
}
Expand Down Expand Up @@ -1692,7 +1694,7 @@ task_process_status QosOrch::handlePortQosMapTable(Consumer& consumer)
}

sai_uint8_t old_pfc_enable = 0;
if (!gPortsOrch->getPortPfc(port.m_port_id, &old_pfc_enable))
if (!gPortsOrch->getPortPfc(port.m_port_id, &old_pfc_enable))
{
SWSS_LOG_ERROR("Failed to retrieve PFC bits on port %s", port_name.c_str());
}
Expand Down
2 changes: 1 addition & 1 deletion tests/test_nhg.py
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
10 changes: 5 additions & 5 deletions tests/test_qos_map.py
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand All @@ -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)

Expand All @@ -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
]
Expand Down Expand Up @@ -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
]
Expand Down Expand Up @@ -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()
Expand Down

0 comments on commit 45e446d

Please sign in to comment.