linux-kernel - Re: [RFC PATCH 1/3] mm: hugetlb: Simplify logic in dequeue_hugetlb_folio

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <ZytBNcUUpDBL3Sm4@localhost.localdomain>
Date: Wed, 6 Nov 2024 11:13:09 +0100
From: Oscar Salvador <osalvador@...e.de>
To: Ackerley Tng <ackerleytng@...gle.com>
Cc: muchun.song@...ux.dev, peterx@...hat.com, akpm@...ux-foundation.org,
	rientjes@...gle.com, fvdl@...gle.com, jthoughton@...gle.com,
	david@...hat.com, isaku.yamahata@...el.com, zhiquan1.li@...el.com,
	fan.du@...el.com, jun.miao@...el.com, tabba@...gle.com,
	quic_eberman@...cinc.com, roypat@...zon.co.uk, jgg@...dia.com,
	jhubbard@...dia.com, seanjc@...gle.com, pbonzini@...hat.com,
	erdemaktas@...gle.com, vannapurve@...gle.com, pgonda@...gle.com,
	linux-kernel@...r.kernel.org, linux-mm@...ck.org
Subject: Re: [RFC PATCH 1/3] mm: hugetlb: Simplify logic in
 dequeue_hugetlb_folio_vma()

On Fri, Oct 11, 2024 at 11:22:36PM +0000, Ackerley Tng wrote:
> Replace arguments avoid_reserve and chg in dequeue_hugetlb_folio_vma()
> so dequeue_hugetlb_folio_vma() is more understandable.
> 
> The new argument, use_hstate_resv, indicates whether the folio to be
> dequeued should be taken from reservations in hstate.
> 
> If use_hstate_resv is true, the folio to be dequeued should be taken
> from reservations in hstate and hence h->resv_huge_pages is
> decremented, and the folio is marked so that the reservation is
> restored.
> 
> If use_hstate_resv is false, then a folio needs to be taken from the
> pool and hence there must exist available_huge_pages(h), failing
> which, goto err.
> 
> The bool use_hstate_resv can be reused within
> dequeue_hugetlb_folio_vma()'s caller, alloc_hugetlb_folio().
> 
> No functional changes are intended.
> 
> As proof, the original two if conditions
> 
> !vma_has_reserves(vma, chg) && !available_huge_pages(h)
> 
> and
> 
> avoid_reserve && !available_huge_pages(h)
> 
> can be combined into
> 
> (avoid_reserve || !vma_has_reserves(vma, chg))
> && !available_huge_pages(h).
> 
> Applying de Morgan's theorem on
> 
> avoid_reserve || !vma_has_reserves(vma, chg)
> 
> yields
> 
> !avoid_reserve && vma_has_reserves(vma, chg),
> 
> hence the simplification is correct.
> 
> Signed-off-by: Ackerley Tng <ackerleytng@...gle.com>

I like this, it is a nice simplification and hugetlb revervation
mechanism is already hard enough to follow.

As already mentioned, Sean's changes look easier to follow.

Reviewed-by: Oscar Salvador <osalvador@...e.de>

On a side note, we might want to convert the 'avoid_reserve' param from
alloc_hugetlb_folio into a bool, as we are using it exactly like that,
so it would seem more natutal.

> ---
>  mm/hugetlb.c | 33 +++++++++++----------------------
>  1 file changed, 11 insertions(+), 22 deletions(-)
> 
> diff --git a/mm/hugetlb.c b/mm/hugetlb.c
> index 190fa05635f4..73165c670739 100644
> --- a/mm/hugetlb.c
> +++ b/mm/hugetlb.c
> @@ -1281,8 +1281,9 @@ static bool vma_has_reserves(struct vm_area_struct *vma, long chg)
>  	}
> 
>  	/*
> -	 * Only the process that called mmap() has reserves for
> -	 * private mappings.
> +	 * Only the process that called mmap() has reserves for private
> +	 * mappings. A child process with MAP_PRIVATE mappings created by their
> +	 * parent have no page reserves.

'page reserves' looks odd. I would say 'reservations'.


-- 
Oscar Salvador
SUSE Labs