Improve ggc-page fragmentation

Improve ggc-page fragmentation

[Andi Kleen]

I ran into problems with virtual memory fragmentation ggc-page on 
large LTO builds. The memory was so fragmented that builds
failed because the compiler would use more than the 64k mappings
Linux allows each process by default. 

For more details see PR 50636

This patchkit includes various improvements to the fragmentation
behaviour plus some optimizations to increase the use of 2MB
pages on modern Linux kernels. This fixes the fragmentation
problem for me and increases the use of huge pages significantly.

My simple benchmarks didn't show a lot of performance improvement
though.

On non Linux kernels the fragmentation problem will be still
somewhat visible (the best fix is using the Linux specific
MADV_DONTNEED), but the new threshold should still improve things
there.

All passed bootstrap and test suite run on x86-64.

-Andi

[PATCH 2/5] Increase the GGC quite size to 2MB

[Andi Kleen]

From: Andi Kleen <ak@linux.intel.com>

Using 2MB allows modern kernels to use 2MB huge pages on x86.

gcc/:

2011-10-08   Andi Kleen <ak@linux.intel.com>

	* ggc-page.c (GGC_QUIRE_SIZE): Increase to 512
---
 gcc/ggc-page.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/gcc/ggc-page.c b/gcc/ggc-page.c
index b0b3b3f..1f52b56 100644
--- a/gcc/ggc-page.c
+++ b/gcc/ggc-page.c
@@ -469,7 +469,7 @@ static struct globals
    can override this by defining GGC_QUIRE_SIZE explicitly.  */
 #ifndef GGC_QUIRE_SIZE
 # ifdef USING_MMAP
-#  define GGC_QUIRE_SIZE 256
+#  define GGC_QUIRE_SIZE 512	/* 2MB for 4K pages */
 # else
 #  define GGC_QUIRE_SIZE 16
 # endif
-- 
1.7.5.4

Re: [PATCH 2/5] Increase the GGC quite size to 2MB

[Richard Guenther]

On Sun, Oct 9, 2011 at 9:55 PM, Andi Kleen <andi@firstfloor.org> wrote:
> From: Andi Kleen <ak@linux.intel.com>
>
> Using 2MB allows modern kernels to use 2MB huge pages on x86.

Ok.

Thanks,
Richard.

> gcc/:
>
> 2011-10-08   Andi Kleen <ak@linux.intel.com>
>
>        * ggc-page.c (GGC_QUIRE_SIZE): Increase to 512
> ---
>  gcc/ggc-page.c |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)
>
> diff --git a/gcc/ggc-page.c b/gcc/ggc-page.c
> index b0b3b3f..1f52b56 100644
> --- a/gcc/ggc-page.c
> +++ b/gcc/ggc-page.c
> @@ -469,7 +469,7 @@ static struct globals
>    can override this by defining GGC_QUIRE_SIZE explicitly.  */
>  #ifndef GGC_QUIRE_SIZE
>  # ifdef USING_MMAP
> -#  define GGC_QUIRE_SIZE 256
> +#  define GGC_QUIRE_SIZE 512   /* 2MB for 4K pages */
>  # else
>  #  define GGC_QUIRE_SIZE 16
>  # endif
> --
> 1.7.5.4
>
>

Re: [PATCH 2/5] Increase the GGC quite size to 2MB

[Jan Hubicka]

> From: Andi Kleen <ak@linux.intel.com>
> 
> Using 2MB allows modern kernels to use 2MB huge pages on x86.
> 
> gcc/:
> 
> 2011-10-08   Andi Kleen <ak@linux.intel.com>
> 
> 	* ggc-page.c (GGC_QUIRE_SIZE): Increase to 512
> ---
>  gcc/ggc-page.c |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)
> 
> diff --git a/gcc/ggc-page.c b/gcc/ggc-page.c
> index b0b3b3f..1f52b56 100644
> --- a/gcc/ggc-page.c
> +++ b/gcc/ggc-page.c
> @@ -469,7 +469,7 @@ static struct globals
>     can override this by defining GGC_QUIRE_SIZE explicitly.  */
>  #ifndef GGC_QUIRE_SIZE
>  # ifdef USING_MMAP
> -#  define GGC_QUIRE_SIZE 256
> +#  define GGC_QUIRE_SIZE 512	/* 2MB for 4K pages */
Perhaps comment that 2MB was chosen to help Kernel huge pages logic?

Honza
>  # else
>  #  define GGC_QUIRE_SIZE 16
>  # endif
> -- 
> 1.7.5.4

[PATCH 3/5] On a Linux kernel ask explicitely for a huge page in ggc

[Andi Kleen]

From: Andi Kleen <ak@linux.intel.com>

Benchmarks show slightly faster build times on a kernel
build, near the measurement error unfortunately.

This will only work with a recent glibc that defines MADV_HUGEPAGE.

2011-10-08   Andi Kleen <ak@linux.intel.com>

	* ggc-page.c (alloc_page): Add madvise for hugepage
---
 gcc/ggc-page.c |    5 +++++
 1 files changed, 5 insertions(+), 0 deletions(-)

diff --git a/gcc/ggc-page.c b/gcc/ggc-page.c
index 1f52b56..6e08cda 100644
--- a/gcc/ggc-page.c
+++ b/gcc/ggc-page.c
@@ -779,6 +779,11 @@ alloc_page (unsigned order)
 
       page = alloc_anon (NULL, G.pagesize * GGC_QUIRE_SIZE);
 
+#if defined(HAVE_MADVISE) && defined(MADV_HUGEPAGE)
+      /* Kernel, I would like to have hugepages, please. */
+      madvise(page, G.pagesize * GGC_QUIRE_SIZE, MADV_HUGEPAGE);
+#endif
+
       /* This loop counts down so that the chain will be in ascending
 	 memory order.  */
       for (i = GGC_QUIRE_SIZE - 1; i >= 1; i--)
-- 
1.7.5.4

Re: [PATCH 3/5] On a Linux kernel ask explicitely for a huge page in ggc

[Richard Guenther]

On Sun, Oct 9, 2011 at 9:55 PM, Andi Kleen <andi@firstfloor.org> wrote:
> From: Andi Kleen <ak@linux.intel.com>
>
> Benchmarks show slightly faster build times on a kernel
> build, near the measurement error unfortunately.
>
> This will only work with a recent glibc that defines MADV_HUGEPAGE.

Will partial unmaps fail or split the page?

Richard.

> 2011-10-08   Andi Kleen <ak@linux.intel.com>
>
>        * ggc-page.c (alloc_page): Add madvise for hugepage
> ---
>  gcc/ggc-page.c |    5 +++++
>  1 files changed, 5 insertions(+), 0 deletions(-)
>
> diff --git a/gcc/ggc-page.c b/gcc/ggc-page.c
> index 1f52b56..6e08cda 100644
> --- a/gcc/ggc-page.c
> +++ b/gcc/ggc-page.c
> @@ -779,6 +779,11 @@ alloc_page (unsigned order)
>
>       page = alloc_anon (NULL, G.pagesize * GGC_QUIRE_SIZE);
>
> +#if defined(HAVE_MADVISE) && defined(MADV_HUGEPAGE)
> +      /* Kernel, I would like to have hugepages, please. */
> +      madvise(page, G.pagesize * GGC_QUIRE_SIZE, MADV_HUGEPAGE);
> +#endif
> +
>       /* This loop counts down so that the chain will be in ascending
>         memory order.  */
>       for (i = GGC_QUIRE_SIZE - 1; i >= 1; i--)
> --
> 1.7.5.4
>
>

