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

modify numa-aware memory headroom policy to consider about inactive pages #383

Merged
merged 3 commits into from
Dec 20, 2023

Conversation

LuyaoZhong
Copy link
Contributor

@LuyaoZhong LuyaoZhong commented Nov 29, 2023

Numa-aware memory headroom was calculated by

reclaimable numa free - 2 * watermark_reserved - config_reserved + reclaimed-core requests

Double watermark reservation reserved the memory too much. Besides, portion of inactive file should be
considered as cold pages on the system, which could be charged to memory headroom since it's easy to
be reclaimed by kernel reclaim mechanism.

Therefore, numa-ware memory headroom will be calculated by

reclaimable numa free - watermark_reserved - config_reserved + cacheBasedRatio * numaInactiveFile + reclaimed-core requests

Copy link

codecov bot commented Nov 29, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (799ebad) 53.78% compared to head (5137da2) 53.57%.
Report is 1 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #383      +/-   ##
==========================================
- Coverage   53.78%   53.57%   -0.22%     
==========================================
  Files         446      446              
  Lines       49411    49422      +11     
==========================================
- Hits        26578    26478     -100     
- Misses      19830    19943     +113     
+ Partials     3003     3001       -2     
Flag Coverage Δ
unittest 53.57% <100.00%> (-0.22%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@waynepeking348
Copy link
Collaborator

pls make the commit message more meaningful to let others know exactly what you wan to modify

@@ -121,6 +122,14 @@ func (p *PolicyNUMAAware) Update() (err error) {
reclaimableMemory += data.Value
general.InfoS("reclaimable numa memory free", "numaID", numaID, "numaFree", general.FormatMemoryQuantity(data.Value))

data, err = p.metaServer.GetNumaMetric(numaID, consts.MetricMemInactiveFileNuma)
if err != nil {
general.InfoS("failed to get numa inactive file", "numaID", numaID)
Copy link
Member

Choose a reason for hiding this comment

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

why not return error here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

inactive file is kind of buffer, we can return headroom without buffer, return error seems a overkill here.

Copy link
Member

Choose a reason for hiding this comment

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

If the loss of this metric is accidental,headroom may change abruptly.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK, it makes sense.

@@ -22,6 +22,8 @@ import (
"testing"
"time"

"github.com/kubewharf/katalyst-core/pkg/config/agent/dynamic/adminqos/reclaimedresource/memoryheadroom"
Copy link
Member

Choose a reason for hiding this comment

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

code style: sort goimports by 3 groups: std, general, project dependencies.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Got it.

cheney-lin
cheney-lin previously approved these changes Nov 30, 2023
@cheney-lin cheney-lin added enhancement New feature or request workflow/need-review review: test succeeded, need to review labels Nov 30, 2023
@waynepeking348 waynepeking348 changed the title Modify numa-aware memory headroom policy modify numa-aware memory headroom policy to consider about inactive pages Nov 30, 2023
waynepeking348
waynepeking348 previously approved these changes Nov 30, 2023
Numa-aware memory headroom was calculated by
    reclaimable numa free - 2 * watermark_reserved - config_reserved + reclaimed-core requests
Double watermark reservation reserved the memory too much. Besides, portion of inactive file should be
considered as cold pages on the system, which could be charged to memory headroom since it's easy to
be reclaimed by kernel reclaim mechanism.

Therefore, numa-ware memory headroom will be calculated by
    reclaimable numa free - watermark_reserved - config_reserved + cacheBasedRatio * numaInactiveFile + reclaimed-core requests
@LuyaoZhong LuyaoZhong dismissed stale reviews from cheney-lin and waynepeking348 via 130a9cc December 4, 2023 08:41
@cheney-lin cheney-lin added workflow/merge-ready merge-ready: code is ready and can be merged and removed workflow/need-review review: test succeeded, need to review labels Dec 11, 2023
@waynepeking348 waynepeking348 merged commit b2eed62 into kubewharf:main Dec 20, 2023
10 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request workflow/merge-ready merge-ready: code is ready and can be merged
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants