mm/khugepaged: cap pageblock contribution to min_free_kbytes
Subsystems (2)
RFC PATCH 1/1 mm/khugepaged: cap pageblock contribution to min_free_kbytes
When THP is enabled, khugepaged can raise min_free_kbytes through set_recommended_min_free_kbytes(), using a heuristic that scales with pageblock_nr_pages. With MIGRATE_PCPTYPES equal to 3, the formula accounts for 11 pageblocks for every eligible populated zone before capping the result at 5% of low memory.
This is reasonable when a pageblock is 2MB, as on common 4KB page configurations, but scales poorly with larger base page sizes. With the default arm64 pageblock sizes, the contribution per eligible zone before the 5% cap is:
4KB pages: 2MB pageblock, 22MB per zone
16KB pages: 32MB pageblock, 352MB per zone
64KB pages: 512MB pageblock, 5.5GB per zone
The 5% limit does not make this harmless. On an arm64 system with 128GB of RAM and 64KB pages, the current calculation resulted in roughly 6.3GB for min_free_kbytes. The corresponding aggregate min, low and high watermarks were 6.3GB, 7.8GB and 9.4GB.
Automatically maintaining multiple GB of additional free memory to improve the probability of 512MB THP allocations seems disproportionate for an opportunistic background optimization.
This RFC proposes capping the pageblock contribution to the recommendation at 2MB. This would limit the contribution to 22MB per eligible zone while leaving configurations with pageblocks of 2MB or smaller unchanged.
Earlier proposals to resolve this problem involved disabling default THP on affected systems [1], adding a knob to disable the recommendation [2], or deriving the recommendation from the largest enabled THP order [3]. A fixed cap requires no new knob and avoids large changes to global watermarks when THP policy is changed at runtime.
Reducing PAGE_BLOCK_MAX_ORDER remains possible, but it is a build-time policy that changes pageblock granularity globally. Its effects extend beyond the scope of adjusting min_free_kbytes.
[1] https://lore.kernel.org/all/CAPm50aLPxJCiVTqqwiz00oMNiqHggB84sXB3x=tv_HUAd5UktQ@mail.gmail.com/
[2] https://lore.kernel.org/all/[email protected]/
[3] https://lore.kernel.org/all/[email protected]/
--- mm/khugepaged.c | 17 +++++++++++++---- 1 file changed, 13 insertions(+), 4 deletions(-)
mm/khugepaged.c +13 -4
@@ -23,6 +23,7 @@#include <linux/ksm.h> #include <linux/pgalloc.h> #include <linux/backing-dev.h> +#include <linux/sizes.h> #include <asm/tlb.h> #include "internal.h"
@@ -3066,6 +3067,7 @@ void set_recommended_min_free_kbytes(void) {struct zone *zone;int nr_zones =0 ; +unsigned long capped_pageblock_nr_pages;unsigned long recommended_min;if (!hugepage_enabled ()) {
@@ -3084,16 +3086,23 @@ void set_recommended_min_free_kbytes(void) nr_zones++; } -/* Ensure 2 pageblocks are free to assist fragmentation avoidance */ - recommended_min = pageblock_nr_pages * nr_zones *2 ; +/* +* Limit the pageblock contribution to 2 MiB. Larger pageblocks can +* otherwise make min_free_kbytes disproportionately large. +*/ + capped_pageblock_nr_pages = +min (pageblock_nr_pages,SZ_2M /PAGE_SIZE ); + +/* Ensure 2 capped units are free to assist fragmentation avoidance */ + recommended_min = capped_pageblock_nr_pages * nr_zones *2 ;/* -* Make sure that on average at least two pageblocks are almost free +* Make sure that on average at least two capped units are almost free * of another type, one for a migratetype to fall back to and a * second to avoid subsequent fallbacks of other types There are 3 * MIGRATE_TYPES we care about. */ - recommended_min += pageblock_nr_pages * nr_zones * + recommended_min += capped_pageblock_nr_pages * nr_zones *MIGRATE_PCPTYPES *MIGRATE_PCPTYPES ;/* don't ever allow to reserve more than 5% of the lowmem */
Signature · 1 line
2.45.0
Quoted 2 lines · Nimrod Oren
When THP is enabled, khugepaged can raise
min_free_kbytesthroughset_recommended_min_free_kbytes(), using a heuristic that scales with
Nitty, but that makes it sound like khugepaged (the process) itself comes in and changes it, rather set_recommended_min_free_kbytes() sets it higher if hugepage_enabled().
Quoted 9 lines
pageblock_nr_pages. WithMIGRATE_PCPTYPESequal to 3, the formula accounts for 11 pageblocks for every eligible populated zone before capping the result at 5% of low memory.This is reasonable when a pageblock is 2MB, as on common 4KB page configurations, but scales poorly with larger base page sizes. With the default arm64 pageblock sizes, the contribution per eligible zone before the 5% cap is:
Quoted 8 lines
4KB pages: 2MB pageblock, 22MB per zone
16KB pages: 32MB pageblock, 352MB per zone
64KB pages: 512MB pageblock, 5.5GB per zoneThe 5% limit does not make this harmless. On an arm64 system with 128GB of RAM and 64KB pages, the current calculation resulted in roughly 6.3GB for
min_free_kbytes. The corresponding aggregate min, low and high watermarks were 6.3GB, 7.8GB and 9.4GB.
Yeah this is an ongoing issue (as you note below). We definitely need to find a better solution.
Quoted 4 lines
Automatically maintaining multiple GB of additional free memory to improve the probability of 512MB THP allocations seems disproportionate for an opportunistic background optimization.
The fact it's in khugepaged.c doesn't mean it's just about khugepaged, it's about avoiding fragmentation for the purposes of obtaining THP pages which can also be obtained from page fault and MADV_COLLAPSE.
Quoted 5 lines
This RFC proposes capping the pageblock contribution to the recommendation at 2MB. This would limit the contribution to 22MB per eligible zone while leaving configurations with pageblocks of 2MB or smaller unchanged.
I guess you're trying to do the most conservative thing possible - but it's a bit of a cheat really.
I mean does it really make sense to reserve 2 MB for the purposes of obtaining 512 MB PMDs? It's essentially giving up on the idea of getting PMD THP altogether but pretending you are still doing so.
I wonder if it wouldn't just be better to explicitly check the page table size and to not even bother trying for > 4 KiB with a comment explaining that it's not a worthwhile trade-off.
I think that'd be the more honest version of this patch wouldn't it?
Or maybe we need not be so concerneda bout scaling across zones...
Quoted 7 lines
Earlier proposals to resolve this problem involved disabling default THP on affected systems [1], adding a knob to disable the recommendation [2], or deriving the recommendation from the largest enabled THP order [3]. A fixed cap requires no new knob and avoids large changes to global watermarks when THP policy is changed at runtime.
How often are people turning THP completely off or on again though?
Quoted 10 lines
Reducing
PAGE_BLOCK_MAX_ORDERremains possible, but it is a build-time policy that changes pageblock granularity globally. Its effects extend beyond the scope of adjustingmin_free_kbytes.[1] https://lore.kernel.org/all/CAPm50aLPxJCiVTqqwiz00oMNiqHggB84sXB3x=tv_HUAd5UktQ@mail.gmail.com/
[2] https://lore.kernel.org/all/[email protected]/
[3] https://lore.kernel.org/all/[email protected]/Signed-off-by Nimrod Oren <[email protected]>
Thanks in any case for taking a more pragmatic approach here.
Quoted 26 lines
--- mm/khugepaged.c | 17 +++++++++++++---- 1 file changed, 13 insertions(+), 4 deletions(-)mm/khugepaged.c +2
-0@@ -23,6 +23,7 @@#include <linux/ksm.h> #include <linux/pgalloc.h> #include <linux/backing-dev.h> +#include <linux/sizes.h> #include <asm/tlb.h> #include "internal.h" @@ -3066,6 +3067,7 @@ void set_recommended_min_free_kbytes(void) {struct zone *zone;int nr_zones =0 ; +unsigned long capped_pageblock_nr_pages;unsigned long recommended_min;if (!hugepage_enabled ()) {@@ -3084,16 +3086,23 @@ void set_recommended_min_free_kbytes(void) nr_zones++;
mm/khugepaged.c void set_recommended_min_free_kbytes(void) · line 3094
Quoted 10 lines
mm/khugepaged.c +6
-2} -/* Ensure 2 pageblocks are free to assist fragmentation avoidance */ - recommended_min = pageblock_nr_pages * nr_zones *2 ; +/* +* Limit the pageblock contribution to 2 MiB. Larger pageblocks can +* otherwise make min_free_kbytes disproportionately large. +*/ + capped_pageblock_nr_pages = +min (pageblock_nr_pages,SZ_2M /PAGE_SIZE );
Yeah this just feels like a hack, to be honest "let's pretend a page block is 2 MiB" essentially :)
Quoted 4 lines
mm/khugepaged.c +3
-0+ +/* Ensure 2 capped units are free to assist fragmentation avoidance */ + recommended_min = capped_pageblock_nr_pages * nr_zones *2 ;
mm/khugepaged.c void set_recommended_min_free_kbytes(void) · line 3106
Quoted 10 lines
mm/khugepaged.c +2
-2/* -* Make sure that on average at least two pageblocks are almost free +* Make sure that on average at least two capped units are almost free * of another type, one for a migratetype to fall back to and a * second to avoid subsequent fallbacks of other types There are 3 * MIGRATE_TYPES we care about. */ - recommended_min += pageblock_nr_pages * nr_zones * + recommended_min += capped_pageblock_nr_pages * nr_zones *MIGRATE_PCPTYPES *MIGRATE_PCPTYPES ;
we're really getting into faking stuff out realms here - pretending a page block is 2 MiB then doing calculations on it as if they were.
We'd be better off (and more honest) with something like:
- if (!hugepage_enabled()) {
+ /* Avoid excessive memory reservation if page blocks are very large. */
+ if (!hugepage_enabled() || PAGE_SIZE > SZ_4K) {
But that seems perhaps unwise.
Maybe a simpler version would be to just attack the issue we actually have with it - it uses too much damn memory :)
So maybe something like the attached? (you'd probably want to make this a sysctl or at least a defined value of course)
General idea is - if we're going to cap, let's just cap instead of pretending that page blocks are of a different size.
Quoted 5 lines
mm/khugepaged.c +0
-1/* don't ever allow to reserve more than 5% of the lowmem */ --2.45.0
Cheers, Lorenzo
----8<----
mm/khugepaged.c +5 -1
@@ -3092,9 +3092,13 @@ void set_recommended_min_free_kbytes(void) recommended_min += pageblock_nr_pages * nr_zones *MIGRATE_PCPTYPES *MIGRATE_PCPTYPES ; -/* don't ever allow to reserve more than 5% of the lowmem */ +/* +* don't ever allow the reservation of more than 1 GiB or 5% of lowmem +* (whichever is smaller). +*/ recommended_min =min (recommended_min, (unsigned long )nr_free_buffer_pages () /20 ); + recommended_min =min (recommended_min,SZ_1G ); recommended_min <<= (PAGE_SHIFT -10 );if (recommended_min > min_free_kbytes) {
Zi Yan
reviewerQuoted 44 lines · Lorenzo Stoakes (ARM)
Quoted 2 lines · Nimrod Oren
When THP is enabled, khugepaged can raise
min_free_kbytesthroughset_recommended_min_free_kbytes(), using a heuristic that scales withNitty, but that makes it sound like khugepaged (the process) itself comes in and changes it, rather
set_recommended_min_free_kbytes()sets it higher ifhugepage_enabled().Quoted 9 lines
pageblock_nr_pages. WithMIGRATE_PCPTYPESequal to 3, the formula accounts for 11 pageblocks for every eligible populated zone before capping the result at 5% of low memory.This is reasonable when a pageblock is 2MB, as on common 4KB page configurations, but scales poorly with larger base page sizes. With the default arm64 pageblock sizes, the contribution per eligible zone before the 5% cap is:
Quoted 8 lines
4KB pages: 2MB pageblock, 22MB per zone
16KB pages: 32MB pageblock, 352MB per zone
64KB pages: 512MB pageblock, 5.5GB per zoneThe 5% limit does not make this harmless. On an arm64 system with 128GB of RAM and 64KB pages, the current calculation resulted in roughly 6.3GB for
min_free_kbytes. The corresponding aggregate min, low and high watermarks were 6.3GB, 7.8GB and 9.4GB.Yeah this is an ongoing issue (as you note below). We definitely need to find a better solution.
Quoted 4 lines
Automatically maintaining multiple GB of additional free memory to improve the probability of 512MB THP allocations seems disproportionate for an opportunistic background optimization.
The fact it's in
khugepaged.cdoesn't mean it's just about khugepaged, it's about avoiding fragmentation for the purposes of obtaining THP pages which can also be obtained from page fault andMADV_COLLAPSE.Quoted 5 lines
This RFC proposes capping the pageblock contribution to the recommendation at 2MB. This would limit the contribution to 22MB per eligible zone while leaving configurations with pageblocks of 2MB or smaller unchanged.
Quoted 6 lines
I guess you're trying to do the most conservative thing possible - but it's a bit of a cheat really.
I mean does it really make sense to reserve 2 MB for the purposes of obtaining 512 MB PMDs? It's essentially giving up on the idea of getting PMD THP altogether but pretending you are still doing so.
Nimrod and I had a discussion about this. It seems that on 64KB base page systems, 2MB mTHP is more reasonable than 512MB. I also assumed that people might use 2MB mTHP instead. min_free_kbytes targets getting PMD THP when under memory pressure, otherwise PMD THP still can be obtained.
But I also now realize that capping pageblock size at 2MB might also hurt other >4KB base page systems, like 16KB base pages, since its per zone reservation is 352MB, much bigger than 22MB.
Quoted 10 lines
I wonder if it wouldn't just be better to explicitly check the page table size and to not even bother trying for > 4 KiB with a comment explaining that it's not a worthwhile trade-off.
I think that'd be the more honest version of this patch wouldn't it?
Or maybe we need not be so concerneda bout scaling across zones...
Quoted 1 line
Quoted 8 lines
Quoted 6 lines
Earlier proposals to resolve this problem involved disabling default THP on affected systems [1], adding a knob to disable the recommendation [2], or deriving the recommendation from the largest enabled THP order [3]. A fixed cap requires no new knob and avoids large changes to global watermarks when THP policy is changed at runtime.
How often are people turning THP completely off or on again though?
Not only just turning THP on or off, but also enabling a new mTHP size or disabling an existing mTHP size. (e.g., anon_enabled_store() used by all mTHP anon_ctrl_attr_grp).
Quoted 78 lines
Quoted 11 lines
Reducing
PAGE_BLOCK_MAX_ORDERremains possible, but it is a build-time policy that changes pageblock granularity globally. Its effects extend beyond the scope of adjustingmin_free_kbytes.[1] https://lore.kernel.org/all/CAPm50aLPxJCiVTqqwiz00oMNiqHggB84sXB3x=tv_HUAd5UktQ@mail.gmail.com/
[2] https://lore.kernel.org/all/[email protected]/
[3] https://lore.kernel.org/all/[email protected]/Signed-off-by Nimrod Oren <[email protected]>Thanks in any case for taking a more pragmatic approach here.
Quoted 26 lines
--- mm/khugepaged.c | 17 +++++++++++++---- 1 file changed, 13 insertions(+), 4 deletions(-)mm/khugepaged.c +2
-0@@ -23,6 +23,7 @@#include <linux/ksm.h> #include <linux/pgalloc.h> #include <linux/backing-dev.h> +#include <linux/sizes.h> #include <asm/tlb.h> #include "internal.h" @@ -3066,6 +3067,7 @@ void set_recommended_min_free_kbytes(void) {struct zone *zone;int nr_zones =0 ; +unsigned long capped_pageblock_nr_pages;unsigned long recommended_min;if (!hugepage_enabled ()) {@@ -3084,16 +3086,23 @@ void set_recommended_min_free_kbytes(void) nr_zones++;mm/khugepaged.c void set_recommended_min_free_kbytes(void) · line 3094
Quoted 10 lines
mm/khugepaged.c +6
-2} -/* Ensure 2 pageblocks are free to assist fragmentation avoidance */ - recommended_min = pageblock_nr_pages * nr_zones *2 ; +/* +* Limit the pageblock contribution to 2 MiB. Larger pageblocks can +* otherwise make min_free_kbytes disproportionately large. +*/ + capped_pageblock_nr_pages = +min (pageblock_nr_pages,SZ_2M /PAGE_SIZE );Yeah this just feels like a hack, to be honest "let's pretend a page block is 2 MiB" essentially :)
Quoted 4 lines
mm/khugepaged.c +3
-0+ +/* Ensure 2 capped units are free to assist fragmentation avoidance */ + recommended_min = capped_pageblock_nr_pages * nr_zones *2 ;mm/khugepaged.c void set_recommended_min_free_kbytes(void) · line 3106
Quoted 10 lines
mm/khugepaged.c +2
-2/* -* Make sure that on average at least two pageblocks are almost free +* Make sure that on average at least two capped units are almost free * of another type, one for a migratetype to fall back to and a * second to avoid subsequent fallbacks of other types There are 3 * MIGRATE_TYPES we care about. */ - recommended_min += pageblock_nr_pages * nr_zones * + recommended_min += capped_pageblock_nr_pages * nr_zones *MIGRATE_PCPTYPES *MIGRATE_PCPTYPES ;we're really getting into faking stuff out realms here - pretending a page block is 2 MiB then doing calculations on it as if they were.
We'd be better off (and more honest) with something like:
(quoted diff) +2
-1- if (!hugepage_enabled()) { + /* Avoid excessive memory reservation if page blocks are very large. */ + if (!hugepage_enabled() || PAGE_SIZE > SZ_4K) {
Quoted 7 lines
But that seems perhaps unwise.
Maybe a simpler version would be to just attack the issue we actually have with it - it uses too much damn memory :)
So maybe something like the attached? (you'd probably want to make this a sysctl or at least a defined value of course)
The approach below makes sense to me. In terms of the actual cap size, I asked Codex to check what possible pageblock_size and their reserved sizes:
1. 1MB pageblock size (s390) and 11MB per zone,
2. 2MB pageblock size and 22MB per zone,
3. 8MB pageblock size and 88MB per zone,
4. 16MB pageblock size and 176MB per zone,
5. 32MB pageblock size and 352MB per zone,
6. 128MB pageblock size and 1.375GB per zone,
7. 512MB pageblock size and 5.5GB per zone.
1GB makes sense to me. 512MB might also work.
Quoted 4 lines
General idea is - if we're going to cap, let's just cap instead of pretending that page blocks are of a different size.
Sounds good to me. Thank you for the suggestion.
Quoted 18 lines
Quoted 5 lines
mm/khugepaged.c +0
-1/* don't ever allow to reserve more than 5% of the lowmem */ --2.45.0
Cheers, Lorenzo
----8<----
mm/khugepaged.c +0
-1@@ -3092,9 +3092,13 @@ void set_recommended_min_free_kbytes(void) recommended_min += pageblock_nr_pages * nr_zones *MIGRATE_PCPTYPES *MIGRATE_PCPTYPES ; -/* don't ever allow to reserve more than 5% of the lowmem */
mm/khugepaged.c void set_recommended_min_free_kbytes(void) · line 3104
Quoted 10 lines
mm/khugepaged.c +5
-0+/* +* don't ever allow the reservation of more than 1 GiB or 5% of lowmem +* (whichever is smaller). +*/ recommended_min =min (recommended_min, (unsigned long )nr_free_buffer_pages () /20 ); + recommended_min =min (recommended_min,SZ_1G ); recommended_min <<= (PAGE_SHIFT -10 );if (recommended_min > min_free_kbytes) {
Best Regards,
Yan, Zi
Quoted 19 lines · Zi Yan
Quoted 5 lines · Lorenzo Stoakes (ARM)
Maybe a simpler version would be to just attack the issue we actually have with it - it uses too much damn memory :)
So maybe something like the attached? (you'd probably want to make this a sysctl or at least a defined value of course)
The approach below makes sense to me. In terms of the actual cap size, I asked Codex to check what possible
pageblock_sizeand their reserved sizes:1. 1MB pageblock size (s390) and 11MB per zone,
2. 2MB pageblock size and 22MB per zone,
3. 8MB pageblock size and 88MB per zone,
4. 16MB pageblock size and 176MB per zone,
5. 32MB pageblock size and 352MB per zone,
6. 128MB pageblock size and 1.375GB per zone,
7. 512MB pageblock size and 5.5GB per zone.
Quoted 2 lines
1GB makes sense to me. 512MB might also work.
Assuming three eligible zones, my original proposal would have been equivalent to a much lower cap of 66MB:
2MB * 11 pageblocks per zone * 3 zones = 66MB
Using a more conservative value also sounds reasonable to me.
Quoted 6 lines
Quoted 3 lines
General idea is - if we're going to cap, let's just cap instead of pretending that page blocks are of a different size.
Sounds good to me. Thank you for the suggestion.
Thanks Lorenzo and Zi for the review.
Capping the final recommendation directly sounds good to me.
Quoted 30 lines · Nimrod Oren
Quoted 19 lines · Zi Yan
Quoted 5 lines · Lorenzo Stoakes (ARM)
Maybe a simpler version would be to just attack the issue we actually have with it - it uses too much damn memory :)
So maybe something like the attached? (you'd probably want to make this a sysctl or at least a defined value of course)
The approach below makes sense to me. In terms of the actual cap size, I asked Codex to check what possible
pageblock_sizeand their reserved sizes:1. 1MB pageblock size (s390) and 11MB per zone,
2. 2MB pageblock size and 22MB per zone,
3. 8MB pageblock size and 88MB per zone,
4. 16MB pageblock size and 176MB per zone,
5. 32MB pageblock size and 352MB per zone,
6. 128MB pageblock size and 1.375GB per zone,
7. 512MB pageblock size and 5.5GB per zone.Quoted 2 lines
1GB makes sense to me. 512MB might also work.
Assuming three eligible zones, my original proposal would have been equivalent to a much lower cap of 66MB:
2MB * 11 pageblocks per zone * 3 zones = 66MB
Using a more conservative value also sounds reasonable to me.
Quoted 8 lines
Quoted 6 lines
Quoted 3 lines
General idea is - if we're going to cap, let's just cap instead of pretending that page blocks are of a different size.
Sounds good to me. Thank you for the suggestion.
Thanks Lorenzo and Zi for the review.
Capping the final recommendation directly sounds good to me.
OK, feel free to respin with that! A Suggested-by is fine for me.
Quoted 2 lines
Cheers, Lorenzo
Quoted 49 lines · Zi Yan
Quoted 47 lines · Lorenzo Stoakes (ARM)
Quoted 2 lines · Nimrod Oren
When THP is enabled, khugepaged can raise
min_free_kbytesthroughset_recommended_min_free_kbytes(), using a heuristic that scales withNitty, but that makes it sound like khugepaged (the process) itself comes in and changes it, rather
set_recommended_min_free_kbytes()sets it higher ifhugepage_enabled().Quoted 9 lines
pageblock_nr_pages. WithMIGRATE_PCPTYPESequal to 3, the formula accounts for 11 pageblocks for every eligible populated zone before capping the result at 5% of low memory.This is reasonable when a pageblock is 2MB, as on common 4KB page configurations, but scales poorly with larger base page sizes. With the default arm64 pageblock sizes, the contribution per eligible zone before the 5% cap is:
Quoted 8 lines
4KB pages: 2MB pageblock, 22MB per zone
16KB pages: 32MB pageblock, 352MB per zone
64KB pages: 512MB pageblock, 5.5GB per zoneThe 5% limit does not make this harmless. On an arm64 system with 128GB of RAM and 64KB pages, the current calculation resulted in roughly 6.3GB for
min_free_kbytes. The corresponding aggregate min, low and high watermarks were 6.3GB, 7.8GB and 9.4GB.Yeah this is an ongoing issue (as you note below). We definitely need to find a better solution.
Quoted 4 lines
Automatically maintaining multiple GB of additional free memory to improve the probability of 512MB THP allocations seems disproportionate for an opportunistic background optimization.
The fact it's in
khugepaged.cdoesn't mean it's just about khugepaged, it's about avoiding fragmentation for the purposes of obtaining THP pages which can also be obtained from page fault andMADV_COLLAPSE.Quoted 5 lines
This RFC proposes capping the pageblock contribution to the recommendation at 2MB. This would limit the contribution to 22MB per eligible zone while leaving configurations with pageblocks of 2MB or smaller unchanged.
I guess you're trying to do the most conservative thing possible - but it's a bit of a cheat really.
Quoted 8 lines
Quoted 3 lines
I mean does it really make sense to reserve 2 MB for the purposes of obtaining 512 MB PMDs? It's essentially giving up on the idea of getting PMD THP altogether but pretending you are still doing so.
Nimrod and I had a discussion about this. It seems that on 64KB base page systems, 2MB mTHP is more reasonable than 512MB. I also assumed that people might use 2MB mTHP instead.
min_free_kbytestargets getting PMD THP when under memory pressure, otherwise PMD THP still can be obtained.
Yeah, it seems 512 MiB PMDs are really a special case. Though Usama mentioned that he found that sizing to be useful on larger machines I remember from LSF!
Quoted 4 lines
But I also now realize that capping pageblock size at 2MB might also hurt other >4KB base page systems, like 16KB base pages, since its per zone reservation is 352MB, much bigger than 22MB.
Right.
I really don't think changing the pageblock is the right way forward, without significant rework of how the pageblock stuff works.
I really dislike 'pretend page blocks are size X' and I also really dislike 'cap page block size without changing default assumptions about PMDs'.
The architecture where this matters meaningfully (arm64) is one where mTHP is available so I think long-term a reworked defragmentation model that accounts for mTHP as well as PMD-sized THP is the way forward.
(I also think it's a little crazy we go to lengths to defragment just for PMD khugepaged).
Quoted 18 lines
Quoted 18 lines
I wonder if it wouldn't just be better to explicitly check the page table size and to not even bother trying for > 4 KiB with a comment explaining that it's not a worthwhile trade-off.
I think that'd be the more honest version of this patch wouldn't it?
Or maybe we need not be so concerneda bout scaling across zones...
Quoted 7 lines
Earlier proposals to resolve this problem involved disabling default THP on affected systems [1], adding a knob to disable the recommendation [2], or deriving the recommendation from the largest enabled THP order [3]. A fixed cap requires no new knob and avoids large changes to global watermarks when THP policy is changed at runtime.
Quoted 5 lines
Quoted 1 line
How often are people turning THP completely off or on again though?
Not only just turning THP on or off, but also enabling a new mTHP size or disabling an existing mTHP size. (e.g.,
anon_enabled_store()used by all mTHPanon_ctrl_attr_grp).
Right yeah, but my point is that set_recommended_min_free_kbytes() only gives a different result if hugepage_enabled() is different.
Quoted 99 lines
Quoted 79 lines
Quoted 7 lines
Reducing
PAGE_BLOCK_MAX_ORDERremains possible, but it is a build-time policy that changes pageblock granularity globally. Its effects extend beyond the scope of adjustingmin_free_kbytes.Quoted 5 lines
[1] https://lore.kernel.org/all/CAPm50aLPxJCiVTqqwiz00oMNiqHggB84sXB3x=tv_HUAd5UktQ@mail.gmail.com/
[2] https://lore.kernel.org/all/[email protected]/
[3] https://lore.kernel.org/all/[email protected]/Signed-off-by Nimrod Oren <[email protected]>Thanks in any case for taking a more pragmatic approach here.
Quoted 26 lines
--- mm/khugepaged.c | 17 +++++++++++++---- 1 file changed, 13 insertions(+), 4 deletions(-)mm/khugepaged.c +2
-0@@ -23,6 +23,7 @@#include <linux/ksm.h> #include <linux/pgalloc.h> #include <linux/backing-dev.h> +#include <linux/sizes.h> #include <asm/tlb.h> #include "internal.h" @@ -3066,6 +3067,7 @@ void set_recommended_min_free_kbytes(void) {struct zone *zone;int nr_zones =0 ; +unsigned long capped_pageblock_nr_pages;unsigned long recommended_min;if (!hugepage_enabled ()) {@@ -3084,16 +3086,23 @@ void set_recommended_min_free_kbytes(void) nr_zones++;mm/khugepaged.c void set_recommended_min_free_kbytes(void) · line 3094
Quoted 10 lines
mm/khugepaged.c +6
-2} -/* Ensure 2 pageblocks are free to assist fragmentation avoidance */ - recommended_min = pageblock_nr_pages * nr_zones *2 ; +/* +* Limit the pageblock contribution to 2 MiB. Larger pageblocks can +* otherwise make min_free_kbytes disproportionately large. +*/ + capped_pageblock_nr_pages = +min (pageblock_nr_pages,SZ_2M /PAGE_SIZE );Yeah this just feels like a hack, to be honest "let's pretend a page block is 2 MiB" essentially :)
Quoted 4 lines
mm/khugepaged.c +3
-0+ +/* Ensure 2 capped units are free to assist fragmentation avoidance */ + recommended_min = capped_pageblock_nr_pages * nr_zones *2 ;mm/khugepaged.c void set_recommended_min_free_kbytes(void) · line 3106
Quoted 10 lines
mm/khugepaged.c +2
-2/* -* Make sure that on average at least two pageblocks are almost free +* Make sure that on average at least two capped units are almost free * of another type, one for a migratetype to fall back to and a * second to avoid subsequent fallbacks of other types There are 3 * MIGRATE_TYPES we care about. */ - recommended_min += pageblock_nr_pages * nr_zones * + recommended_min += capped_pageblock_nr_pages * nr_zones *MIGRATE_PCPTYPES *MIGRATE_PCPTYPES ;we're really getting into faking stuff out realms here - pretending a page block is 2 MiB then doing calculations on it as if they were.
We'd be better off (and more honest) with something like:
(quoted diff) +2
-1- if (!hugepage_enabled()) { + /* Avoid excessive memory reservation if page blocks are very large. */ + if (!hugepage_enabled() || PAGE_SIZE > SZ_4K) {Quoted 7 lines
But that seems perhaps unwise.
Maybe a simpler version would be to just attack the issue we actually have with it - it uses too much damn memory :)
So maybe something like the attached? (you'd probably want to make this a sysctl or at least a defined value of course)
The approach below makes sense to me. In terms of the actual cap size, I asked Codex to check what possible
pageblock_sizeand their reserved sizes:1. 1MB pageblock size (s390) and 11MB per zone,
2. 2MB pageblock size and 22MB per zone,
3. 8MB pageblock size and 88MB per zone,
4. 16MB pageblock size and 176MB per zone,
5. 32MB pageblock size and 352MB per zone,
6. 128MB pageblock size and 1.375GB per zone,
7. 512MB pageblock size and 5.5GB per zone.
Quoted 1 line
1GB makes sense to me. 512MB might also work.
Yeah I felt this was a bit of a 'dumb' solution but ultimately if we are saying 'restrict how much memory this uses to something sane' then that's what we should just do.
It might be a good bandaid at least for this issue which is ongoing, well known, and painful.
I think at 352MB per zone and 2-3 zones for 32 MiB pageblock, plus reserves of 2*512 MiB for 512 MiB pageblock makes 1 GiB a sensible choice here overall.
Quoted 8 lines
Quoted 6 lines
General idea is - if we're going to cap, let's just cap instead of pretending that page blocks are of a different size.
Sounds good to me. Thank you for the suggestion.
Thanks!
Quoted 28 lines
Quoted 28 lines
Quoted 6 lines
mm/khugepaged.c +0
-1/* don't ever allow to reserve more than 5% of the lowmem */ --2.45.0
Cheers, Lorenzo
----8<----
mm/khugepaged.c +5
-1@@ -3092,9 +3092,13 @@ void set_recommended_min_free_kbytes(void) recommended_min += pageblock_nr_pages * nr_zones *MIGRATE_PCPTYPES *MIGRATE_PCPTYPES ; -/* don't ever allow to reserve more than 5% of the lowmem */ +/* +* don't ever allow the reservation of more than 1 GiB or 5% of lowmem +* (whichever is smaller). +*/ recommended_min =min (recommended_min, (unsigned long )nr_free_buffer_pages () /20 ); + recommended_min =min (recommended_min,SZ_1G ); recommended_min <<= (PAGE_SHIFT -10 );
Quoted 5 lines
mm/khugepaged.c void set_recommended_min_free_kbytes(void) · line 3104
Quoted 1 line
mm/khugepaged.c +0
-0if (recommended_min > min_free_kbytes) {Best Regards,
Yan, Zi
Cheers, Lorenzo
Quoted 26 lines · Lorenzo Stoakes (ARM)
Quoted 19 lines · Zi Yan
Quoted 5 lines · Lorenzo Stoakes (ARM)
Maybe a simpler version would be to just attack the issue we actually have with it - it uses too much damn memory :)
So maybe something like the attached? (you'd probably want to make this a sysctl or at least a defined value of course)
The approach below makes sense to me. In terms of the actual cap size, I asked Codex to check what possible
pageblock_sizeand their reserved sizes:1. 1MB pageblock size (s390) and 11MB per zone,
2. 2MB pageblock size and 22MB per zone,
3. 8MB pageblock size and 88MB per zone,
4. 16MB pageblock size and 176MB per zone,
5. 32MB pageblock size and 352MB per zone,
6. 128MB pageblock size and 1.375GB per zone,
7. 512MB pageblock size and 5.5GB per zone.Quoted 1 line
1GB makes sense to me. 512MB might also work.
Yeah I felt this was a bit of a 'dumb' solution but ultimately if we are saying 'restrict how much memory this uses to something sane' then that's what we should just do.
Quoted 6 lines
It might be a good bandaid at least for this issue which is ongoing, well known, and painful.
I think at 352MB per zone and 2-3 zones for 32 MiB pageblock, plus reserves of 2*512 MiB for 512 MiB pageblock makes 1 GiB a sensible choice here overall.
It's worth noting that while users who need more headroom can simply raise min_free_kbytes further via sysctl, those who wish to lower the value can't set it persistently since set_recommended_min_free_kbytes() may raise it back to the recommended value.
I think 1 GiB sounds good, but given this behavior where raising is more accessible than lowering, I wonder if we should opt for a lower value.
A new sysctl for the cap could address this, but I'm not sure whether adding one would be justified.