[watermarkorch] only perform periodic clear if the polling is on #781

mykolaf · 2019-02-04T15:11:47Z

Signed-off-by: Mykola Faryma mykolaf@mellanox.com
No need to periodically clear watermarks if the polling is disabled.

What I did
Changes:

make watermarkorch aware of Flex Counter config
enable/disable periodic timer based on Flex Counter status of watermarks.
Why I did it

How I verified it

 # config watermark telemetry interval 10

Apr 25 13:02:21.782115 sonic DEBUG swss#orchagent: :- doTask: Periodic watermark cleared by timer!
Apr 25 13:02:31.762604 sonic DEBUG swss#orchagent: :- doTask: Periodic watermark cleared by timer!
Apr 25 13:02:41.772739 sonic DEBUG swss#orchagent: :- doTask: Periodic watermark cleared by timer!

 # config watermark telemetry interval 15

Apr 25 13:02:51.756292 sonic DEBUG swss#orchagent: :- doTask: Periodic watermark cleared by timer!
Apr 25 13:03:06.765902 sonic DEBUG swss#orchagent: :- doTask: Periodic watermark cleared by timer!

 # counterpoll watermark disable

Apr 25 13:03:21.743069 sonic DEBUG swss#orchagent: :- doTask: Periodic watermark cleared by timer!

 # counterpoll watermark enable

Apr 25 13:07:31.556155 sonic DEBUG swss#orchagent: :- doTask: Periodic watermark cleared by timer!

 # counterpoll watermark enable
 # counterpoll watermark enable
 # counterpoll watermark enable

Apr 25 13:07:46.547644 sonic DEBUG swss#orchagent: :- doTask: Periodic watermark cleared by timer!

Details if related

orchagent/watermarkorch.cpp

wendani · 2019-03-02T08:19:03Z

orchagent/watermarkorch.cpp

+                m_telemetryTimer->start();
+                m_isTimerEnabled = true;
+            }
+            else if (i.first == "FLEX_COUNTER_STATUS" && i.second == "disable" && m_isTimerEnabled == true)


If "FLEX_COUNTER_STATUS" of the first processed key, say PG_WATERMARK, is "enable", while the second "QUEUE_WATERMARK" is "disable", the telemetryTimer will not start.

@mykolaf can you fix the logic?

wendani · 2019-03-14T06:51:02Z

orchagent/watermarkorch.cpp

+                m_telemetryTimer->start();
+                m_isTimerEnabled = true;
+            }
+            else if (i.first == "FLEX_COUNTER_STATUS" && i.second == "disable" && m_isTimerEnabled == true)


@mykolaf can you fix the logic?

wendani · 2019-03-20T02:24:24Z

orchagent/watermarkorch.h

 private:
-    bool m_isTimerEnabled = false;
+    bool m_qWmEnabled = false;
+    bool m_pgWmEnabled = false;



Instead of defining one Boolean variable for each watermark category, we may choose to use only one uint8_t variable, with each category being a bit set in it, up to 8 categories.

Now status is stored as a uint8_t

wendani · 2019-03-20T03:39:39Z

orchagent/watermarkorch.cpp

