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

feat: [intel_rdt] Added support for LLC misses monitoring event #4296

Merged
merged 8 commits into from May 14, 2024

Conversation

aleksinx
Copy link
Contributor

@aleksinx aleksinx commented Mar 25, 2024

Added llc_miss metric

ChangeLog: intel_rdt: Added support for LLC misses monitoring event

@aleksinx aleksinx changed the title [intel_rdt] Added support for LLC misses monitoring event feat: [intel_rdt] Added support for LLC misses monitoring event Mar 25, 2024
src/intel_rdt.c Outdated Show resolved Hide resolved
src/intel_rdt.c Outdated Show resolved Hide resolved
src/intel_rdt.c Outdated Show resolved Hide resolved
src/intel_rdt.c Outdated Show resolved Hide resolved
src/intel_rdt.c Outdated Show resolved Hide resolved
@aleksinx aleksinx force-pushed the intel_rdt_llc_miss branch 4 times, most recently from 3d597dc to 846e064 Compare April 26, 2024 07:23
Copy link
Contributor

@eero-t eero-t left a comment

Choose a reason for hiding this comment

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

After the couple of (now) redundant ifdefs are removed, I think this is good to go!

src/intel_rdt.c Outdated
Comment on lines 156 to 158
#if PQOS_VERSION < 40000
const enum pqos_mon_event events = group->event;
#endif
Copy link
Contributor

Choose a reason for hiding this comment

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

This is unnecessary and confuses code. Just remove it and use group->event directly in that one place below.

src/intel_rdt.c Outdated
Comment on lines 206 to 208
#if PQOS_VERSION < 40400
const struct pqos_event_values *values = &group->values;
#endif
Copy link
Contributor

Choose a reason for hiding this comment

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

This is unnecessary now. Just use group->values directly in that one place below.

Copy link
Contributor

@eero-t eero-t left a comment

Choose a reason for hiding this comment

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

Thanks! Approved.

Copy link
Contributor

@mkobyli mkobyli left a comment

Choose a reason for hiding this comment

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

Thanks, looks good to me!

@eero-t eero-t merged commit e817577 into collectd:main May 14, 2024
23 of 24 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants