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

[Waiman Long <longman@redhat.com>]

On 3/15/23 17:43, Yosry Ahmed wrote:
> On Wed, Mar 15, 2023 at 5:19 AM Matthew Wilcox <willy@infradead.org> wrote:
>> On Wed, Mar 15, 2023 at 12:04:10AM -0700, Yosry Ahmed wrote:
>>> On Tue, Mar 14, 2023 at 9:54 PM Matthew Wilcox <willy@infradead.org> wrote:
>>>> On Mon, Mar 13, 2023 at 02:08:53PM -0700, 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));
>>>>> }
>>>> If you look at my commit becacb04fdd4, I was preserving the existing
>>>> behaviour of page_memcg_check() when passed a tail page.  It would
>>>> previously, rightly or wrongly, read the memcg_data from the tail page
>>>> and get back NULL.
>>> Right, I looked at that. I also looked at 1b7e4464d43a which added
>>> folio_memcg() and changed page_memcg()'s behavior to use page_folio()
>>> to retrieve the memcg from the head, which made me wonder why
>>> different decisions were made for these 2 helpers.
>>>
>>> Were the users of page_memcg() already passing in head pages only?
>> There were 18 months between those commits ... I'd learned to be
>> more careful about maintaining the semantics instead of changing
>> them to "what they should have been".
>>
>>>> I suspect that was not the intended behaviour, but I do not think this
>>>> patch is the right fix; it simply papers over the problem and maybe
>>>> creates a new one.  Callers of page_memcg_check() should be eliminated,
>>>> precisely because of this ambiguity.  It's up to the people who understand
>>>> each of the callers who need to make the decision to always convert the
>>>> page that they have to a folio and ask about its memcg, or whether they
>>>> want to preserve the existing behaviour of returning NULL for tail pages.
>>>>
>>>> So, I say NACK to this patch as it does not preserve existing behaviour,
>>>> and does not advance our understanding of what we have wrought.
>>> I am not sure which patch you are NACKing, the original patch from
>>> Hugh (adding compound_head() to page_cgroup_ino()) or the suggested
>>> alternative patch which changes page_memcg_check() to use
>>> page_folio().
>> Both patches are NACKed.  page_memcg_check() needs to go away
>> because it has the tail page ambiguity.  Both callers should be using
>> folio_memcg_check() directly and resolving for themselves what the
>> correct behaviour is when seeing a tail page.
>>
> I agree. I even suggested this to Michal in one of the replies.
>
> For page_cgroup_ino() we can simply pass in
> folio_memcg(page_folio(page)), which would mimic what Hugh's patch is
> doing for page_cgroup_ino().
>
> For page owner, I am not sure if we want to do something similar
> (which would start printing the memcg for tail pages as well), or
> explicitly excluding tail pages and THEN do
> folio_memcg(page_folio(page)) to get the memcg of head pages. Waiman,
> what do you think?

I prefer the current behavior of printing information for the head page 
only. I am open to suggestion of the best APIs to use.

Cheers,
Longman