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.

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

Jump to bottom

Add RDT Memory Bandwidth Monitoring (MBM) and Cache Monitoring Technology (CMT) statistics. #2292

Merged
merged 3 commits into from
May 13, 2020
Merged

Add RDT Memory Bandwidth Monitoring (MBM) and Cache Monitoring Technology (CMT) statistics. #2292

merged 3 commits into from
May 13, 2020

Conversation

Creatone
Copy link
Contributor

@Creatone Creatone commented Apr 3, 2020

This pull request introduces the reading of Intel RDT Memory Bandwidth Monitoring statistics.

@Creatone Creatone force-pushed the creatone/extend-intelrdt branch 2 times, most recently from e7b8e36 to 1f865d7 Compare April 3, 2020 14:00
@kolyshkin
Copy link
Contributor

@Creatone can you please avoid the merge commit? You can do so by using git pull --rebase instead of git rebase.

intelrdt: change parseCpuInfoFile to return struct
7fa13b2 
Signed-off-by: Paweł Szulik <pawel.szulik@intel.com>
@Creatone
Copy link
Contributor Author

Creatone commented Apr 8, 2020 •
edited
Loading

@kolyshkin Done.

kolyshkin
kolyshkin requested changes Apr 9, 2020
View reviewed changes
Copy link
Contributor

@kolyshkin kolyshkin left a comment

Choose a reason for hiding this comment

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

Thanks for the rework, it looks better now. Left some more comments.

@Creatone Creatone force-pushed the creatone/extend-intelrdt branch 2 times, most recently from 531cfeb to d2b4e6b Compare April 9, 2020 18:12
kolyshkin
kolyshkin requested changes Apr 9, 2020
View reviewed changes
libcontainer/intelrdt/mbm.go Outdated Show resolved Hide resolved
intelrdt: add mbm stats
d1e4c7b 
Signed-off-by: Paweł Szulik <pawel.szulik@intel.com>
@Creatone Creatone changed the title Add RDT Memory Bandwidth Monitoring (MBM) statistics. Add RDT Memory Bandwidth Monitoring (MBM) and Cache Monitoring Technology (CMT) statistics. Apr 15, 2020
@Creatone
Copy link
Contributor Author

@AkihiroSuda Could you review this?

AkihiroSuda
AkihiroSuda reviewed Apr 17, 2020
View reviewed changes
types/events.go Outdated
@@ -113,6 +115,12 @@ type IntelRdt struct {

// The memory bandwidth schema in 'container_id' group
MemBwSchema string `json:"mem_bw_schema,omitempty"`

// The memory bandwidth monitoring statistics from NUMA nodes in 'container_id' group
MBMStats *[]intelrdt.MBMNumaNodeStats `json:"mbm_statistics,omitempty"`
Copy link
Member

Choose a reason for hiding this comment

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

maybe: json:"mbm_stats,omitempty"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

good idea

Copy link
Contributor

Choose a reason for hiding this comment

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

@Creatone ^^^

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@kolyshkin this comment is outdated. String was already changed long time ago.

@AkihiroSuda
Copy link
Member

Looks good but a single nit

@AkihiroSuda
Copy link
Member

AkihiroSuda commented Apr 17, 2020 •
edited
Loading

LGTM

Approved with PullApprove

@Creatone
Copy link
Contributor Author

@cyphar Could you review this?

kolyshkin
kolyshkin approved these changes Apr 21, 2020
View reviewed changes
Copy link
Contributor

@kolyshkin kolyshkin left a comment

Choose a reason for hiding this comment

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

LGTM (once a nit by @AkihiroSuda is acted upon)

@Creatone
Copy link
Contributor Author

@kolyshkin

AkihiroSuda
AkihiroSuda reviewed Apr 25, 2020
View reviewed changes
libcontainer/intelrdt/stats.go Outdated
@@ -33,6 +46,12 @@ type Stats struct {

// The memory bandwidth schema in 'container_id' group
MemBwSchema string `json:"mem_bw_schema,omitempty"`

// The memory bandwidth monitoring statistics from NUMA nodes in 'container_id' group
MBMStats *[]MBMNumaNodeStats `json:"mbm_statistics,omitempty"`
Copy link
Member

Choose a reason for hiding this comment

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

statistics -> stats?

intelrdt: Add Cache Monitoring Technology stats
799d948 
Signed-off-by: Paweł Szulik <pawel.szulik@intel.com>
@AkihiroSuda
Copy link
Member

AkihiroSuda commented Apr 25, 2020 •
edited
Loading

LGTM

Approved with PullApprove

@Creatone
Copy link
Contributor Author

@kolyshkin Is it possible to merge this?

@h-vetinari h-vetinari mentioned this pull request Apr 29, 2020
Closed
@AkihiroSuda
Copy link
Member

@kolyshkin PTAL

@AkihiroSuda
Copy link
Member

@kolyshkin LGTY?

@kolyshkin
Copy link
Contributor

kolyshkin commented May 13, 2020 •
edited by AkihiroSuda
Loading

One small nit in json name (stats vs statistics) but I'm fine either way

LGTM

Approved with PullApprove

@kolyshkin kolyshkin merged commit 4b71877 into opencontainers:master May 13, 2020
@xiaochenshen xiaochenshen mentioned this pull request Nov 30, 2020
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kolyshkin kolyshkin kolyshkin approved these changes

@AkihiroSuda AkihiroSuda AkihiroSuda left review comments

Assignees
No one assigned
Labels
enhancement
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@Creatone @kolyshkin @AkihiroSuda