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

Fix the handling of memory allocation for tmpfs. #40

Merged
merged 1 commit into from Dec 20, 2016

Conversation

rhatdan
Copy link
Member

@rhatdan rhatdan commented Dec 20, 2016

If the total memory on the system is less then the max memory in cgroup
then do not set memory limit on tmpfs, allow the kernel to set the default.

@rhatdan
Copy link
Member Author

rhatdan commented Dec 20, 2016

@mrunalp PTAL

@rhatdan
Copy link
Member Author

rhatdan commented Dec 20, 2016

https://bugzilla.redhat.com/show_bug.cgi?id=1406435
Covers this issue in a little more depth.

@mrunalp
Copy link
Collaborator

mrunalp commented Dec 20, 2016 via email

@mrunalp
Copy link
Collaborator

mrunalp commented Dec 20, 2016

This crashed when I tried to start docker container setting memory.

@mrunalp
Copy link
Collaborator

mrunalp commented Dec 20, 2016

This is what I see in the logs:

19003aa33a5e56ad0c9bb35eafd1e7065b.scope/memory.limit_in_bytes
Dec 20 13:39:17 dhcp-16-119.sjc.redhat.com oci-systemd-hook[8522]: systemdhook <debug>: LIMIT: 1073741824
Dec 20 13:39:17 dhcp-16-119.sjc.redhat.com oci-systemd-hook[8522]: systemdhook <debug>: Limit in bytes: 1073741824
Dec 20 13:39:17 dhcp-16-119.sjc.redhat.com oci-systemd-hook[8522]: systemdhook <error>: Failed to mount tmpfs at /tmp: Invalid argument

@rhatdan
Copy link
Member Author

rhatdan commented Dec 20, 2016

Ok what command did you use to cause this?

@@ -510,9 +527,9 @@ static int prestart(const char *rootfs,
}

if (!strcmp("", mount_label)) {
rc = asprintf(&options, "mode=1777,size=%" PRIu64 "k", memory_limit_in_kb);
rc = asprintf(&options, "mode=1777%s", memory_str);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Missing a comma between mode and size.

if (memory_limit_in_bytes < total_memory) {
/* Set it to half of limit in kb */
uint64_t memory_limit_in_kb = memory_limit_in_bytes / 2048;
snprintf(memory_str, sizeof(memory_str)-1 , "size=%" PRIu64 "k", memory_limit_in_kb);
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think you can add the comma before size here.

@mrunalp
Copy link
Collaborator

mrunalp commented Dec 20, 2016

@rhatdan

docker run -it --rm --memory=1G fedora-httpd

If the total memory on the system is less then the max memory in cgroup
then do not set memory limit on tmpfs, allow the kernel to set the default.
@rhatdan
Copy link
Member Author

rhatdan commented Dec 20, 2016

Put the "," back in, and am setting up a test.

@rhatdan
Copy link
Member Author

rhatdan commented Dec 20, 2016

docker run -ti --memory=1G fedora mount | grep /tmp
tmpfs on /tmp type tmpfs (rw,nosuid,nodev,relatime,context="system_u:object_r:container_file_t:s0:c133,c919",size=524288k)

docker run -ti fedora mount | grep /tmp
tmpfs on /tmp type tmpfs (rw,nosuid,nodev,relatime,context="system_u:object_r:container_file_t:s0:c197,c202")

@mrunalp
Copy link
Collaborator

mrunalp commented Dec 20, 2016

LGTM

@mrunalp mrunalp merged commit c6776e8 into projectatomic:master Dec 20, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants