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.

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

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

Merged
merged 1 commit into from
Jun 20, 2019

Conversation

renukamanavalan
Copy link
Contributor

What I did

Why I did it

How I verified it

Details if related

@qiluo-msft
Copy link
Contributor

qiluo-msft commented Jun 20, 2019

I see one benefit of original code. If the time used in doTask is not trivially short, it will schedule the next timer after everything done. It has no risk to schedule more frequent than what could be handled.

@renukamanavalan
Copy link
Contributor Author

If you see that as a solid benefit, we should for one shot timer, instead of periodic one. That way upon firing, you complete the task and start the next timer run.

@lguohan
Copy link
Contributor

lguohan commented Jun 20, 2019

@renukamanavalan , can you add more descriptions in the pr?

@lguohan
Copy link
Contributor

lguohan commented Jun 20, 2019

what if the interval changed, how to you reset the timer interval?

@renukamanavalan
Copy link
Contributor Author

  1. When CRMOrch starts, it starts a periodic timer for a default interval of 5 mins and saves it in m_pollinginterval.
  2. Whenever polling interval changes, it calls CrmOrch::handleSetCommand, which sets the new interval and restarts timer.
  3. Whenever timer firing is sensed (through epoll), the CRMOrch::doTask(timer &) is called.

CRMOrch stops & starts the time in each call to doTask. No other Orch client that uses SelectableTimer does that.

Fix: Remove the redundant restart.

If it is intended that we restart the timer upon each firing, then we should not go for periodic timer, and instead call one shot timer. Upon getting called, do the task and start the next one shot timer.

@renukamanavalan
Copy link
Contributor Author

BTW, this is not a fix for any issue/bug. I just happened to notice, while analyzing SelectableTimer::readData failure.

@renukamanavalan renukamanavalan merged commit 6bdf7f2 into sonic-net:master Jun 20, 2019
@renukamanavalan renukamanavalan deleted the redundant branch June 20, 2019 16:11
@lguohan
Copy link
Contributor

lguohan commented Jun 20, 2019

does this cause readdata failure?

@renukamanavalan
Copy link
Contributor Author

Nope! By theory, it should not.

@wendani
Copy link
Contributor

wendani commented Jun 20, 2019

We requested some similar change in watermarkorch #781

renukamanavalan added a commit to renukamanavalan/sonic-swss that referenced this pull request Jun 24, 2019
renukamanavalan added a commit to renukamanavalan/sonic-swss that referenced this pull request Jun 24, 2019
EdenGri pushed a commit to EdenGri/sonic-swss that referenced this pull request Feb 28, 2022
* Add watchdogutil to control the hw watchdog

* fix LGTM

* Fixed based on review comments

* replace the is_armed() and get_remaining_time to status() subcommand

* syntax error

* Add more info to the output

* re-format of output

* remove spaces

* change the version number of watchdogutil

* Change the output parsing for the watchdog arm case

* typo

* fix more review comments
oleksandrivantsiv pushed a commit to oleksandrivantsiv/sonic-swss that referenced this pull request Mar 1, 2023
To satisfy LGTM alert and for performance reasons to skip making unnecessary copy of struct.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants