| Message ID | 20231108164920.3401565-1-jackmanb@google.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | [v2] mm/page_alloc: Dedupe some memcg uncharging logic | expand |
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 > >
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>
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 --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++;
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(-)