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

fix(inputs.intel_powerstat): reduce msr read latency on preempt-rt kernels #13829

Conversation

alysondeives
Copy link

fixes: #13828

Using cyclictest utility from rt-tests [1] results on latency increase when telegraf is executed on a preempt-rt kernel. This increase occurs because of the concurrent read of MSR with goroutines.

To fix this issue, the MSR read was changed to be sequential.

[1] https://git.kernel.org/pub/scm/utils/rt-tests/rt-tests.git

@telegraf-tiger telegraf-tiger bot added fix pr to fix corresponding bug plugin/input 1. Request for new input plugins 2. Issues/PRs that are related to input plugins labels Aug 24, 2023
@powersj powersj requested a review from zak-pawel August 24, 2023 15:31
…rnels

Using cyclictest utility from rt-tests [1] results on latency increase
when telegraf is executed on a preempt-rt kernel. This increase occurs
because of the concurrent read of MSR with goroutines.

To fix this issue, the MSR read was changed to be sequential.

[1] https://git.kernel.org/pub/scm/utils/rt-tests/rt-tests.git

Signed-off-by: Alyson Deives Pereira <[email protected]>
@alysondeives alysondeives force-pushed the intel-powerstat-fix-msr-latency branch from d585543 to 9050c50 Compare August 24, 2023 19:34
@p-zak p-zak requested review from p-zak and removed request for zak-pawel August 24, 2023 20:51
Copy link
Collaborator

@p-zak p-zak left a comment

Choose a reason for hiding this comment

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

I described background for concurrent reading in #13828, here is summary:

I'm ok with reading MSRs sequentially as long as:

  • it will depend on config parameter, for example: read_method with possible values: concurrent or sequential
  • default will be: concurrent (to have calculations as precise as possible by default and also for backward compatibility)
  • it will be very well described in README.md, sample.conf and code to show user pros and cons of each option

@powersj powersj added the waiting for response waiting for response from contributor label Aug 25, 2023
@alysondeives
Copy link
Author

HI @p-zak thanks for the explanation on MSR reads.
I will follow your recommendations and update my pull request.

@telegraf-tiger telegraf-tiger bot removed the waiting for response waiting for response from contributor label Aug 25, 2023
@powersj powersj added the waiting for response waiting for response from contributor label Aug 29, 2023
@telegraf-tiger
Copy link
Contributor

Hello! I am closing this issue due to inactivity. I hope you were able to resolve your problem, if not please try posting this question in our Community Slack or Community Forums or provide additional details in this issue and reqeust that it be re-opened. Thank you!

@telegraf-tiger telegraf-tiger bot closed this Sep 13, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
fix pr to fix corresponding bug plugin/input 1. Request for new input plugins 2. Issues/PRs that are related to input plugins waiting for response waiting for response from contributor
Projects
None yet
3 participants