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

libcontainer/intelrdt: rm init() from intelrdt.go #2678

Merged
merged 1 commit into from
Dec 22, 2020
Merged

libcontainer/intelrdt: rm init() from intelrdt.go #2678

merged 1 commit into from
Dec 22, 2020

Conversation

xiaochenshen
Copy link
Contributor

Use sync.Once to init Intel RDT when needed for a small speedup to
operations which do not require Intel RDT.

Simplify IntelRdtManager initialization in LinuxFactory.

Signed-off-by: Xiaochen Shen xiaochen.shen@intel.com

@xiaochenshen xiaochenshen mentioned this pull request Nov 10, 2020
Merged
@xiaochenshen
Copy link
Contributor Author

xiaochenshen commented Nov 10, 2020 •
edited
Loading

This is the follow-up PR for #2643 :
@kolyshkin suggested and reviewed this change. But appreciate the input from @cyphar on the matter.

See details in #2643 (comment).

Please help review the third commit only, the first and second commits are dependencies.

crosbymichael
crosbymichael previously approved these changes Nov 10, 2020
View reviewed changes
Copy link
Member

@crosbymichael crosbymichael left a comment

Choose a reason for hiding this comment

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

LGTM

@xiaochenshen
Copy link
Contributor Author

Rebased for dependency #2677 has been merged.
No code change.

@Creatone
Copy link
Contributor

LGTM

AkihiroSuda
AkihiroSuda previously approved these changes Nov 16, 2020
View reviewed changes
@xiaochenshen
Copy link
Contributor Author

@crosbymichael @AkihiroSuda
Thank you very much for code review and approval!
This PR has a dependent commit which is addressed in #2643 .
Could you also help review #2643 which one more LGTM needed to merge? Thank you!

@xiaochenshen
Copy link
Contributor Author

xiaochenshen commented Nov 18, 2020 •
edited
Loading

@crosbymichael @AkihiroSuda @kolyshkin @Creatone
Code rebased to merge with master branch: just removed the dependent commits, the patch for this PR keeps unchanged.
Please help re-approve.
Thank you!

@xiaochenshen xiaochenshen force-pushed the rdt-rm-init branch 5 times, most recently from d012059 to 1a74585 Compare November 19, 2020 04:50
@xiaochenshen
Copy link
Contributor Author

Travis-CI always fails on cases "Go: 1.15.x", "Go: 1.14.x", and "Go: tip" with the same error:
https://travis-ci.org/github/opencontainers/runc/jobs/744584826#L1922

Looks like something wrong with Travis-CI itself?

@xiaochenshen xiaochenshen force-pushed the rdt-rm-init branch 2 times, most recently from 8ac1042 to 481ce35 Compare November 20, 2020 12:54
@xiaochenshen
Copy link
Contributor Author

The Travis-CI system issue seems to be fixed already.

@crosbymichael @AkihiroSuda @kolyshkin @Creatone
Code rebased but the patch keeps unchanged.
Could you help re-approve at your convenience?
Thank you!

@xiaochenshen
Copy link
Contributor Author

@crosbymichael @AkihiroSuda @kolyshkin
This PR is pending for a long time. I think just re-approvals are required for merge. Am I right?
The code change in the old version of the patch has been approved by you. But then this PR is rebased against master branch (without code change), the original approvals are outdated.

Could you help re-approve at your convenience?
Thank you very much for help!

kolyshkin
kolyshkin reviewed Dec 16, 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

kolyshkin
kolyshkin approved these changes Dec 16, 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

kolyshkin
kolyshkin requested changes Dec 16, 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.

On a second thought, I think it needs to be changed to not require user call Init explicitly (see details above).

@xiaochenshen
Copy link
Contributor Author

On a second thought, I think it needs to be changed to not require user call Init explicitly (see details above).

@kolyshkin Thank you very much for code review and good suggestions. I will make change to prevent calling Init() explicitly by outside caller.

@xiaochenshen xiaochenshen force-pushed the rdt-rm-init branch 2 times, most recently from 0634d26 to 325a74d Compare December 16, 2020 15:16
@xiaochenshen
Copy link
Contributor Author

@kolyshkin
I have made the change according to your suggestion.

Change log:

  1. Remove intelrdt.Init() call in IntelRdtFs().
  2. Privatize and rename Init() to featuresInit().
  3. Call featuresInit() in Is{CAT | MBA | MBASc | CMT | MBM}Enabled().

@xiaochenshen
libcontainer/intelrdt: rm init() from intelrdt.go
325a74d 
Use sync.Once to init Intel RDT when needed for a small speedup to
operations which do not require Intel RDT.

Simplify IntelRdtManager initialization in LinuxFactory.

Signed-off-by: Xiaochen Shen <xiaochen.shen@intel.com>
kolyshkin
kolyshkin approved these changes Dec 17, 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, and thanks for working on this.

@AkihiroSuda PTAL

@AkihiroSuda AkihiroSuda merged commit 5efdc0c into opencontainers:master Dec 22, 2020
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 approved these changes

@crosbymichael crosbymichael Awaiting requested review from crosbymichael

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

Successfully merging this pull request may close these issues.
5 participants
@xiaochenshen @Creatone @crosbymichael @kolyshkin @AkihiroSuda