Skip to content

Commit 9f2e27b

Browse files
stephenxsstephenxs
authored
[QoS] Fix issue: the WRED profile can not be set if current min > new max or current max < new min (#2379)
What I did Fix issue: Setting a WRED profile can fail in case the current min threshold is greater than the new max threshold or the current max threshold is less than the new min threshold for any color and at any time. Eg. Current min=1M, max=2M, new min=3M, new max=4M The min threshold is set first, so current max (2M) < new min (3M), which violates the condition This is because there can be only one attribute in each SAI SET operation, which means the vendor SAI does not understand the whole information of the new attributes and can only perform the sanity check against each SET operation. Signed-off-by: Stephen Sun [email protected] Why I did it Fix the issue How I verified it Manual test and mock test. Details if related The fix The thresholds that have been applied to SAI will be stored in orchagent. The original logic is to handle each attribute to be set and append it to an attribute list. To resolve the issue, a deferred attribute list is introduced and will be appended to the original attribute list after all the attributes have been handled. In the new logic, each threshold to be set will be checked against the stored data. In case it violates the condition, the violating attribute will be deferred, done via putting it into the deferred attributes list. For any color, there can be only 1 threshold violating the condition. Otherwise, it means both new min > old max and new max > old min, which means either old max < old min or new max < new min, which means either old or new data is illegal. This can not happen because illegal data will not be applied and stored. By doing so, the other threshold will be applied first, which extends the threshold range and breaks the violating condition. A logic is also introduced to guarantee the min threshold is always less than the max threshold in the new profile to be set. For the above example, In the new logic, the min threshold will be deferred so the new max threshold will be applied first and then the new min is applied. In this flow, there is no violation at any time. min = 1M, max = 2M => min = 1M, max = 4M => min = 3M, max = 4M
1 parent 1e1438e commit 9f2e27b

File tree

4 files changed

+409
-19
lines changed

4 files changed

+409
-19
lines changed

‎orchagent/qosorch.cpp

+123-18
Original file line numberDiff line numberDiff line change
@@ -489,47 +489,140 @@ bool WredMapHandler::convertBool(string str, bool &val)
489489
return true;
490490
}
491491

