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
