On Thu, Nov 7, 2019 at 4:57 PM Mike Kravetz mike.kravetz@oracle.com wrote:

On 10/29/19 6:36 PM, Mina Almasry wrote:

...

Augments hugetlb_cgroup_charge_cgroup to be able to charge hugetlb usage or hugetlb reservation counter.

Adds a new interface to uncharge a hugetlb_cgroup counter via hugetlb_cgroup_uncharge_counter.

Integrates the counter with hugetlb_cgroup, via hugetlb_cgroup_init, hugetlb_cgroup_have_usage, and hugetlb_cgroup_css_offline.

Signed-off-by: Mina Almasry almasrymina@google.com

---

include/linux/hugetlb_cgroup.h | 67 +++++++++++++--------- mm/hugetlb.c | 17 +++--- mm/hugetlb_cgroup.c | 100 +++++++++++++++++++++++++-------- 3 files changed, 130 insertions(+), 54 deletions(-)

diff --git a/include/linux/hugetlb_cgroup.h b/include/linux/hugetlb_cgroup.h index 063962f6dfc6a..1bb58a63af586 100644 --- a/include/linux/hugetlb_cgroup.h +++ b/include/linux/hugetlb_cgroup.h @@ -22,27 +22,35 @@ struct hugetlb_cgroup;

• Minimum page order trackable by hugetlb cgroup.
• At least 3 pages are necessary for all the tracking information.

*/ -#define HUGETLB_CGROUP_MIN_ORDER 2 +#define HUGETLB_CGROUP_MIN_ORDER 3

Correct me if misremembering, but I think the reson you changed this was so that you could use page[3].private. Correct? In that case isn't page[3] the last page of an order 2 allocation? If my understanding is correct, then leave HUGETLB_CGROUP_MIN_ORDER as is and update the preceding comment to say that at least 4 pages are necessary.

Yes, I just misunderstood what MIN_ORDER means. I'll revert the code change.

...

#ifdef CONFIG_CGROUP_HUGETLB

-static inline struct hugetlb_cgroup *hugetlb_cgroup_from_page(struct page *page) +static inline struct hugetlb_cgroup *hugetlb_cgroup_from_page(struct page *page,

•                                                       bool reserved)
  

{ VM_BUG_ON_PAGE(!PageHuge(page), page);

  if (compound_order(page) < HUGETLB_CGROUP_MIN_ORDER)
          return NULL;

• return (struct hugetlb_cgroup *)page[2].private;
  

• if (reserved)
  

•         return (struct hugetlb_cgroup *)page[3].private;
  

• else
  

•         return (struct hugetlb_cgroup *)page[2].private;
  

}

-static inline -int set_hugetlb_cgroup(struct page *page, struct hugetlb_cgroup *h_cg) +static inline int set_hugetlb_cgroup(struct page *page,

•                              struct hugetlb_cgroup *h_cg,
  

•                              bool reservation)
  

{ VM_BUG_ON_PAGE(!PageHuge(page), page);

  if (compound_order(page) < HUGETLB_CGROUP_MIN_ORDER)
          return -1;

• page[2].private = (unsigned long)h_cg;
  

• if (reservation)
  

•         page[3].private = (unsigned long)h_cg;
  

• else
  

•         page[2].private = (unsigned long)h_cg;
  return 0;
  

}

@@ -52,26 +60,33 @@ static inline bool hugetlb_cgroup_disabled(void) }

extern int hugetlb_cgroup_charge_cgroup(int idx, unsigned long nr_pages,

•                                 struct hugetlb_cgroup **ptr);
  

•                                 struct hugetlb_cgroup **ptr,
  

•                                 bool reserved);
  

extern void hugetlb_cgroup_commit_charge(int idx, unsigned long nr_pages, struct hugetlb_cgroup *h_cg,

•                                  struct page *page);
  

•                                  struct page *page, bool reserved);
  

extern void hugetlb_cgroup_uncharge_page(int idx, unsigned long nr_pages,

•                                  struct page *page);
  

•                                  struct page *page, bool reserved);
  

extern void hugetlb_cgroup_uncharge_cgroup(int idx, unsigned long nr_pages,

•                                    struct hugetlb_cgroup *h_cg);
  

•                                    struct hugetlb_cgroup *h_cg,
  

•                                    bool reserved);
  

+extern void hugetlb_cgroup_uncharge_counter(struct page_counter *p,

•                                     unsigned long nr_pages);
  

extern void hugetlb_cgroup_file_init(void) __init; extern void hugetlb_cgroup_migrate(struct page *oldhpage, struct page *newhpage);

#else -static inline struct hugetlb_cgroup *hugetlb_cgroup_from_page(struct page *page) +static inline struct hugetlb_cgroup *hugetlb_cgroup_from_page(struct page *page,

•                                                       bool reserved)
  

{ return NULL; }

-static inline -int set_hugetlb_cgroup(struct page *page, struct hugetlb_cgroup *h_cg) +static inline int set_hugetlb_cgroup(struct page *page,

•                              struct hugetlb_cgroup *h_cg, bool reserved)
  

{ return 0; } @@ -81,28 +96,30 @@ static inline bool hugetlb_cgroup_disabled(void) return true; }

-static inline int -hugetlb_cgroup_charge_cgroup(int idx, unsigned long nr_pages,

•                      struct hugetlb_cgroup **ptr)
  

+static inline int hugetlb_cgroup_charge_cgroup(int idx, unsigned long nr_pages,

•                                        struct hugetlb_cgroup **ptr,
  

•                                        bool reserved)
  

{ return 0; }

-static inline void -hugetlb_cgroup_commit_charge(int idx, unsigned long nr_pages,

•                      struct hugetlb_cgroup *h_cg,
  

•                      struct page *page)
  

+static inline void hugetlb_cgroup_commit_charge(int idx, unsigned long nr_pages,

•                                         struct hugetlb_cgroup *h_cg,
  

•                                         struct page *page,
  

•                                         bool reserved)
  

{ }

-static inline void -hugetlb_cgroup_uncharge_page(int idx, unsigned long nr_pages, struct page *page) +static inline void hugetlb_cgroup_uncharge_page(int idx, unsigned long nr_pages,

•                                         struct page *page,
  

•                                         bool reserved)
  

{ }

-static inline void -hugetlb_cgroup_uncharge_cgroup(int idx, unsigned long nr_pages,

•                        struct hugetlb_cgroup *h_cg)
  

+static inline void hugetlb_cgroup_uncharge_cgroup(int idx,

•                                           unsigned long nr_pages,
  

•                                           struct hugetlb_cgroup *h_cg,
  

•                                           bool reserved)
  

{ }

diff --git a/mm/hugetlb.c b/mm/hugetlb.c index 37195cd60a345..325d5454bf168 100644 --- a/mm/hugetlb.c +++ b/mm/hugetlb.c @@ -1140,7 +1140,7 @@ static void update_and_free_page(struct hstate *h, struct page *page) 1 << PG_active | 1 << PG_private | 1 << PG_writeback); }

• VM_BUG_ON_PAGE(hugetlb_cgroup_from_page(page), page);
  

• VM_BUG_ON_PAGE(hugetlb_cgroup_from_page(page, false), page);
  

Shouldn't we add a VM_BUG_ON_PAGE for reserved as well? Oh, I see it is done in a later patch. I guess it is not here as there are no users of reserved yet?

Yes precisely.

Same observation in other places in hugetlb.c below. Since you add the API changes for reserved here, as well as define page[3].private to store info, I don't think it would hurt to add the initialization and checking and cleanup here as well. Thoughts?

Yes that makes sense as well. I'll take a look.

...

  set_compound_page_dtor(page, NULL_COMPOUND_DTOR);
  set_page_refcounted(page);
  if (hstate_is_gigantic(h)) {

@@ -1250,8 +1250,8 @@ void free_huge_page(struct page *page)

  spin_lock(&hugetlb_lock);
  clear_page_huge_active(page);

• hugetlb_cgroup_uncharge_page(hstate_index(h),
  

•                              pages_per_huge_page(h), page);
  

• hugetlb_cgroup_uncharge_page(hstate_index(h), pages_per_huge_page(h),
  

•                              page, false);
  if (restore_reserve)
          h->resv_huge_pages++;
  
  

@@ -1277,7 +1277,7 @@ static void prep_new_huge_page(struct hstate *h, struct page *page, int nid) INIT_LIST_HEAD(&page->lru); set_compound_page_dtor(page, HUGETLB_PAGE_DTOR); spin_lock(&hugetlb_lock);

• set_hugetlb_cgroup(page, NULL);
  

• set_hugetlb_cgroup(page, NULL, false);
  h->nr_huge_pages++;
  h->nr_huge_pages_node[nid]++;
  spin_unlock(&hugetlb_lock);
  

@@ -2063,7 +2063,8 @@ struct page *alloc_huge_page(struct vm_area_struct *vma, gbl_chg = 1; }

• ret = hugetlb_cgroup_charge_cgroup(idx, pages_per_huge_page(h), &h_cg);
  

• ret = hugetlb_cgroup_charge_cgroup(idx, pages_per_huge_page(h), &h_cg,
  

•                                    false);
  if (ret)
          goto out_subpool_put;
  
  

@@ -2087,7 +2088,8 @@ struct page *alloc_huge_page(struct vm_area_struct *vma, list_move(&page->lru, &h->hugepage_activelist); /* Fall through */ }

• hugetlb_cgroup_commit_charge(idx, pages_per_huge_page(h), h_cg, page);
  

• hugetlb_cgroup_commit_charge(idx, pages_per_huge_page(h), h_cg, page,
  

•                              false);
  spin_unlock(&hugetlb_lock);
  
  set_page_private(page, (unsigned long)spool);
  

@@ -2111,7 +2113,8 @@ struct page *alloc_huge_page(struct vm_area_struct *vma, return page;

out_uncharge_cgroup:

• hugetlb_cgroup_uncharge_cgroup(idx, pages_per_huge_page(h), h_cg);
  

• hugetlb_cgroup_uncharge_cgroup(idx, pages_per_huge_page(h), h_cg,
  

•                                false);
  

out_subpool_put: if (map_chg || avoid_reserve) hugepage_subpool_put_pages(spool, 1); diff --git a/mm/hugetlb_cgroup.c b/mm/hugetlb_cgroup.c index 1ed4448ca41d3..854117513979b 100644 --- a/mm/hugetlb_cgroup.c +++ b/mm/hugetlb_cgroup.c @@ -73,8 +73,12 @@ static inline bool hugetlb_cgroup_have_usage(struct hugetlb_cgroup *h_cg) int idx;

  for (idx = 0; idx < hugetlb_max_hstate; idx++) {

•         if (page_counter_read(&h_cg->hugepage[idx]))
  

•         if (page_counter_read(
  

•                     hugetlb_cgroup_get_counter(h_cg, idx, true)) ||
  

•             page_counter_read(
  

•                     hugetlb_cgroup_get_counter(h_cg, idx, false))) {
                  return true;
  

•         }
  }
  return false;
  

} @@ -85,18 +89,32 @@ static void hugetlb_cgroup_init(struct hugetlb_cgroup *h_cgroup, int idx;

  for (idx = 0; idx < HUGE_MAX_HSTATE; idx++) {

•         struct page_counter *counter = &h_cgroup->hugepage[idx];
          struct page_counter *parent = NULL;
  

Should we perhaps rename 'parent' to 'fault_parent' to be consistent?

Yes that makes sense; will do.

That makes me think if perhaps the naming in the previous patch should be more explicit. Make the existing names explicitly contin 'fault' as the new names contain 'reservation'. Just a thought.

You mean change the names of the actual user-facing files? I'm all for better names but that would break existing users that read/write the hugetlb_cgroup.2MB.usage_in_bytes/limit_in_bytes users, and so I would assume is a no-go.

...

•         struct page_counter *reserved_parent = NULL;
          unsigned long limit;
          int ret;
  
  

•         if (parent_h_cgroup)
  

•                 parent = &parent_h_cgroup->hugepage[idx];
  

•         page_counter_init(counter, parent);
  

•         if (parent_h_cgroup) {
  

•                 parent = hugetlb_cgroup_get_counter(parent_h_cgroup,
  

•                                                     idx, false);
  

•                 reserved_parent = hugetlb_cgroup_get_counter(
  

•                         parent_h_cgroup, idx, true);
  

•         }
  

•         page_counter_init(hugetlb_cgroup_get_counter(h_cgroup, idx,
  

•                                                      false),
  

•                           parent);
  

•         page_counter_init(hugetlb_cgroup_get_counter(h_cgroup, idx,
  

•                                                      true),
  

•                           reserved_parent);
  
          limit = round_down(PAGE_COUNTER_MAX,
                             1 << huge_page_order(&hstates[idx]));
  

•         ret = page_counter_set_max(counter, limit);
  

•         ret = page_counter_set_max(
  

•                 hugetlb_cgroup_get_counter(h_cgroup, idx, false),
  

•                 limit);
  

•         ret = page_counter_set_max(
  

•                 hugetlb_cgroup_get_counter(h_cgroup, idx, true), limit);
          VM_BUG_ON(ret);
  }
  

} @@ -126,6 +144,26 @@ static void hugetlb_cgroup_css_free(struct cgroup_subsys_state *css) kfree(h_cgroup); }

+static void hugetlb_cgroup_move_parent_reservation(int idx,

•                                            struct hugetlb_cgroup *h_cg)
  

+{

• struct hugetlb_cgroup *parent = parent_hugetlb_cgroup(h_cg);
  

• /* Move the reservation counters. */
  

• if (!parent_hugetlb_cgroup(h_cg)) {
  

•         parent = root_h_cgroup;
  

•         /* root has no limit */
  

•         page_counter_charge(
  

•                 &root_h_cgroup->reserved_hugepage[idx],
  

•                 page_counter_read(
  

•                         hugetlb_cgroup_get_counter(h_cg, idx, true)));
  

• }
  

• /* Take the pages off the local counter */
  

• page_counter_cancel(
  

•         hugetlb_cgroup_get_counter(h_cg, idx, true),
  

•         page_counter_read(hugetlb_cgroup_get_counter(h_cg, idx, true)));
  

+}

I know next to nothing about cgroups and am just comparing this to the existing hugetlb_cgroup_move_parent() routine. hugetlb_cgroup_move_parent updates the cgroup pointer in each page being moved. Do we need to do something similar for reservations being moved (move pointer in reservation)?

Oh, good catch. Yes I need to be doing that. I should probably consolidate those routines so the code doesn't miss things like this.

Thanks again for reviewing!

-- Mike Kravetz

...

/*

• Should be called with hugetlb_lock held.

@@ -142,7 +180,7 @@ static void hugetlb_cgroup_move_parent(int idx, struct hugetlb_cgroup *h_cg, struct hugetlb_cgroup *page_hcg; struct hugetlb_cgroup *parent = parent_hugetlb_cgroup(h_cg);

• page_hcg = hugetlb_cgroup_from_page(page);
  

• page_hcg = hugetlb_cgroup_from_page(page, false);
  /*
   * We can have pages in active list without any cgroup
   * ie, hugepage with less than 3 pages. We can safely
  

@@ -161,7 +199,7 @@ static void hugetlb_cgroup_move_parent(int idx, struct hugetlb_cgroup *h_cg, /* Take the pages off the local counter */ page_counter_cancel(counter, nr_pages);

• set_hugetlb_cgroup(page, parent);
  

• set_hugetlb_cgroup(page, parent, false);
  

out: return; } @@ -180,6 +218,7 @@ static void hugetlb_cgroup_css_offline(struct cgroup_subsys_state *css) do { for_each_hstate(h) { spin_lock(&hugetlb_lock);

•                 hugetlb_cgroup_move_parent_reservation(idx, h_cg);
                  list_for_each_entry(page, &h->hugepage_activelist, lru)
                          hugetlb_cgroup_move_parent(idx, h_cg, page);
  
  

@@ -191,7 +230,7 @@ static void hugetlb_cgroup_css_offline(struct cgroup_subsys_state *css) }

int hugetlb_cgroup_charge_cgroup(int idx, unsigned long nr_pages,

•                          struct hugetlb_cgroup **ptr)
  

•                          struct hugetlb_cgroup **ptr, bool reserved)
  

{ int ret = 0; struct page_counter *counter; @@ -214,8 +253,11 @@ int hugetlb_cgroup_charge_cgroup(int idx, unsigned long nr_pages, } rcu_read_unlock();

• if (!page_counter_try_charge(&h_cg->hugepage[idx], nr_pages, &counter))
  

• if (!page_counter_try_charge(hugetlb_cgroup_get_counter(h_cg, idx,
  

•                                                         reserved),
  

•                              nr_pages, &counter)) {
          ret = -ENOMEM;
  

• }
  css_put(&h_cg->css);
  

done: *ptr = h_cg; @@ -225,12 +267,12 @@ int hugetlb_cgroup_charge_cgroup(int idx, unsigned long nr_pages, /* Should be called with hugetlb_lock held */ void hugetlb_cgroup_commit_charge(int idx, unsigned long nr_pages, struct hugetlb_cgroup *h_cg,

•                           struct page *page)
  

•                           struct page *page, bool reserved)
  

{ if (hugetlb_cgroup_disabled() || !h_cg) return;

• set_hugetlb_cgroup(page, h_cg);
  

• set_hugetlb_cgroup(page, h_cg, reserved);
  return;
  

}

@@ -238,23 +280,26 @@ void hugetlb_cgroup_commit_charge(int idx, unsigned long nr_pages,

• Should be called with hugetlb_lock held

*/ void hugetlb_cgroup_uncharge_page(int idx, unsigned long nr_pages,

•                           struct page *page)
  

•                           struct page *page, bool reserved)
  

{ struct hugetlb_cgroup *h_cg;

  if (hugetlb_cgroup_disabled())
          return;
  lockdep_assert_held(&hugetlb_lock);

• h_cg = hugetlb_cgroup_from_page(page);
  

• h_cg = hugetlb_cgroup_from_page(page, reserved);
  if (unlikely(!h_cg))
          return;
  

• set_hugetlb_cgroup(page, NULL);
  

• page_counter_uncharge(&h_cg->hugepage[idx], nr_pages);
  

• set_hugetlb_cgroup(page, NULL, reserved);
  

• page_counter_uncharge(hugetlb_cgroup_get_counter(h_cg, idx, reserved),
  

•                       nr_pages);
  

• return;
  

}

void hugetlb_cgroup_uncharge_cgroup(int idx, unsigned long nr_pages,

•                             struct hugetlb_cgroup *h_cg)
  

•                             struct hugetlb_cgroup *h_cg, bool reserved)
  

