-
Notifications
You must be signed in to change notification settings - Fork 529
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.
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
[PfcWd]: Make polling interval configurable #435
Conversation
depends on sonic-net/sonic-swss-common#183 and sonic-net/sonic-sairedis#289 |
retest this please |
orchagent/pfcwdorch.h
Outdated
@@ -108,7 +109,7 @@ class PfcWdSwOrch: public PfcWdOrch<DropHandler, ForwardHandler> | |||
const vector<sai_queue_attr_t> c_queueAttrIds; | |||
|
|||
shared_ptr<DBConnector> m_pfcWdDb = nullptr; | |||
shared_ptr<ProducerStateTable> m_pfcWdTable = nullptr; | |||
shared_ptr<ProducerTable> m_pfcWdTable = nullptr; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why switch to ProducerTable?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same questions here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
With producerstatetable/consumerstatetable, when producer wants to set a single field, the consumer will receive the state of all the fields of the key and set them one more time. Eg. We set F1 v1. first, and then when setting F2 v2 later, consumer can still receive F1 V1 when handling F2 V2 and set F1 V1 one more time. This is not ideal as we have the use case to change the value of a single field. Besides, using statetable will trigger the current syncd error that the same plugin is added more than once in addQueueCounterPlugins function, and that's how I figure this out.
Since we do not set the field value for a single key for flexcounter frequently in a very short time, there is no benefit to use statetable, but results in more overhead. So switch to producertable/cosumertable.
We can remove syncd error log in addQueueCounterPlugins and use stateTable, though I cannot see the benefit of using stateTable.
const auto &field = fvField(valuePair); | ||
const auto &value = fvValue(valuePair); | ||
|
||
if (field == POLL_INTERVAL_FIELD) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a benefit of making this field global and not per port? If yes, what will happen in case if I will enable WD on some port(s) without setting global polling interval?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Polling interval is global by design. It will use the default value if poll interval is not set specifically.
d69f5fc
to
e85e431
Compare
Signed-off-by: Sihui Han <sihan@microsoft.com>
e85e431
to
21520a8
Compare
retest this please |
Signed-off-by: Sihui Han <sihan@microsoft.com>
* [PfcWd]: Make polling interval configurable Signed-off-by: Sihui Han <sihan@microsoft.com> * rebase master * fix the build error Signed-off-by: Sihui Han <sihan@microsoft.com>
Signed-off-by: Sihui Han sihan@microsoft.com
What I did
Make the polling interval configurable
Why I did it
Extend the feature.
How I verified it
Test on DUT
Details if related