Re: [PATCH 3/5] On a Linux kernel ask explicitely for a huge page in ggc

[Jakub Jelinek]

On Mon, Oct 10, 2011 at 12:15:14PM +0200, Richard Guenther wrote:
> On Sun, Oct 9, 2011 at 9:55 PM, Andi Kleen <andi@firstfloor.org> wrote:
> > From: Andi Kleen <ak@linux.intel.com>
> >
> > Benchmarks show slightly faster build times on a kernel
> > build, near the measurement error unfortunately.
> >
> > This will only work with a recent glibc that defines MADV_HUGEPAGE.
> 
> Will partial unmaps fail or split the page?

I think it will not do anything, because with G.pagesize * GGC_QUIRE_SIZE
being just 2MB (and most likely not 2MB aligned either) it would do
something only if it happens to be 2MB aligned or the following VMA
is also MADV_HUGEPAGE hinted and no pages in the 2MB region have been
paged in yet.
So, either we need to ensure that the address is likely 2MB aligned,
or use on x86_64 even bigger GGC_QUIRE_SIZE (such as 16MB or 32MB) and
then huge pages would be likely used at least for the aligned inner
huge pages in the region.

Additionally, IMHO we shouldn't use this MADV_HUGEPAGE size on all hosts
that define it, that is a useless syscall for hosts which don't support
huge pages.  So should be guarded by some macro defined in host headers.
> > +#if defined(HAVE_MADVISE) && defined(MADV_HUGEPAGE)
> > +      /* Kernel, I would like to have hugepages, please. */
> > +      madvise(page, G.pagesize * GGC_QUIRE_SIZE, MADV_HUGEPAGE);

Please watch formatting, space is missing after madvise.

	Jakub

Re: [PATCH 3/5] On a Linux kernel ask explicitely for a huge page in ggc

[Andi Kleen]

On Mon, Oct 10, 2011 at 12:29:03PM +0200, Jakub Jelinek wrote:
> On Mon, Oct 10, 2011 at 12:15:14PM +0200, Richard Guenther wrote:
> > On Sun, Oct 9, 2011 at 9:55 PM, Andi Kleen <andi@firstfloor.org> wrote:
> > > From: Andi Kleen <ak@linux.intel.com>
> > >
> > > Benchmarks show slightly faster build times on a kernel
> > > build, near the measurement error unfortunately.
> > >
> > > This will only work with a recent glibc that defines MADV_HUGEPAGE.
> > 
> > Will partial unmaps fail or split the page?
> 
> I think it will not do anything, because with G.pagesize * GGC_QUIRE_SIZE

On large builds (LTO) I see the vmstat THP counter increasing, 
it doesn't do too much on small builds.

> being just 2MB (and most likely not 2MB aligned either) it would do
> something only if it happens to be 2MB aligned or the following VMA
> is also MADV_HUGEPAGE hinted and no pages in the 2MB region have been
> paged in yet.
> So, either we need to ensure that the address is likely 2MB aligned,
> or use on x86_64 even bigger GGC_QUIRE_SIZE (such as 16MB or 32MB) and
> then huge pages would be likely used at least for the aligned inner
> huge pages in the region.

Hmm, that gets too complicated for mee. I'll drop the patch for now.
It's not strictly needed to fix the fragmentation problem
and it's also possible to force THP from the kernel.

-Andi
-- 
ak@linux.intel.com -- Speaking for myself only.

Re: [PATCH 3/5] On a Linux kernel ask explicitely for a huge page in ggc

[Andi Kleen]

On Mon, Oct 10, 2011 at 12:15:14PM +0200, Richard Guenther wrote:
> On Sun, Oct 9, 2011 at 9:55 PM, Andi Kleen <andi@firstfloor.org> wrote:
> > From: Andi Kleen <ak@linux.intel.com>
> >
> > Benchmarks show slightly faster build times on a kernel
> > build, near the measurement error unfortunately.
> >
> > This will only work with a recent glibc that defines MADV_HUGEPAGE.
> 
> Will partial unmaps fail or split the page?

They split the page.

-Andi

[PATCH 5/5] Add error checking to lto_section_read

[Andi Kleen]

From: Andi Kleen <ak@linux.intel.com>

Various callers of lto_section_read segfault on a NULL return
when the mmap fails. Add some internal_errors to give a better
message to the user.

gcc/lto/:

2011-10-09   Andi Kleen <ak@linux.intel.com>

	* lto.c (lto_section_read): Call internal_error on IO or mmap errors.
---
 gcc/lto/lto.c |   11 +++++++++--
 1 files changed, 9 insertions(+), 2 deletions(-)

