mm, hugetlb: fix "treat_as_movable" condition in htlb_alloc_mask

PATCH In review Alexandru Moise 2 participants 4 messages Sep 29, 2017 – Sep 30, 2017

Subsystems (2)

Memory management HugeTLB

lore

Share & tools

lore

Alexandru Moise

author

2017-09-29 15:14 UTC #0 lore raw

PATCH 1/1 mm, hugetlb: fix "treat_as_movable" condition in htlb_alloc_mask

If hugepage_migration_supported() returns true, this renders the hugepages_treat_as_movable sysctl completely pointless.

Let's keep this behavior optional by switching the if() condition from || to &&.

Signed-off-by Alexandru Moise <[email protected]>

---
 mm/hugetlb.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

mm/hugetlb.c +1 -1

index 424b0ef08a60..ab28de0122af 100644
@@ -926,7 +926,7 @@ static struct page *dequeue_huge_page_nodemask(struct hstate *h, gfp_t gfp_mask,
 /* Movability of hugepages depends on migration support. */
 static inline gfp_t htlb_alloc_mask(struct hstate *h)
 {
-	if (hugepages_treat_as_movable || hugepage_migration_supported(h))
+	if (hugepages_treat_as_movable && hugepage_migration_supported(h))
 		return GFP_HIGHUSER_MOVABLE;
 	else
 		return GFP_HIGHUSER;

Signature · 7 lines

2.14.2

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to [email protected].  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>

Alexandru Moise

author

2017-09-29 20:43 UTC #1 lore raw

Quoted 27 lines · Alexandru Moise

If hugepage_migration_supported() returns true, this renders the hugepages_treat_as_movable sysctl completely pointless.
Let's keep this behavior optional by switching the if() condition from || to &&.
Signed-off-by Alexandru Moise <[email protected]>
---
 mm/hugetlb.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
mm/hugetlb.c +1 -1
index 424b0ef08a60..ab28de0122af 100644
@@ -926,7 +926,7 @@ static struct page *dequeue_huge_page_nodemask(struct hstate *h, gfp_t gfp_mask,
 /* Movability of hugepages depends on migration support. */
 static inline gfp_t htlb_alloc_mask(struct hstate *h)
 {
-	if (hugepages_treat_as_movable || hugepage_migration_supported(h))
+	if (hugepages_treat_as_movable && hugepage_migration_supported(h))
 		return GFP_HIGHUSER_MOVABLE;
 	else
 		return GFP_HIGHUSER;
Signature · 1 line
2.14.2

I seem to have terribly misunderstood the semantics of this flag wrt hugepages, please ignore this for now.

../Alex

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to [email protected]. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>

Mike Kravetz

2017-09-29 21:16 UTC #2 lore raw

Adding Anshuman

Quoted 13 lines · Alexandru Moise

Quoted 12 lines · Alexandru Moise

mm/hugetlb.c +1 -1

index 424b0ef08a60..ab28de0122af 100644
@@ -926,7 +926,7 @@ static struct page *dequeue_huge_page_nodemask(struct hstate *h, gfp_t gfp_mask,
 /* Movability of hugepages depends on migration support. */
 static inline gfp_t htlb_alloc_mask(struct hstate *h)
 {
-	if (hugepages_treat_as_movable || hugepage_migration_supported(h))
+	if (hugepages_treat_as_movable && hugepage_migration_supported(h))
 		return GFP_HIGHUSER_MOVABLE;

Quoted 8 lines

Quoted 5 lines
mm/hugetlb.c +0 -0
 	else
 		return GFP_HIGHUSER;
Signature · 1 line
2.14.2
I seem to have terribly misunderstood the semantics of this flag wrt hugepages, please ignore this for now.

That is Okay, it made me look at this code more closely.

static inline bool hugepage_migration_supported(struct hstate *h) {

#ifdef CONFIG_ARCH_ENABLE_HUGEPAGE_MIGRATION
        if ((huge_page_shift(h) == PMD_SHIFT) ||
                (huge_page_shift(h) == PGDIR_SHIFT))
                return true;
        else
                return false;
#else
        return false;

#endif
}

So, hugepage_migration_supported() can only return true if ARCH_ENABLE_HUGEPAGE_MIGRATION is defined. Commit c177c81e09e5 restricts hugepage_migration_support to x86_64. So, ARCH_ENABLE_HUGEPAGE_MIGRATION is only defined for x86_64.

Commit 94310cbcaa3c added the ability to migrate gigantic hugetlb pages at the PGD level. This added the check for PGD level pages to hugepage_migration_supported(), which is only there if ARCH_ENABLE_HUGEPAGE_MIGRATION is defined. IIUC, this functionality was added for powerpc. Yet, powerpc does not define ARCH_ENABLE_HUGEPAGE_MIGRATION (unless I am missing something).

Signature · 7 lines

Mike Kravetz

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to [email protected].  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>

Alexandru Moise

author

2017-09-30 08:35 UTC #3 lore raw

Quoted 37 lines · Mike Kravetz

Adding Anshuman
Quoted 13 lines · Alexandru Moise
Quoted 12 lines · Alexandru Moise
mm/hugetlb.c +1 -1
index 424b0ef08a60..ab28de0122af 100644
@@ -926,7 +926,7 @@ static struct page *dequeue_huge_page_nodemask(struct hstate *h, gfp_t gfp_mask,
 /* Movability of hugepages depends on migration support. */
 static inline gfp_t htlb_alloc_mask(struct hstate *h)
 {
-	if (hugepages_treat_as_movable || hugepage_migration_supported(h))
+	if (hugepages_treat_as_movable && hugepage_migration_supported(h))
 		return GFP_HIGHUSER_MOVABLE;
Quoted 8 lines
Quoted 5 lines
mm/hugetlb.c +0 -0
 	else
 		return GFP_HIGHUSER;
Signature · 1 line
2.14.2
I seem to have terribly misunderstood the semantics of this flag wrt hugepages, please ignore this for now.
That is Okay, it made me look at this code more closely.
static inline bool hugepage_migration_supported(struct hstate *h) {
#ifdef CONFIG_ARCH_ENABLE_HUGEPAGE_MIGRATION
        if ((huge_page_shift(h) == PMD_SHIFT) ||
                (huge_page_shift(h) == PGDIR_SHIFT))
                return true;
        else
                return false;
#else
        return false;

Quoted 2 lines

#endif
}

The real problem is that I still get movable hugepages somehow even when that hugepages_treat_as_movable is 0, I need to dig a bit deeper because this behavior really should be optional.

Tools like mcelog are not hugepage aware (IIRC) so users should be able to rather choose between the balance of having their hugepage using application run for longer or run with the higher risk of memory corruption.

Quoted 5 lines

So, hugepage_migration_supported() can only return true if ARCH_ENABLE_HUGEPAGE_MIGRATION is defined. Commit c177c81e09e5 restricts hugepage_migration_support to x86_64. So, ARCH_ENABLE_HUGEPAGE_MIGRATION is only defined for x86_64.

Hmm?

linux$ grep -rin ARCH_ENABLE_HUGEPAGE_MIGRATION * arch/powerpc/platforms/Kconfig.cputype:311:config ARCH_ENABLE_HUGEPAGE_MIGRATION arch/x86/Kconfig:2345:config ARCH_ENABLE_HUGEPAGE_MIGRATION

It is present on PPC_BOOK3S_64

../Alex

Quoted 10 lines

Commit 94310cbcaa3c added the ability to migrate gigantic hugetlb pages at the PGD level. This added the check for PGD level pages to hugepage_migration_supported(), which is only there if ARCH_ENABLE_HUGEPAGE_MIGRATION is defined. IIUC, this functionality was added for powerpc. Yet, powerpc does not define ARCH_ENABLE_HUGEPAGE_MIGRATION (unless I am missing something).
Signature · 1 line
Mike Kravetz