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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Wed, 8 Nov 2023 12:19:23 -0800
From:   Yosry Ahmed <yosryahmed@...gle.com>
To:     Brendan Jackman <jackmanb@...gle.com>,
        Roman Gushchin <roman.gushchin@...ux.dev>
Cc:     linux-mm@...ck.org, linux-kernel@...r.kernel.org,
        Johannes Weiner <hannes@...xchg.org>,
        Michal Hocko <mhocko@...nel.org>,
        Shakeel Butt <shakeelb@...gle.com>,
        Muchun Song <muchun.song@...ux.dev>,
        Andrew Morton <akpm@...ux-foundation.org>
Subject: Re: [PATCH v2] mm/page_alloc: Dedupe some memcg uncharging logic

On Wed, Nov 8, 2023 at 8:49 AM Brendan Jackman <jackmanb@...gle.com> 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@...gle.com>
> ---
>  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@...gle.com>

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