diff --git a/gcc/lto/lto.c b/gcc/lto/lto.c
index a77eeb4..dc16db4 100644
--- a/gcc/lto/lto.c
+++ b/gcc/lto/lto.c
@@ -1237,7 +1237,10 @@ lto_read_section_data (struct lto_file_decl_data *file_data,
     {
       fd = open (file_data->file_name, O_RDONLY|O_BINARY);
       if (fd == -1)
-	return NULL;
+        {
+	  internal_error ("Cannot open %s", file_data->file_name);
+	  return NULL;
+        }
       fd_name = xstrdup (file_data->file_name);
     }
 
@@ -1255,7 +1258,10 @@ lto_read_section_data (struct lto_file_decl_data *file_data,
   result = (char *) mmap (NULL, computed_len, PROT_READ, MAP_PRIVATE,
 			  fd, computed_offset);
   if (result == MAP_FAILED)
-    return NULL;
+    {
+      internal_error ("Cannot map %s", file_data->file_name);
+      return NULL;
+    }
 
   return result + diff;
 #else
@@ -1264,6 +1270,7 @@ lto_read_section_data (struct lto_file_decl_data *file_data,
       || read (fd, result, len) != (ssize_t) len)
     {
       free (result);
+      internal_error ("Cannot read %s", file_data->file_name);
       result = NULL;
     }
 #ifdef __MINGW32__
-- 
1.7.5.4

Re: [PATCH 5/5] Add error checking to lto_section_read

[Richard Guenther]

On Sun, Oct 9, 2011 at 9:55 PM, Andi Kleen <andi@firstfloor.org> wrote:
> From: Andi Kleen <ak@linux.intel.com>
>
> Various callers of lto_section_read segfault on a NULL return
> when the mmap fails. Add some internal_errors to give a better
> message to the user.

Hm, shouldn't these be fatal_error () then?  Ok with that change.

Thanks,
Richard.

> gcc/lto/:
>
> 2011-10-09   Andi Kleen <ak@linux.intel.com>
>
>        * lto.c (lto_section_read): Call internal_error on IO or mmap errors.
> ---
>  gcc/lto/lto.c |   11 +++++++++--
>  1 files changed, 9 insertions(+), 2 deletions(-)
>
> diff --git a/gcc/lto/lto.c b/gcc/lto/lto.c
> index a77eeb4..dc16db4 100644
> --- a/gcc/lto/lto.c
> +++ b/gcc/lto/lto.c
> @@ -1237,7 +1237,10 @@ lto_read_section_data (struct lto_file_decl_data *file_data,
>     {
>       fd = open (file_data->file_name, O_RDONLY|O_BINARY);
>       if (fd == -1)
> -       return NULL;
> +        {
> +         internal_error ("Cannot open %s", file_data->file_name);
> +         return NULL;
> +        }
>       fd_name = xstrdup (file_data->file_name);
>     }
>
> @@ -1255,7 +1258,10 @@ lto_read_section_data (struct lto_file_decl_data *file_data,
>   result = (char *) mmap (NULL, computed_len, PROT_READ, MAP_PRIVATE,
>                          fd, computed_offset);
>   if (result == MAP_FAILED)
> -    return NULL;
> +    {
> +      internal_error ("Cannot map %s", file_data->file_name);
> +      return NULL;
> +    }
>
>   return result + diff;
>  #else
> @@ -1264,6 +1270,7 @@ lto_read_section_data (struct lto_file_decl_data *file_data,
>       || read (fd, result, len) != (ssize_t) len)
>     {
>       free (result);
> +      internal_error ("Cannot read %s", file_data->file_name);
>       result = NULL;
>     }
>  #ifdef __MINGW32__
> --
> 1.7.5.4
>
>

[PATCH 4/5] Add a freeing threshold for the garbage collector.

[Andi Kleen]

From: Andi Kleen <ak@linux.intel.com>

Add a threshold to avoid freeing pages back too early to the OS.
This avoid virtual memory map fragmentation.

Based on a idea from Honza

ggc/doc/:

2011-10-08   Andi Kleen <ak@linux.intel.com>

	PR other/50636
	* invoke.texi (ggc-free-threshold, ggc-free-min): Add.

ggc/:

2011-10-08   Andi Kleen <ak@linux.intel.com>

	PR other/50636
	* ggc-page.c (ggc_collect): Add free threshold.
	* params.def (GGC_FREE_THRESHOLD, GGC_FREE_MIN): Add.
---
 gcc/doc/invoke.texi |   11 +++++++++++
 gcc/ggc-page.c      |   13 +++++++++----
 gcc/params.def      |   10 ++++++++++
 3 files changed, 30 insertions(+), 4 deletions(-)

diff --git a/gcc/doc/invoke.texi b/gcc/doc/invoke.texi
index ef7ac68..6557f66 100644
--- a/gcc/doc/invoke.texi
+++ b/gcc/doc/invoke.texi
@@ -8837,6 +8837,17 @@ very large effectively disables garbage collection.  Setting this
 parameter and @option{ggc-min-expand} to zero causes a full collection
 to occur at every opportunity.
 
+@item ggc-free-threshold
+
+Only free memory back to the system when it would free more than this
+many percent of the total allocated memory. Default is 20 percent.
+This avoids memory fragmentation.
+
+@item ggc-free-min
+
+Only free memory back to the system when it would free more than this.
+Unit is kilobytes. 
+
 @item max-reload-search-insns
 The maximum number of instruction reload should look backward for equivalent
 register.  Increasing values mean more aggressive optimization, making the
diff --git a/gcc/ggc-page.c b/gcc/ggc-page.c
index 6e08cda..cd1c41a 100644
--- a/gcc/ggc-page.c
+++ b/gcc/ggc-page.c
@@ -1968,14 +1968,19 @@ ggc_collect (void)
   if (GGC_DEBUG_LEVEL >= 2)
     fprintf (G.debug_file, "BEGIN COLLECTING\n");
 
+  /* Release the pages we freed the last time we collected, but didn't
+     reuse in the interim.  But only do this if this would free a 
+     reasonable number of pages. Otherwise hold on to them
+     to avoid virtual memory fragmentation. */
+  if (G.bytes_mapped - G.allocated >= 
+	(PARAM_VALUE (GGC_FREE_THRESHOLD) / 100.0) * G.bytes_mapped &&
+      G.bytes_mapped - G.allocated >= (size_t)PARAM_VALUE (GGC_FREE_MIN) * 1024)
+    release_pages ();
+
   /* Zero the total allocated bytes.  This will be recalculated in the
      sweep phase.  */
   G.allocated = 0;
 
-  /* Release the pages we freed the last time we collected, but didn't
-     reuse in the interim.  */
-  release_pages ();
-
   /* Indicate that we've seen collections at this context depth.  */
   G.context_depth_collections = ((unsigned long)1 << (G.context_depth + 1)) - 1;
 
diff --git a/gcc/params.def b/gcc/params.def
index 5e49c48..ca28715 100644
--- a/gcc/params.def
+++ b/gcc/params.def
@@ -561,6 +561,16 @@ DEFPARAM(GGC_MIN_HEAPSIZE,
 #undef GGC_MIN_EXPAND_DEFAULT
 #undef GGC_MIN_HEAPSIZE_DEFAULT
 
+DEFPARAM(GGC_FREE_THRESHOLD,
+	"ggc-free-threshold",
+	"Dont free memory back to system less this percent of the total memory",
+	20, 0, 100)
+
+DEFPARAM(GGC_FREE_MIN,
+	 "ggc-free-min",
+	 "Dont free less memory than this back to the system, in kilobytes",
+	 8 * 1024, 0, 0)
+
 DEFPARAM(PARAM_MAX_RELOAD_SEARCH_INSNS,
 	 "max-reload-search-insns",
 	 "The maximum number of instructions to search backward when looking for equivalent reload",
-- 
1.7.5.4

Re: [PATCH 4/5] Add a freeing threshold for the garbage collector.

[Richard Guenther]

On Sun, Oct 9, 2011 at 9:55 PM, Andi Kleen <andi@firstfloor.org> wrote:
> From: Andi Kleen <ak@linux.intel.com>
>
> Add a threshold to avoid freeing pages back too early to the OS.
> This avoid virtual memory map fragmentation.
>
> Based on a idea from Honza

Less than 20% looks high.  Shouldn't ggc-free-min be enough to
avoid fragmentation?

This will hide gc bugs with always-collect (ggc-checking), so
the parameter(s) need to be adjusted for that case to always
give pages back.  The current values should probably be printed
where the two existing ones are printed as well (with -v).

Thanks,
Richard.

> ggc/doc/:
>
> 2011-10-08   Andi Kleen <ak@linux.intel.com>
>
>        PR other/50636
>        * invoke.texi (ggc-free-threshold, ggc-free-min): Add.
>
> ggc/:
>
> 2011-10-08   Andi Kleen <ak@linux.intel.com>
>
>        PR other/50636
>        * ggc-page.c (ggc_collect): Add free threshold.
>        * params.def (GGC_FREE_THRESHOLD, GGC_FREE_MIN): Add.
> ---
>  gcc/doc/invoke.texi |   11 +++++++++++
>  gcc/ggc-page.c      |   13 +++++++++----
>  gcc/params.def      |   10 ++++++++++
>  3 files changed, 30 insertions(+), 4 deletions(-)
>
> diff --git a/gcc/doc/invoke.texi b/gcc/doc/invoke.texi
> index ef7ac68..6557f66 100644
> --- a/gcc/doc/invoke.texi
> +++ b/gcc/doc/invoke.texi
> @@ -8837,6 +8837,17 @@ very large effectively disables garbage collection.  Setting this
>  parameter and @option{ggc-min-expand} to zero causes a full collection
>  to occur at every opportunity.
>
> +@item ggc-free-threshold
> +
> +Only free memory back to the system when it would free more than this
> +many percent of the total allocated memory. Default is 20 percent.
> +This avoids memory fragmentation.
> +
> +@item ggc-free-min
> +
> +Only free memory back to the system when it would free more than this.
> +Unit is kilobytes.
> +
>  @item max-reload-search-insns
>  The maximum number of instruction reload should look backward for equivalent
>  register.  Increasing values mean more aggressive optimization, making the
> diff --git a/gcc/ggc-page.c b/gcc/ggc-page.c
> index 6e08cda..cd1c41a 100644
> --- a/gcc/ggc-page.c
> +++ b/gcc/ggc-page.c
> @@ -1968,14 +1968,19 @@ ggc_collect (void)
>   if (GGC_DEBUG_LEVEL >= 2)
>     fprintf (G.debug_file, "BEGIN COLLECTING\n");
>
> +  /* Release the pages we freed the last time we collected, but didn't
> +     reuse in the interim.  But only do this if this would free a
> +     reasonable number of pages. Otherwise hold on to them
> +     to avoid virtual memory fragmentation. */
> +  if (G.bytes_mapped - G.allocated >=
> +       (PARAM_VALUE (GGC_FREE_THRESHOLD) / 100.0) * G.bytes_mapped &&
> +      G.bytes_mapped - G.allocated >= (size_t)PARAM_VALUE (GGC_FREE_MIN) * 1024)
> +    release_pages ();
> +
>   /* Zero the total allocated bytes.  This will be recalculated in the
>      sweep phase.  */
>   G.allocated = 0;
>
> -  /* Release the pages we freed the last time we collected, but didn't
> -     reuse in the interim.  */
> -  release_pages ();
> -
>   /* Indicate that we've seen collections at this context depth.  */
>   G.context_depth_collections = ((unsigned long)1 << (G.context_depth + 1)) - 1;
>
> diff --git a/gcc/params.def b/gcc/params.def
> index 5e49c48..ca28715 100644
> --- a/gcc/params.def
> +++ b/gcc/params.def
> @@ -561,6 +561,16 @@ DEFPARAM(GGC_MIN_HEAPSIZE,
>  #undef GGC_MIN_EXPAND_DEFAULT
>  #undef GGC_MIN_HEAPSIZE_DEFAULT
>
> +DEFPARAM(GGC_FREE_THRESHOLD,
> +       "ggc-free-threshold",
> +       "Dont free memory back to system less this percent of the total memory",
> +       20, 0, 100)
> +
> +DEFPARAM(GGC_FREE_MIN,
> +        "ggc-free-min",
> +        "Dont free less memory than this back to the system, in kilobytes",
> +        8 * 1024, 0, 0)
> +
>  DEFPARAM(PARAM_MAX_RELOAD_SEARCH_INSNS,
>         "max-reload-search-insns",
>         "The maximum number of instructions to search backward when looking for equivalent reload",
> --
> 1.7.5.4
>
>

Re: [PATCH 4/5] Add a freeing threshold for the garbage collector.

[Andi Kleen]

On Mon, Oct 10, 2011 at 12:20:53PM +0200, Richard Guenther wrote:
> On Sun, Oct 9, 2011 at 9:55 PM, Andi Kleen <andi@firstfloor.org> wrote:
> > From: Andi Kleen <ak@linux.intel.com>
> >
> > Add a threshold to avoid freeing pages back too early to the OS.
> > This avoid virtual memory map fragmentation.
> >
> > Based on a idea from Honza
> 
> Less than 20% looks high.  Shouldn't ggc-free-min be enough to
> avoid fragmentation?

It depends on the working set. If there's more to garbage collect
than max(ggc-free-min, threshold*total) a host without MADV_DONTNEED
will get holes. And ggc-free-min isn't very much on a large 
build.

So it seems safer to me to have a threshold which adjusts for large
working sets. What value do you prefer instead of 20%? (or just 0)

> 
> This will hide gc bugs with always-collect (ggc-checking), so
> the parameter(s) need to be adjusted for that case to always
> give pages back.  The current values should probably be printed
> where the two existing ones are printed as well (with -v).

Will fix.
-Andi

Re: [PATCH 4/5] Add a freeing threshold for the garbage collector.

[Richard Guenther]

On Mon, Oct 10, 2011 at 3:58 PM, Andi Kleen <andi@firstfloor.org> wrote:
> On Mon, Oct 10, 2011 at 12:20:53PM +0200, Richard Guenther wrote:
>> On Sun, Oct 9, 2011 at 9:55 PM, Andi Kleen <andi@firstfloor.org> wrote:
>> > From: Andi Kleen <ak@linux.intel.com>
>> >
>> > Add a threshold to avoid freeing pages back too early to the OS.
>> > This avoid virtual memory map fragmentation.
>> >
>> > Based on a idea from Honza
>>
>> Less than 20% looks high.  Shouldn't ggc-free-min be enough to
>> avoid fragmentation?
>
> It depends on the working set. If there's more to garbage collect
> than max(ggc-free-min, threshold*total) a host without MADV_DONTNEED
> will get holes. And ggc-free-min isn't very much on a large
> build.
>
> So it seems safer to me to have a threshold which adjusts for large
> working sets. What value do you prefer instead of 20%? (or just 0)

I'm not sure honestly - 10% maybe?  I realize it's quite arbitrary ...

>>
>> This will hide gc bugs with always-collect (ggc-checking), so
>> the parameter(s) need to be adjusted for that case to always
>> give pages back.  The current values should probably be printed
>> where the two existing ones are printed as well (with -v).
>
> Will fix.
> -Andi
>

[PATCH 1/5] Use MADV_DONTNEED for freeing in garbage collector

[Andi Kleen]

From: Andi Kleen <ak@linux.intel.com>

Use the Linux MADV_DONTNEED call to unmap free pages in the garbage
collector.Then keep the unmapped pages in the free list. This avoid
excessive memory fragmentation on large LTO bulds, which can lead
to gcc bumping into the Linux vm_max_map limit per process.

Based on a idea from Jakub.

gcc/:

2011-10-08   Andi Kleen <ak@linux.intel.com>

	PR other/50636
	* config.in, configure: Regenerate.
	* configure.ac (madvise): Add to AC_CHECK_FUNCS.
	* ggc-page.c (USING_MADVISE): Add.
	(page_entry): Add unmapped field.
	(alloc_page): Check for unmapped pages.
	(release_pages): Add USING_MADVISE branch.
---
 gcc/config.in    |    6 ++++++
 gcc/configure    |    2 +-
 gcc/configure.ac |    2 +-
 gcc/ggc-page.c   |   48 +++++++++++++++++++++++++++++++++++++++++++++++-
 4 files changed, 55 insertions(+), 3 deletions(-)

diff --git a/gcc/config.in b/gcc/config.in
index f2847d8..e8148b6 100644
--- a/gcc/config.in
+++ b/gcc/config.in
@@ -1276,6 +1276,12 @@
 #endif
 
 
+/* Define to 1 if you have the `madvise' function. */
+#ifndef USED_FOR_TARGET
+#undef HAVE_MADVISE
+#endif
+
+
 /* Define to 1 if you have the <malloc.h> header file. */
 #ifndef USED_FOR_TARGET
 #undef HAVE_MALLOC_H
diff --git a/gcc/configure b/gcc/configure
index cb55dda..4a54adf 100755
--- a/gcc/configure
+++ b/gcc/configure
@@ -9001,7 +9001,7 @@ fi
 for ac_func in times clock kill getrlimit setrlimit atoll atoq \
 	sysconf strsignal getrusage nl_langinfo \
 	gettimeofday mbstowcs wcswidth mmap setlocale \
-	clearerr_unlocked feof_unlocked   ferror_unlocked fflush_unlocked fgetc_unlocked fgets_unlocked   fileno_unlocked fprintf_unlocked fputc_unlocked fputs_unlocked   fread_unlocked fwrite_unlocked getchar_unlocked getc_unlocked   putchar_unlocked putc_unlocked
+	clearerr_unlocked feof_unlocked   ferror_unlocked fflush_unlocked fgetc_unlocked fgets_unlocked   fileno_unlocked fprintf_unlocked fputc_unlocked fputs_unlocked   fread_unlocked fwrite_unlocked getchar_unlocked getc_unlocked   putchar_unlocked putc_unlocked madvise
 do :
   as_ac_var=`$as_echo "ac_cv_func_$ac_func" | $as_tr_sh`
 ac_fn_c_check_func "$LINENO" "$ac_func" "$as_ac_var"
diff --git a/gcc/configure.ac b/gcc/configure.ac
index a7b94e6..357902e 100644
--- a/gcc/configure.ac
+++ b/gcc/configure.ac
@@ -1027,7 +1027,7 @@ define(gcc_UNLOCKED_FUNCS, clearerr_unlocked feof_unlocked dnl
 AC_CHECK_FUNCS(times clock kill getrlimit setrlimit atoll atoq \
 	sysconf strsignal getrusage nl_langinfo \
 	gettimeofday mbstowcs wcswidth mmap setlocale \
-	gcc_UNLOCKED_FUNCS)
+	gcc_UNLOCKED_FUNCS madvise)
 
 if test x$ac_cv_func_mbstowcs = xyes; then
   AC_CACHE_CHECK(whether mbstowcs works, gcc_cv_func_mbstowcs_works,
diff --git a/gcc/ggc-page.c b/gcc/ggc-page.c
index 624f029..b0b3b3f 100644
--- a/gcc/ggc-page.c
+++ b/gcc/ggc-page.c
@@ -50,6 +50,10 @@ along with GCC; see the file COPYING3.  If not see
 #define USING_MALLOC_PAGE_GROUPS
 #endif
 
+#if defined(HAVE_MADVISE) && defined(MADV_DONTNEED)
+# define USING_MADVISE
+#endif
+
 /* Strategy:
 
    This garbage-collecting allocator allocates objects on one of a set
@@ -277,6 +281,9 @@ typedef struct page_entry
   /* The lg of size of objects allocated from this page.  */
   unsigned char order;
 
+  /* Unmapped page? */
+  bool unmapped;
+
   /* A bit vector indicating whether or not objects are in use.  The
      Nth bit is one if the Nth object on this page is allocated.  This
      array is dynamically sized.  */
@@ -740,6 +747,10 @@ alloc_page (unsigned order)
 
   if (p != NULL)
     {
+      if (p->unmapped)
+        G.bytes_mapped += p->bytes;
+      p->unmapped = false;
+
       /* Recycle the allocated memory from this page ...  */
       *pp = p->next;
       page = p->page;
@@ -956,7 +967,42 @@ free_page (page_entry *entry)
 static void
 release_pages (void)
 {
-#ifdef USING_MMAP
+#ifdef USING_MADVISE
+  page_entry *p, *start_p;
+  char *start;
+  size_t len;
+
+  for (p = G.free_pages; p; )
+    {
+      if (p->unmapped)
+        {
+          p = p->next;
+          continue;
+        }
+      start = p->page;
+      len = p->bytes;
+      start_p = p;
+      p = p->next;
+      while (p && p->page == start + len)
+        {
+          len += p->bytes;
+          p = p->next;
+        }
+      /* Give the page back to the kernel, but don't free the mapping.
+         This avoids fragmentation in the virtual memory map of the 
+ 	 process. Next time we can reuse it by just touching it. */
+      madvise (start, len, MADV_DONTNEED);
+      /* Don't count those pages as mapped to not touch the garbage collector
+         unnecessarily. */
+      G.bytes_mapped -= len;
+      while (start_p != p)
+        {
+          start_p->unmapped = true;
+          start_p = start_p->next;
+        }
+    }
+#endif
+#if defined(USING_MMAP) && !defined(USING_MADVISE)
   page_entry *p, *next;
   char *start;
   size_t len;
-- 
1.7.5.4

Re: [PATCH 1/5] Use MADV_DONTNEED for freeing in garbage collector

[Richard Guenther]

On Sun, Oct 9, 2011 at 9:55 PM, Andi Kleen <andi@firstfloor.org> wrote:
> From: Andi Kleen <ak@linux.intel.com>
>
> Use the Linux MADV_DONTNEED call to unmap free pages in the garbage
> collector.Then keep the unmapped pages in the free list. This avoid
> excessive memory fragmentation on large LTO bulds, which can lead
> to gcc bumping into the Linux vm_max_map limit per process.
>
> Based on a idea from Jakub.

Shouldn't we prefer still "mapped" pages when allocating?  Thus, keep
the freepages list "sorted"?

With the new params to call release_pages less, how does this
interact with using MADV_DONTNEED?  The only reason to delay
MADV_DONTNEED is to avoid splitting huge-pages?  Which would
mean that we should rather be better at controlling where we allocate
from from the free-list?

Richard.

> gcc/:
>
> 2011-10-08   Andi Kleen <ak@linux.intel.com>
>
>        PR other/50636
>        * config.in, configure: Regenerate.
>        * configure.ac (madvise): Add to AC_CHECK_FUNCS.
>        * ggc-page.c (USING_MADVISE): Add.
>        (page_entry): Add unmapped field.
>        (alloc_page): Check for unmapped pages.
>        (release_pages): Add USING_MADVISE branch.
> ---
>  gcc/config.in    |    6 ++++++
>  gcc/configure    |    2 +-
>  gcc/configure.ac |    2 +-
>  gcc/ggc-page.c   |   48 +++++++++++++++++++++++++++++++++++++++++++++++-
>  4 files changed, 55 insertions(+), 3 deletions(-)
>
> diff --git a/gcc/config.in b/gcc/config.in
> index f2847d8..e8148b6 100644
> --- a/gcc/config.in
> +++ b/gcc/config.in
> @@ -1276,6 +1276,12 @@
>  #endif
>
>
> +/* Define to 1 if you have the `madvise' function. */
> +#ifndef USED_FOR_TARGET
> +#undef HAVE_MADVISE
> +#endif
> +
> +
>  /* Define to 1 if you have the <malloc.h> header file. */
>  #ifndef USED_FOR_TARGET
>  #undef HAVE_MALLOC_H
> diff --git a/gcc/configure b/gcc/configure
> index cb55dda..4a54adf 100755
> --- a/gcc/configure
> +++ b/gcc/configure
> @@ -9001,7 +9001,7 @@ fi
>  for ac_func in times clock kill getrlimit setrlimit atoll atoq \
>        sysconf strsignal getrusage nl_langinfo \
>        gettimeofday mbstowcs wcswidth mmap setlocale \
> -       clearerr_unlocked feof_unlocked   ferror_unlocked fflush_unlocked fgetc_unlocked fgets_unlocked   fileno_unlocked fprintf_unlocked fputc_unlocked fputs_unlocked   fread_unlocked fwrite_unlocked getchar_unlocked getc_unlocked   putchar_unlocked putc_unlocked
> +       clearerr_unlocked feof_unlocked   ferror_unlocked fflush_unlocked fgetc_unlocked fgets_unlocked   fileno_unlocked fprintf_unlocked fputc_unlocked fputs_unlocked   fread_unlocked fwrite_unlocked getchar_unlocked getc_unlocked   putchar_unlocked putc_unlocked madvise
>  do :
>   as_ac_var=`$as_echo "ac_cv_func_$ac_func" | $as_tr_sh`
>  ac_fn_c_check_func "$LINENO" "$ac_func" "$as_ac_var"
> diff --git a/gcc/configure.ac b/gcc/configure.ac
> index a7b94e6..357902e 100644
> --- a/gcc/configure.ac
> +++ b/gcc/configure.ac
> @@ -1027,7 +1027,7 @@ define(gcc_UNLOCKED_FUNCS, clearerr_unlocked feof_unlocked dnl
>  AC_CHECK_FUNCS(times clock kill getrlimit setrlimit atoll atoq \
>        sysconf strsignal getrusage nl_langinfo \
>        gettimeofday mbstowcs wcswidth mmap setlocale \
> -       gcc_UNLOCKED_FUNCS)
> +       gcc_UNLOCKED_FUNCS madvise)
>
>  if test x$ac_cv_func_mbstowcs = xyes; then
>   AC_CACHE_CHECK(whether mbstowcs works, gcc_cv_func_mbstowcs_works,
> diff --git a/gcc/ggc-page.c b/gcc/ggc-page.c
> index 624f029..b0b3b3f 100644
> --- a/gcc/ggc-page.c
> +++ b/gcc/ggc-page.c
> @@ -50,6 +50,10 @@ along with GCC; see the file COPYING3.  If not see
>  #define USING_MALLOC_PAGE_GROUPS
>  #endif
>
> +#if defined(HAVE_MADVISE) && defined(MADV_DONTNEED)
> +# define USING_MADVISE
> +#endif
> +
>  /* Strategy:
>
>    This garbage-collecting allocator allocates objects on one of a set
> @@ -277,6 +281,9 @@ typedef struct page_entry
>   /* The lg of size of objects allocated from this page.  */
>   unsigned char order;
>
> +  /* Unmapped page? */
> +  bool unmapped;
> +
>   /* A bit vector indicating whether or not objects are in use.  The
>      Nth bit is one if the Nth object on this page is allocated.  This
>      array is dynamically sized.  */
> @@ -740,6 +747,10 @@ alloc_page (unsigned order)
>
>   if (p != NULL)
>     {
> +      if (p->unmapped)
> +        G.bytes_mapped += p->bytes;
> +      p->unmapped = false;
> +
>       /* Recycle the allocated memory from this page ...  */
>       *pp = p->next;
>       page = p->page;
> @@ -956,7 +967,42 @@ free_page (page_entry *entry)
>  static void
>  release_pages (void)
>  {
> -#ifdef USING_MMAP
> +#ifdef USING_MADVISE
> +  page_entry *p, *start_p;
> +  char *start;
> +  size_t len;
> +
> +  for (p = G.free_pages; p; )
> +    {
> +      if (p->unmapped)
> +        {
> +          p = p->next;
> +          continue;
> +        }
> +      start = p->page;
> +      len = p->bytes;
> +      start_p = p;
> +      p = p->next;
> +      while (p && p->page == start + len)
> +        {
> +          len += p->bytes;
> +          p = p->next;
> +        }
> +      /* Give the page back to the kernel, but don't free the mapping.
> +         This avoids fragmentation in the virtual memory map of the
> +        process. Next time we can reuse it by just touching it. */
> +      madvise (start, len, MADV_DONTNEED);
> +      /* Don't count those pages as mapped to not touch the garbage collector
> +         unnecessarily. */
> +      G.bytes_mapped -= len;
> +      while (start_p != p)
> +        {
> +          start_p->unmapped = true;
> +          start_p = start_p->next;
> +        }
> +    }
> +#endif
> +#if defined(USING_MMAP) && !defined(USING_MADVISE)
>   page_entry *p, *next;
>   char *start;
>   size_t len;
> --
> 1.7.5.4
>
>

Re: [PATCH 1/5] Use MADV_DONTNEED for freeing in garbage collector

[Jakub Jelinek]

On Mon, Oct 10, 2011 at 12:25:15PM +0200, Richard Guenther wrote:
> On Sun, Oct 9, 2011 at 9:55 PM, Andi Kleen <andi@firstfloor.org> wrote:
> > From: Andi Kleen <ak@linux.intel.com>
> >
> > Use the Linux MADV_DONTNEED call to unmap free pages in the garbage
> > collector.Then keep the unmapped pages in the free list. This avoid
> > excessive memory fragmentation on large LTO bulds, which can lead
> > to gcc bumping into the Linux vm_max_map limit per process.
> >
> > Based on a idea from Jakub.
> 
> Shouldn't we prefer still "mapped" pages when allocating?  Thus, keep
> the freepages list "sorted"?

I don't see why.  MADV_DONTNEED isn't perfect, what it does (on Linux)
is that it zaps the whole page range, which essentially brings it into
the exact same state as immediately after mmap.  Any touch of the
pages will result in a zeroed page being inserted into the page tables.

4 years ago there was a MADV_FREE proposal which behaved much better
(page was removed from page tables only when the kernel actually needed
them for something else, if the page wasn't needed and has been accessed
again by the application, it would still contain the old content (which
the app couldn't rely on, it could as well be cleared), but it would be much
cheaper in that case.  With MADV_FREE it would be actually preferrable
to pick the MADV_FREEd pages over picking up freshly munmapped but not yet
touched pages.

> With the new params to call release_pages less, how does this
> interact with using MADV_DONTNEED?  The only reason to delay
> MADV_DONTNEED is to avoid splitting huge-pages?  Which would

Not just that.  MADV_DONTNEED needs to flush the dirty pages from the page
tables and when they are touched again, they need to be cleared (or
pre-cleared pages inserted).  So, while MADV_DONTNEED is less expensive than
munmap + mmap, it is still not free.

> > 2011-10-08   Andi Kleen <ak@linux.intel.com>

Two space in between name and <.
> >
> >        PR other/50636
> >        * config.in, configure: Regenerate.

Please write each file on a separate line, and better below
* configure.ac line because of which they have been regenerated.

> >
> > +  /* Unmapped page? */
> > +  bool unmapped;
> > +

Not sure if unmapped is the best name of the flag here, because
it hasn't been unmapped, it just has been madvised.  Under unmap
most people would imagine munmap I'd say.

	Jakub

Re: [PATCH 1/5] Use MADV_DONTNEED for freeing in garbage collector

[Richard Guenther]

On Mon, Oct 10, 2011 at 12:45 PM, Jakub Jelinek <jakub@redhat.com> wrote:
> On Mon, Oct 10, 2011 at 12:25:15PM +0200, Richard Guenther wrote:
>> On Sun, Oct 9, 2011 at 9:55 PM, Andi Kleen <andi@firstfloor.org> wrote:
>> > From: Andi Kleen <ak@linux.intel.com>
>> >
>> > Use the Linux MADV_DONTNEED call to unmap free pages in the garbage
>> > collector.Then keep the unmapped pages in the free list. This avoid
>> > excessive memory fragmentation on large LTO bulds, which can lead
>> > to gcc bumping into the Linux vm_max_map limit per process.
>> >
>> > Based on a idea from Jakub.
>>
>> Shouldn't we prefer still "mapped" pages when allocating?  Thus, keep
>> the freepages list "sorted"?
>
> I don't see why.  MADV_DONTNEED isn't perfect, what it does (on Linux)
> is that it zaps the whole page range, which essentially brings it into
> the exact same state as immediately after mmap.  Any touch of the
> pages will result in a zeroed page being inserted into the page tables.

Which means we save the zeroing when allocating non-MADV_DONTNEEDed
pages first.  And will be eventually able to unmap zapped pages.

> 4 years ago there was a MADV_FREE proposal which behaved much better
> (page was removed from page tables only when the kernel actually needed
> them for something else, if the page wasn't needed and has been accessed
> again by the application, it would still contain the old content (which
> the app couldn't rely on, it could as well be cleared), but it would be much
> cheaper in that case.  With MADV_FREE it would be actually preferrable
> to pick the MADV_FREEd pages over picking up freshly munmapped but not yet
> touched pages.
>
>> With the new params to call release_pages less, how does this
>> interact with using MADV_DONTNEED?  The only reason to delay
>> MADV_DONTNEED is to avoid splitting huge-pages?  Which would
>
> Not just that.  MADV_DONTNEED needs to flush the dirty pages from the page
> tables and when they are touched again, they need to be cleared (or
> pre-cleared pages inserted).  So, while MADV_DONTNEED is less expensive than
> munmap + mmap, it is still not free.

But it's free at madvise time.  munmap is "synchronous" at least (well,
when file-backed).

>> > 2011-10-08   Andi Kleen <ak@linux.intel.com>
>
> Two space in between name and <.
>> >
>> >        PR other/50636
>> >        * config.in, configure: Regenerate.
>
> Please write each file on a separate line, and better below
> * configure.ac line because of which they have been regenerated.
>
>> >
>> > +  /* Unmapped page? */
>> > +  bool unmapped;
>> > +
>
> Not sure if unmapped is the best name of the flag here, because
> it hasn't been unmapped, it just has been madvised.  Under unmap
> most people would imagine munmap I'd say.
>
>        Jakub
>

Re: [PATCH 1/5] Use MADV_DONTNEED for freeing in garbage collector

[Jakub Jelinek]

On Mon, Oct 10, 2011 at 01:11:13PM +0200, Richard Guenther wrote:
> > I don't see why.  MADV_DONTNEED isn't perfect, what it does (on Linux)
> > is that it zaps the whole page range, which essentially brings it into
> > the exact same state as immediately after mmap.  Any touch of the
> > pages will result in a zeroed page being inserted into the page tables.
> 
> Which means we save the zeroing when allocating non-MADV_DONTNEEDed
> pages first.  And will be eventually able to unmap zapped pages.

Well, there are 3 kind of pages in G.free_pages list after the patch.
1) pages added by alloc_page (GGC_QUIRK_SIZE - 1 pages each time, for p->bytes ==  G.pagesize only)
2) pages on which free_page has been called during last ggc_collect's sweep_pages
3) MADV_DONTNEED hinted pages from release_pages

Pages of 1) and 3) category have the same cost, pages of 2) category are
cheaper to access.  Pages of the 3) category are guaranteed to be at the
end of list (simply because free_pages marks all pages in the G.free_pages
list).  So this patch doesn't make things worse than it was before in this
regard, though maybe we should munmap p->bytes != G.pagesize pages right
away in release_pages instead of MADV_DONTNEED marking them.  They are
rarely to be reused.  For p->bytes == G.pagesize pages (the vast majority)
on the other side 1) shouldn't be really added until there are no 2) and 3)
category pages.

> > Not just that.  MADV_DONTNEED needs to flush the dirty pages from the page
> > tables and when they are touched again, they need to be cleared (or
> > pre-cleared pages inserted).  So, while MADV_DONTNEED is less expensive than
> > munmap + mmap, it is still not free.
> 
> But it's free at madvise time.  munmap is "synchronous" at least (well,
> when file-backed).

The zapping means some TLB flush and page table clearly, so not free even at
madvise time.  And here we are talking about anon memory anyway, so not
file-backed.

	Jakub

Re: [PATCH 1/5] Use MADV_DONTNEED for freeing in garbage collector

[Andi Kleen]

> Shouldn't we prefer still "mapped" pages when allocating?  Thus, keep
> the freepages list "sorted"?

Possibly. I can look at it in a followup if you want. 
I would prefer to not complicate this patch too much.

> 
> With the new params to call release_pages less, how does this
> interact with using MADV_DONTNEED?  The only reason to delay
> MADV_DONTNEED is to avoid splitting huge-pages?  Which would
> mean that we should rather be better at controlling where we allocate
> from from the free-list?

I first had a patch that tried to cluster inside the freelist
with multiple passes (and only free aligned quire clusters first), but it 
ran into various problems, so I chose this simpler approach.

With MADV_DONTNEED the param is not really needed I think,
I mainly added the param for the benefit of hosts that don't 
have MADV_DONTNEED to let them not suffer from fragmentation too much.
It would be possible to set the thresholds all to 0 if MADV_DONTNEED
is available.

-Andi

Re: [PATCH 1/5] Use MADV_DONTNEED for freeing in garbage collector

[Richard Guenther]

On Mon, Oct 10, 2011 at 4:04 PM, Andi Kleen <andi@firstfloor.org> wrote:
>> Shouldn't we prefer still "mapped" pages when allocating?  Thus, keep
>> the freepages list "sorted"?
>
> Possibly. I can look at it in a followup if you want.
> I would prefer to not complicate this patch too much.
>
>>
>> With the new params to call release_pages less, how does this
>> interact with using MADV_DONTNEED?  The only reason to delay
>> MADV_DONTNEED is to avoid splitting huge-pages?  Which would
>> mean that we should rather be better at controlling where we allocate
>> from from the free-list?
>
> I first had a patch that tried to cluster inside the freelist
> with multiple passes (and only free aligned quire clusters first), but it
> ran into various problems, so I chose this simpler approach.
>
> With MADV_DONTNEED the param is not really needed I think,
> I mainly added the param for the benefit of hosts that don't
> have MADV_DONTNEED to let them not suffer from fragmentation too much.
> It would be possible to set the thresholds all to 0 if MADV_DONTNEED
> is available.

So can we move the param patch back as a possible followup?

Thanks,
Richard.

> -Andi
>

Re: [PATCH 1/5] Use MADV_DONTNEED for freeing in garbage collector

[Andi Kleen]

> So can we move the param patch back as a possible followup?

I can drop it, however it will still mean fragmentation on any
non Linux (or rather non MADV_DONTNEED) hosts.

-Andi

Re: [PATCH 1/5] Use MADV_DONTNEED for freeing in garbage collector

[Andi Kleen]

Andi Kleen <andi@firstfloor.org> writes:

> From: Andi Kleen <ak@linux.intel.com>
>
> Use the Linux MADV_DONTNEED call to unmap free pages in the garbage
> collector.Then keep the unmapped pages in the free list. This avoid
> excessive memory fragmentation on large LTO bulds, which can lead
> to gcc bumping into the Linux vm_max_map limit per process.

Could I have a decision on this patch please? The problem in PR50636
is still there and this is the minimum fix to fix it on Linux
as far as I know.

If this patch is not the right way to go I would
appreciate some guidance on an alternative (but low cost)
implementation. Note I don't have capacity for any overly
complicated solutions.

Thanks,

-Andi

-- 
ak@linux.intel.com -- Speaking for myself only

Re: [PATCH 1/5] Use MADV_DONTNEED for freeing in garbage collector

[Richard Guenther]

On Sun, Oct 16, 2011 at 7:30 AM, Andi Kleen <andi@firstfloor.org> wrote:
> Andi Kleen <andi@firstfloor.org> writes:
>
>> From: Andi Kleen <ak@linux.intel.com>
>>
>> Use the Linux MADV_DONTNEED call to unmap free pages in the garbage
>> collector.Then keep the unmapped pages in the free list. This avoid
>> excessive memory fragmentation on large LTO bulds, which can lead
>> to gcc bumping into the Linux vm_max_map limit per process.
>
> Could I have a decision on this patch please? The problem in PR50636
> is still there and this is the minimum fix to fix it on Linux
> as far as I know.
>
> If this patch is not the right way to go I would
> appreciate some guidance on an alternative (but low cost)
> implementation. Note I don't have capacity for any overly
> complicated solutions.

The patch looks generally ok, but you are never giving back pages to the
system, and as we have other memory allocations that do not use the
ggc pools you drain virtual memory on 32bit hosts.  Is any other patch
in this series compensating for it?  If not I'd say we should munmap the
pages when a full mapped range (2MB) is free.  Can you rename
'unmapped' to 'discarded' please?  That would be less confusing.

Thanks,
Richard.

> Thanks,
>
> -Andi
>
> --
> ak@linux.intel.com -- Speaking for myself only
>

Re: [PATCH 1/5] Use MADV_DONTNEED for freeing in garbage collector

[Andi Kleen]

On Sun, Oct 16, 2011 at 12:38:16PM +0200, Richard Guenther wrote:
> On Sun, Oct 16, 2011 at 7:30 AM, Andi Kleen <andi@firstfloor.org> wrote:
> > Andi Kleen <andi@firstfloor.org> writes:
> >
> >> From: Andi Kleen <ak@linux.intel.com>
> >>
> >> Use the Linux MADV_DONTNEED call to unmap free pages in the garbage
> >> collector.Then keep the unmapped pages in the free list. This avoid
> >> excessive memory fragmentation on large LTO bulds, which can lead
> >> to gcc bumping into the Linux vm_max_map limit per process.
> >
> > Could I have a decision on this patch please? The problem in PR50636
> > is still there and this is the minimum fix to fix it on Linux
> > as far as I know.
> >
> > If this patch is not the right way to go I would
> > appreciate some guidance on an alternative (but low cost)
> > implementation. Note I don't have capacity for any overly
> > complicated solutions.
> 
> The patch looks generally ok, but you are never giving back pages to the

It gives back pages, just not virtual address space. But I guess that is 
what you meant.

On the other hand this patch can actually give you more virtual
address space when you need large regions (>2 pages or so). 
The reason is that the old allocation pattern fragments the whole
address space badly and only leaves these small holes. With the madvise
patch that does not happen, ggc is all in a compacted chunk.

> system, and as we have other memory allocations that do not use the
> ggc pools you drain virtual memory on 32bit hosts.  Is any other patch
> in this series compensating for it?  If not I'd say we should munmap the
> pages when a full mapped range (2MB) is free.  Can you rename

I wrote such a patch initially, but ran into various problems, so 
I dropped it from the series. I can revisit it.

> 'unmapped' to 'discarded' please?  That would be less confusing.

Ok I can do that. 

Was that an approval?

-Andi

Re: [PATCH 1/5] Use MADV_DONTNEED for freeing in garbage collector

[Richard Guenther]

On Sun, Oct 16, 2011 at 8:33 PM, Andi Kleen <andi@firstfloor.org> wrote:
> On Sun, Oct 16, 2011 at 12:38:16PM +0200, Richard Guenther wrote:
>> On Sun, Oct 16, 2011 at 7:30 AM, Andi Kleen <andi@firstfloor.org> wrote:
>> > Andi Kleen <andi@firstfloor.org> writes:
>> >
>> >> From: Andi Kleen <ak@linux.intel.com>
>> >>
>> >> Use the Linux MADV_DONTNEED call to unmap free pages in the garbage
>> >> collector.Then keep the unmapped pages in the free list. This avoid
>> >> excessive memory fragmentation on large LTO bulds, which can lead
>> >> to gcc bumping into the Linux vm_max_map limit per process.
>> >
>> > Could I have a decision on this patch please? The problem in PR50636
>> > is still there and this is the minimum fix to fix it on Linux
>> > as far as I know.
>> >
>> > If this patch is not the right way to go I would
>> > appreciate some guidance on an alternative (but low cost)
>> > implementation. Note I don't have capacity for any overly
>> > complicated solutions.
>>
>> The patch looks generally ok, but you are never giving back pages to the
>
> It gives back pages, just not virtual address space. But I guess that is
> what you meant.
>
> On the other hand this patch can actually give you more virtual
> address space when you need large regions (>2 pages or so).
> The reason is that the old allocation pattern fragments the whole
> address space badly and only leaves these small holes. With the madvise
> patch that does not happen, ggc is all in a compacted chunk.

Sure, but we do compete with the glibc heap with virtual memory usage
(I wonder if GGC should simply use malloc/free ...).  So I am worried
that we run out of address space earlier this way.  But I guess we can
revisit this when we run into actual problems ...

>> system, and as we have other memory allocations that do not use the
>> ggc pools you drain virtual memory on 32bit hosts.  Is any other patch
>> in this series compensating for it?  If not I'd say we should munmap the
>> pages when a full mapped range (2MB) is free.  Can you rename
>
> I wrote such a patch initially, but ran into various problems, so
> I dropped it from the series. I can revisit it.

Yes, please revisit it.  It should be as simple as scanning for a
large chunk in free_pages I suppose.

>> 'unmapped' to 'discarded' please?  That would be less confusing.
>
> Ok I can do that.
>
> Was that an approval?

Ok with the rename.

Thanks,
Richard.

> -Andi
>