+            if (i.first == "interval")
+            {
+                auto intervT = timespec { .tv_sec = to_uint<uint32_t>(i.second.c_str()) , .tv_nsec = 0 };
+                m_telemetryTimer->setInterval(intervT);


I just checked the implementation of setInterval(), https://github.com/Azure/sonic-swss-common/blob/master/common/selectabletimer.cpp#L63. It only changes the value of the itimerspec struct, not sync the value to the timer. If the timer is not armed, it will not start the timer. This is good as we may not want to start the timer, but only want to change the interval. If the timer is armed, we will need a call to timerfd_settime(), which is called by SelectableTimer::start() and SelectableTimer::reset(), to sync the actual change to the timer. This means the change will take effect at a later time when the current timer expires and the SelectableTimer doTask handler is called. If we do not care about the interval to take immediate effect. This is also fine.

The timer is expected to be changed after current interval has ended. This is stated in the design.

OK, let's follow the design for now. Then, we need a call to sync the telemetry interval change to the actual timer in the doTask(SelectableTimer &timer) if we see there is a change in the telemetry interval

wendani · 2019-03-20T03:49:25Z

orchagent/watermarkorch.cpp

+         * if it is disabled we will not restart the timer at the end of current interval */
+        if (isTimerEnabled())
+        {
+            m_telemetryTimer->reset();


The struct timerspec it_interval field of timerfd_settime(), when calling SelectableTimer::start(), is not zero, so the timer will re-arm automatically. If telemetry is enabled, we do not need to call SelectableTimer::reset() in the SelectableTimer doTask handler, because the timer will automatically re-arm. Your comment on line 210 looks wrong. Instead, we need to deal with the opposite case---if the telemetry is disabled, we need to explicitly call SelectableTimer::stop() (, which is made in the handleFcConfigUpdate?). Have you tested your change?

wendani · 2019-03-20T04:11:29Z

orchagent/watermarkorch.cpp

+        }
+        if (isTimerEnabled())
+        {
+            m_telemetryTimer->start();


If the telemetry timer has been started, a call to SelectableTimer::start() will reset the timer settings. This resets the current time to expiration, causing the intervals to be uneven that the interval length, where an enable cli issued, are different from others. We should refrain from calling SelectableTimer::start() if there is no change to the timer interval value itself.

Added logic to avoid uneven intervals.

wendani · 2019-04-15T22:23:46Z

orchagent/watermarkorch.cpp

@@ -170,16 +207,18 @@ void WatermarkOrch::doTask(SelectableTimer &timer)



For line 198 to 206, how can you be so sure that periodic timer kicks in first before user issues the clear watermark through cli? That m_pg_ids, m_unicast_queue_ids, and m_multicast_queue_ids is already populated for the latter? Can you give your rationale behind?

The idea of introducing m_pg_ids, m_unicast_queue_ids, and m_multicast_queue_ids , I guess, is to have a cached copy of the oids at hand to remove the need of further queries into obtaining these oids for clear command. The only opportunity/condition to update this cached copy is when a watermark clear command is issued by timer and the doTask() handler finds that the vector is empty. So we assume here that when this update opportunity comes, the pg, uni_queue, and multi_queue oids are generated complete and will not be subject to any further changes.

The sources of the oids comes from the COUNTERS_DB *MAP table, which is set when FlexCounterOrch receives any signal on "FLEX_COUNTER_STATUS" field to call the generate*Map() function. This signal in the current orchagent initialization sequence, I guess, comes for granted after the port initialization is done, with all pg and queue oids being settled. So we will get a list of pg and queue oids complete and ready at the time the cached oid copy still hits the emptiness condition.

Also, to introduce cached copy, we need to evaluate the risk of running out-of-sync to the original source. I think it should be fine for pg and queue oids as they remain unchanged upon creation during the life cycle of SONiC running

The main idea of having m_pg_ids, m_unicast_queue_ids, m_multicast_queue_ids is to avoid querying them every time, performing deserialization of oids. This things are not expected to change.

wendani · 2019-04-17T16:52:12Z

This question is not on the PR per se. There are two sources that will set a TABLE in COUNTERS_DB, one from user/timer clear command and the other from the flex counter thread lus script execution. In this case, does redis provide a lock mechanism internally to ensure mutual exclusion on writing to a table? Also @qiluo-msft for insights

qiluo-msft · 2019-04-17T19:05:20Z

@wendani if the Redis written by single transactions, they are mutual exclusive.

Signed-off-by: Mykola Faryma <mykolaf@mellanox.com>

wendani · 2019-04-22T17:22:21Z

orchagent/watermarkorch.cpp

+            if (i.first == "interval")
+            {
+                auto intervT = timespec { .tv_sec = to_uint<uint32_t>(i.second.c_str()) , .tv_nsec = 0 };
+                m_telemetryTimer->setInterval(intervT);


OK, let's follow the design for now. Then, we need a call to sync the telemetry interval change to the actual timer in the doTask(SelectableTimer &timer) if we see there is a change in the telemetry interval

wendani · 2019-04-23T06:49:04Z

Synced offline with Qi. A single transaction is in terms of a hget call or a hset call.

The watermark lua script (b750a4b#diff-f8446e5c7632fc3a23e06c9f9419828c) updates a value in the typical 'read-modify-write' fashion as hget-compare-hset. A user or a timer clear, either directly issued through CLI or delegated to WatermarkOrch via the APPL_DB event channel, runs in a different context from the syncd flex counter thread. If the user clears the value in between the hget and hset, we can have a race condition situation.

One thing we can do in the lua script is to have a value's hset call follow as close as possible to its hget call, rather than hgetting all three values and then hsetting all three values if any update is needed. This may helping reduce the time interval between hget and hset, at least not introducing any more delay between the two operations, and alleviate the problem to some extent.

mykolaf · 2019-04-23T06:53:16Z

@wendani I can enhance the lua script in separate PR.

Signed-off-by: Mykola Faryma <mykolaf@mellanox.com>

liat-grozovik · 2019-04-24T14:54:13Z

@wendani the changes in the Lux script should go with the PR or a seperate PR? anything else from this PR is missing?

wendani

Approve on the condition that it is verified on mlnx dut before merge:

telemetry timer is auto rearmed
change of telemetry interval
enable/disable watermark polling, telemetry timer is started/stopped

mykolaf · 2019-04-25T13:18:13Z

@wendani Just doublechecked, updated the PR message. Ready to merge

* [watermarkorch] only perform periodic clear if the polling is on Signed-off-by: Mykola Faryma <mykolaf@mellanox.com> * [watermarkorch] only set timer interval on change Signed-off-by: Mykola Faryma <mykolaf@mellanox.com> * fix logic Signed-off-by: Mykola Faryma <mykolaf@mellanox.com> * fix comments Signed-off-by: Mykola Faryma <mykolaf@mellanox.com> * fix change timer Signed-off-by: Mykola Faryma <mykolaf@mellanox.com>

mykolaf force-pushed the wm_bugfix branch from 8287ae7 to 28da058 Compare February 5, 2019 09:58

lguohan requested a review from wendani February 5, 2019 20:39

wendani reviewed Feb 21, 2019

View reviewed changes

orchagent/watermarkorch.cpp Outdated Show resolved Hide resolved

wendani reviewed Mar 2, 2019

View reviewed changes

liat-grozovik added the Request for 201811 Branch label Mar 13, 2019

wendani suggested changes Mar 14, 2019

View reviewed changes

mykolaf force-pushed the wm_bugfix branch 3 times, most recently from 934d354 to 5a119d2 Compare March 18, 2019 20:59

wendani reviewed Mar 20, 2019

View reviewed changes

wendani approved these changes Mar 20, 2019

View reviewed changes

wendani reviewed Mar 20, 2019

View reviewed changes

wendani suggested changes Mar 20, 2019

View reviewed changes

wendani reviewed Apr 15, 2019

View reviewed changes

Mykola Faryma added 3 commits April 18, 2019 15:15

[watermarkorch] only perform periodic clear if the polling is on

dd3f3a1

Signed-off-by: Mykola Faryma <mykolaf@mellanox.com>

[watermarkorch] only set timer interval on change

1b6363b

Signed-off-by: Mykola Faryma <mykolaf@mellanox.com>

fix logic

53d9766

Signed-off-by: Mykola Faryma <mykolaf@mellanox.com>

mykolaf force-pushed the wm_bugfix branch from 5a119d2 to 48ee316 Compare April 19, 2019 14:16

fix comments

bc9a1e8

Signed-off-by: Mykola Faryma <mykolaf@mellanox.com>

mykolaf force-pushed the wm_bugfix branch from 48ee316 to bc9a1e8 Compare April 19, 2019 15:00

wendani suggested changes Apr 22, 2019

View reviewed changes

fix change timer

ef2232c

Signed-off-by: Mykola Faryma <mykolaf@mellanox.com>

wendani approved these changes Apr 24, 2019

View reviewed changes

volodymyrsamotiy approved these changes May 7, 2019

View reviewed changes

liat-grozovik merged commit f8792d5 into sonic-net:master May 7, 2019

yxieca added the Included in 201811 Branch label May 9, 2019

wendani mentioned this pull request Jun 20, 2019

Set timer only when interval changes. Not in each firing of the timer. #945

Merged

mykolaf deleted the wm_bugfix branch February 18, 2020 13:18

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[watermarkorch] only perform periodic clear if the polling is on #781

[watermarkorch] only perform periodic clear if the polling is on #781

mykolaf commented Feb 4, 2019 •

edited

Loading

wendani Mar 2, 2019

wendani Mar 14, 2019

wendani Mar 14, 2019

wendani Mar 20, 2019 •

edited

Loading

mykolaf Apr 19, 2019

wendani Mar 20, 2019 •

edited

Loading

mykolaf Apr 19, 2019

wendani Apr 22, 2019 •

edited

Loading

wendani Mar 20, 2019

wendani Mar 20, 2019 •

edited

Loading

mykolaf Apr 19, 2019

wendani Apr 15, 2019 •

edited

Loading

wendani Apr 17, 2019 •

edited

Loading

mykolaf Apr 19, 2019

wendani commented Apr 17, 2019

qiluo-msft commented Apr 17, 2019

wendani Apr 22, 2019 •

edited

Loading

wendani commented Apr 23, 2019 •

edited

Loading

mykolaf commented Apr 23, 2019

liat-grozovik commented Apr 24, 2019

wendani left a comment

mykolaf commented Apr 25, 2019

		@@ -170,16 +207,18 @@ void WatermarkOrch::doTask(SelectableTimer &timer)

[watermarkorch] only perform periodic clear if the polling is on #781

[watermarkorch] only perform periodic clear if the polling is on #781

Conversation

mykolaf commented Feb 4, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wendani Mar 20, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wendani Mar 20, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wendani Apr 22, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wendani Mar 20, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wendani Apr 15, 2019 • edited Loading

Choose a reason for hiding this comment

wendani Apr 17, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wendani commented Apr 17, 2019

qiluo-msft commented Apr 17, 2019

wendani Apr 22, 2019 • edited Loading

Choose a reason for hiding this comment

wendani commented Apr 23, 2019 • edited Loading

mykolaf commented Apr 23, 2019

liat-grozovik commented Apr 24, 2019

wendani left a comment

Choose a reason for hiding this comment

mykolaf commented Apr 25, 2019

mykolaf commented Feb 4, 2019 •

edited

Loading

wendani Mar 20, 2019 •

edited

Loading

wendani Mar 20, 2019 •

edited

Loading

wendani Apr 22, 2019 •

edited

Loading

wendani Mar 20, 2019 •

edited

Loading

wendani Apr 15, 2019 •

edited

Loading

wendani Apr 17, 2019 •

edited

Loading

wendani Apr 22, 2019 •

edited

Loading

wendani commented Apr 23, 2019 •

edited

Loading