Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 5 commits into from
May 7, 2019
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
dd3f3a1
[watermarkorch] only perform periodic clear if the polling is on
Feb 4, 2019
1b6363b
[watermarkorch] only set timer interval on change
Feb 26, 2019
53d9766
fix logic
Mar 18, 2019
bc9a1e8
fix comments
Apr 18, 2019
ef2232c
fix change timer
Apr 24, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 6 additions & 1 deletion orchagent/orchdaemon.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -148,7 +148,12 @@ bool OrchDaemon::init()
CFG_DTEL_EVENT_TABLE_NAME
};

WatermarkOrch *wm_orch = new WatermarkOrch(m_configDb, CFG_WATERMARK_TABLE_NAME);
vector<string> wm_tables = {
CFG_WATERMARK_TABLE_NAME,
CFG_FLEX_COUNTER_TABLE_NAME
};

WatermarkOrch *wm_orch = new WatermarkOrch(m_configDb, wm_tables);

/*
* The order of the orch list is important for state restore of warm start and
Expand Down
101 changes: 79 additions & 22 deletions orchagent/watermarkorch.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,8 +14,8 @@
extern PortsOrch *gPortsOrch;


WatermarkOrch::WatermarkOrch(DBConnector *db, const string tableName):
Orch(db, tableName)
WatermarkOrch::WatermarkOrch(DBConnector *db, const vector<string> &tables):
Orch(db, tables)
{
SWSS_LOG_ENTER();

Expand All @@ -36,9 +36,6 @@ WatermarkOrch::WatermarkOrch(DBConnector *db, const string tableName):
m_telemetryTimer = new SelectableTimer(intervT);
auto executorT = new ExecutableTimer(m_telemetryTimer, this, "WM_TELEMETRY_TIMER");
Orch::addExecutor(executorT);
m_telemetryTimer->start();

m_telemetryInterval = DEFAULT_TELEMETRY_INTERVAL;
}

WatermarkOrch::~WatermarkOrch()
Expand Down Expand Up @@ -66,19 +63,13 @@ void WatermarkOrch::doTask(Consumer &consumer)

if (op == SET_COMMAND)
{
if (key == "TELEMETRY_INTERVAL")
if (consumer.getTableName() == CFG_WATERMARK_TABLE_NAME)
{
for (std::pair<std::basic_string<char>, std::basic_string<char> > i: fvt)
{
if (i.first == "interval")
{
m_telemetryInterval = to_uint<uint32_t>(i.second.c_str());
}
else
{
SWSS_LOG_WARN("Unsupported key: %s", i.first.c_str());
}
}
handleWmConfigUpdate(key, fvt);
}
else if (consumer.getTableName() == CFG_FLEX_COUNTER_TABLE_NAME)
{
handleFcConfigUpdate(key, fvt);
}
}
else if (op == DEL_COMMAND)
Expand All @@ -94,13 +85,74 @@ void WatermarkOrch::doTask(Consumer &consumer)
}
}

void WatermarkOrch::handleWmConfigUpdate(const std::string &key, const std::vector<FieldValueTuple> &fvt)
{
SWSS_LOG_ENTER();
if (key == "TELEMETRY_INTERVAL")
{
for (std::pair<std::basic_string<char>, std::basic_string<char> > i: fvt)
{
if (i.first == "interval")
{
auto intervT = timespec { .tv_sec = to_uint<uint32_t>(i.second.c_str()) , .tv_nsec = 0 };
m_telemetryTimer->setInterval(intervT);
Copy link
Contributor

@wendani wendani Mar 20, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

@wendani wendani Apr 22, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

// reset the timer interval when current timer expires
m_timerChanged = true;
}
else
{
SWSS_LOG_WARN("Unsupported key: %s", i.first.c_str());
}
}
}
}

void WatermarkOrch::handleFcConfigUpdate(const std::string &key, const std::vector<FieldValueTuple> &fvt)
{
SWSS_LOG_ENTER();
uint8_t prevStatus = m_wmStatus;
if (key == "QUEUE_WATERMARK" || key == "PG_WATERMARK")
{
for (std::pair<std::basic_string<char>, std::basic_string<char> > i: fvt)
{
if (i.first == "FLEX_COUNTER_STATUS")
{
if (i.second == "enable")
{
m_wmStatus = (uint8_t) (m_wmStatus | groupToMask.at(key));
}
else if (i.second == "disable")
{
m_wmStatus = (uint8_t) (m_wmStatus & ~(groupToMask.at(key)));
}
}
}
if (!prevStatus && m_wmStatus)
{
m_telemetryTimer->start();
Copy link
Contributor

@wendani wendani Mar 20, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added logic to avoid uneven intervals.

}
SWSS_LOG_DEBUG("Status of WMs: %u", m_wmStatus);
}
}

void WatermarkOrch::doTask(NotificationConsumer &consumer)
{
SWSS_LOG_ENTER();
if (!gPortsOrch->isPortReady())
{
return;
}

if (m_pg_ids.empty())
{
init_pg_ids();
}

if (m_multicast_queue_ids.empty() and m_unicast_queue_ids.empty())
{
init_queue_ids();
}

std::string op;
std::string data;
std::vector<swss::FieldValueTuple> values;
Expand Down Expand Up @@ -170,16 +222,21 @@ void WatermarkOrch::doTask(SelectableTimer &timer)

Copy link
Contributor

@wendani wendani Apr 15, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor

@wendani wendani Apr 17, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

if (&timer == m_telemetryTimer)
{
/* If the interval was changed */
auto intervT = timespec { .tv_sec = m_telemetryInterval , .tv_nsec = 0 };
m_telemetryTimer->setInterval(intervT);
m_telemetryTimer->reset();
if (m_timerChanged)
{
m_telemetryTimer->reset();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

m_timerChanged = false;
}
if (!m_wmStatus)
{
m_telemetryTimer->stop();
}

