Skip to content
This repository was archived by the owner on May 12, 2021. It is now read-only.

kernel: pmem device should map pages#385

Merged
jodh-intel merged 1 commit intokata-containers:masterfrom
devimc:topic/fixDAX
Mar 11, 2019
Merged

kernel: pmem device should map pages#385
jodh-intel merged 1 commit intokata-containers:masterfrom
devimc:topic/fixDAX

Conversation

@devimc
Copy link
Copy Markdown

@devimc devimc commented Mar 7, 2019 •
edited
Loading

enable ZONE_DEVICE config to support map pages, pmem_should_map_pages()
function fails if this config is not enabled.

fixes #378

Signed-off-by: Julio Montes <julio.montes@intel.com

@devimc devimc mentioned this pull request Mar 7, 2019
Closed
@devimc
Copy link
Copy Markdown
Author

devimc commented Mar 7, 2019

/test

@jodh-intel
Copy link
Copy Markdown

jodh-intel commented Mar 8, 2019

Hi @devimc - a few points:

  • No "Fixes" comment.
  • Please could you add some details into the commit to explain this change.
  • Is this purely x86_64-specific?

@grahamwhaley
Copy link
Copy Markdown
Contributor

grahamwhaley commented Mar 8, 2019

/cc @alicefr @nitkon @Pennyzct - do any of your arch's use NVDIMM/DAX for kata? If so, you probably need this patch as well for the 4.19 kernel (or DAX doesn't actually happen - it fails to map but the kernel carries on, and it costs us a bunch of memory footprint)

@devimc
Copy link
Copy Markdown
Author

devimc commented Mar 8, 2019

@jodh-intel changes applied

Is this purely x86_64-specific?

I think so

@devimc
Copy link
Copy Markdown
Author

devimc commented Mar 8, 2019

/test

jodh-intel
jodh-intel approved these changes Mar 8, 2019
View reviewed changes
Copy link
Copy Markdown

@jodh-intel jodh-intel left a comment

Choose a reason for hiding this comment

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

Thanks @devimc.

lgtm

@grahamwhaley
Copy link
Copy Markdown
Contributor

grahamwhaley commented Mar 8, 2019

@devimc - see kata-containers/runtime#1323 - that is NVDIMM for ARM - does that thus make this need adding for that arch as well?

@devimc
Copy link
Copy Markdown
Author

devimc commented Mar 8, 2019

@grahamwhaley good point, @Pennyzct any thoughts ?

@jodh-intel
Copy link
Copy Markdown

jodh-intel commented Mar 8, 2019

@grahamwhaley - could we enforce a suitable number of acks if we added a CODEOWNERS and added a rule for kernel/configs/* such that acks are required from @kata-containers/kernel?

@grahamwhaley
Copy link
Copy Markdown
Contributor

grahamwhaley commented Mar 8, 2019

@jodh-intel - I'm not quite sure what your goal is, but we could:

  • set a group of folks of which we must get acks from (I think it would be one ack from the group at least) for any config changes
  • set up individual folks, or lists of folks, for specific architecture files.

The CODEOWNERS file works very much like a .gitignore iirc - see https://help.github.com/en/articles/about-code-owners#codeowners-syntax

@jodh-intel
Copy link
Copy Markdown

jodh-intel commented Mar 8, 2019

@grahamwhaley - well somthing like what we do for the agent where we require additional acks for protocol buffer changes:

@grahamwhaley
Copy link
Copy Markdown
Contributor

grahamwhaley commented Mar 8, 2019

I'm sure we could. Questions are I guess:

  • who needs to review
  • and what do they need to review

so... you did ask for this .... but, PRs always welcome :-)

@jodh-intel
Copy link
Copy Markdown

jodh-intel commented Mar 8, 2019

Well, I'm proposing we get the kernel folk to review all changes to kernel-related files. I can put a PR together later if nobody jumps in before me... :)

kernel: pmem device should map pages
d1a9aa7 
enable ZONE_DEVICE config to support map pages, pmem_should_map_pages()
function fails if this config is not enabled.

fixes kata-containers#378

Signed-off-by: Julio Montes <julio.montes@intel.com
@devimc
Copy link
Copy Markdown
Author

devimc commented Mar 8, 2019

/test

1 similar comment
@sboeuf
Copy link
Copy Markdown

sboeuf commented Mar 8, 2019

/test

@alicefr
Copy link
Copy Markdown

alicefr commented Mar 11, 2019

@grahamwhaley for s390 we don't have NVDIMM/DAX. It's not necessary to add those options

@nitkon
Copy link
Copy Markdown
Contributor

nitkon commented Mar 11, 2019

@grahamwhaley: For ppc64le, we have nvdimm/dax and there would be a PR soon to get that rootfs image option working for Power.

@jodh-intel jodh-intel merged commit 5f95c9a into kata-containers:master Mar 11, 2019
@devimc devimc mentioned this pull request Mar 12, 2019
Merged
@Pennyzct
Copy link
Copy Markdown
Contributor

Pennyzct commented Mar 13, 2019

Hi~ @devimc @grahamwhaley I have disabled dax on kata-containers/runtime#1323. since dax is not fully supported on arm64, and missing ZONE_DEVICE is one major cause. but we have already been working on that. ;)

@devimc
Copy link
Copy Markdown
Author

devimc commented Mar 13, 2019

@Pennyzct thank you!

@jodh-intel jodh-intel mentioned this pull request Mar 15, 2019
Closed
@devimc devimc deleted the topic/fixDAX branch April 8, 2019 16:14
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

3 more reviewers

@amshinde amshinde amshinde approved these changes

@jodh-intel jodh-intel jodh-intel approved these changes

@sboeuf sboeuf sboeuf approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Memory footprint increment: DAX unsupported by block device. Turning off DAX

8 participants

@devimc @jodh-intel @grahamwhaley @sboeuf @alicefr @nitkon @Pennyzct @amshinde