[QoS] Fix issue: the WRED profile can not be set if current min > new…

… max or current max < new min (sonic-net#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 stephens@nvidia.com

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

orchagent/qosorch.cpp

@@ -489,47 +489,140 @@ bool WredMapHandler::convertBool(string str, bool &val)
     return true;
 }
 
+void WredMapHandler::appendThresholdToAttributeList(sai_attr_id_t type,
+                                                    sai_uint32_t threshold,
+                                                    bool needDefer,
+                                                    vector<sai_attribute_t> &normalQueue,
+                                                    vector<sai_attribute_t> &deferredQueue,
+                                                    sai_uint32_t &newThreshold)
+{
+    sai_attribute_t attr;
+
+    attr.id = type;
+    attr.value.u32 = threshold;
+    if (needDefer)
+    {
+        deferredQueue.push_back(attr);
+    }
+    else
+    {
+        normalQueue.push_back(attr);
+    }
+    newThreshold = threshold;
+}
+
+WredMapHandler::qos_wred_thresholds_store_t WredMapHandler::m_wredProfiles;
+
 bool WredMapHandler::convertFieldValuesToAttributes(KeyOpFieldsValuesTuple &tuple, vector<sai_attribute_t> &attribs)
 {
     SWSS_LOG_ENTER();
     sai_attribute_t attr;
+    vector<sai_attribute_t> deferred_attributes;
+    auto &key = kfvKey(tuple);
+    auto &storedProfile = WredMapHandler::m_wredProfiles[key];
+    qos_wred_thresholds_t currentProfile = storedProfile;
+    sai_uint32_t threshold;
+
+    /*
+     * Setting 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 at any time, on some vendor's platforms.
+     *
+     * The root cause
+     * There can be only one attribute in each SAI SET operation, which means
+     * the vendor SAI do not have a big picture regarding what attributes are being set
+     * and can only perform the sanity check against each SET operation.
+     * In the above case, the sanity check will fail.
+     *
+     * 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 2nd half 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 2nd half 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 can 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 example:
+     * Current min=1M, max=2M, new min=3M, new max=4M
+     * The min is set first, so current max (2M) < new min (3M), which violates the condition
+     * 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.
+     *  min = 1M, max = 2M
+     *  => min = 1M, max = 4M
+     *  => min = 3M, max = 4M
+     */
+
     for (auto i = kfvFieldsValues(tuple).begin(); i != kfvFieldsValues(tuple).end(); i++)
     {
         if (fvField(*i) == yellow_max_threshold_field_name)
         {
-            attr.id = SAI_WRED_ATTR_YELLOW_MAX_THRESHOLD;
-            attr.value.s32 = stoi(fvValue(*i));
-            attribs.push_back(attr);
+            threshold = stoi(fvValue(*i));
+            appendThresholdToAttributeList(SAI_WRED_ATTR_YELLOW_MAX_THRESHOLD,
+                                           threshold,
+                                           (storedProfile.yellow_min_threshold > threshold),
+                                           attribs,
+                                           deferred_attributes,
+                                           currentProfile.yellow_max_threshold);
         }
         else if (fvField(*i) == yellow_min_threshold_field_name)
         {
-            attr.id = SAI_WRED_ATTR_YELLOW_MIN_THRESHOLD;
-            attr.value.s32 = stoi(fvValue(*i));
-            attribs.push_back(attr);
+            threshold = stoi(fvValue(*i));
+            appendThresholdToAttributeList(SAI_WRED_ATTR_YELLOW_MIN_THRESHOLD,
+                                           threshold,
+                                           (storedProfile.yellow_max_threshold < threshold),
+                                           attribs,
+                                           deferred_attributes,
+                                           currentProfile.yellow_min_threshold);
         }
         else if (fvField(*i) == green_max_threshold_field_name)
         {
-            attr.id = SAI_WRED_ATTR_GREEN_MAX_THRESHOLD;
-            attr.value.s32 = stoi(fvValue(*i));
-            attribs.push_back(attr);
+            threshold = stoi(fvValue(*i));
+            appendThresholdToAttributeList(SAI_WRED_ATTR_GREEN_MAX_THRESHOLD,
+                                           threshold,
+                                           (storedProfile.green_min_threshold > threshold),
+                                           attribs,
+                                           deferred_attributes,
+                                           currentProfile.green_max_threshold);
         }
         else if (fvField(*i) == green_min_threshold_field_name)
         {
-           attr.id = SAI_WRED_ATTR_GREEN_MIN_THRESHOLD;
-           attr.value.s32 = stoi(fvValue(*i));
-           attribs.push_back(attr);
+            threshold = stoi(fvValue(*i));
+            appendThresholdToAttributeList(SAI_WRED_ATTR_GREEN_MIN_THRESHOLD,
+                                           threshold,
+                                           (storedProfile.green_max_threshold < threshold),
+                                           attribs,
+                                           deferred_attributes,
+                                           currentProfile.green_min_threshold);
         }
         else if (fvField(*i) == red_max_threshold_field_name)
         {
-            attr.id = SAI_WRED_ATTR_RED_MAX_THRESHOLD;
-            attr.value.s32 = stoi(fvValue(*i));
-            attribs.push_back(attr);
+            threshold = stoi(fvValue(*i));
+            appendThresholdToAttributeList(SAI_WRED_ATTR_RED_MAX_THRESHOLD,
+                                           threshold,
+                                           (storedProfile.red_min_threshold > threshold),
+                                           attribs,
+                                           deferred_attributes,
+                                           currentProfile.red_max_threshold);
         }
         else if (fvField(*i) == red_min_threshold_field_name)
         {
-            attr.id = SAI_WRED_ATTR_RED_MIN_THRESHOLD;
-            attr.value.s32 = stoi(fvValue(*i));
-            attribs.push_back(attr);
+            threshold = stoi(fvValue(*i));
+            appendThresholdToAttributeList(SAI_WRED_ATTR_RED_MIN_THRESHOLD,
+                                           threshold,
+                                           (storedProfile.red_max_threshold < threshold),
+                                           attribs,
+                                           deferred_attributes,
+                                           currentProfile.red_min_threshold);
         }
         else if (fvField(*i) == green_drop_probability_field_name)
         {
@@ -588,6 +681,18 @@ bool WredMapHandler::convertFieldValuesToAttributes(KeyOpFieldsValuesTuple &tupl
             return false;
         }
     }
+
+    if ((currentProfile.green_min_threshold > currentProfile.green_max_threshold)
+        || (currentProfile.yellow_min_threshold > currentProfile.yellow_max_threshold)
+        || (currentProfile.red_min_threshold > currentProfile.red_max_threshold))
+    {
+        SWSS_LOG_ERROR("Wrong wred profile: min threshold is greater than max threshold");
+        return false;
+    }
+
+    attribs.insert(attribs.end(), deferred_attributes.begin(), deferred_attributes.end());
+    storedProfile = currentProfile;
+
     return true;
 }
 

orchagent/qosorch.h

@@ -112,6 +112,24 @@ class WredMapHandler : public QosMapHandler
 protected:
     bool convertEcnMode(string str, sai_ecn_mark_mode_t &ecn_val);
     bool convertBool(string str, bool &val);
+private:
+    void appendThresholdToAttributeList(sai_attr_id_t type,
+                                        sai_uint32_t threshold,
+                                        bool needDefer,
+                                        vector<sai_attribute_t> &normalQueue,
+                                        vector<sai_attribute_t> &deferredQueue,
+                                        sai_uint32_t &newThreshold);
+    typedef struct {
+        sai_uint32_t green_max_threshold;
+        sai_uint32_t green_min_threshold;
+        sai_uint32_t yellow_max_threshold;
+        sai_uint32_t yellow_min_threshold;
+        sai_uint32_t red_max_threshold;
+        sai_uint32_t red_min_threshold;
+    } qos_wred_thresholds_t;
+    typedef map<string, qos_wred_thresholds_t> qos_wred_thresholds_store_t;
+
+    static qos_wred_thresholds_store_t m_wredProfiles;
 };
 
 

tests/mock_tests/mock_orchagent_main.h

@@ -12,8 +12,8 @@
 #include "mirrororch.h"
 #define private public
 #include "bufferorch.h"
-#undef private
 #include "qosorch.h"
+#undef private
 #include "vrforch.h"
 #include "vnetorch.h"
 #include "vxlanorch.h"