clearSingleWm(m_periodicWatermarkTable.get(), "SAI_INGRESS_PRIORITY_GROUP_STAT_XOFF_ROOM_WATERMARK_BYTES", m_pg_ids);
clearSingleWm(m_periodicWatermarkTable.get(), "SAI_INGRESS_PRIORITY_GROUP_STAT_SHARED_WATERMARK_BYTES", m_pg_ids);
clearSingleWm(m_periodicWatermarkTable.get(), "SAI_QUEUE_STAT_SHARED_WATERMARK_BYTES", m_unicast_queue_ids);
clearSingleWm(m_periodicWatermarkTable.get(), "SAI_QUEUE_STAT_SHARED_WATERMARK_BYTES", m_multicast_queue_ids);
SWSS_LOG_INFO("Periodic watermark cleared by timer!");
SWSS_LOG_DEBUG("Periodic watermark cleared by timer!");
}
}

Expand Down
25 changes: 22 additions & 3 deletions orchagent/watermarkorch.h
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,17 +1,27 @@
#ifndef WATERMARKORCH_H
#define WATERMARKORCH_H

#include <map>

#include "orch.h"
#include "port.h"

#include "notificationconsumer.h"
#include "timer.h"

const uint8_t queue_wm_status_mask = 1 << 0;
const uint8_t pg_wm_status_mask = 1 << 1;

static const map<string, const uint8_t> groupToMask =
{
{ "QUEUE_WATERMARK", queue_wm_status_mask },
{ "PG_WATERMARK", pg_wm_status_mask }
};

class WatermarkOrch : public Orch
{
public:
WatermarkOrch(DBConnector *db, const std::string tableName);
WatermarkOrch(DBConnector *db, const vector<string> &tables);
virtual ~WatermarkOrch(void);

void doTask(Consumer &consumer);
Expand All @@ -21,6 +31,9 @@ class WatermarkOrch : public Orch
void init_pg_ids();
void init_queue_ids();

void handleWmConfigUpdate(const std::string &key, const std::vector<FieldValueTuple> &fvt);
void handleFcConfigUpdate(const std::string &key, const std::vector<FieldValueTuple> &fvt);

void clearSingleWm(Table *table, string wm_name, vector<sai_object_id_t> &obj_ids);

shared_ptr<Table> getCountersTable(void)
Expand All @@ -34,6 +47,14 @@ class WatermarkOrch : public Orch
}

private:
/*
[7-2] - unused
[1] - pg wm status
[0] - queue wm status (least significant bit)
*/
uint8_t m_wmStatus = 0;
bool m_timerChanged = false;

Copy link
Contributor

@wendani wendani Mar 20, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Now status is stored as a uint8_t

shared_ptr<DBConnector> m_countersDb = nullptr;
shared_ptr<DBConnector> m_appDb = nullptr;
shared_ptr<Table> m_countersTable = nullptr;
Expand All @@ -47,8 +68,6 @@ class WatermarkOrch : public Orch
vector<sai_object_id_t> m_unicast_queue_ids;
vector<sai_object_id_t> m_multicast_queue_ids;
vector<sai_object_id_t> m_pg_ids;

int m_telemetryInterval;
};

#endif // WATERMARKORCH_H