diff mbox series

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

Message ID 20231108164920.3401565-1-jackmanb@google.com
State New
Headers
Series [v2] mm/page_alloc: Dedupe some memcg uncharging logic |

Commit Message

Brendan Jackman Nov. 8, 2023, 4:49 p.m. UTC 
  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@google.com>
---
 mm/page_alloc.c | 12 ++++--------
 1 file changed, 4 insertions(+), 8 deletions(-)

Comments

Yosry Ahmed Nov. 8, 2023, 8:19 p.m. UTC | #1 
On Wed, Nov 8, 2023 at 8:49 AM Brendan Jackman <jackmanb@google.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@google.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@google.com>

>         if (is_check_pages_enabled()) {
>                 if (free_page_is_bad(page))
>                         bad++;
> --
> 2.42.0.869.gea05f2083d-goog
>
>
Shakeel Butt Nov. 8, 2023, 11:51 p.m. UTC | #2 
On Wed, Nov 8, 2023 at 8:49 AM Brendan Jackman <jackmanb@google.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@google.com>

Thanks for the patch. Actually the PageMemcgKmem/folio_memcg_kmem
check should be in memcg_kmem_uncharge_page() and not in
__memcg_kmem_uncharge_page(). Anyways, that is orthogonal to this
patch.

Acked-by: Shakeel Butt <shakeelb@google.com>
Yosry Ahmed Nov. 9, 2023, 12:07 a.m. UTC | #3 
On Wed, Nov 8, 2023 at 3:52 PM Shakeel Butt <shakeelb@google.com> wrote:
>
> On Wed, Nov 8, 2023 at 8:49 AM Brendan Jackman <jackmanb@google.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@google.com>
>
> Thanks for the patch. Actually the PageMemcgKmem/folio_memcg_kmem
> check should be in memcg_kmem_uncharge_page() and not in
> __memcg_kmem_uncharge_page(). Anyways, that is orthogonal to this
> patch.

Agreed. If we move the check into memcg_kmem_uncharge_page(), perhaps
we should call it directly here instead of doing the checks, since
there won't be an extra function call as it is inline, right? We can
also make __memcg_kmem_uncharge_page static to mm/memcontrol.c

I suspect the same can be done for __memcg_kmem_charge_page() as well.
diff mbox series

Patch

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);
 	if (is_check_pages_enabled()) {
 		if (free_page_is_bad(page))
 			bad++;