| Message ID | b2b7be300469aa59933f08257c794675895d49f8.1728684491.git.ackerleytng@google.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | Reduce dependence on vmas deep in hugetlb allocation code | expand |
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. ---
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 >
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@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'.
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--; }
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(-) -- 2.47.0.rc1.288.g06298d1525-goog