Re: [PATCH v2 1/1] memcg/hugetlb: Adding hugeTLB counters to memcg

[Yosry Ahmed <yosryahmed@google.com>]

On Wed, Oct 23, 2024 at 2:32 PM Joshua Hahn <joshua.hahnjy@gmail.com> wrote:
>
> Hi Yosry, thank you for taking the time to review my patch. It seems
> like I made a
> lot of silly spelling / grammar / style mistakes in this patch, I'll
> be more mindful of
> these in the future.
>
> On Wed, Oct 23, 2024 at 5:15 PM Yosry Ahmed <yosryahmed@google.com> wrote:
> >
> > Hi Joshua,
> > [...]
> > > diff --git a/include/linux/mmzone.h b/include/linux/mmzone.h
> > > index 17506e4a2835..d3ba49a974b2 100644
> > > --- a/include/linux/mmzone.h
> > > +++ b/include/linux/mmzone.h
> > > @@ -215,6 +215,9 @@ enum node_stat_item {
> > >  #ifdef CONFIG_NUMA_BALANCING
> > >         PGPROMOTE_SUCCESS,      /* promote successfully */
> > >         PGPROMOTE_CANDIDATE,    /* candidate pages to promote */
> > > +#endif
> > > +#ifdef CONFIG_HUGETLB_PAGE
> > > +       HUGETLB_B,
> >
> > Why '_B'?
>
> I added _B because I am measuring the statistics in bytes (not pages).
> IIRC some stat items use _B at the end to denote that the unit is in bytes,
> so I put it at the end in the spirit of maintaining consistency. However, if
> is not the case, I will remove it in the next version.

I think ~all the memcg stats are exported in bytes, the ones that have
_B are also counted in bytes. I think in this case we are counting the
stats in pages.

See memcg_page_state_output_unit() and memcg_page_state_unit() for more details.

>
> > [...]
> > >  #endif
> > >         /* PGDEMOTE_*: pages demoted */
> > >         PGDEMOTE_KSWAPD,
> > > diff --git a/mm/hugetlb.c b/mm/hugetlb.c
> > > index 190fa05635f4..055bc91858e4 100644
> > > --- a/mm/hugetlb.c
> > > +++ b/mm/hugetlb.c
> > > @@ -1925,6 +1925,8 @@ void free_huge_folio(struct folio *folio)
> > >                                      pages_per_huge_page(h), folio);
> > >         hugetlb_cgroup_uncharge_folio_rsvd(hstate_index(h),
> > >                                           pages_per_huge_page(h), folio);
> > > +       if (cgrp_dfl_root.flags & CGRP_ROOT_MEMORY_HUGETLB_ACCOUNTING)
> >
> > I think we already have a couple of these checks and this patch adds a
> > few more, perhaps we should add a helper at this point to improve
> > readability? Maybe something like memcg_accounts_hugetlb()?
> > [...]
>
> As far as I can tell, these checks only come up in mem_cgroup_hugetlb_try_charge
> and cgroup_show_options. Shakeel already requested a cleanup of the try charge
> function in the v1 thread, so I think the best course of action is to
> add the helper
> function in this patch (series) and use that helper function in
> another patch series to
> clean up the remaining functions.

Introducing the helper in this patch/series and using it in further
cleanups makes sense to me.

>
> I'll be sure to add the other grammar / style changes that you
> mentioned above in
> v3 as well. Thank you again for your feedback!
>
> Joshua