Skip to content

Commit 45e446d

Browse files
abanu-msabanu-ms
authored
[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
1 parent b1b5b29 commit 45e446d

File tree

6 files changed

+30
-28
lines changed

6 files changed

+30
-28
lines changed

‎orchagent/cbf/cbfnhgorch.cpp‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -308,7 +308,7 @@ bool CbfNhg::sync()
308308
nhg_attr.value.u32 = static_cast<sai_uint32_t>(m_members.size());
309309
nhg_attrs.push_back(move(nhg_attr));
310310

311-
if (nhg_attr.value.u32 > gNhgMapOrch->getMaxFcVal())
311+
if (nhg_attr.value.u32 > gNhgMapOrch->getMaxNumFcs())
312312
{
313313
/* If there are more members than FCs then this may be an error, as some members won't be used. */
314314
SWSS_LOG_WARN("More CBF NHG members configured than supported Forwarding Classes");

‎orchagent/cbf/nhgmaporch.cpp‎

Lines changed: 12 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -294,34 +294,34 @@ void NhgMapOrch::decRefCount(const string &index)
294294
}
295295

296296
/*
297-
* Get the max FC value supported by the switch.
297+
* Get the maximum number of FC classes supported by the switch.
298298
*/
299-
sai_uint8_t NhgMapOrch::getMaxFcVal()
299+
sai_uint8_t NhgMapOrch::getMaxNumFcs()
300300
{
301301
SWSS_LOG_ENTER();
302302

303-
static int max_fc_val = -1;
303+
static int max_num_fcs = -1;
304304

305305
/*
306-
* Get the maximum value allowed for FC if it wasn't already initialized.
306+
* Get the maximum number of FC classes if it wasn't already initialized.
307307
*/
308-
if (max_fc_val == -1)
308+
if (max_num_fcs == -1)
309309
{
310310
sai_attribute_t attr;
311311
attr.id = SAI_SWITCH_ATTR_MAX_NUMBER_OF_FORWARDING_CLASSES;
312312

313313
if (sai_switch_api->get_switch_attribute(gSwitchId, 1, &attr) == SAI_STATUS_SUCCESS)
314314
{
315-
max_fc_val = attr.value.u8;
315+
max_num_fcs = attr.value.u8;
316316
}
317317
else
318318
{
319319
SWSS_LOG_WARN("Switch does not support FCs");
320-
max_fc_val = 0;
320+
max_num_fcs = 0;
321321
}
322322
}
323323

324-
return static_cast<sai_uint8_t>(max_fc_val);
324+
return static_cast<sai_uint8_t>(max_num_fcs);
325325
}
326326

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

345345
unordered_map<sai_uint32_t, sai_int32_t> fc_map;
346-
sai_uint8_t max_fc_val = getMaxFcVal();
346+
sai_uint8_t max_num_fcs = getMaxNumFcs();
347347

348348
/*
349349
* Create the map while validating that the values are positive
@@ -353,13 +353,13 @@ pair<bool, unordered_map<sai_uint32_t, sai_int32_t>> NhgMapOrch::getMap(const ve
353353
try
354354
{
355355
/*
356-
* Check the FC value is valid.
356+
* Check the FC value is valid. FC value must be in range [0, max_num_fcs).
357357
*/
358358
auto fc = stoi(fvField(*it));
359359

360-
if ((fc < 0) || (fc > max_fc_val))
360+
if ((fc < 0) || (fc >= max_num_fcs))
361361
{
362-
SWSS_LOG_ERROR("FC value %d is either negative or greater than max value %d", fc, max_fc_val);
362+
SWSS_LOG_ERROR("FC value %d is either negative or greater than max value %d", fc, max_num_fcs - 1);
363363
success = false;
364364
break;
365365
}

‎orchagent/cbf/nhgmaporch.h‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -43,9 +43,9 @@ class NhgMapOrch : public Orch
4343
void decRefCount(const string &key);
4444

4545
/*
46-
* Get the max FC value supported by the switch.
46+
* Get the maximum number of FC classes supported by the switch.
4747
*/
48-
static sai_uint8_t getMaxFcVal();
48+
static sai_uint8_t getMaxNumFcs();
4949

5050
private:
5151
/*

‎orchagent/qosorch.cpp‎

Lines changed: 9 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -840,7 +840,7 @@ bool DscpToFcMapHandler::convertFieldValuesToAttributes(KeyOpFieldsValuesTuple &
840840
{
841841
SWSS_LOG_ENTER();
842842

843-
sai_uint8_t max_fc_val = NhgMapOrch::getMaxFcVal();
843+
sai_uint8_t max_num_fcs = NhgMapOrch::getMaxNumFcs();
844844

845845
sai_attribute_t list_attr;
846846
list_attr.id = SAI_QOS_MAP_ATTR_MAP_TO_VALUE_LIST;
@@ -867,10 +867,11 @@ bool DscpToFcMapHandler::convertFieldValuesToAttributes(KeyOpFieldsValuesTuple &
867867
}
868868
list_attr.value.qosmap.list[ind].key.dscp = static_cast<sai_uint8_t>(value);
869869

870+
// FC value must be in range [0, max_num_fcs)
870871
value = stoi(fvValue(*i));
871-
if ((value < 0) || (value > max_fc_val))
872+
if ((value < 0) || (value >= max_num_fcs))
872873
{
873-
SWSS_LOG_ERROR("FC value %d is either negative, or bigger than max value %d", value, max_fc_val);
874+
SWSS_LOG_ERROR("FC value %d is either negative, or bigger than max value %d", value, max_num_fcs - 1);
874875
delete[] list_attr.value.qosmap.list;
875876
return false;
876877
}
@@ -933,7 +934,7 @@ bool ExpToFcMapHandler::convertFieldValuesToAttributes(KeyOpFieldsValuesTuple &t
933934
{
934935
SWSS_LOG_ENTER();
935936

936-
sai_uint8_t max_fc_val = NhgMapOrch::getMaxFcVal();
937+
sai_uint8_t max_num_fcs = NhgMapOrch::getMaxNumFcs();
937938

938939
sai_attribute_t list_attr;
939940
list_attr.id = SAI_QOS_MAP_ATTR_MAP_TO_VALUE_LIST;
@@ -960,10 +961,11 @@ bool ExpToFcMapHandler::convertFieldValuesToAttributes(KeyOpFieldsValuesTuple &t
960961
}
961962
list_attr.value.qosmap.list[ind].key.mpls_exp = static_cast<sai_uint8_t>(value);
962963

964+
// FC value must be in range [0, max_num_fcs)
963965
value = stoi(fvValue(*i));
964-
if ((value < 0) || (value > max_fc_val))
966+
if ((value < 0) || (value >= max_num_fcs))
965967
{
966-
SWSS_LOG_ERROR("FC value %d is either negative, or bigger than max value %hu", value, max_fc_val);
968+
SWSS_LOG_ERROR("FC value %d is either negative, or bigger than max value %hu", value, max_num_fcs - 1);
967969
delete[] list_attr.value.qosmap.list;
968970
return false;
969971
}
@@ -1692,7 +1694,7 @@ task_process_status QosOrch::handlePortQosMapTable(Consumer& consumer)
16921694
}
16931695

16941696
sai_uint8_t old_pfc_enable = 0;
1695-
if (!gPortsOrch->getPortPfc(port.m_port_id, &old_pfc_enable))
1697+
if (!gPortsOrch->getPortPfc(port.m_port_id, &old_pfc_enable))
16961698
{
16971699
SWSS_LOG_ERROR("Failed to retrieve PFC bits on port %s", port_name.c_str());
16981700
}

‎tests/test_nhg.py‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1980,7 +1980,7 @@ def data_validation_test():
19801980
# Test validation errors
19811981
nhg_maps = [
19821982
('-1', '0'), # negative FC
1983-
('64', '0'), # greater than max FC value
1983+
('63', '0'), # greater than max FC value
19841984
('a', '0'), # non-integer FC
19851985
('0', '-1'), # negative NH index
19861986
('0', 'a'), # non-integer NH index

‎tests/test_qos_map.py‎

Lines changed: 5 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -139,7 +139,7 @@ def test_dscp_to_fc(self, dvs):
139139
self.init_test(dvs)
140140

141141
# Create a DSCP_TO_FC map
142-
dscp_map = [(str(i), str(i)) for i in range(0, 64)]
142+
dscp_map = [(str(i), str(i)) for i in range(0, 63)]
143143
self.dscp_ps.set("AZURE", swsscommon.FieldValuePairs(dscp_map))
144144

145145
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):
153153
assert(fvs.get("SAI_QOS_MAP_ATTR_TYPE") == "SAI_QOS_MAP_TYPE_DSCP_TO_FORWARDING_CLASS")
154154

155155
# Modify the map
156-
dscp_map = [(str(i), '0') for i in range(0, 64)]
156+
dscp_map = [(str(i), '0') for i in range(0, 63)]
157157
self.dscp_ps.set("AZURE", swsscommon.FieldValuePairs(dscp_map))
158158
time.sleep(1)
159159

@@ -174,7 +174,7 @@ def test_dscp_to_fc(self, dvs):
174174
('-1', '0'), # negative DSCP
175175
('64', '0'), # DSCP greater than max value
176176
('0', '-1'), # negative FC
177-
('0', '64'), # FC greater than max value
177+
('0', '63'), # FC greater than max value
178178
('a', '0'), # non-integer DSCP
179179
('0', 'a'), # non-integet FC
180180
]
@@ -228,7 +228,7 @@ def test_exp_to_fc(self, dvs):
228228
('-1', '0'), # negative EXP
229229
('8', '0'), # EXP greater than max value
230230
('0', '-1'), # negative FC
231-
('0', '64'), # FC greater than max value
231+
('0', '63'), # FC greater than max value
232232
('a', '0'), # non-integer EXP
233233
('0', 'a'), # non-integet FC
234234
]
@@ -258,7 +258,7 @@ def test_per_port_cbf_binding(self, dvs):
258258
self.init_test(dvs)
259259

260260
# Create a DSCP_TO_FC map
261-
dscp_map = [(str(i), str(i)) for i in range(0, 64)]
261+
dscp_map = [(str(i), str(i)) for i in range(0, 63)]
262262
self.dscp_ps.set("AZURE", swsscommon.FieldValuePairs(dscp_map))
263263
self.asic_db.wait_for_n_keys(self.ASIC_QOS_MAP_STR, self.asic_qos_map_count + 1)
264264
dscp_map_id = self.get_qos_id()

0 commit comments

Comments
 (0)