From: Michal Hocko <mhocko@kernel.org>
To: Muchun Song <songmuchun@bytedance.com>
Cc: cl@linux.com, penberg@kernel.org, rientjes@google.com,
iamjoonsoo.kim@lge.com, akpm@linux-foundation.org,
linux-mm@kvack.org, linux-kernel@vger.kernel.org,
Roman Gushchin <guro@fb.com>
Subject: Re: [PATCH] mm/slab: Add a __GFP_ACCOUNT GFP flag check for slab allocation
Date: Mon, 15 Jun 2020 11:13:35 +0200 [thread overview]
Message-ID: <20200615091335.GD25296@dhcp22.suse.cz> (raw)
In-Reply-To: <20200614063858.85118-1-songmuchun@bytedance.com>
[Cc Roman]
On Sun 14-06-20 14:38:58, Muchun Song wrote:
> When a kmem_cache is initialized with SLAB_ACCOUNT slab flag, we must
> not call kmem_cache_alloc with __GFP_ACCOUNT GFP flag. In this case,
> we can be accounted to kmemcg twice. This is not correct. So we add a
> __GFP_ACCOUNT GFP flag check for slab allocation.
>
> We also introduce a new helper named fixup_gfp_flags to do that check.
> We can reuse the fixup_gfp_flags for SLAB/SLUB.
>
> Signed-off-by: Muchun Song <songmuchun@bytedance.com>
> ---
> mm/slab.c | 10 +---------
> mm/slab.h | 21 +++++++++++++++++++++
> mm/slub.c | 10 +---------
> 3 files changed, 23 insertions(+), 18 deletions(-)
>
> diff --git a/mm/slab.c b/mm/slab.c
> index 9350062ffc1a..6e0110bef2d6 100644
> --- a/mm/slab.c
> +++ b/mm/slab.c
> @@ -126,8 +126,6 @@
>
> #include <trace/events/kmem.h>
>
> -#include "internal.h"
> -
> #include "slab.h"
>
> /*
> @@ -2579,13 +2577,7 @@ static struct page *cache_grow_begin(struct kmem_cache *cachep,
> * Be lazy and only check for valid flags here, keeping it out of the
> * critical path in kmem_cache_alloc().
> */
> - if (unlikely(flags & GFP_SLAB_BUG_MASK)) {
> - gfp_t invalid_mask = flags & GFP_SLAB_BUG_MASK;
> - flags &= ~GFP_SLAB_BUG_MASK;
> - pr_warn("Unexpected gfp: %#x (%pGg). Fixing up to gfp: %#x (%pGg). Fix your code!\n",
> - invalid_mask, &invalid_mask, flags, &flags);
> - dump_stack();
> - }
> + flags = fixup_gfp_flags(cachep, flags);
> WARN_ON_ONCE(cachep->ctor && (flags & __GFP_ZERO));
> local_flags = flags & (GFP_CONSTRAINT_MASK|GFP_RECLAIM_MASK);
>
> diff --git a/mm/slab.h b/mm/slab.h
> index 815e4e9a94cd..0b91f2a7b033 100644
> --- a/mm/slab.h
> +++ b/mm/slab.h
> @@ -109,6 +109,7 @@ struct memcg_cache_params {
> #include <linux/kmemleak.h>
> #include <linux/random.h>
> #include <linux/sched/mm.h>
> +#include "internal.h"
>
> /*
> * State of the slab allocator.
> @@ -627,6 +628,26 @@ struct kmem_cache_node {
>
> };
>
> +static inline gfp_t fixup_gfp_flags(struct kmem_cache *s, gfp_t flags)
> +{
> + gfp_t invalid_mask = 0;
> +
> + if (unlikely(flags & GFP_SLAB_BUG_MASK))
> + invalid_mask |= flags & GFP_SLAB_BUG_MASK;
> +
> + if (unlikely(flags & __GFP_ACCOUNT && s->flags & SLAB_ACCOUNT))
> + invalid_mask |= __GFP_ACCOUNT;
> +
> + if (unlikely(invalid_mask)) {
> + flags &= ~invalid_mask;
> + pr_warn("Unexpected gfp: %#x (%pGg). Fixing up to gfp: %#x (%pGg). Fix your code!\n",
> + invalid_mask, &invalid_mask, flags, &flags);
> + dump_stack();
> + }
> +
> + return flags;
> +}
> +
> static inline struct kmem_cache_node *get_node(struct kmem_cache *s, int node)
> {
> return s->node[node];
> diff --git a/mm/slub.c b/mm/slub.c
> index b8f798b50d44..49b5cb7da318 100644
> --- a/mm/slub.c
> +++ b/mm/slub.c
> @@ -37,8 +37,6 @@
>
> #include <trace/events/kmem.h>
>
> -#include "internal.h"
> -
> /*
> * Lock order:
> * 1. slab_mutex (Global Mutex)
> @@ -1745,13 +1743,7 @@ static struct page *allocate_slab(struct kmem_cache *s, gfp_t flags, int node)
>
> static struct page *new_slab(struct kmem_cache *s, gfp_t flags, int node)
> {
> - if (unlikely(flags & GFP_SLAB_BUG_MASK)) {
> - gfp_t invalid_mask = flags & GFP_SLAB_BUG_MASK;
> - flags &= ~GFP_SLAB_BUG_MASK;
> - pr_warn("Unexpected gfp: %#x (%pGg). Fixing up to gfp: %#x (%pGg). Fix your code!\n",
> - invalid_mask, &invalid_mask, flags, &flags);
> - dump_stack();
> - }
> + flags = fixup_gfp_flags(s, flags);
>
> return allocate_slab(s,
> flags & (GFP_RECLAIM_MASK | GFP_CONSTRAINT_MASK), node);
> --
> 2.11.0
--
Michal Hocko
SUSE Labs
next prev parent reply other threads:[~2020-06-15 9:13 UTC|newest]
Thread overview: 6+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-06-14 6:38 [PATCH] mm/slab: Add a __GFP_ACCOUNT GFP flag check for slab allocation Muchun Song
2020-06-14 10:18 ` kernel test robot
2020-06-14 11:05 ` [External] " Muchun Song
2020-06-15 9:13 ` Michal Hocko [this message]
2020-06-15 13:08 ` Vlastimil Babka
2020-06-15 13:32 ` [External] " Muchun Song
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20200615091335.GD25296@dhcp22.suse.cz \
--to=mhocko@kernel.org \
--cc=akpm@linux-foundation.org \
--cc=cl@linux.com \
--cc=guro@fb.com \
--cc=iamjoonsoo.kim@lge.com \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-mm@kvack.org \
--cc=penberg@kernel.org \
--cc=rientjes@google.com \
--cc=songmuchun@bytedance.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).