On Wed, Nov 17, 2021 at 7:55 PM Muchun Song [off-list ref] wrote:

On Thu, Nov 18, 2021 at 8:13 AM Mike Kravetz [off-list ref] wrote:

quoted

On 11/17/21 12:18, Mina Almasry wrote:
...

quoted

diff --git a/mm/hugetlb_cgroup.c b/mm/hugetlb_cgroup.c

...

quoted

@@ -288,11 +317,21 @@ static void __hugetlb_cgroup_commit_charge(int idx, unsigned long nr_pages,
                                         struct hugetlb_cgroup *h_cg,
                                         struct page *page, bool rsvd)
 {
+     unsigned long *usage;
+

I assume the use of a pointer is just to make the following WRITE_ONCE
look better?  I prefer the suggestion by Muchun:

unsigned long usage = h_cg->nodeinfo[page_to_nid(page)]->usage[idx];

usage += nr_pages;
WRITE_ONCE(h_cg->nodeinfo[page_to_nid(page)]->usage[idx], usage);

I had to think for just a second 'why are we using/passing a pointer?'.
Not insisting we use Muchun's suggestion, it just caused me to think
a little more than necessary.

At least I have the same question here. For me I think it's
unnecessary to use a pointer.

Hmm to be honest I would have not thought it would be preferable to
duplicate a long string like
h_cg->nodeinfo[page_to_nid(page)]->usage[idx], and then for future
code changes to keep them in sync. I think Marco had the same thinking
and that was his initial suggestion, but I don't mind much either way.
I'll submit another iteration with the change :-)

quoted

In any case, I would move the variable usage inside the
'if (!rsvd)' block.

quoted

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

      __set_hugetlb_cgroup(page, h_cg, rsvd);
-     return;
+     if (!rsvd) {
+             usage = &h_cg->nodeinfo[page_to_nid(page)]->usage[idx];
+             /*
+              * This write is not atomic due to fetching *usage and writing
+              * to it, but that's fine because we call this with
+              * hugetlb_lock held anyway.
+              */
+             WRITE_ONCE(*usage, *usage + nr_pages);
+     }
 }

 void hugetlb_cgroup_commit_charge(int idx, unsigned long nr_pages,

@@ -316,6 +355,7 @@ static void __hugetlb_cgroup_uncharge_page(int idx, unsigned long nr_pages,
                                         struct page *page, bool rsvd)
 {
      struct hugetlb_cgroup *h_cg;
+     unsigned long *usage;

Same here.

Otherwise, looks good to me.
--
Mike Kravetz