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

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"