On Tue, Mar 14, 2023 at 12:45 PM Yosry Ahmed <yosryahmed@xxxxxxxxxx> wrote:
>
> On Tue, Mar 14, 2023 at 3:02 AM Michal Hocko <mhocko@xxxxxxxx> wrote:
> >
> > On Mon 13-03-23 14:08:53, 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));
> > > }
> >
> > I would just stick with the existing code and put a comment that this
> > function shouldn't be used in any new code and the folio counterpart
> > should be used instead.
>
> Would you mind explaining the rationale?
>
> If existing users are not passing in tail pages and we are telling new
> users not to use it, what's the point of leaving the PageTail() check?
>
> Is page owner doing the right thing by discounting pages with
> page->memcg_data = 0 in print_page_owner_memcg() ? Wouldn't this not
> show the memcg of tail pages?
>
> If page owner also needs a compound_head()/ page_folio() call before
> checking the page memcg, perhaps we should convert both call sites to
> page_memcg_check() to folio_memcg_check() and remove it now?
.. or add page_folio() inside page_memcg_check() as suggested initially.
>
> >
> > --
> > Michal Hocko
> > SUSE Labs
