Skip to content

Tune consumer order in BufferOrch::doTask() based on dependency for warm start #590

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

Merged
qiluo-msft merged 5 commits into from
Aug 24, 2018
Merged

Tune consumer order in BufferOrch::doTask() based on dependency for warm start #590

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
8bda1b5
initBufferReadyLists will pops consumers, so warm start will not redo
qiluo-msft Aug 23, 2018
8614d39
revert initBufferReadyLists()
qiluo-msft Aug 23, 2018
3b86fda
Merge branch 'master' into qiluo/warmbuff
qiluo-msft Aug 24, 2018
46ba706
(comment)
qiluo-msft Aug 24, 2018
b456b57
(typo)
qiluo-msft Aug 24, 2018
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
32 changes: 32 additions & 0 deletions orchagent/bufferorch.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -635,6 +635,38 @@ task_process_status BufferOrch::processEgressBufferProfileList(Consumer &consume
return task_process_status::task_success;
}

void BufferOrch::doTask()
{
// The hidden dependency tree:
// ref: https://github.com/opencomputeproject/SAI/blob/master/doc/QOS/SAI-Proposal-buffers-Ver4.docx
// 2 SAI model
// 3.1 Ingress priority group (PG) configuration
// 3.2.1 Buffer profile configuration
//
// buffer poll
Copy link
Copy Markdown
Contributor

@lguohan lguohan Aug 24, 2018 •
edited by qiluo-msft
Loading

Choose a reason for hiding this comment

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

buffer pool #Resolved

// └── buffer profile
// ├── buffer port ingress profile list
// ├── buffer port egress profile list
// ├── buffer queue
// └── buffer pq table

auto pool_consumer = getExecutor((CFG_BUFFER_POOL_TABLE_NAME));
pool_consumer->drain();

auto profile_consumer = getExecutor(CFG_BUFFER_PROFILE_TABLE_NAME);
profile_consumer->drain();

for(auto &it : m_consumerMap)
{
auto consumer = it.second.get();
if (consumer == profile_consumer)
continue;
if (consumer == pool_consumer)
continue;
consumer->drain();
}
Copy link
Copy Markdown
Contributor

@jipanyang jipanyang Aug 24, 2018 •
edited by qiluo-msft
Loading

Choose a reason for hiding this comment

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

Do you mean the configuration for other buffer tables like CFG_BUFFER_QUEUE_TABLE_NAME, CFG_BUFFER_PG_TABLE_NAME, CFG_BUFFER_PORT_INGRESS_PROFILE_LIST_NAME and CFG_BUFFER_PORT_EGRESS_PROFILE_LIST_NAME have dependency on CFG_BUFFER_PROFILE_TABLE_NAME & CFG_BUFFER_POOL_TABLE_NAME? CFG_BUFFER_PROFILE_TABLE_NAME has dependency on CFG_BUFFER_POOL_TABLE_NAME?

I didn't see existing code checking that dependency. #Resolved

Copy link
Copy Markdown
Contributor Author

@qiluo-msft qiluo-msft Aug 24, 2018

Choose a reason for hiding this comment

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

Exactly, so I added comments. The ground truth is inside SAI design doc.
For code, task_process_status::task_need_retry possibly indicates a dependency missing. They don't show the dependency relationship as specific as SAI design doc.

In reply to: 212513136 [](ancestors = 212513136)

Copy link
Copy Markdown
Contributor

@jipanyang jipanyang Aug 24, 2018 •
edited by qiluo-msft
Loading

Choose a reason for hiding this comment

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

If there is dependency, the exact dependency chain should be clarified and the problem should be fixed in normal code logic which applies to cold boot too.

#Resolved

Copy link
Copy Markdown
Contributor

@lguohan lguohan Aug 24, 2018

Choose a reason for hiding this comment

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

it is difficult to do this in the normal boot since the input sequence is unknown. we need to get the dependency at the entry level and then dispatched to task. in warmboot, since we have all the data, we can just solve the dependency at the table level.

Copy link
Copy Markdown
Contributor Author

@qiluo-msft qiluo-msft Aug 24, 2018

Choose a reason for hiding this comment

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

During cold start, the task_process_status::task_need_retry handles dependency correctly, ie. anything missing will postpone task processing. So there is no problem to fix.
I agree this code is not explicit in dependency relationship. This should be improved in future.

In reply to: 212527615 [](ancestors = 212527615)

}

void BufferOrch::doTask(Consumer &consumer)
{
SWSS_LOG_ENTER();
Expand Down
1 change: 1 addition & 0 deletions orchagent/bufferorch.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,7 @@ class BufferOrch : public Orch
typedef map<string, buffer_table_handler> buffer_table_handler_map;
typedef pair<string, buffer_table_handler> buffer_handler_pair;

virtual void doTask() override;
virtual void doTask(Consumer& consumer);
void initTableHandlers();
void initBufferReadyLists(DBConnector *db);
Expand Down
2 changes: 1 addition & 1 deletion orchagent/orch.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -182,7 +182,7 @@ class Orch
virtual bool bake();

/* Iterate all consumers in m_consumerMap and run doTask(Consumer) */
void doTask();
virtual void doTask();

/* Run doTask against a specific executor */
virtual void doTask(Consumer &consumer) = 0;
Expand Down
16 changes: 13 additions & 3 deletions orchagent/orchdaemon.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -264,7 +264,11 @@ bool OrchDaemon::init()

if (WarmStart::isWarmStart())
{
warmRestoreAndSyncUp();
bool suc = warmRestoreAndSyncUp();
if (!suc)
{
return false;
}
Copy link
Copy Markdown
Contributor

@stcheng stcheng Aug 23, 2018 •
edited by qiluo-msft
Loading

Choose a reason for hiding this comment

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

from line 265 - 274 it could be ine one line return !WarmStart::isWarmStart || warmRestoreAndSyncUp(); #Resolved

Copy link
Copy Markdown
Contributor Author

@qiluo-msft qiluo-msft Aug 23, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

It is equivalent. I prefer no change for easy reading. #Closed

}

return true;
Expand Down Expand Up @@ -336,7 +340,7 @@ void OrchDaemon::start()
* Try to perform orchagent state restore and dynamic states sync up if
* warm start reqeust is detected.
*/
void OrchDaemon::warmRestoreAndSyncUp()
bool OrchDaemon::warmRestoreAndSyncUp()
{
WarmStart::setWarmStartState("orchagent", WarmStart::INIT);

Expand Down Expand Up @@ -366,7 +370,12 @@ void OrchDaemon::warmRestoreAndSyncUp()
* orchagent should be in exact same state of pre-shutdown.
* Perform restore validation as needed.
*/
warmRestoreValidation();
bool suc = warmRestoreValidation();
if (!suc)
{
SWSS_LOG_ERROR("Orchagent state restore failed");
return false;
}

SWSS_LOG_NOTICE("Orchagent state restore done");

Expand All @@ -377,6 +386,7 @@ void OrchDaemon::warmRestoreAndSyncUp()
* The "RECONCILED" state of orchagent doesn't mean the state related to neighbor is up to date.
*/
WarmStart::setWarmStartState("orchagent", WarmStart::RECONCILED);
return true;
}

/*
Expand Down
2 changes: 1 addition & 1 deletion orchagent/orchdaemon.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,7 @@ class OrchDaemon

bool init();
void start();
void warmRestoreAndSyncUp();
bool warmRestoreAndSyncUp();
void getTaskToSync(vector<string> &ts);
bool warmRestoreValidation();
private:
Expand Down