This is the mail archive of the libc-alpha@sourceware.org mailing list for the glibc project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: [PATCH v2] Allow shrinking of arena heaps using mmap based onovercommit settings when available


> --- /dev/null
> +++ b/sysdeps/generic/malloc-sysdep.h
> @@ -0,0 +1,28 @@
> +/* System-specific implementations for some malloc support functions.

Make it:

/* System-specific malloc support functions.  Generic version.

> +
> +   Copyright (C) 2012 Free Software Foundation, Inc.

Drop this blank line.

> +#include <unistd.h>

There's no need for this.  Drop it.

> +/* Force an unmap on heap shrink for setuid programs.  This is so that the
> +   kernel immediately drops the PTE for the mapping.  */

Say "for a secure exec" rather than "for setuid programs".  Talk about
user-observable effects, not kernel/hardware implementation details:

/* Force an unmap when the heap shrinks in a secure exec.
   This ensures that the old data pages immediately cease to be accessible. */

> --- /dev/null
> +++ b/sysdeps/unix/sysv/linux/malloc-sysdep.h
> @@ -0,0 +1,61 @@
> +/* System-specific implementations for some malloc support functions.

Make it:

/* System-specific malloc support functions.  Linux version.

> +
> +   Copyright (C) 2012 Free Software Foundation, Inc.

Drop this blank line.

> +/* The Linux kernel overcommits address space by default and if there is not
> +   enough memory available, it uses various parameters to decide the process to
> +   kill.  It is however possible to disable or curb this overcommit behaviour
> +   by setting the proc sysctl vm.overcommit_memory to the value '2' and with
> +   that, a process is only allowed to use the maximum of a pre-determined
> +   fraction of the total address space.  In such a case, we want to make sure
> +   that we are judicious with our heap usage as well, and explicitly give away
> +   the freed top of the heap to reduce our commit charge.  See the proc(5) man
> +   page to know more about overcommit behaviour.
> +
> +   Other than that, we also force an unmap for setuid programs.  */

Please use American English spelling ("behavior") for consistency with the
rest of the code base.

> +  if (__builtin_expect (may_shrink_heap, 0))
> +    return may_shrink_heap;

Reverse the sense of the test and put the code before the return inside the if.

> +  int fd = open_not_cancel_2 ("/proc/sys/vm/overcommit_memory",
> +			      O_RDONLY | O_CLOEXEC);
> +
> +  if (fd != -1)

Drop the blank line.  We usually use 'fd < 0', though it doesn't matter much.

> +    {
> +      char val = 0;

There is no need to initialize this.

> +      int n = read_not_cancel (fd, &val, 1);

Use ssize_t.

> +      may_shrink_heap = (n > 0 && val == 2);

There's no need for parens here.  Correct the constant as previously
mentioned.  The fact that this slipped through indicates you didn't do much
testing.  Please report what testing you've done on the exact patch version
you post next.


Thanks,
Roland


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]