On 10/10/2018 03:09 PM, Michal Hocko wrote:
> On Wed 10-10-18 08:39:22, Anshuman Khandual wrote:
> [...]
>> 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;
>
> Exactly what I've had in mind. It would be great to have a comment in
> hugepage_movable_supported to explain why we are not supporting giga
> pages even though they are migrateable and why we need that distinction.
sure, will do.
>
>> The above patch is in addition to the following later patch in the series.
> [...]
>> 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);
>> }
>
> Yes making hugepage_migration_supported to have an arch override is
> definitely the right thing to do. Whether the above approach rather than
> a weak symbol is better is a matter of taste and I do not feel strongly
> about that.
Okay then, will carry this forward and re-spin the patch series. Thank you
for your detailed review till now.
