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@google.com>
---
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.
*/
if (is_vma_resv_set(vma, HPAGE_RESV_OWNER)) {
/*
@@ -1394,8 +1395,7 @@ static unsigned long available_huge_pages(struct hstate *h)
static struct folio *dequeue_hugetlb_folio_vma(struct hstate *h,
struct vm_area_struct *vma,
- unsigned long address, int avoid_reserve,
- long chg)
+ unsigned long address, bool use_hstate_resv)
{
struct folio *folio = NULL;
struct mempolicy *mpol;
@@ -1403,16 +1403,7 @@ static struct folio *dequeue_hugetlb_folio_vma(struct hstate *h,
nodemask_t *nodemask;
int nid;
- /*
- * A child process with MAP_PRIVATE mappings created by their parent
- * have no page reserves. This check ensures that reservations are
- * not "stolen". The child may still get SIGKILLed
- */
- if (!vma_has_reserves(vma, chg) && !available_huge_pages(h))
- goto err;
-
- /* If reserves cannot be used, ensure enough pages are in the pool */
- if (avoid_reserve && !available_huge_pages(h))
+ if (!use_hstate_resv && !available_huge_pages(h))
goto err;
gfp_mask = htlb_alloc_mask(h);
@@ -1430,7 +1421,7 @@ static struct folio *dequeue_hugetlb_folio_vma(struct hstate *h,
folio = dequeue_hugetlb_folio_nodemask(h, gfp_mask,
nid, nodemask);
- if (folio && !avoid_reserve && vma_has_reserves(vma, chg)) {
+ if (folio && use_hstate_resv) {
folio_set_hugetlb_restore_reserve(folio);
h->resv_huge_pages--;
}
@@ -2973,6 +2964,7 @@ struct folio *alloc_hugetlb_folio(struct vm_area_struct *vma,
struct mem_cgroup *memcg;
bool deferred_reserve;
gfp_t gfp = htlb_alloc_mask(h) | __GFP_RETRY_MAYFAIL;
+ bool use_hstate_resv;
memcg = get_mem_cgroup_from_current();
memcg_charge_ret = mem_cgroup_hugetlb_try_charge(memcg, gfp, nr_pages);
@@ -3033,20 +3025,17 @@ struct folio *alloc_hugetlb_folio(struct vm_area_struct *vma,
if (ret)
goto out_uncharge_cgroup_reservation;
+ use_hstate_resv = !avoid_reserve && vma_has_reserves(vma, gbl_chg);
+
spin_lock_irq(&hugetlb_lock);
- /*
- * glb_chg is passed to indicate whether or not a page must be taken
- * from the global free pool (global change). gbl_chg == 0 indicates
- * a reservation exists for the allocation.
- */
- folio = dequeue_hugetlb_folio_vma(h, vma, addr, avoid_reserve, gbl_chg);
+ folio = dequeue_hugetlb_folio_vma(h, vma, addr, use_hstate_resv);
if (!folio) {
spin_unlock_irq(&hugetlb_lock);
folio = alloc_buddy_hugetlb_folio_with_mpol(h, vma, addr);
if (!folio)
goto out_uncharge_cgroup;
spin_lock_irq(&hugetlb_lock);
- if (!avoid_reserve && vma_has_reserves(vma, gbl_chg)) {
+ if (use_hstate_resv) {
folio_set_hugetlb_restore_reserve(folio);
h->resv_huge_pages--;
}
--
2.47.0.rc1.288.g06298d1525-googOn 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@google.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@suse.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
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.
Some spacing is definitely good.. as Sean pointed out.
>
> Signed-off-by: Ackerley Tng <ackerleytng@google.com>
> ---
> 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.
> */
> if (is_vma_resv_set(vma, HPAGE_RESV_OWNER)) {
> /*
> @@ -1394,8 +1395,7 @@ static unsigned long available_huge_pages(struct hstate *h)
>
> static struct folio *dequeue_hugetlb_folio_vma(struct hstate *h,
> struct vm_area_struct *vma,
> - unsigned long address, int avoid_reserve,
> - long chg)
> + unsigned long address, bool use_hstate_resv)
Here "avoid_reserve" + "chg" is indeed confusing, especially with the prior
"if (avoid_reserve) gbl_chg = 1;". The new flag can make it slightly
easier to understand indeed for dequeue_hugetlb_folio_vma() alone.
I still feel like there can be something more to be cleaned up here even
after your patch 2-3, but I suppose this could be seen as a small-step
forward, considering one patch change will be harder to review. Feel free
to take:
Acked-by: Peter Xu <peterx@redhat.com>
> {
> struct folio *folio = NULL;
> struct mempolicy *mpol;
> @@ -1403,16 +1403,7 @@ static struct folio *dequeue_hugetlb_folio_vma(struct hstate *h,
> nodemask_t *nodemask;
> int nid;
>
> - /*
> - * A child process with MAP_PRIVATE mappings created by their parent
> - * have no page reserves. This check ensures that reservations are
> - * not "stolen". The child may still get SIGKILLed
> - */
> - if (!vma_has_reserves(vma, chg) && !available_huge_pages(h))
> - goto err;
> -
> - /* If reserves cannot be used, ensure enough pages are in the pool */
> - if (avoid_reserve && !available_huge_pages(h))
> + if (!use_hstate_resv && !available_huge_pages(h))
> goto err;
>
> gfp_mask = htlb_alloc_mask(h);
> @@ -1430,7 +1421,7 @@ static struct folio *dequeue_hugetlb_folio_vma(struct hstate *h,
> folio = dequeue_hugetlb_folio_nodemask(h, gfp_mask,
> nid, nodemask);
>
> - if (folio && !avoid_reserve && vma_has_reserves(vma, chg)) {
> + if (folio && use_hstate_resv) {
> folio_set_hugetlb_restore_reserve(folio);
> h->resv_huge_pages--;
> }
> @@ -2973,6 +2964,7 @@ struct folio *alloc_hugetlb_folio(struct vm_area_struct *vma,
> struct mem_cgroup *memcg;
> bool deferred_reserve;
> gfp_t gfp = htlb_alloc_mask(h) | __GFP_RETRY_MAYFAIL;
> + bool use_hstate_resv;
>
> memcg = get_mem_cgroup_from_current();
> memcg_charge_ret = mem_cgroup_hugetlb_try_charge(memcg, gfp, nr_pages);
> @@ -3033,20 +3025,17 @@ struct folio *alloc_hugetlb_folio(struct vm_area_struct *vma,
> if (ret)
> goto out_uncharge_cgroup_reservation;
>
> + use_hstate_resv = !avoid_reserve && vma_has_reserves(vma, gbl_chg);
> +
> spin_lock_irq(&hugetlb_lock);
> - /*
> - * glb_chg is passed to indicate whether or not a page must be taken
> - * from the global free pool (global change). gbl_chg == 0 indicates
> - * a reservation exists for the allocation.
> - */
> - folio = dequeue_hugetlb_folio_vma(h, vma, addr, avoid_reserve, gbl_chg);
> + folio = dequeue_hugetlb_folio_vma(h, vma, addr, use_hstate_resv);
> if (!folio) {
> spin_unlock_irq(&hugetlb_lock);
> folio = alloc_buddy_hugetlb_folio_with_mpol(h, vma, addr);
> if (!folio)
> goto out_uncharge_cgroup;
> spin_lock_irq(&hugetlb_lock);
> - if (!avoid_reserve && vma_has_reserves(vma, gbl_chg)) {
> + if (use_hstate_resv) {
> folio_set_hugetlb_restore_reserve(folio);
> h->resv_huge_pages--;
> }
> --
> 2.47.0.rc1.288.g06298d1525-goog
>
--
Peter Xu
On Fri, Oct 11, 2024, 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). The period here, and the comma later, are weird. > 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. Whitespace exists for a reason :-) --- 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. ---
© 2016 - 2024 Red Hat, Inc.