Received-SPF: pass (google.com: domain of linux-kernel+bounces-93509-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:4601:e00::3 as permitted sender) client-ip=2604:1380:4601:e00::3;
Message-ID: <3eda72bd-25ad-4518-b38e-b63f75e5e94d@linux.alibaba.com>
Date: Wed, 6 Mar 2024 16:35:26 +0800
Precedence: bulk
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH 2/3] mm: hugetlb: make the hugetlb migration strategy
 consistent
To: Oscar Salvador <osalvador@suse.de>
Cc: akpm@linux-foundation.org, muchun.song@linux.dev, david@redhat.com,
 linmiaohe@huawei.com, naoya.horiguchi@nec.com, mhocko@kernel.org,
 linux-mm@kvack.org, linux-kernel@vger.kernel.org
References: <cover.1709041586.git.baolin.wang@linux.alibaba.com>
 <bdca97e6c59f1b58f09e18defac0c47d1e38f44b.1709041586.git.baolin.wang@linux.alibaba.com>
 <Zd38_AYtr9QKp-F6@localhost.localdomain>
 <8d35b8ae-b8d8-4237-bfcf-ed63c0bb4223@linux.alibaba.com>
 <Zd7xnrzGb_8QiqcE@localhost.localdomain>
From: Baolin Wang <baolin.wang@linux.alibaba.com>
In-Reply-To: <Zd7xnrzGb_8QiqcE@localhost.localdomain>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit


On 2024/2/28 16:41, Oscar Salvador wrote:
> On Wed, Feb 28, 2024 at 03:40:08PM +0800, Baolin Wang wrote:
>>
>>
>> On 2024/2/27 23:17, Oscar Salvador wrote:
>>> On Tue, Feb 27, 2024 at 09:52:26PM +0800, Baolin Wang wrote:
>>>
>>>> --- a/mm/hugetlb.c
>>>> +++ b/mm/hugetlb.c
>>>> @@ -2567,13 +2567,38 @@ static struct folio *alloc_surplus_hugetlb_folio(struct hstate *h,
>>>>    }
>>>>    static struct folio *alloc_migrate_hugetlb_folio(struct hstate *h, gfp_t gfp_mask,
>>>> -				     int nid, nodemask_t *nmask)
>>>> +				     int nid, nodemask_t *nmask, int reason)
>>>
>>> I still dislike taking the reason argument this far, and I'd rather have
>>> this as a boolean specifing whether we allow fallback on other nodes.
>>> That would mean parsing the reason in alloc_migration_target().
>>> If we don't add a new helper e.g: gfp_allow_fallback(), we can just do
>>> it right there an opencode it with a e.g: macro etc.
>>>
>>> Although doing it in an inline helper might help hiding these details.
>>>
>>> That's my take on this, but let's see what others have to say.
>>
>> Sure. I also expressed my preference for hiding these details within the
>> hugetlb core as much as possible.
>>
>> Muchun, what do you think? Thanks.
> 
> JFTR: I'm talking about https://lore.kernel.org/linux-mm/ZdxXLTDZn8fD3pEn@localhost.localdomain/
> or maybe something cleaner which doesn't need a new helper (we could if
> we want though):
> 
> diff --git a/include/linux/hugetlb.h b/include/linux/hugetlb.h
> index c1ee640d87b1..ddd794e861e6 100644
> --- a/include/linux/hugetlb.h
> +++ b/include/linux/hugetlb.h
> @@ -73,6 +73,16 @@ struct resv_map {
>   #endif
>   };
> 
> +#define MIGRATE_MEMORY_HOTPLUG	1UL << MR_MEMORY_HOTPLUG
> +#define MIGRATE_MEMORY_FAILURE	1UL << MR_MEMORY_FAILURE
> +#define MIGRATE_SYSCALL		1UL << MR_SYSCALL
> +#define MIGRATE_MBIND		1UL << MR_MEMPOLICY_MBIND
> +#define HTLB_ALLOW_FALLBACK	(MIGRATE_MEMORY_HOTPLUG| \
> +				 MIGRATE_MEMORY_FAILURE| \
> +				 MIGRATE_SYSCALL| \
> +				 MIGRATE_MBIND)
> +
> +
>   /*
>    * Region tracking -- allows tracking of reservations and instantiated pages
>    *                    across the pages in a mapping.
> diff --git a/mm/hugetlb.c b/mm/hugetlb.c
> index ed1581b670d4..7e8d6b5885d6 100644
> --- a/mm/hugetlb.c
> +++ b/mm/hugetlb.c
> @@ -2619,7 +2619,7 @@ struct folio *alloc_buddy_hugetlb_folio_with_mpol(struct hstate *h,
> 
>   /* folio migration callback function */
>   struct folio *alloc_hugetlb_folio_nodemask(struct hstate *h, int preferred_nid,
> -		nodemask_t *nmask, gfp_t gfp_mask)
> +		nodemask_t *nmask, gfp_t gfp_mask, bool allow_fallback)
>   {
>   	spin_lock_irq(&hugetlb_lock);
>   	if (available_huge_pages(h)) {
> @@ -2634,6 +2634,12 @@ struct folio *alloc_hugetlb_folio_nodemask(struct hstate *h, int preferred_nid,
>   	}
>   	spin_unlock_irq(&hugetlb_lock);
> 
> +	/*
> +	 * We cannot fallback to other nodes, as we could break the per-node pool
> +	 */
> +	if (!allow_fallback)
> +		gfp_mask |= GFP_THISNODE; > +
>   	return alloc_migrate_hugetlb_folio(h, gfp_mask, preferred_nid, nmask);
>   }
> 
> diff --git a/mm/migrate.c b/mm/migrate.c
> index cc9f2bcd73b4..c1f1d011629d 100644
> --- a/mm/migrate.c
> +++ b/mm/migrate.c
> @@ -2016,10 +2016,15 @@ struct folio *alloc_migration_target(struct folio *src, unsigned long private)
> 
>   	if (folio_test_hugetlb(src)) {
>   		struct hstate *h = folio_hstate(src);
> +		bool allow_fallback = false;
> +
> +		if ((1UL << reason) & HTLB_ALLOW_FALLBACK)
> +			allow_fallback = true;

IMHO, users also should not be aware of these hugetlb logics.

> 
>   		gfp_mask = htlb_modify_alloc_mask(h, gfp_mask);
>   		return alloc_hugetlb_folio_nodemask(h, nid,
> -						mtc->nmask, gfp_mask);
> +						mtc->nmask, gfp_mask,
> +						allow_fallback);

'allow_fallback' can be confusing, that means it is 'allow_fallback' for 
a new temporary hugetlb allocation, but not 'allow_fallback' for an 
available hugetlb allocation in alloc_hugetlb_folio_nodemask().