On Tue, Mar 14, 2023 at 8:07 PM Waiman Long <longman@xxxxxxxxxx> wrote:
>
> On 3/13/23 17:08, Yosry Ahmed wrote:
> > On Mon, Mar 13, 2023 at 12:44 PM Andrew Morton
> > <akpm@xxxxxxxxxxxxxxxxxxxx> wrote:
> >> On Mon, 13 Mar 2023 08:34:52 +0000 Yosry Ahmed <yosryahmed@xxxxxxxxxx> wrote:
> >>
> >>> From: Hugh Dickins <hughd@xxxxxxxxxx>
> >>>
> >>> In a kernel with added WARN_ON_ONCE(PageTail) in page_memcg_check(), we
> >>> observed a warning from page_cgroup_ino() when reading
> >>> /proc/kpagecgroup.
> >> If this is the only known situation in which page_memcg_check() is
> >> passed a tail page, why does page_memcg_check() have
> >>
> >> if (PageTail(page))
> >> return NULL;
> >>
> >> ? Can we remove this to simplify, streamline and clarify?
> > I guess it's a safety check so that we don't end up trying to cast a
> > tail page to a folio. My opinion is to go one step further and change
> > page_memcg_check() to do return the memcg of the head page, i.e:
> >
> > static inline struct mem_cgroup *page_memcg_check(struct page *page)
> > {
> > return folio_memcg_check(page_folio(page));
> > }
> >
> > This makes it consistent with page_memcg(), and makes sure future
> > users are getting the "correct" memcg for whatever page they pass in.
> > I am interested to hear other folks' opinions here.
> >
> > 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().
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.
>
> Cheers,
> Longman
>
