Received-SPF: pass (google.com: domain of linux-kernel+bounces-77454-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45d1:ec00::1 as permitted sender) client-ip=2604:1380:45d1:ec00::1;
Date: Thu, 22 Feb 2024 23:15:39 +0100
From: Oscar Salvador <osalvador@suse.de>
To: Baolin Wang <baolin.wang@linux.alibaba.com>
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
Subject: Re: [RFC PATCH 2/3] mm: hugetlb: make the hugetlb migration strategy
 consistent
Message-ID: <ZdfHi142dvQuN7B-@localhost.localdomain>
References: <cover.1708507022.git.baolin.wang@linux.alibaba.com>
 <0514e5139b17ecf3cd9e09d86c93e586c56688dc.1708507022.git.baolin.wang@linux.alibaba.com>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <0514e5139b17ecf3cd9e09d86c93e586c56688dc.1708507022.git.baolin.wang@linux.alibaba.com>

On Wed, Feb 21, 2024 at 05:27:54PM +0800, Baolin Wang wrote:
> Based on the analysis of the various scenarios above, determine whether fallback is
> permitted according to the migration reason in alloc_hugetlb_folio_nodemask().

Hi Baolin,

The high level reasoning makes sense to me, taking a step back and
thinking about all cases and possible outcomes makes sense to me.

I plan to look closer, but I something that caught my eye:


>  	}
>  	spin_unlock_irq(&hugetlb_lock);
>  
> +	if (gfp_mask & __GFP_THISNODE)
> +		goto alloc_new;
> +
> +	/*
> +	 * Note: the memory offline, memory failure and migration syscalls can break
> +	 * the per-node hugetlb pool. Other cases can not allocate new hugetlb on
> +	 * other nodes.
> +	 */
> +	switch (reason) {
> +	case MR_MEMORY_HOTPLUG:
> +	case MR_MEMORY_FAILURE:
> +	case MR_SYSCALL:
> +	case MR_MEMPOLICY_MBIND:
> +		allowed_fallback = true;
> +		break;
> +	default:
> +		break;
> +	}
> +
> +	if (!allowed_fallback)
> +		gfp_mask |= __GFP_THISNODE;

I think it would be better if instead of fiddling with gfp here,
have htlb_alloc_mask() have a second argument with the MR_reason,
do the switch there and enable GFP_THISNODE.
Then alloc_hugetlb_folio_nodemask() would already get the right mask.

I think that that might be more clear as it gets encapsulated in the
function that directly gives us the gfp.

Does that makes sense?


-- 
Oscar Salvador
SUSE Labs