Re: [PATCH 1/4] mm/hugetlb: Enable PUD level huge page migration

[Anshuman Khandual <anshuman.khandual@arm.com>]

On 10/09/2018 07:44 PM, Michal Hocko wrote:
> On Fri 05-10-18 13:04:43, Anshuman Khandual wrote:
>> Does the following sound close enough to what you are looking for ?
> 
> I do not think so

Okay.

> 
>> diff --git a/include/linux/hugetlb.h b/include/linux/hugetlb.h
>> index 9df1d59..070c419 100644
>> --- a/include/linux/hugetlb.h
>> +++ b/include/linux/hugetlb.h
>> @@ -504,6 +504,13 @@ static inline bool hugepage_migration_supported(struct hstate *h)
>>         return arch_hugetlb_migration_supported(h);
>>  }
>>  
>> +static inline bool hugepage_movable_required(struct hstate *h)
>> +{
>> +       if (hstate_is_gigantic(h))
>> +               return true;
>> +       return false;
>> +}
>> +
> 
> Apart from naming (hugepage_movable_supported?) the above doesn't do the
> most essential thing to query whether the hugepage migration is
> supported at all. Apart from that i would expect the logic to be revers.

My assumption was hugepage_migration_supported() should only be called from
unmap_and_move_huge_page() but we can add that here as well to limit the
set further.

> We do not really support giga pages migration enough to support them in
> movable zone.

Reversing the logic here would change gfp_t for a large number of huge pages.
Current implementation is very liberal in assigning __GFP_MOVABLE for multiple
huge page sizes (all most all of them when migration is enabled). But I guess
it should be okay because after all we are tying to control which all sizes
should be movable and which all should not be.

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
}

static inline gfp_t htlb_alloc_mask(struct hstate *h)
{
        if (hugepage_migration_supported(h))
                return GFP_HIGHUSER_MOVABLE;
        else
                return GFP_HIGHUSER;
}

>> @@ -1652,6 +1655,9 @@ struct page *alloc_huge_page_nodemask(struct hstate *h, int preferred_nid,
>>  {
>>         gfp_t gfp_mask = htlb_alloc_mask(h);
>>  
>> +       if (hugepage_movable_required(h))
>> +               gfp_mask |= __GFP_MOVABLE;
>> +
> 
> And besides that this really want to live in htlb_alloc_mask because
> this is really an allocation policy. It would be unmap_and_move_huge_page
> to call hugepage_migration_supported. The later is the one to allow for
> an arch specific override.
> 
> Makes sense?
> 

hugepage_migration_supported() will be checked inside hugepage_movable_supported().
A changed version ....

diff --git a/include/linux/hugetlb.h b/include/linux/hugetlb.h
index 9df1d59..4bcbf1e 100644
--- a/include/linux/hugetlb.h
+++ b/include/linux/hugetlb.h
@@ -504,6 +504,16 @@ static inline bool hugepage_migration_supported(struct hstate *h)
        return arch_hugetlb_migration_supported(h);
 }
 
+static inline bool hugepage_movable_supported(struct hstate *h)
+{
+       if (!hugepage_migration_supported(h)) --> calls arch override restricting the set
+               return false;
+
+       if (hstate_is_gigantic(h)	--------> restricts the set further
+               return false;
+       return true;
+}
+
 static inline spinlock_t *huge_pte_lockptr(struct hstate *h,
                                           struct mm_struct *mm, pte_t *pte)
 {
@@ -600,6 +610,11 @@ static inline bool hugepage_migration_supported(struct hstate *h)
        return false;
 }
 
+static inline bool hugepage_movable_supported(struct hstate *h)
+{
+       return false;
+}
+
 static inline spinlock_t *huge_pte_lockptr(struct hstate *h,
                                           struct mm_struct *mm, pte_t *pte)
 {
diff --git a/mm/hugetlb.c b/mm/hugetlb.c
index 3c21775..a5a111d 100644
--- a/mm/hugetlb.c
+++ b/mm/hugetlb.c
@@ -919,7 +919,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 (hugepage_migration_supported(h))
+       if (hugepage_movable_supported(h))
                return GFP_HIGHUSER_MOVABLE;
        else
                return GFP_HIGHUSER;


The above patch is in addition to the following later patch in the series.

    mm/hugetlb: Enable arch specific huge page size support for migration
    
    Architectures like arm64 have HugeTLB page sizes which are different than
    generic sizes at PMD, PUD, PGD level and implemented via contiguous bits.
    At present these special size HugeTLB pages cannot be identified through
    macros like (PMD|PUD|PGDIR)_SHIFT and hence chosen not be migrated.
    
    Enabling migration support for these special HugeTLB page sizes along with
    the generic ones (PMD|PUD|PGD) would require identifying all of them on a
    given platform. A platform specific hook can precisely enumerate all huge
    page sizes supported for migration. Instead of comparing against standard
    huge page orders let hugetlb_migration_support() function call a platform
    hook arch_hugetlb_migration_support(). Default definition for the platform
    hook maintains existing semantics which checks standard huge page order.
    But an architecture can choose to override the default and provide support
    for a comprehensive set of huge page sizes.
    
    Signed-off-by: Anshuman Khandual <anshuman.khandual@arm.com>

diff --git a/include/linux/hugetlb.h b/include/linux/hugetlb.h
index 9c1b77f..9df1d59 100644
--- a/include/linux/hugetlb.h
+++ b/include/linux/hugetlb.h
@@ -479,18 +479,29 @@ static inline pgoff_t basepage_index(struct page *page)
 extern int dissolve_free_huge_page(struct page *page);
 extern int dissolve_free_huge_pages(unsigned long start_pfn,
                                    unsigned long end_pfn);
-static inline bool hugepage_migration_supported(struct hstate *h)
-{
+
 #ifdef CONFIG_ARCH_ENABLE_HUGEPAGE_MIGRATION
+#ifndef arch_hugetlb_migration_supported
+static inline bool arch_hugetlb_migration_supported(struct hstate *h)
+{
        if ((huge_page_shift(h) == PMD_SHIFT) ||
                (huge_page_shift(h) == PUD_SHIFT) ||
                        (huge_page_shift(h) == PGDIR_SHIFT))
                return true;
        else
                return false;
+}
+#endif
 #else
+static inline bool arch_hugetlb_migration_supported(struct hstate *h)
+{
        return false;
+}
 #endif
+
+static inline bool hugepage_migration_supported(struct hstate *h)
+{
+       return arch_hugetlb_migration_supported(h);
 }