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

[Waiman Long <longman@redhat.com>]

On 3/13/23 17:08, Yosry Ahmed wrote:
> On Mon, Mar 13, 2023 at 12:44 PM Andrew Morton
> <akpm@linux-foundation.org> wrote:
>> On Mon, 13 Mar 2023 08:34:52 +0000 Yosry Ahmed <yosryahmed@google.com> wrote:
>>
>>> From: Hugh Dickins <hughd@google.com>
>>>
>>> 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.

Cheers,
Longman