From: DJ Delorie <dj@redhat.com>
To: Adhemerval Zanella <adhemerval.zanella@linaro.org>
Cc: libc-alpha@sourceware.org, siddhesh@sourceware.org,
nmanthey@conp-solutions.com, guillaume@morinfr.org
Subject: Re: [PATCH v5 1/7] malloc: Add madvise support for Transparent Huge Pages
Date: Tue, 14 Dec 2021 22:06:30 -0500 [thread overview]
Message-ID: <xnmtl2a7nt.fsf@greed.delorie.com> (raw)
In-Reply-To: <20211214185806.4109231-2-adhemerval.zanella@linaro.org>
Minor tweaks to a comment but otherwise LGTM
Reviewed-by: DJ Delorie <dj@redhat.com>
Adhemerval Zanella via Libc-alpha <libc-alpha@sourceware.org> writes:
> +* On Linux, a new tunable, glibc.malloc.hugetlb, can be used to
> + make malloc issue madvise plus MADV_HUGEPAGE on mmap and sbrk calls.
> + It might improve performance with Transparent Huge Pages madvise mode
> + depending of the workload.
Suggest replacing "It" with "Setting this" but it's just NEWS. Ok.
> diff --git a/Rules b/Rules
> @@ -157,6 +157,7 @@ tests: $(tests:%=$(objpfx)%.out) $(tests-internal:%=$(objpfx)%.out) \
> $(tests-container:%=$(objpfx)%.out) \
> $(tests-mcheck:%=$(objpfx)%-mcheck.out) \
> $(tests-malloc-check:%=$(objpfx)%-malloc-check.out) \
> + $(tests-malloc-hugetlb1:%=$(objpfx)%-malloc-hugetlb1.out) \
Ok.
> tests-expected = $(tests) $(tests-internal) $(tests-printers) \
> $(tests-container) $(tests-malloc-check:%=%-malloc-check) \
> + $(tests-malloc-hugetlb1:%=%-malloc-hugetlb1) \
> $(tests-mcheck:%=%-mcheck)
Ok.
> +binaries-malloc-hugetlb1-tests = $(tests-malloc-hugetlb1:%=%-malloc-hugetlb1)
Ok.
> +binaries-malloc-hugetlb1-tests =
Ok.
> +ifneq "$(strip $(binaries-malloc-hugetlb1-tests))" ""
> +$(addprefix $(objpfx),$(binaries-malloc-hugetlb1-tests)): %-malloc-hugetlb1: %.o \
> + $(link-extra-libs-tests) \
> + $(sort $(filter $(common-objpfx)lib%,$(link-libc))) \
> + $(addprefix $(csu-objpfx),start.o) $(+preinit) $(+postinit)
> + $(+link-tests)
> +endif
Adds build rules for new targets, ok.
> +# All malloc-hugetlb1 tests will be run with GLIBC_TUNABLE=glibc.malloc.hugetlb=1
> +define malloc-hugetlb1-ENVS
> +$(1)-malloc-hugetlb1-ENV += GLIBC_TUNABLES=glibc.malloc.hugetlb=1
> +endef
> +$(foreach t,$(tests-malloc-hugetlb1),$(eval $(call malloc-hugetlb1-ENVS,$(t))))
Ok.
> diff --git a/elf/dl-tunables.list b/elf/dl-tunables.list
> + hugetlb {
> + type: INT_32
> + minval: 0
> + maxval: 1
> + }
Ok.
> diff --git a/elf/tst-rtld-list-tunables.exp b/elf/tst-rtld-list-tunables.exp
> +glibc.malloc.hugetlb: 0 (min: 0, max: 1)
Ok.
> diff --git a/malloc/Makefile b/malloc/Makefile
>
> +# Run all testes with GLIBC_TUNABLE=glibc.malloc.hugetlb=1 that check the
> +# Transparent Huge Pages support. We need exclude some tests that define
> +# the ENV vars.
> +tests-exclude-hugetlb1 = \
> + tst-compathooks-off \
> + tst-compathooks-on \
> + tst-interpose-nothread \
> + tst-interpose-thread \
> + tst-interpose-static-nothread \
> + tst-interpose-static-thread \
> + tst-malloc-usable \
> + tst-malloc-usable-tunables \
> + tst-mallocstate
> +tests-malloc-hugetlb1 = \
> + $(filter-out $(tests-exclude-hugetlb1), $(tests))
Ok.
> diff --git a/malloc/arena.c b/malloc/arena.c
> +TUNABLE_CALLBACK_FNDECL (set_hugetlb, int32_t)
Ok.
> + TUNABLE_GET (hugetlb, int32_t, TUNABLE_CALLBACK (set_hugetlb));
Ok.
> @@ -508,6 +510,9 @@ new_heap (size_t size, size_t top_pad)
> +
> + madvise_thp (p2, size);
Ok.
> diff --git a/malloc/malloc-internal.h b/malloc/malloc-internal.h
> +#include <malloc-hugepages.h>
Ok.
> diff --git a/malloc/malloc.c b/malloc/malloc.c
> +#if HAVE_TUNABLES
> + /* Transparent Large Page support. */
> + INTERNAL_SIZE_T thp_pagesize;
> +#endif
Ok.
> @@ -2008,6 +2013,20 @@ free_perturb (char *p, size_t n)
> +/* ----------- Routines dealing with transparent huge pages ----------- */
> +
> +static inline void
> +madvise_thp (void *p, INTERNAL_SIZE_T size)
> +{
> +#if HAVE_TUNABLES && defined (MADV_HUGEPAGE)
> + /* Do not consider areas smaller than a huge page or if the tunable is
> + not active. */
> + if (mp_.thp_pagesize == 0 || size < mp_.thp_pagesize)
> + return;
> + __madvise (p, size, MADV_HUGEPAGE);
> +#endif
> +}
Ok.
> + madvise_thp (mm, size);
Ok.
> if (size > 0)
> {
> brk = (char *) (MORECORE (size));
> + if (brk != (char *) (MORECORE_FAILURE))
> + madvise_thp (brk, size);
> LIBC_PROBE (memory_sbrk_more, 2, brk, size);
> }
Ok.
> if (mbrk != MAP_FAILED)
> {
> + madvise_thp (mbrk, size);
Ok.
> + else
> + madvise_thp (snd_brk, correction);
Ok.
> @@ -2988,6 +3015,8 @@ mremap_chunk (mchunkptr p, size_t new_size)
> if (cp == MAP_FAILED)
> return 0;
>
> + madvise_thp (cp, new_size);
Ok.
> +#if HAVE_TUNABLES
> +static __always_inline int
> +do_set_hugetlb (int32_t value)
> +{
> + if (value == 1)
> + {
> + enum malloc_thp_mode_t thp_mode = __malloc_thp_mode ();
> + /*
> + Only enables THP usage is system does support it and has at least
> + always or madvise mode. Otherwise the madvise() call is wasteful.
> + */
> + if (thp_mode == malloc_thp_mode_madvise)
> + mp_.thp_pagesize = __malloc_default_thp_pagesize ();
> + }
> + return 0;
> +}
> +#endif
s/is/if/
The "always or madvise mode" comment doesn't match the "== madvise"
logic. I suspect the logic is ok, so... ok with fixed comment.
> diff --git a/manual/tunables.texi b/manual/tunables.texi
> +@deftp Tunable glibc.malloc.hugetlb
> +This tunable controls the usage of Huge Pages on @code{malloc} calls. The
> +default value is @code{0}, which disables any additional support on
> +@code{malloc}.
> +
> +Setting its value to @code{1} enables the use of @code{madvise} with
> +@code{MADV_HUGEPAGE} after memory allocation with @code{mmap}. It is enabled
> +only if the system supports Transparent Huge Page (currently only on Linux).
> +@end deftp
Ok.
> diff --git a/sysdeps/generic/Makefile b/sysdeps/generic/Makefile
> +ifeq ($(subdir),malloc)
> +sysdep_malloc_debug_routines += malloc-hugepages
> +endif
> +
> +ifeq ($(subdir),misc)
> +sysdep_routines += malloc-hugepages
> +endif
Ok.
> diff --git a/sysdeps/generic/malloc-hugepages.c b/sysdeps/generic/malloc-hugepages.c
> +/* Huge Page support. Generic implementation.
> + Copyright (C) 2021 Free Software Foundation, Inc.
> + This file is part of the GNU C Library.
> +
> + The GNU C Library is free software; you can redistribute it and/or
> + modify it under the terms of the GNU Lesser General Public License as
> + published by the Free Software Foundation; either version 2.1 of the
> + License, or (at your option) any later version.
> +
> + The GNU C Library is distributed in the hope that it will be useful,
> + but WITHOUT ANY WARRANTY; without even the implied warranty of
> + MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
> + Lesser General Public License for more details.
> +
> + You should have received a copy of the GNU Lesser General Public
> + License along with the GNU C Library; see the file COPYING.LIB. If
> + not, see <https://www.gnu.org/licenses/>. */
> +
> +#include <malloc-hugepages.h>
> +
> +unsigned long int
> +__malloc_default_thp_pagesize (void)
> +{
> + return 0;
> +}
> +
> +enum malloc_thp_mode_t
> +__malloc_thp_mode (void)
> +{
> + return malloc_thp_mode_not_supported;
> +}
Ok.
> diff --git a/sysdeps/generic/malloc-hugepages.h b/sysdeps/generic/malloc-hugepages.h
> +/* Malloc huge page support. Generic implementation.
> + Copyright (C) 2021 Free Software Foundation, Inc.
> + This file is part of the GNU C Library.
> +
> + The GNU C Library is free software; you can redistribute it and/or
> + modify it under the terms of the GNU Lesser General Public License as
> + published by the Free Software Foundation; either version 2.1 of the
> + License, or (at your option) any later version.
> +
> + The GNU C Library is distributed in the hope that it will be useful,
> + but WITHOUT ANY WARRANTY; without even the implied warranty of
> + MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
> + Lesser General Public License for more details.
> +
> + You should have received a copy of the GNU Lesser General Public
> + License along with the GNU C Library; see the file COPYING.LIB. If
> + not, see <https://www.gnu.org/licenses/>. */
> +
> +#ifndef _MALLOC_HUGEPAGES_H
> +#define _MALLOC_HUGEPAGES_H
> +
> +#include <stddef.h>
> +
> +/* Return the default transparent huge page size. */
> +unsigned long int __malloc_default_thp_pagesize (void) attribute_hidden;
> +
> +enum malloc_thp_mode_t
> +{
> + malloc_thp_mode_always,
> + malloc_thp_mode_madvise,
> + malloc_thp_mode_never,
> + malloc_thp_mode_not_supported
> +};
> +
> +enum malloc_thp_mode_t __malloc_thp_mode (void) attribute_hidden;
> +
> +#endif /* _MALLOC_HUGEPAGES_H */
Ok.
> diff --git a/sysdeps/unix/sysv/linux/malloc-hugepages.c b/sysdeps/unix/sysv/linux/malloc-hugepages.c
> +/* Huge Page support. Linux implementation.
> + Copyright (C) 2021 Free Software Foundation, Inc.
> + This file is part of the GNU C Library.
> +
> + The GNU C Library is free software; you can redistribute it and/or
> + modify it under the terms of the GNU Lesser General Public License as
> + published by the Free Software Foundation; either version 2.1 of the
> + License, or (at your option) any later version.
> +
> + The GNU C Library is distributed in the hope that it will be useful,
> + but WITHOUT ANY WARRANTY; without even the implied warranty of
> + MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
> + Lesser General Public License for more details.
> +
> + You should have received a copy of the GNU Lesser General Public
> + License along with the GNU C Library; see the file COPYING.LIB. If
> + not, see <https://www.gnu.org/licenses/>. */
> +
> +#include <intprops.h>
> +#include <malloc-hugepages.h>
> +#include <not-cancel.h>
> +
Ok.
> +unsigned long int
> +__malloc_default_thp_pagesize (void)
> +{
> + int fd = __open64_nocancel (
> + "/sys/kernel/mm/transparent_hugepage/hpage_pmd_size", O_RDONLY);
> + if (fd == -1)
> + return 0;
> +
> + char str[INT_BUFSIZE_BOUND (unsigned long int)];
> + ssize_t s = __read_nocancel (fd, str, sizeof (str));
> + __close_nocancel (fd);
> + if (s < 0)
> + return 0;
> +
> + unsigned long int r = 0;
> + for (ssize_t i = 0; i < s; i++)
> + {
> + if (str[i] == '\n')
> + break;
> + r *= 10;
> + r += str[i] - '0';
> + }
> + return r;
> +}
Ok.
> +enum malloc_thp_mode_t
> +__malloc_thp_mode (void)
> +{
> + int fd = __open64_nocancel ("/sys/kernel/mm/transparent_hugepage/enabled",
> + O_RDONLY);
> + if (fd == -1)
> + return malloc_thp_mode_not_supported;
> +
> + static const char mode_always[] = "[always] madvise never\n";
> + static const char mode_madvise[] = "always [madvise] never\n";
> + static const char mode_never[] = "always madvise [never]\n";
> +
> + char str[sizeof(mode_always)];
> + ssize_t s = __read_nocancel (fd, str, sizeof (str));
> + __close_nocancel (fd);
> +
> + if (s == sizeof (mode_always) - 1)
> + {
> + if (strcmp (str, mode_always) == 0)
> + return malloc_thp_mode_always;
> + else if (strcmp (str, mode_madvise) == 0)
> + return malloc_thp_mode_madvise;
> + else if (strcmp (str, mode_never) == 0)
> + return malloc_thp_mode_never;
> + }
> + return malloc_thp_mode_not_supported;
> +}
Ok.
next prev parent reply other threads:[~2021-12-15 3:06 UTC|newest]
Thread overview: 19+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-12-14 18:57 [PATCH v5 0/7] malloc: Improve Huge Page support Adhemerval Zanella
2021-12-14 18:58 ` [PATCH v5 1/7] malloc: Add madvise support for Transparent Huge Pages Adhemerval Zanella
2021-12-15 3:06 ` DJ Delorie [this message]
2021-12-15 12:12 ` Adhemerval Zanella
2021-12-14 18:58 ` [PATCH v5 2/7] malloc: Add THP/madvise support for sbrk Adhemerval Zanella
2021-12-15 3:28 ` DJ Delorie
2021-12-14 18:58 ` [PATCH v5 3/7] malloc: Move mmap logic to its own function Adhemerval Zanella
2021-12-15 3:30 ` DJ Delorie
2021-12-14 18:58 ` [PATCH v5 4/7] malloc: Add Huge Page support for mmap() Adhemerval Zanella
2021-12-15 4:26 ` DJ Delorie
2021-12-15 13:08 ` Adhemerval Zanella
2021-12-15 17:43 ` DJ Delorie
2021-12-14 18:58 ` [PATCH v5 5/7] malloc: Add huge page support to arenas Adhemerval Zanella
2021-12-15 4:51 ` DJ Delorie
2021-12-15 15:06 ` Adhemerval Zanella
2021-12-14 18:58 ` [PATCH v5 6/7] malloc: Move MORECORE fallback mmap to sysmalloc_mmap_fallback Adhemerval Zanella
2021-12-15 4:55 ` DJ Delorie
2021-12-14 18:58 ` [PATCH v5 7/7] malloc: Enable huge page support on main arena Adhemerval Zanella
2021-12-15 4:59 ` DJ Delorie
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=xnmtl2a7nt.fsf@greed.delorie.com \
--to=dj@redhat.com \
--cc=adhemerval.zanella@linaro.org \
--cc=guillaume@morinfr.org \
--cc=libc-alpha@sourceware.org \
--cc=nmanthey@conp-solutions.com \
--cc=siddhesh@sourceware.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).