Skip to content

Commit ec11131

Browse files
abanu-msTACappleman
abanu-ms
authored and
TACappleman
committed
[cbf] Fix max FC value (sonic-net#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 64fca5c commit ec11131

File tree

6 files changed

+29
-27
lines changed

6 files changed

+29
-27
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: 8 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -808,7 +808,7 @@ bool DscpToFcMapHandler::convertFieldValuesToAttributes(KeyOpFieldsValuesTuple &
808808
{
809809
SWSS_LOG_ENTER();
810810

811-
sai_uint8_t max_fc_val = NhgMapOrch::getMaxFcVal();
811+
sai_uint8_t max_num_fcs = NhgMapOrch::getMaxNumFcs();
812812

813813
sai_attribute_t list_attr;
814814
list_attr.id = SAI_QOS_MAP_ATTR_MAP_TO_VALUE_LIST;
@@ -835,10 +835,11 @@ bool DscpToFcMapHandler::convertFieldValuesToAttributes(KeyOpFieldsValuesTuple &
835835
}
836836
list_attr.value.qosmap.list[ind].key.dscp = static_cast<sai_uint8_t>(value);
837837

838+
// FC value must be in range [0, max_num_fcs)
838839
value = stoi(fvValue(*i));
839-
if ((value < 0) || (value > max_fc_val))
840+
if ((value < 0) || (value >= max_num_fcs))
840841
{
841-
SWSS_LOG_ERROR("FC value %d is either negative, or bigger than max value %d", value, max_fc_val);
842+
SWSS_LOG_ERROR("FC value %d is either negative, or bigger than max value %d", value, max_num_fcs - 1);
842843
delete[] list_attr.value.qosmap.list;
843844
return false;
844845
}
@@ -901,7 +902,7 @@ bool ExpToFcMapHandler::convertFieldValuesToAttributes(KeyOpFieldsValuesTuple &t
901902
{
902903
SWSS_LOG_ENTER();
903904

904-
sai_uint8_t max_fc_val = NhgMapOrch::getMaxFcVal();
905+
sai_uint8_t max_num_fcs = NhgMapOrch::getMaxNumFcs();
905906

906907
sai_attribute_t list_attr;
907908
list_attr.id = SAI_QOS_MAP_ATTR_MAP_TO_VALUE_LIST;
@@ -928,10 +929,11 @@ bool ExpToFcMapHandler::convertFieldValuesToAttributes(KeyOpFieldsValuesTuple &t
928929
}
929930
list_attr.value.qosmap.list[ind].key.mpls_exp = static_cast<sai_uint8_t>(value);
930931

932+
// FC value must be in range [0, max_num_fcs)
931933
value = stoi(fvValue(*i));
932-
if ((value < 0) || (value > max_fc_val))
934+
if ((value < 0) || (value >= max_num_fcs))
933935
{
934-
SWSS_LOG_ERROR("FC value %d is either negative, or bigger than max value %hu", value, max_fc_val);
936+
SWSS_LOG_ERROR("FC value %d is either negative, or bigger than max value %hu", value, max_num_fcs - 1);
935937
delete[] list_attr.value.qosmap.list;
936938
return false;
937939
}

‎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)