diff mbox series

UBUNTU: SAUCE: mm/page_alloc.c: disable memory reclaim watermark boosting by default

Message ID 20200327184648.1389388-2-andrea.righi@canonical.com
State New
Headers show
Series UBUNTU: SAUCE: mm/page_alloc.c: disable memory reclaim watermark boosting by default | expand

Commit Message

Andrea Righi March 27, 2020, 6:46 p.m. UTC 
BugLink: https://bugs.launchpad.net/bugs/1861359

High watermark boosting can cause large swap activity under certain
memory intensive workloads, making the system very unresponsive (screen
does not refresh, keyboard not responding, etc.).

Disable this feature by default to prevent potential large swap
activity.

Signed-off-by: Sultan Alsawaf <sultan.alsawaf@canonical.com>
Signed-off-by: Andrea Righi <andrea.righi@canonical.com>
---
 mm/page_alloc.c | 13 -------------
 1 file changed, 13 deletions(-)

Comments

Colin Ian King March 27, 2020, 7:21 p.m. UTC | #1 
On 27/03/2020 18:46, Andrea Righi wrote:
> BugLink: https://bugs.launchpad.net/bugs/1861359
> 
> High watermark boosting can cause large swap activity under certain
> memory intensive workloads, making the system very unresponsive (screen
> does not refresh, keyboard not responding, etc.).
> 
> Disable this feature by default to prevent potential large swap
> activity.
> 
> Signed-off-by: Sultan Alsawaf <sultan.alsawaf@canonical.com>
> Signed-off-by: Andrea Righi <andrea.righi@canonical.com>
> ---
>  mm/page_alloc.c | 13 -------------
>  1 file changed, 13 deletions(-)
> 
> diff --git a/mm/page_alloc.c b/mm/page_alloc.c
> index d387ca74cb5a..7ab52a62c5ef 100644
> --- a/mm/page_alloc.c
> +++ b/mm/page_alloc.c
> @@ -315,20 +315,7 @@ compound_page_dtor * const compound_page_dtors[] = {
>  
>  int min_free_kbytes = 1024;
>  int user_min_free_kbytes = -1;
> -#ifdef CONFIG_DISCONTIGMEM
> -/*
> - * DiscontigMem defines memory ranges as separate pg_data_t even if the ranges
> - * are not on separate NUMA nodes. Functionally this works but with
> - * watermark_boost_factor, it can reclaim prematurely as the ranges can be
> - * quite small. By default, do not boost watermarks on discontigmem as in
> - * many cases very high-order allocations like THP are likely to be
> - * unsupported and the premature reclaim offsets the advantage of long-term
> - * fragmentation avoidance.
> - */
>  int watermark_boost_factor __read_mostly;
> -#else
> -int watermark_boost_factor __read_mostly = 15000;
> -#endif
>  int watermark_scale_factor = 10;
>  
>  static unsigned long nr_kernel_pages __initdata;
> 

Given that this was introduced by commit 24512228b7a3f412b5 ("mm: do not
boost watermarks to avoid fragmentation for the DISCONTIG memory model")
for parisc changing this back shouldn't hurt our supported architectures
as this returns it back to the original behavior.  This patch looks like
a revert of 24512228b7a3f412b5 - could we just revert that commit rather
than add a SAUCE patch? It may be worth referencing the problematic commit.

Anyhow, the fix is good to me.

Acked-by: Colin Ian King <colin.king@canonical.com>
Sultan Alsawaf March 27, 2020, 7:26 p.m. UTC | #2 
On Fri, Mar 27, 2020 at 07:21:30PM +0000, Colin Ian King wrote:
> On 27/03/2020 18:46, Andrea Righi wrote:
> > BugLink: https://bugs.launchpad.net/bugs/1861359
> > 
> > High watermark boosting can cause large swap activity under certain
> > memory intensive workloads, making the system very unresponsive (screen
> > does not refresh, keyboard not responding, etc.).
> > 
> > Disable this feature by default to prevent potential large swap
> > activity.
> > 
> > Signed-off-by: Sultan Alsawaf <sultan.alsawaf@canonical.com>
> > Signed-off-by: Andrea Righi <andrea.righi@canonical.com>
> > ---
> >  mm/page_alloc.c | 13 -------------
> >  1 file changed, 13 deletions(-)
> > 
> > diff --git a/mm/page_alloc.c b/mm/page_alloc.c
> > index d387ca74cb5a..7ab52a62c5ef 100644
> > --- a/mm/page_alloc.c
> > +++ b/mm/page_alloc.c
> > @@ -315,20 +315,7 @@ compound_page_dtor * const compound_page_dtors[] = {
> >  
> >  int min_free_kbytes = 1024;
> >  int user_min_free_kbytes = -1;
> > -#ifdef CONFIG_DISCONTIGMEM
> > -/*
> > - * DiscontigMem defines memory ranges as separate pg_data_t even if the ranges
> > - * are not on separate NUMA nodes. Functionally this works but with
> > - * watermark_boost_factor, it can reclaim prematurely as the ranges can be
> > - * quite small. By default, do not boost watermarks on discontigmem as in
> > - * many cases very high-order allocations like THP are likely to be
> > - * unsupported and the premature reclaim offsets the advantage of long-term
> > - * fragmentation avoidance.
> > - */
> >  int watermark_boost_factor __read_mostly;
> > -#else
> > -int watermark_boost_factor __read_mostly = 15000;
> > -#endif
> >  int watermark_scale_factor = 10;
> >  
> >  static unsigned long nr_kernel_pages __initdata;
> > 
> 
> Given that this was introduced by commit 24512228b7a3f412b5 ("mm: do not
> boost watermarks to avoid fragmentation for the DISCONTIG memory model")
> for parisc changing this back shouldn't hurt our supported architectures
> as this returns it back to the original behavior.  This patch looks like
> a revert of 24512228b7a3f412b5 - could we just revert that commit rather
> than add a SAUCE patch? It may be worth referencing the problematic commit.
> 
> Anyhow, the fix is good to me.
> 
> Acked-by: Colin Ian King <colin.king@canonical.com>
> 
> 
> -- 
> kernel-team mailing list
> kernel-team@lists.ubuntu.com
> https://lists.ubuntu.com/mailman/listinfo/kernel-team

This patch is different from reverting 24512228b7a3f412b5 because that commit
sets the default watermark_scale_factor to 0 only for CONFIG_DISCONTIGMEM=y.
This commit sets the default watermark_scale_factor to 0 for everything.

Also, watermark boosting is a severely flawed "feature" that should not be used
at all. See my lp comment [1] for more details.

Sultan

[1] https://bugs.launchpad.net/ubuntu/+source/linux/+bug/1861359/comments/56
Colin Ian King March 27, 2020, 7:28 p.m. UTC | #3 
On 27/03/2020 19:26, Sultan Alsawaf wrote:
> On Fri, Mar 27, 2020 at 07:21:30PM +0000, Colin Ian King wrote:
>> On 27/03/2020 18:46, Andrea Righi wrote:
>>> BugLink: https://bugs.launchpad.net/bugs/1861359
>>>
>>> High watermark boosting can cause large swap activity under certain
>>> memory intensive workloads, making the system very unresponsive (screen
>>> does not refresh, keyboard not responding, etc.).
>>>
>>> Disable this feature by default to prevent potential large swap
>>> activity.
>>>
>>> Signed-off-by: Sultan Alsawaf <sultan.alsawaf@canonical.com>
>>> Signed-off-by: Andrea Righi <andrea.righi@canonical.com>
>>> ---
>>>  mm/page_alloc.c | 13 -------------
>>>  1 file changed, 13 deletions(-)
>>>
>>> diff --git a/mm/page_alloc.c b/mm/page_alloc.c
>>> index d387ca74cb5a..7ab52a62c5ef 100644
>>> --- a/mm/page_alloc.c
>>> +++ b/mm/page_alloc.c
>>> @@ -315,20 +315,7 @@ compound_page_dtor * const compound_page_dtors[] = {
>>>  
>>>  int min_free_kbytes = 1024;
>>>  int user_min_free_kbytes = -1;
>>> -#ifdef CONFIG_DISCONTIGMEM
>>> -/*
>>> - * DiscontigMem defines memory ranges as separate pg_data_t even if the ranges
>>> - * are not on separate NUMA nodes. Functionally this works but with
>>> - * watermark_boost_factor, it can reclaim prematurely as the ranges can be
>>> - * quite small. By default, do not boost watermarks on discontigmem as in
>>> - * many cases very high-order allocations like THP are likely to be
>>> - * unsupported and the premature reclaim offsets the advantage of long-term
>>> - * fragmentation avoidance.
>>> - */
>>>  int watermark_boost_factor __read_mostly;
>>> -#else
>>> -int watermark_boost_factor __read_mostly = 15000;
>>> -#endif
>>>  int watermark_scale_factor = 10;
>>>  
>>>  static unsigned long nr_kernel_pages __initdata;
>>>
>>
>> Given that this was introduced by commit 24512228b7a3f412b5 ("mm: do not
>> boost watermarks to avoid fragmentation for the DISCONTIG memory model")
>> for parisc changing this back shouldn't hurt our supported architectures
>> as this returns it back to the original behavior.  This patch looks like
>> a revert of 24512228b7a3f412b5 - could we just revert that commit rather
>> than add a SAUCE patch? It may be worth referencing the problematic commit.
>>
>> Anyhow, the fix is good to me.
>>
>> Acked-by: Colin Ian King <colin.king@canonical.com>
>>
>>
>> -- 
>> kernel-team mailing list
>> kernel-team@lists.ubuntu.com
>> https://lists.ubuntu.com/mailman/listinfo/kernel-team
> 
> This patch is different from reverting 24512228b7a3f412b5 because that commit
> sets the default watermark_scale_factor to 0 only for CONFIG_DISCONTIGMEM=y.
> This commit sets the default watermark_scale_factor to 0 for everything.

Ah, I missed that. Thanks for clarifying. :-)

> 
> Also, watermark boosting is a severely flawed "feature" that should not be used
> at all. See my lp comment [1] for more details.

/me nods.

> 
> Sultan
> 
> [1] https://bugs.launchpad.net/ubuntu/+source/linux/+bug/1861359/comments/56
>
Sultan Alsawaf March 27, 2020, 7:29 p.m. UTC | #4 
On Fri, Mar 27, 2020 at 07:21:30PM +0000, Colin Ian King wrote:
> On 27/03/2020 18:46, Andrea Righi wrote:
> > BugLink: https://bugs.launchpad.net/bugs/1861359
> > 
> > High watermark boosting can cause large swap activity under certain
> > memory intensive workloads, making the system very unresponsive (screen
> > does not refresh, keyboard not responding, etc.).
> > 
> > Disable this feature by default to prevent potential large swap
> > activity.
> > 
> > Signed-off-by: Sultan Alsawaf <sultan.alsawaf@canonical.com>
> > Signed-off-by: Andrea Righi <andrea.righi@canonical.com>
> > ---
> >  mm/page_alloc.c | 13 -------------
> >  1 file changed, 13 deletions(-)
> > 
> > diff --git a/mm/page_alloc.c b/mm/page_alloc.c
> > index d387ca74cb5a..7ab52a62c5ef 100644
> > --- a/mm/page_alloc.c
> > +++ b/mm/page_alloc.c
> > @@ -315,20 +315,7 @@ compound_page_dtor * const compound_page_dtors[] = {
> >  
> >  int min_free_kbytes = 1024;
> >  int user_min_free_kbytes = -1;
> > -#ifdef CONFIG_DISCONTIGMEM
> > -/*
> > - * DiscontigMem defines memory ranges as separate pg_data_t even if the ranges
> > - * are not on separate NUMA nodes. Functionally this works but with
> > - * watermark_boost_factor, it can reclaim prematurely as the ranges can be
> > - * quite small. By default, do not boost watermarks on discontigmem as in
> > - * many cases very high-order allocations like THP are likely to be
> > - * unsupported and the premature reclaim offsets the advantage of long-term
> > - * fragmentation avoidance.
> > - */
> >  int watermark_boost_factor __read_mostly;
> > -#else
> > -int watermark_boost_factor __read_mostly = 15000;
> > -#endif
> >  int watermark_scale_factor = 10;
> >  
> >  static unsigned long nr_kernel_pages __initdata;
> > 
> 
> Given that this was introduced by commit 24512228b7a3f412b5 ("mm: do not
> boost watermarks to avoid fragmentation for the DISCONTIG memory model")
> for parisc changing this back shouldn't hurt our supported architectures
> as this returns it back to the original behavior.  This patch looks like
> a revert of 24512228b7a3f412b5 - could we just revert that commit rather
> than add a SAUCE patch? It may be worth referencing the problematic commit.
> 
> Anyhow, the fix is good to me.
> 
> Acked-by: Colin Ian King <colin.king@canonical.com>
> 
> 
> -- 
> kernel-team mailing list
> kernel-team@lists.ubuntu.com
> https://lists.ubuntu.com/mailman/listinfo/kernel-team

Typo in my last email: meant to say watermark_boost_factor is set to 0 with this
patch, not watermark_scale_factor.

Sultan
Andrea Righi March 30, 2020, 8:42 a.m. UTC | #5 
On Fri, Mar 27, 2020 at 07:46:49PM +0100, Andrea Righi wrote:
> BugLink: https://bugs.launchpad.net/bugs/1861359
> 
> High watermark boosting can cause large swap activity under certain
> memory intensive workloads, making the system very unresponsive (screen
> does not refresh, keyboard not responding, etc.).
> 
> Disable this feature by default to prevent potential large swap
> activity.
> 
> Signed-off-by: Sultan Alsawaf <sultan.alsawaf at canonical.com>
> Signed-off-by: Andrea Righi <andrea.righi at canonical.com>
> ---
>  mm/page_alloc.c | 13 -------------
>  1 file changed, 13 deletions(-)
> 
> diff --git a/mm/page_alloc.c b/mm/page_alloc.c
> index d387ca74cb5a..7ab52a62c5ef 100644
> --- a/mm/page_alloc.c
> +++ b/mm/page_alloc.c
> @@ -315,20 +315,7 @@ compound_page_dtor * const compound_page_dtors[] = {
>  
>  int min_free_kbytes = 1024;
>  int user_min_free_kbytes = -1;
> -#ifdef CONFIG_DISCONTIGMEM
> -/*
> - * DiscontigMem defines memory ranges as separate pg_data_t even if the ranges
> - * are not on separate NUMA nodes. Functionally this works but with
> - * watermark_boost_factor, it can reclaim prematurely as the ranges can be
> - * quite small. By default, do not boost watermarks on discontigmem as in
> - * many cases very high-order allocations like THP are likely to be
> - * unsupported and the premature reclaim offsets the advantage of long-term
> - * fragmentation avoidance.
> - */
>  int watermark_boost_factor __read_mostly;
> -#else
> -int watermark_boost_factor __read_mostly = 15000;
> -#endif
>  int watermark_scale_factor = 10;
>  
>  static unsigned long nr_kernel_pages __initdata;
> -- 
> 2.25.1
> 
>
diff mbox series

Patch

diff --git a/mm/page_alloc.c b/mm/page_alloc.c
index d387ca74cb5a..7ab52a62c5ef 100644
--- a/mm/page_alloc.c
+++ b/mm/page_alloc.c
@@ -315,20 +315,7 @@  compound_page_dtor * const compound_page_dtors[] = {
 
 int min_free_kbytes = 1024;
 int user_min_free_kbytes = -1;
-#ifdef CONFIG_DISCONTIGMEM
-/*
- * DiscontigMem defines memory ranges as separate pg_data_t even if the ranges
- * are not on separate NUMA nodes. Functionally this works but with
- * watermark_boost_factor, it can reclaim prematurely as the ranges can be
- * quite small. By default, do not boost watermarks on discontigmem as in
- * many cases very high-order allocations like THP are likely to be
- * unsupported and the premature reclaim offsets the advantage of long-term
- * fragmentation avoidance.
- */
 int watermark_boost_factor __read_mostly;
-#else
-int watermark_boost_factor __read_mostly = 15000;
-#endif
 int watermark_scale_factor = 10;
 
 static unsigned long nr_kernel_pages __initdata;