492+
void WredMapHandler::appendThresholdToAttributeList(sai_attr_id_t type,
493+
sai_uint32_t threshold,
494+
bool needDefer,
495+
vector<sai_attribute_t> &normalQueue,
496+
vector<sai_attribute_t> &deferredQueue,
497+
sai_uint32_t &newThreshold)
498+
{
499+
sai_attribute_t attr;
500+
501+
attr.id = type;
502+
attr.value.u32 = threshold;
503+
if (needDefer)
504+
{
505+
deferredQueue.push_back(attr);
506+
}
507+
else
508+
{
509+
normalQueue.push_back(attr);
510+
}
511+
newThreshold = threshold;
512+
}
513+
514+
WredMapHandler::qos_wred_thresholds_store_t WredMapHandler::m_wredProfiles;
515+
492516
bool WredMapHandler::convertFieldValuesToAttributes(KeyOpFieldsValuesTuple &tuple, vector<sai_attribute_t> &attribs)
493517
{
494518
SWSS_LOG_ENTER();
495519
sai_attribute_t attr;
520+
vector<sai_attribute_t> deferred_attributes;
521+
auto &key = kfvKey(tuple);
522+
auto &storedProfile = WredMapHandler::m_wredProfiles[key];
523+
qos_wred_thresholds_t currentProfile = storedProfile;
524+
sai_uint32_t threshold;
525+
526+
/*
527+
* Setting WRED profile can fail in case
528+
* - the current min threshold is greater than the new max threshold
529+
* - or the current max threshold is less than the new min threshold
530+
* for any color at any time, on some vendor's platforms.
531+
*
532+
* The root cause
533+
* There can be only one attribute in each SAI SET operation, which means
534+
* the vendor SAI do not have a big picture regarding what attributes are being set
535+
* and can only perform the sanity check against each SET operation.
536+
* In the above case, the sanity check will fail.
537+
*
538+
* The fix
539+
* The thresholds that have been applied to SAI will be stored in orchagent.
540+
*
541+
* The original logic is to handle each attribute to be set and append it to an attribute list.
542+
* To resolve the issue, a 2nd half attribute list is introduced and
543+
* will be appended to the original attribute list after all the attributes have been handled.
544+
*
545+
* In the new logic, each threshold to be set will be checked against the stored data.
546+
* In case it violates the condition, the violating attribute will be deferred, done via putting it into the 2nd half attributes list.
547+
*
548+
* For any color, there can be only 1 threshold violating the condition.
549+
* Otherwise, it means both new min > old max and new max > old min, which means either old max < old min or new max < new min,
550+
* which means either old or new data is illegal.
551+
* This can not happen because illegal data can not be applied and stored.
552+
*
553+
* By doing so, the other threshold will be applied first, which extends the threshold range and breaks the violating condition.
554+
* A logic is also introduced to guarantee the min threshold is always less than the max threshold in the new profile to be set.
555+
*
556+
* For example:
557+
* Current min=1M, max=2M, new min=3M, new max=4M
558+
* The min is set first, so current max (2M) < new min (3M), which violates the condition
559+
* By the new logic, min threshold will be deferred so the new max will be applied first and then the new min is applied and no violating.
560+
* min = 1M, max = 2M
561+
* => min = 1M, max = 4M
562+
* => min = 3M, max = 4M
563+
*/
564+
496565
for (auto i = kfvFieldsValues(tuple).begin(); i != kfvFieldsValues(tuple).end(); i++)
497566
{
498567
if (fvField(*i) == yellow_max_threshold_field_name)
499568
{
500-
attr.id = SAI_WRED_ATTR_YELLOW_MAX_THRESHOLD;
501-
attr.value.s32 = stoi(fvValue(*i));
502-
attribs.push_back(attr);
569+
threshold = stoi(fvValue(*i));
570+
appendThresholdToAttributeList(SAI_WRED_ATTR_YELLOW_MAX_THRESHOLD,
571+
threshold,
572+
(storedProfile.yellow_min_threshold > threshold),
573+
attribs,
574+
deferred_attributes,
575+
currentProfile.yellow_max_threshold);
503576
}
504577
else if (fvField(*i) == yellow_min_threshold_field_name)
505578
{
506-
attr.id = SAI_WRED_ATTR_YELLOW_MIN_THRESHOLD;
507-
attr.value.s32 = stoi(fvValue(*i));
508-
attribs.push_back(attr);
579+
threshold = stoi(fvValue(*i));
580+
appendThresholdToAttributeList(SAI_WRED_ATTR_YELLOW_MIN_THRESHOLD,
581+
threshold,
582+
(storedProfile.yellow_max_threshold < threshold),
583+
attribs,
584+
deferred_attributes,
585+
currentProfile.yellow_min_threshold);
509586
}
510587
else if (fvField(*i) == green_max_threshold_field_name)
511588
{
512-
attr.id = SAI_WRED_ATTR_GREEN_MAX_THRESHOLD;
513-
attr.value.s32 = stoi(fvValue(*i));
514-
attribs.push_back(attr);
589+
threshold = stoi(fvValue(*i));
590+
appendThresholdToAttributeList(SAI_WRED_ATTR_GREEN_MAX_THRESHOLD,
591+
threshold,
592+
(storedProfile.green_min_threshold > threshold),
593+
attribs,
594+
deferred_attributes,
595+
currentProfile.green_max_threshold);
515596
}
516597
else if (fvField(*i) == green_min_threshold_field_name)
517598
{
518-
attr.id = SAI_WRED_ATTR_GREEN_MIN_THRESHOLD;
519-
attr.value.s32 = stoi(fvValue(*i));
520-
attribs.push_back(attr);
599+
threshold = stoi(fvValue(*i));
600+
appendThresholdToAttributeList(SAI_WRED_ATTR_GREEN_MIN_THRESHOLD,
601+
threshold,
602+
(storedProfile.green_max_threshold < threshold),
603+
attribs,
604+
deferred_attributes,
605+
currentProfile.green_min_threshold);
521606
}
522607
else if (fvField(*i) == red_max_threshold_field_name)
523608
{
524-
attr.id = SAI_WRED_ATTR_RED_MAX_THRESHOLD;
525-
attr.value.s32 = stoi(fvValue(*i));
526-
attribs.push_back(attr);
609+
threshold = stoi(fvValue(*i));
610+
appendThresholdToAttributeList(SAI_WRED_ATTR_RED_MAX_THRESHOLD,
611+
threshold,
612+
(storedProfile.red_min_threshold > threshold),
613+
attribs,
614+
deferred_attributes,
615+
currentProfile.red_max_threshold);
527616
}
528617
else if (fvField(*i) == red_min_threshold_field_name)
529618
{
530-
attr.id = SAI_WRED_ATTR_RED_MIN_THRESHOLD;
531-
attr.value.s32 = stoi(fvValue(*i));
532-
attribs.push_back(attr);
619+
threshold = stoi(fvValue(*i));
620+
appendThresholdToAttributeList(SAI_WRED_ATTR_RED_MIN_THRESHOLD,
621+
threshold,
622+
(storedProfile.red_max_threshold < threshold),
623+
attribs,
624+
deferred_attributes,
625+
currentProfile.red_min_threshold);
533626
}
534627
else if (fvField(*i) == green_drop_probability_field_name)
535628
{
@@ -588,6 +681,18 @@ bool WredMapHandler::convertFieldValuesToAttributes(KeyOpFieldsValuesTuple &tupl
588681
return false;
589682
}
590683
}
684+
685+
if ((currentProfile.green_min_threshold > currentProfile.green_max_threshold)
686+
|| (currentProfile.yellow_min_threshold > currentProfile.yellow_max_threshold)
687+
|| (currentProfile.red_min_threshold > currentProfile.red_max_threshold))
688+
{
689+
SWSS_LOG_ERROR("Wrong wred profile: min threshold is greater than max threshold");
690+
return false;
691+
}
692+
693+
attribs.insert(attribs.end(), deferred_attributes.begin(), deferred_attributes.end());
694+
storedProfile = currentProfile;
695+
591696
return true;
592697
}
593698

‎orchagent/qosorch.h

+18
Original file line numberDiff line numberDiff line change
@@ -112,6 +112,24 @@ class WredMapHandler : public QosMapHandler
112112
protected:
113113
bool convertEcnMode(string str, sai_ecn_mark_mode_t &ecn_val);
114114
bool convertBool(string str, bool &val);
115+
private:
116+
void appendThresholdToAttributeList(sai_attr_id_t type,
117+
sai_uint32_t threshold,
118+
bool needDefer,
119+
vector<sai_attribute_t> &normalQueue,
120+
vector<sai_attribute_t> &deferredQueue,
121+
sai_uint32_t &newThreshold);
122+
typedef struct {
123+
sai_uint32_t green_max_threshold;
124+
sai_uint32_t green_min_threshold;
125+
sai_uint32_t yellow_max_threshold;
126+
sai_uint32_t yellow_min_threshold;
127+
sai_uint32_t red_max_threshold;
128+
sai_uint32_t red_min_threshold;
129+
} qos_wred_thresholds_t;
130+
typedef map<string, qos_wred_thresholds_t> qos_wred_thresholds_store_t;
131+
132+
static qos_wred_thresholds_store_t m_wredProfiles;
115133
};
116134

117135

‎tests/mock_tests/mock_orchagent_main.h

+1-1
Original file line numberDiff line numberDiff line change
@@ -12,8 +12,8 @@
1212
#include "mirrororch.h"
1313
#define private public
1414
#include "bufferorch.h"
15-
#undef private
1615
#include "qosorch.h"
16+
#undef private
1717
#include "vrforch.h"
1818
#include "vnetorch.h"
1919
#include "vxlanorch.h"

0 commit comments

Comments
 (0)