{ if (hugetlb_cgroup_disabled() || !h_cg) return; @@ -262,8 +307,17 @@ void hugetlb_cgroup_uncharge_cgroup(int idx, unsigned long nr_pages, if (huge_page_order(&hstates[idx]) < HUGETLB_CGROUP_MIN_ORDER) return;

• page_counter_uncharge(&h_cg->hugepage[idx], nr_pages);
  

• return;
  

• page_counter_uncharge(hugetlb_cgroup_get_counter(h_cg, idx, reserved),
  

•                       nr_pages);
  

+}

+void hugetlb_cgroup_uncharge_counter(struct page_counter *p,

•                              unsigned long nr_pages)
  

+{

• if (hugetlb_cgroup_disabled() || !p)
  

•         return;
  

• page_counter_uncharge(p, nr_pages);
  

}

static u64 hugetlb_cgroup_read_u64(struct cgroup_subsys_state *css, @@ -475,6 +529,7 @@ void __init hugetlb_cgroup_file_init(void) void hugetlb_cgroup_migrate(struct page *oldhpage, struct page *newhpage) { struct hugetlb_cgroup *h_cg;

• struct hugetlb_cgroup *h_cg_reservation;
  struct hstate *h = page_hstate(oldhpage);
  
  if (hugetlb_cgroup_disabled())
  

@@ -482,11 +537,12 @@ void hugetlb_cgroup_migrate(struct page *oldhpage, struct page *newhpage)

  VM_BUG_ON_PAGE(!PageHuge(oldhpage), oldhpage);
  spin_lock(&hugetlb_lock);

• h_cg = hugetlb_cgroup_from_page(oldhpage);
  

• set_hugetlb_cgroup(oldhpage, NULL);
  

• h_cg = hugetlb_cgroup_from_page(oldhpage, false);
  

• h_cg_reservation = hugetlb_cgroup_from_page(oldhpage, true);
  

• set_hugetlb_cgroup(oldhpage, NULL, false);
  
  /* move the h_cg details to new cgroup */
  

• set_hugetlb_cgroup(newhpage, h_cg);
  

• set_hugetlb_cgroup(newhpage, h_cg_reservation, true);
  list_move(&newhpage->lru, &h->hugepage_activelist);
  spin_unlock(&hugetlb_lock);
  return;
  

-- 2.24.0.rc1.363.gb1bccd3e3d-goog

On Fri, Nov 8, 2019 at 4:01 PM Mike Kravetz mike.kravetz@oracle.com wrote:

On 11/8/19 3:48 PM, Mina Almasry wrote:

...

On Thu, Nov 7, 2019 at 4:57 PM Mike Kravetz mike.kravetz@oracle.com wrote:

...

On 10/29/19 6:36 PM, Mina Almasry wrote:

...

@@ -22,27 +22,35 @@ struct hugetlb_cgroup;

• Minimum page order trackable by hugetlb cgroup.
• At least 3 pages are necessary for all the tracking information.

*/ -#define HUGETLB_CGROUP_MIN_ORDER 2 +#define HUGETLB_CGROUP_MIN_ORDER 3

Correct me if misremembering, but I think the reson you changed this was so that you could use page[3].private. Correct? In that case isn't page[3] the last page of an order 2 allocation? If my understanding is correct, then leave HUGETLB_CGROUP_MIN_ORDER as is and update the preceding comment to say that at least 4 pages are necessary.

Yes, I just misunderstood what MIN_ORDER means. I'll revert the code change.

But, do update the comment please.

Will do.

<snip> >>> @@ -85,18 +89,32 @@ static void hugetlb_cgroup_init(struct hugetlb_cgroup *h_cgroup, >>> int idx; >>> >>> for (idx = 0; idx < HUGE_MAX_HSTATE; idx++) { >>> - struct page_counter *counter = &h_cgroup->hugepage[idx]; >>> struct page_counter *parent = NULL; >> >> Should we perhaps rename 'parent' to 'fault_parent' to be consistent? > > Yes that makes sense; will do. > >> That makes me think if perhaps the naming in the previous patch should >> be more explicit. Make the existing names explicitly contin 'fault' as >> the new names contain 'reservation'. >> Just a thought. >> > > You mean change the names of the actual user-facing files? I'm all for > better names but that would break existing users that read/write the > hugetlb_cgroup.2MB.usage_in_bytes/limit_in_bytes users, and so I would > assume is a no-go. >

I was thinking about internal variables/definitions such as:

+enum {

• /* Tracks hugetlb memory faulted in. */
• HUGETLB_RES_USAGE,
• /* Tracks hugetlb memory reserved. */
• HUGETLB_RES_RESERVATION_USAGE,
• /* Limit for hugetlb memory faulted in. */
• HUGETLB_RES_LIMIT,
• /* Limit for hugetlb memory reserved. */
• HUGETLB_RES_RESERVATION_LIMIT,
• /* Max usage for hugetlb memory faulted in. */
• HUGETLB_RES_MAX_USAGE,
• /* Max usage for hugetlb memory reserved. */
• HUGETLB_RES_RESERVATION_MAX_USAGE,
• /* Faulted memory accounting fail count. */
• HUGETLB_RES_FAILCNT,
• /* Reserved memory accounting fail count. */
• HUGETLB_RES_RESERVATION_FAILCNT,
• HUGETLB_RES_NULL,
• HUGETLB_RES_MAX,

+};

But, I guess the existing definitions (such as HUGETLB_RES_LIMIT) correspond closely to the externally visible name. In that case, you should leave them as is and ignore my comment.

<ship> >>> @@ -126,6 +144,26 @@ static void hugetlb_cgroup_css_free(struct cgroup_subsys_state *css) >>> kfree(h_cgroup); >>> } >>> >>> +static void hugetlb_cgroup_move_parent_reservation(int idx, >>> + struct hugetlb_cgroup *h_cg) >>> +{ >>> + struct hugetlb_cgroup *parent = parent_hugetlb_cgroup(h_cg); >>> + >>> + /* Move the reservation counters. */ >>> + if (!parent_hugetlb_cgroup(h_cg)) { >>> + parent = root_h_cgroup; >>> + /* root has no limit */ >>> + page_counter_charge( >>> + &root_h_cgroup->reserved_hugepage[idx], >>> + page_counter_read( >>> + hugetlb_cgroup_get_counter(h_cg, idx, true))); >>> + } >>> + >>> + /* Take the pages off the local counter */ >>> + page_counter_cancel( >>> + hugetlb_cgroup_get_counter(h_cg, idx, true), >>> + page_counter_read(hugetlb_cgroup_get_counter(h_cg, idx, true))); >>> +} >> >> I know next to nothing about cgroups and am just comparing this to the >> existing hugetlb_cgroup_move_parent() routine. hugetlb_cgroup_move_parent >> updates the cgroup pointer in each page being moved. Do we need to do >> something similar for reservations being moved (move pointer in reservation)? >> > > Oh, good catch. Yes I need to be doing that. I should probably > consolidate those routines so the code doesn't miss things like this.

This might get a bit ugly/complicated? Seems like you will need to examine all hugetlbfs inodes and vma's mapping those inodes.

Hmm yes on closer look it does seem like this is not straightforward. I'll write a test that does this reparenting so I can start running into the issue and poke for solutions. Off the top of my head, I think maybe we can just not reparent the hugetlb reservations - the hugetlb_cgroup stays alive until all its memory is uncharged. That shouldn't be too bad. Today, I think memcg doesn't reparent memory when it gets offlined.

I'll poke at this a bit and come back with suggestions, you may want to hold off reviewing the rest of the patches until then.

-- Mike Kravetz

On Fri, Nov 8, 2019 at 4:46 PM Mike Kravetz mike.kravetz@oracle.com wrote:

On 11/8/19 4:40 PM, Mina Almasry wrote:

...

On Fri, Nov 8, 2019 at 4:01 PM Mike Kravetz mike.kravetz@oracle.com wrote:

...

On 11/8/19 3:48 PM, Mina Almasry wrote:

...

On Thu, Nov 7, 2019 at 4:57 PM Mike Kravetz mike.kravetz@oracle.com wrote:

...

On 10/29/19 6:36 PM, Mina Almasry wrote:

...

@@ -22,27 +22,35 @@ struct hugetlb_cgroup;

• Minimum page order trackable by hugetlb cgroup.
• At least 3 pages are necessary for all the tracking information.

*/ -#define HUGETLB_CGROUP_MIN_ORDER 2 +#define HUGETLB_CGROUP_MIN_ORDER 3

Correct me if misremembering, but I think the reson you changed this was so that you could use page[3].private. Correct? In that case isn't page[3] the last page of an order 2 allocation? If my understanding is correct, then leave HUGETLB_CGROUP_MIN_ORDER as is and update the preceding comment to say that at least 4 pages are necessary.

Yes, I just misunderstood what MIN_ORDER means. I'll revert the code change.

But, do update the comment please.

Will do.

...

<snip> >>> @@ -85,18 +89,32 @@ static void hugetlb_cgroup_init(struct hugetlb_cgroup *h_cgroup, >>> int idx; >>> >>> for (idx = 0; idx < HUGE_MAX_HSTATE; idx++) { >>> - struct page_counter *counter = &h_cgroup->hugepage[idx]; >>> struct page_counter *parent = NULL; >> >> Should we perhaps rename 'parent' to 'fault_parent' to be consistent? > > Yes that makes sense; will do. > >> That makes me think if perhaps the naming in the previous patch should >> be more explicit. Make the existing names explicitly contin 'fault' as >> the new names contain 'reservation'. >> Just a thought. >> > > You mean change the names of the actual user-facing files? I'm all for > better names but that would break existing users that read/write the > hugetlb_cgroup.2MB.usage_in_bytes/limit_in_bytes users, and so I would > assume is a no-go. >

I was thinking about internal variables/definitions such as:

+enum {

• /* Tracks hugetlb memory faulted in. */
• HUGETLB_RES_USAGE,
• /* Tracks hugetlb memory reserved. */
• HUGETLB_RES_RESERVATION_USAGE,
• /* Limit for hugetlb memory faulted in. */
• HUGETLB_RES_LIMIT,
• /* Limit for hugetlb memory reserved. */
• HUGETLB_RES_RESERVATION_LIMIT,
• /* Max usage for hugetlb memory faulted in. */
• HUGETLB_RES_MAX_USAGE,
• /* Max usage for hugetlb memory reserved. */
• HUGETLB_RES_RESERVATION_MAX_USAGE,
• /* Faulted memory accounting fail count. */
• HUGETLB_RES_FAILCNT,
• /* Reserved memory accounting fail count. */
• HUGETLB_RES_RESERVATION_FAILCNT,
• HUGETLB_RES_NULL,
• HUGETLB_RES_MAX,

+};

But, I guess the existing definitions (such as HUGETLB_RES_LIMIT) correspond closely to the externally visible name. In that case, you should leave them as is and ignore my comment.

<ship> >>> @@ -126,6 +144,26 @@ static void hugetlb_cgroup_css_free(struct cgroup_subsys_state *css) >>> kfree(h_cgroup); >>> } >>> >>> +static void hugetlb_cgroup_move_parent_reservation(int idx, >>> + struct hugetlb_cgroup *h_cg) >>> +{ >>> + struct hugetlb_cgroup *parent = parent_hugetlb_cgroup(h_cg); >>> + >>> + /* Move the reservation counters. */ >>> + if (!parent_hugetlb_cgroup(h_cg)) { >>> + parent = root_h_cgroup; >>> + /* root has no limit */ >>> + page_counter_charge( >>> + &root_h_cgroup->reserved_hugepage[idx], >>> + page_counter_read( >>> + hugetlb_cgroup_get_counter(h_cg, idx, true))); >>> + } >>> + >>> + /* Take the pages off the local counter */ >>> + page_counter_cancel( >>> + hugetlb_cgroup_get_counter(h_cg, idx, true), >>> + page_counter_read(hugetlb_cgroup_get_counter(h_cg, idx, true))); >>> +} >> >> I know next to nothing about cgroups and am just comparing this to the >> existing hugetlb_cgroup_move_parent() routine. hugetlb_cgroup_move_parent >> updates the cgroup pointer in each page being moved. Do we need to do >> something similar for reservations being moved (move pointer in reservation)? >> > > Oh, good catch. Yes I need to be doing that. I should probably > consolidate those routines so the code doesn't miss things like this.

This might get a bit ugly/complicated? Seems like you will need to examine all hugetlbfs inodes and vma's mapping those inodes.

Hmm yes on closer look it does seem like this is not straightforward. I'll write a test that does this reparenting so I can start running into the issue and poke for solutions. Off the top of my head, I think maybe we can just not reparent the hugetlb reservations - the hugetlb_cgroup stays alive until all its memory is uncharged. That shouldn't be too bad. Today, I think memcg doesn't reparent memory when it gets offlined.

I'll poke at this a bit and come back with suggestions, you may want to hold off reviewing the rest of the patches until then.

Ok, if we start considering what the correct cgroup reparenting semantics should be it would be good to get input from others with more cgroup experience.

So I looked into this and prototyped a couple of solutions:

1. We could repartent the hugetlb reservation using the same approach that today we repartent hugetlb faults. Basically today faulted hugetlb pages live on hstate->hugepage_activelist. When a hugetlb cgroup gets offlined, this list is transversed and any pages on it that point to the cgroup being offlined and reparented. hugetlb_lock is used to make sure cgroup offlining doesn't race with a page being freed. I can add another list, but one that has pointers to the reservations made. When the cgroup is being offlined, it transverses this list, and reparents any reservations (which will need to acquire the proper resv_map->lock to do the parenting). hugetlb_lock needs also to be acquired here to make sure that resv_map release doesn't race with another thread reparenting the memory in that resv map.

Pros: Maintains current parenting behavior, and makes sure that reparenting of reservations works exactly the same way as reparenting of hugetlb faults. Cons: Code is a bit complex. There may be subtle object lifetime bugs, since I'm not 100% sure acquiring hugetlb_lock removes all races.

2. We could just not reparent hugetlb reservations. I.e. on hugetlb cgroup offlining, the hugetlb faults get reparented (which maintains current user facing behavior), but hugetlb reservation charges remain charged to the hugetlb cgroup. The cgroup lives as a zombie until all the reservations are uncharged.

Pros: Much easier implementation. Converges behavior of memcg and hugetlb cgroup, since memcg also doesn't reparent memory charged to it. Cons: Behavior change as hugetlb cgroups will become zombies if there are reservations charged to them. I've discussed offlist with Shakeel, and AFAICT there are absolutely no user facing behavior change to zombie cgroups. Only if the user is specifically detecting for zombies.

I'm torn between these 2 options right now, but leaning towards #2. I think I will propose #2 in a patch for review, and if anyone is broken by that (again, my understanding is that is very unlikely), then I propose a patch that reverts the changes in #2 and implements the changes in #1.

Any feedback from Shakeel or other people with cgroup expertise (especially for hugetlb cgroup or memcg) is very useful here.

-- Mike Kravetz

On Fri, Nov 1, 2019 at 4:23 PM Mike Kravetz mike.kravetz@oracle.com wrote:

On 10/29/19 6:36 PM, Mina Almasry wrote:

...

A follow up patch in this series adds hugetlb cgroup uncharge info the file_region entries in resv->regions. The cgroup uncharge info may differ for different regions, so they can no longer be coalesced at region_add time. So, disable region coalescing in region_add in this patch.

Behavior change:

Say a resv_map exists like this [0->1], [2->3], and [5->6].

Then a region_chg/add call comes in region_chg/add(f=0, t=5).

Old code would generate resv->regions: [0->5], [5->6]. New code would generate resv->regions: [0->1], [1->2], [2->3], [3->5], [5->6].

Special care needs to be taken to handle the resv->adds_in_progress variable correctly. In the past, only 1 region would be added for every region_chg and region_add call. But now, each call may add multiple regions, so we can no longer increment adds_in_progress by 1 in region_chg, or decrement adds_in_progress by 1 after region_add or region_abort. Instead, region_chg calls add_reservation_in_range() to count the number of regions needed and allocates those, and that info is passed to region_add and region_abort to decrement adds_in_progress correctly.

I think this commit message should also mention that we have changed the assumption previously made in the code that region_add after region_chg could never fail. It is described in comments, but would be worth a mention here as well.

Will fix.

...

Signed-off-by: Mina Almasry almasrymina@google.com

Thanks for the continued updates. I can not spot any major issues with this version of the patch. There are some questions and suggestions embedded below. With those addressed, you can add:

Reviewed-by: Mike Kravetz mike.kravetz@oracle.com

As mentioned previously, this reservation code is quite fragile. Changing it does make me nervous. The good thing is that there is not a 'significant' change in behavior for the normal/expected code paths. It is the handling of the race conditions and unexpected behavior that mostly makes me nervous. I suspect you have not exercised these paths. One thing we could do is add some debug code to force execution of those paths. For example, make add_reservation_in_range(count_only) always return 1 no matter how many regions are needed. This will force region_add more frequently perform additional allocations.

Will test this and report back with fixes if I run into issues.

This patch was of greatest concern to me as it impacts all hugetlbfs users, not just those using new cgroup functionality. With it in good shape, I will start to look at the others. However, my cgroup experience/understanding is limited. So, it would be great if others can also review the cgroup specific changes.

Thanks in advance.

...

---

Changes in v8:

• Addressed comments from Mike, especially:
  • fixing region_add not allocating enough regions sometimes.
  • reverted vma_*_reservation API changes.

Changes in v7:

• region_chg no longer allocates (t-f) / 2 file_region entries.

Changes in v6:

• Fix bug in number of region_caches allocated by region_chg

---

mm/hugetlb.c | 315 ++++++++++++++++++++++++++++++++++----------------- 1 file changed, 213 insertions(+), 102 deletions(-)

diff --git a/mm/hugetlb.c b/mm/hugetlb.c index 8d8aa89a9928e..1162eeaf8d160 100644 --- a/mm/hugetlb.c +++ b/mm/hugetlb.c @@ -244,110 +244,178 @@ struct file_region { long to; };

+/* Helper that removes a struct file_region from the resv_map cache and returns

• • it for use.
• */

+static struct file_region * +get_file_region_entry_from_cache(struct resv_map *resv, long from, long to) +{

• struct file_region *nrg = NULL;
  

• VM_BUG_ON(resv->region_cache_count <= 0);
  

• resv->region_cache_count--;
  

• nrg = list_first_entry(&resv->region_cache, struct file_region, link);
  

• VM_BUG_ON(!nrg);
  

• list_del(&nrg->link);
  

• nrg->from = from;
  

• nrg->to = to;
  

• return nrg;
  

+}

/* Must be called with resv->lock held. Calling this with count_only == true

• will count the number of pages to be added but will not modify the linked

• • list.

• • list. If regions_needed != NULL and count_only == true, then regions_needed
• • will indicate the number of file_regions needed in the cache to carry out to
• • add the regions for this range.
  */

static long add_reservation_in_range(struct resv_map *resv, long f, long t,

•                              bool count_only)
  

•                              long *regions_needed, bool count_only)
  

{

• long chg = 0;
  

• long add = 0;
  struct list_head *head = &resv->regions;
  

• long last_accounted_offset = f;
  struct file_region *rg = NULL, *trg = NULL, *nrg = NULL;
  
  

• /* Locate the region we are before or in. */
  

• list_for_each_entry (rg, head, link)
  

•         if (f <= rg->to)
  

•                 break;
  

• if (regions_needed)
  

•         *regions_needed = 0;
  
  

• /* Round our left edge to the current segment if it encloses us. */
  

• if (f > rg->from)
  

•         f = rg->from;
  

• chg = t - f;
  

• /* In this loop, we essentially handle an entry for the range
  

•  * [last_accounted_offset, rg->from), at every iteration, with some
  

•  * bounds checking.
  

•  */
  

• list_for_each_entry_safe(rg, trg, head, link) {
  

•         /* Skip irrelevant regions that start before our range. */
  

•         if (rg->from < f) {
  

•                 /* If this region ends after the last accounted offset,
  

•                  * then we need to update last_accounted_offset.
  

•                  */
  

•                 if (rg->to > last_accounted_offset)
  

•                         last_accounted_offset = rg->to;
  

•                 continue;
  

•         }
  
  

• /* Check for and consume any regions we now overlap with. */
  

• nrg = rg;
  

• list_for_each_entry_safe (rg, trg, rg->link.prev, link) {
  

•         if (&rg->link == head)
  

•                 break;
  

•         /* When we find a region that starts beyond our range, we've
  

•          * finished.
  

•          */
          if (rg->from > t)
                  break;
  
  

•         /* We overlap with this area, if it extends further than
  

•          * us then we must extend ourselves.  Account for its
  

•          * existing reservation.
  

•         /* Add an entry for last_accounted_offset -> rg->from, and
  

•          * update last_accounted_offset.
           */
  

•         if (rg->to > t) {
  

•                 chg += rg->to - t;
  

•                 t = rg->to;
  

•         if (rg->from > last_accounted_offset) {
  

•                 add += rg->from - last_accounted_offset;
  

•                 if (!count_only) {
  

•                         nrg = get_file_region_entry_from_cache(
  

•                                 resv, last_accounted_offset, rg->from);
  

•                         list_add(&nrg->link, rg->link.prev);
  

•                 } else if (regions_needed)
  

•                         *regions_needed += 1;
          }
  

•         chg -= rg->to - rg->from;
  
  

•         if (!count_only && rg != nrg) {
  

•                 list_del(&rg->link);
  

•                 kfree(rg);
  

•         }
  

•         last_accounted_offset = rg->to;
  

That last assignment is unneeded. Correct?

Not to make you nervous, but this assignment is needed.

The basic idea is that there are 2 loop invariants here: 1. Everything before last_accounted_offset is filled in with file_regions. 2. rg points to the first region past last_account_offset.

Each loop iteration compares rg->from to last_accounted_offset, and if there is a gap, it creates a new region to fill this gap. Then this assignment restores loop invariant #2 by assigning last_accounted_offset to rg->to, since now everything before rg->to is filled in with file_regions.

...

  }

• if (!count_only) {
  

•         nrg->from = f;
  

•         nrg->to = t;
  

• /* Handle the case where our range extends beyond
  

•  * last_accounted_offset.
  

•  */
  

• if (last_accounted_offset < t) {
  

•         add += t - last_accounted_offset;
  

•         if (!count_only) {
  

•                 nrg = get_file_region_entry_from_cache(
  

•                         resv, last_accounted_offset, t);
  

•                 list_add(&nrg->link, rg->link.prev);
  

•         } else if (regions_needed)
  

•                 *regions_needed += 1;
  

•         last_accounted_offset = t;
  }
  
  

• return chg;
  

• return add;
  

}

/*

• Add the huge page range represented by [f, t) to the reserve

• • map. Existing regions will be expanded to accommodate the specified
• • range, or a region will be taken from the cache. Sufficient regions
• • must exist in the cache due to the previous call to region_chg with
• • the same range.

• • map. Regions will be taken from the cache to fill in this range.
• • Sufficient regions should exist in the cache due to the previous
• • call to region_chg with the same range, but in some cases the cache will not
• • have sufficient entries. The extra needed entries will be allocated.

Let's mention that the reason sufficient entries may not exist is due to races with other code doing region_add or region_del.

Will fix.

...

• • regions_needed is the out value provided by a previous call to region_chg.

• • Return the number of new huge pages added to the map. This
• • number is greater than or equal to zero.

• • Return the number of new huge pages added to the map. This number is greater
• • than or equal to zero. If file_region entries needed to be allocated for
• • this operation and we were not able to allocate, it ruturns -ENOMEM.
• • region_add of regions of length 1 never allocate file_regions and cannot
• • fail.

Let's add the reason why region_add for regions of length 1 will never fail. Specifically, region_chg will always allocate at least 1 entry and a region_add for 1 page will only require at most 1 entry.

Will fix.

...

*/ -static long region_add(struct resv_map *resv, long f, long t) +static long region_add(struct resv_map *resv, long f, long t,

•                long in_regions_needed)
  

{

• struct list_head *head = &resv->regions;
  

• struct file_region *rg, *nrg;
  

• long add = 0;
  

• long add = 0, actual_regions_needed = 0, i = 0;
  

• struct file_region *trg = NULL, *rg = NULL;
  

• struct list_head allocated_regions;
  

• INIT_LIST_HEAD(&allocated_regions);
  
  spin_lock(&resv->lock);
  

• /* Locate the region we are either in or before. */
  

• list_for_each_entry(rg, head, link)
  

•         if (f <= rg->to)
  

•                 break;
  

+retry:

• /* Count how many regions are actually needed to execute this add. */
  

• add_reservation_in_range(resv, f, t, &actual_regions_needed, true);
  
  /*
  

•  * If no region exists which can be expanded to include the
  

•  * specified range, pull a region descriptor from the cache
  

•  * and use it for this range.
  

•  * Check for sufficient descriptors in the cache to accommodate
  

•  * this add operation. Note that actual_regions_needed may be greater
  

•  * than in_regions_needed. In this case, we need to make sure that we
  

•  * allocate extra entries, such that we have enough for all the
  

•  * existing adds_in_progress, plus the excess needed for this
  

•  * operation.
   */
  

• if (&rg->link == head || t < rg->from) {
  

•         VM_BUG_ON(resv->region_cache_count <= 0);
  

• if (resv->region_cache_count <
  

•     resv->adds_in_progress +
  

•             (actual_regions_needed - in_regions_needed)) {
  

•         /* region_add operation of range 1 should never need to
  

•          * allocate file_region entries.
  

•          */
  

•         VM_BUG_ON(t - f <= 1);
  
  

•         resv->region_cache_count--;
  

•         nrg = list_first_entry(&resv->region_cache, struct file_region,
  

•                                 link);
  

•         list_del(&nrg->link);
  

•         /* Must drop lock to allocate a new descriptor. */
  

•         spin_unlock(&resv->lock);
  

•         for (i = 0; i < (actual_regions_needed - in_regions_needed);
  

•              i++) {
  

•                 trg = kmalloc(sizeof(*trg), GFP_KERNEL);
  

•                 if (!trg)
  

•                         goto out_of_memory;
  

•                 list_add(&trg->link, &allocated_regions);
  

•         }
  

•         spin_lock(&resv->lock);
  
  

•         nrg->from = f;
  

•         nrg->to = t;
  

•         list_add(&nrg->link, rg->link.prev);
  

•         list_for_each_entry_safe(rg, trg, &allocated_regions, link) {
  

•                 list_del(&rg->link);
  

•                 list_add(&rg->link, &resv->region_cache);
  

•                 resv->region_cache_count++;
  

•         }
  
  

•         add += t - f;
  

•         goto out_locked;
  

•         goto retry;
  }
  
  

• add = add_reservation_in_range(resv, f, t, false);
  

• add = add_reservation_in_range(resv, f, t, NULL, false);
  

• resv->adds_in_progress -= in_regions_needed;
  
  

-out_locked:

• resv->adds_in_progress--;
  spin_unlock(&resv->lock);
  VM_BUG_ON(add < 0);
  

Does this VM_BUG_ON make sense here? You are testing if add_reservation_in_range() returned a negative value. Right? Perhaps this was previously combined with the out_of_memory situation.

Yes it tests add_reservation_in_range() returned a negative value, which indicates something very wrong happened. Doesn't seem critical but doesn't hurt either.

...

  return add;

+out_of_memory:

• list_for_each_entry_safe(rg, trg, &allocated_regions, link) {
  

•         list_del(&rg->link);
  

•         kfree(rg);
  

• }
  

• return -ENOMEM;
  

}

/* @@ -357,49 +425,72 @@ static long region_add(struct resv_map *resv, long f, long t)

• call to region_add that will actually modify the reserve
• map to add the specified range [f, t). region_chg does
• not change the number of huge pages represented by the

• • map. A new file_region structure is added to the cache
• • as a placeholder, so that the subsequent region_add
• • call will have all the regions it needs and will not fail.

• • map. A number of new file_region structures is added to the cache as a
• • placeholder, for the subsequent region_add call to use. At least 1
• • file_region structure is added.
• • out_regions_needed is the number of regions added to the
• • resv->adds_in_progress. This value needs to be provided to a follow up call
• • to region_add or region_abort for proper accounting.
  • Returns the number of huge pages that need to be added to the existing
  • reservation map for the range [f, t). This number is greater or equal to
  • zero. -ENOMEM is returned if a new file_region structure or cache entry
  • is needed and can not be allocated.
  */

-static long region_chg(struct resv_map *resv, long f, long t) +static long region_chg(struct resv_map *resv, long f, long t,

•                long *out_regions_needed)
  

{

• long chg = 0;
  

• struct file_region *trg = NULL, *rg = NULL;
  

• long chg = 0, i = 0, to_allocate = 0;
  

• struct list_head allocated_regions;
  

• INIT_LIST_HEAD(&allocated_regions);
  
  spin_lock(&resv->lock);
  

-retry_locked:

• resv->adds_in_progress++;
  

• /* Count how many hugepages in this range are NOT respresented. */
  

• chg = add_reservation_in_range(resv, f, t, out_regions_needed, true);
  

• if (*out_regions_needed < 1)
  

•         *out_regions_needed = 1;
  

Perhaps just me, but I would prefer an explicit check for zero here. That is the only possible value less than 1, correct? Otherwise, on a quick read of this code one might think out_regions_needed could be a negative number or error code.

Will fix.

...

• resv->adds_in_progress += *out_regions_needed;
  
  /*
   * Check for sufficient descriptors in the cache to accommodate
   * the number of in progress add operations.
   */
  

• if (resv->adds_in_progress > resv->region_cache_count) {
  

•         struct file_region *trg;
  

• while (resv->region_cache_count < resv->adds_in_progress) {
  

•         to_allocate = resv->adds_in_progress - resv->region_cache_count;
  

When I first looked at this while loop, I thought we would be calling add_reservation_in_range again after dropping and reacquiring the lock. It took me a minute to realize that that is not required. We only need to get the number of entries that were sugggested by the first call, and that number is incorporated in resv->adds_in_progress. If things have changed, the subsequent region_add can deal with it. It might be a good idea to note this in the commnent.

Will add.

-- Mike Kravetz

...

•         VM_BUG_ON(resv->adds_in_progress - resv->region_cache_count > 1);
          /* Must drop lock to allocate a new descriptor. */
  

•         resv->adds_in_progress--;
          spin_unlock(&resv->lock);
  

•         trg = kmalloc(sizeof(*trg), GFP_KERNEL);
  

•         if (!trg)
  

•                 return -ENOMEM;
  

•         for (i = 0; i < to_allocate; i++) {
  

•                 trg = kmalloc(sizeof(*trg), GFP_KERNEL);
  

•                 if (!trg)
  

•                         goto out_of_memory;
  

•                 list_add(&trg->link, &allocated_regions);
  

•         }
  
          spin_lock(&resv->lock);
  

•         list_add(&trg->link, &resv->region_cache);
  

•         resv->region_cache_count++;
  

•         goto retry_locked;
  

• }
  
  

• chg = add_reservation_in_range(resv, f, t, true);
  

•         list_for_each_entry_safe(rg, trg, &allocated_regions, link) {
  

•                 list_del(&rg->link);
  

•                 list_add(&rg->link, &resv->region_cache);
  

•                 resv->region_cache_count++;
  

•         }
  

• }
  
  spin_unlock(&resv->lock);
  return chg;
  

+out_of_memory:

• list_for_each_entry_safe(rg, trg, &allocated_regions, link) {
  

•         list_del(&rg->link);
  

•         kfree(rg);
  

• }
  

• return -ENOMEM;
  

}

/* @@ -407,17 +498,20 @@ static long region_chg(struct resv_map *resv, long f, long t)

• of the resv_map keeps track of the operations in progress between
• calls to region_chg and region_add. Operations are sometimes
• aborted after the call to region_chg. In such cases, region_abort

• • is called to decrement the adds_in_progress counter.

• • is called to decrement the adds_in_progress counter. regions_needed
• • is the value returned by the region_chg call, it is used to decrement
• • the adds_in_progress counter.
  • NOTE: The range arguments [f, t) are not needed or used in this
  • routine. They are kept to make reading the calling code easier as
  • arguments will match the associated region_chg call.
  */

-static void region_abort(struct resv_map *resv, long f, long t) +static void region_abort(struct resv_map *resv, long f, long t,

•                  long regions_needed)
  

{ spin_lock(&resv->lock); VM_BUG_ON(!resv->region_cache_count);

• resv->adds_in_progress--;
  

• resv->adds_in_progress -= regions_needed;
  spin_unlock(&resv->lock);
  

}

@@ -1904,6 +1998,7 @@ static long __vma_reservation_common(struct hstate *h, struct resv_map *resv; pgoff_t idx; long ret;

• long dummy_out_regions_needed;
  
  resv = vma_resv_map(vma);
  if (!resv)
  

@@ -1912,20 +2007,29 @@ static long __vma_reservation_common(struct hstate *h, idx = vma_hugecache_offset(h, vma, addr); switch (mode) { case VMA_NEEDS_RESV:

•         ret = region_chg(resv, idx, idx + 1);
  

•         ret = region_chg(resv, idx, idx + 1, &dummy_out_regions_needed);
  

•         /* We assume that vma_reservation_* routines always operate on
  

•          * 1 page, and that adding to resv map a 1 page entry can only
  

•          * ever require 1 region.
  

•          */
  

•         VM_BUG_ON(dummy_out_regions_needed != 1);
          break;
  case VMA_COMMIT_RESV:
  

•         ret = region_add(resv, idx, idx + 1);
  

•         ret = region_add(resv, idx, idx + 1, 1);
  

•         /* region_add calls of range 1 should never fail. */
  

•         VM_BUG_ON(ret < 0);
          break;
  case VMA_END_RESV:
  

•         region_abort(resv, idx, idx + 1);
  

•         region_abort(resv, idx, idx + 1, 1);
          ret = 0;
          break;
  case VMA_ADD_RESV:
  

•         if (vma->vm_flags & VM_MAYSHARE)
  

•                 ret = region_add(resv, idx, idx + 1);
  

•         else {
  

•                 region_abort(resv, idx, idx + 1);
  

•         if (vma->vm_flags & VM_MAYSHARE) {
  

•                 ret = region_add(resv, idx, idx + 1, 1);
  

•                 /* region_add calls of range 1 should never fail. */
  

•                 VM_BUG_ON(ret < 0);
  

•         } else {
  

•                 region_abort(resv, idx, idx + 1, 1);
                  ret = region_del(resv, idx, idx + 1);
          }
          break;
  

@@ -4578,12 +4682,12 @@ int hugetlb_reserve_pages(struct inode *inode, struct vm_area_struct *vma, vm_flags_t vm_flags) {

• long ret, chg;
  

• long ret, chg, add = -1;
  struct hstate *h = hstate_inode(inode);
  struct hugepage_subpool *spool = subpool_inode(inode);
  struct resv_map *resv_map;
  struct hugetlb_cgroup *h_cg;
  

• long gbl_reserve;
  

• long gbl_reserve, regions_needed = 0;
  
  /* This should never happen */
  if (from > to) {
  

@@ -4613,7 +4717,7 @@ int hugetlb_reserve_pages(struct inode *inode, */ resv_map = inode_resv_map(inode);

•         chg = region_chg(resv_map, from, to);
  

•         chg = region_chg(resv_map, from, to, &regions_needed);
  
  } else {
          /* Private mapping. */
  

@@ -4683,9 +4787,14 @@ int hugetlb_reserve_pages(struct inode *inode, * else has to be done for private mappings here */ if (!vma || vma->vm_flags & VM_MAYSHARE) {

•         long add = region_add(resv_map, from, to);
  

•         if (unlikely(chg > add)) {
  

•         add = region_add(resv_map, from, to, regions_needed);
  

•         if (unlikely(add < 0)) {
  

•                 hugetlb_acct_memory(h, -gbl_reserve);
  

•                 /* put back original number of pages, chg */
  

•                 (void)hugepage_subpool_put_pages(spool, chg);
  

•                 goto out_err;
  

•         } else if (unlikely(chg > add)) {
                  /*
                   * pages in this range were added to the reserve
                   * map between region_chg and region_add.  This
  

@@ -4703,9 +4812,11 @@ int hugetlb_reserve_pages(struct inode *inode, return 0; out_err: if (!vma || vma->vm_flags & VM_MAYSHARE)

•         /* Don't call region_abort if region_chg failed */
  

•         if (chg >= 0)
  

•                 region_abort(resv_map, from, to);
  

•         /* Only call region_abort if the region_chg succeeded but the
  

•          * region_add failed or didn't run.
  

•          */
  

•         if (chg >= 0 && add < 0)
  

•                 region_abort(resv_map, from, to, regions_needed);
  if (vma && is_vma_resv_set(vma, HPAGE_RESV_OWNER))
          kref_put(&resv_map->refs, resv_map_release);
  return ret;
  

-- 2.24.0.rc1.363.gb1bccd3e3d-goog