Re: [PATCH v2] mm/page_alloc: Dedupe some memcg uncharging logic

[Yosry Ahmed]

On Wed, Nov 8, 2023 at 8:49 AM Brendan Jackman <jackmanb@xxxxxxxxxx> wrote:
>
> The duplication makes it seem like some work is required before
> uncharging in the !PageHWPoison case. But it isn't, so we can simplify
> the code a little.
>
> Note the PageMemcgKmem check is redundant, but I've left it in as it
> avoids an unnecessary function call.
>
> Signed-off-by: Brendan Jackman <jackmanb@xxxxxxxxxx>
> ---
>  mm/page_alloc.c | 12 ++++--------
>  1 file changed, 4 insertions(+), 8 deletions(-)
>
> diff --git a/mm/page_alloc.c b/mm/page_alloc.c
> index 733732e7e0ba..dd5e8a759d27 100644
> --- a/mm/page_alloc.c
> +++ b/mm/page_alloc.c
> @@ -1086,13 +1086,11 @@ static __always_inline bool free_pages_prepare(struct page *page,
>         trace_mm_page_free(page, order);
>         kmsan_free_page(page, order);
>
> +       if (memcg_kmem_online() && PageMemcgKmem(page))
> +               __memcg_kmem_uncharge_page(page, order);
> +
>         if (unlikely(PageHWPoison(page)) && !order) {
> -               /*
> -                * Do not let hwpoison pages hit pcplists/buddy
> -                * Untie memcg state and reset page's owner
> -                */
> -               if (memcg_kmem_online() && PageMemcgKmem(page))
> -                       __memcg_kmem_uncharge_page(page, order);
> +               /* Do not let hwpoison pages hit pcplists/buddy */
>                 reset_page_owner(page, order);
>                 page_table_check_free(page, order);
>                 return false;
> @@ -1123,8 +1121,6 @@ static __always_inline bool free_pages_prepare(struct page *page,
>         }
>         if (PageMappingFlags(page))
>                 page->mapping = NULL;
> -       if (memcg_kmem_online() && PageMemcgKmem(page))
> -               __memcg_kmem_uncharge_page(page, order);

Nothing happening in the function before this point seems to affect
the memcg uncharging. It only acts on the head page, and most of the
code up until here is acting on tail pages.

LGTM, but I'd be more comfortable if Roman took a look as well.

Reviewed-by: Yosry Ahmed <yosryahmed@xxxxxxxxxx>

>         if (is_check_pages_enabled()) {
>                 if (free_page_is_bad(page))
>                         bad++;
> --
> 2.42.0.869.gea05f2083d-goog
>
>