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
