Re: [PATCH] memcg: page_cgroup_ino() get memcg from compound_head(page)

[Yosry Ahmed <yosryahmed@google.com>]

On Tue, Mar 14, 2023 at 8:33 PM Waiman Long <longman@redhat.com> wrote:
>
> On 3/14/23 23:10, Yosry Ahmed wrote:
> >>> The only other user today is print_page_owner_memcg(). I am not sure
> >>> if it's doing the right thing by explicitly reading page->memcg_data,
> >>> but it is already excluding pages that have page->memcg_data == 0,
> >>> which should be the case for tail pages.
> >> It is reading memcg_data directly to see if it is slab cache page. It is
> >> currently skipping page that does not have memcg_data set.
> > IIUC this skips tail pages, because they should always have
> > page->memcg_data == 0, even if they are charged to a memcg. To
> > correctly get their memcg we should read it from the
> > compound_head()/page_folio().
> The purpose of that function is mainly to report pages that have a
> reference to a memcg, especially the dead one. So by counting the
> occurrence of a particular cgroup name, we can have a rough idea of
> that. So only head page has relevance here and we can skip the tail pages.

I see. If you update the code to not check memcg_data directly, and we
update page_memcg_check() to return memcg of the head page, this will
stop skipping tail pages. Is this okay?

If not, we can explicitly skip tail pages in print_page_owner_memcg()
if we decide to check the head's memcg in page_memcg_check().

> >
> > My 2c, we can check PageSlab() to print the extra message for slab
> > pages, instead of reading memcg_data directly, which kinda breaks the
> > abstraction created by the various helpers for reading a page memcg.
> > Someone can easily change something in how memcg_data is interpreted
> > in those helpers without realizing that page_owner is also reading it.
>
> You are right. We should be using a helper if available. I will send a
> patch to fix that.

Great, thanks!

>
> Thanks,
> Longman
>