Re: [RFC PATCH 2/3] mm: hugetlb: Refactor vma_has_reserves() to should_use_hstate_resv()

[Oscar Salvador]

On Tue, Nov 05, 2024 at 01:46:39PM -0500, Peter Xu wrote:
> I wonder if this patch could be merged with the 3rd, IIUC this can
> fundamentally be seen as a movement of what patch 3 was removed.

I think it makes sense to merge it, yes.

> Furthermore, I do feel like should_use_hstate_resv() could be redundant on
> its own on many things.

...
 
> Then let's look at chg==0 processing all above: what does it say?  I
> suppose it means "we don't need another global reservation".  It means if
> chg==0 we always will use an existing reservation.  From math POV it also
> is true, so it can already be moved out ahead, IIUC, like this:
> 
> static bool should_use_hstate_resv(struct vm_area_struct *vma, long chg,
> 				   bool avoid_reserve)
> {
> 	if (avoid_reserve)
> 		return false;
> 
>         if (chg == 0)
>                 return true;
> 
> 	if (vma->vm_flags & VM_NORESERVE)
>                 return false;
> 
> 	if (vma->vm_flags & VM_MAYSHARE)
>                 return false;
> 
> 	if (is_vma_resv_set(vma, HPAGE_RESV_OWNER))
>                 return false;
> 
> 	return false;             <--------------------- [1]
> }
> 
> Move on.  If I read it right, above [1] is exactly for avoid_reserve==1
> case, where it basically says "it's !NORESERVE, private, and it's not the
> vma resv owner, either fork() or CoW".  If my reading is correct, it means
> after your patch 2, [1] should never be reachable at all.. I would hope
> adding a panic() right above would be ok.
> 
> And irrelevant of whether my understanding is correct.. math-wise above is
> also already the same as:
> 
> static bool should_use_hstate_resv(struct vm_area_struct *vma, long chg,
> 				   bool avoid_reserve)
> {
> 	if (avoid_reserve)
> 		return false;
> 
>         if (chg == 0)
>                 return true;
> 
> 	return false;
> }

I have been looking into this because hugetlb reservations always make
me uneasy, but I think you are right.

CoW and fork both set avoid_reserve to 1,

 copy_hugetlb_range
  ...
  alloc_hugetlb_folio(dst_vma, addr, 1)

 hugetlb_wp
  outside_reserve = 1
  alloc_hugetlb_folio(..., outside_reserve)

So I think you are right and this can be simplified.

I would not add a panic though, maybe some kind of warning (VM_DEBUG?).


-- 
Oscar Salvador
SUSE Labs