PageKmemcg flag is currently defined as a page type (like buddy,
offline, table and guard). Semantically it means that the page
was accounted as a kernel memory by the page allocator and has
to be uncharged on the release.
As a side effect of defining the flag as a page type, the accounted
page can't be mapped to userspace (look at page_has_type() and
comments above). In particular, this blocks the accounting of
vmalloc-backed memory used by some bpf maps, because these maps
do map the memory to userspace.
One option is to fix it by complicating the access to page->mapcount,
which provides some free bits for page->page_type.
But it's way better to move this flag into page->memcg_data flags.
Indeed, the flag makes no sense without enabled memory cgroups
and memory cgroup pointer set in particular.
This commit replaces PageKmemcg() and __SetPageKmemcg() with
PageMemcgKmem() and an open-coded OR operation setting the memcg
pointer with the MEMCG_DATA_KMEM bit. __ClearPageKmemcg() can be
simple deleted, as the whole memcg_data is zeroed at once.
As a bonus, on !CONFIG_MEMCG build the PageMemcgKmem() check will
be compiled out.
Signed-off-by: Roman Gushchin <[email protected]>
---
include/linux/memcontrol.h | 39 ++++++++++++++++++++++++++++++++------
include/linux/page-flags.h | 11 ++---------
mm/memcontrol.c | 16 +++++-----------
mm/page_alloc.c | 4 ++--
4 files changed, 42 insertions(+), 28 deletions(-)
diff --git a/include/linux/memcontrol.h b/include/linux/memcontrol.h
index 5e05599e1f74..8c15978294b2 100644
--- a/include/linux/memcontrol.h
+++ b/include/linux/memcontrol.h
@@ -346,8 +346,10 @@ extern struct mem_cgroup *root_mem_cgroup;
enum page_memcg_data_flags {
/* page->memcg_data is a pointer to an objcgs vector */
MEMCG_DATA_OBJCGS = (1UL << 0),
+ /* page has been accounted as a non-slab kernel page */
+ MEMCG_DATA_KMEM = (1UL << 1),
/* the next bit after the last actual flag */
- __NR_MEMCG_DATA_FLAGS = (1UL << 1),
+ __NR_MEMCG_DATA_FLAGS = (1UL << 2),
};
#define MEMCG_DATA_FLAGS_MASK (__NR_MEMCG_DATA_FLAGS - 1)
@@ -369,8 +371,12 @@ enum page_memcg_data_flags {
*/
static inline struct mem_cgroup *page_memcg(struct page *page)
{
+ unsigned long memcg_data = page->memcg_data;
+
VM_BUG_ON_PAGE(PageSlab(page), page);
- return (struct mem_cgroup *)page->memcg_data;
+ VM_BUG_ON_PAGE(memcg_data & MEMCG_DATA_OBJCGS, page);
+
+ return (struct mem_cgroup *)(memcg_data & ~MEMCG_DATA_FLAGS_MASK);
}
/*
@@ -416,7 +422,7 @@ static inline struct mem_cgroup *page_memcg_check(struct page *page)
if (memcg_data & MEMCG_DATA_OBJCGS)
return NULL;
- return (struct mem_cgroup *)memcg_data;
+ return (struct mem_cgroup *)(memcg_data & ~MEMCG_DATA_FLAGS_MASK);
}
/*
@@ -438,6 +444,20 @@ static inline void set_page_memcg(struct page *page, struct mem_cgroup *memcg)
}
+/*
+ * PageMemcgKmem - check if the page has MemcgKmem flag set
+ * @page: a pointer to the page struct
+ *
+ * Checks if the page has MemcgKmem flag set. The caller must ensure that
+ * the page has an associated memory cgroup. It's not safe to call this function
+ * against some types of pages, e.g. slab pages.
+ */
+static inline bool PageMemcgKmem(struct page *page)
+{
+ VM_BUG_ON_PAGE(page->memcg_data & MEMCG_DATA_OBJCGS, page);
+ return page->memcg_data & MEMCG_DATA_KMEM;
+}
+
#ifdef CONFIG_MEMCG_KMEM
/*
* page_objcgs - get the object cgroups vector associated with a page
@@ -453,8 +473,8 @@ static inline struct obj_cgroup **page_objcgs(struct page *page)
{
unsigned long memcg_data = READ_ONCE(page->memcg_data);
- VM_BUG_ON_PAGE(memcg_data &&
- !(memcg_data & MEMCG_DATA_OBJCGS), page);
+ VM_BUG_ON_PAGE(memcg_data && !(memcg_data & MEMCG_DATA_OBJCGS), page);
+ VM_BUG_ON_PAGE(memcg_data & MEMCG_DATA_KMEM, page);
return (struct obj_cgroup **)(memcg_data & ~MEMCG_DATA_FLAGS_MASK);
}
@@ -471,9 +491,11 @@ static inline struct obj_cgroup **page_objcgs_check(struct page *page)
{
unsigned long memcg_data = READ_ONCE(page->memcg_data);
- if (memcg_data && (memcg_data & MEMCG_DATA_OBJCGS))
+ if (memcg_data && (memcg_data & MEMCG_DATA_OBJCGS)) {
+ VM_BUG_ON_PAGE(memcg_data & MEMCG_DATA_KMEM, page);
return (struct obj_cgroup **)
(memcg_data & ~MEMCG_DATA_FLAGS_MASK);
+ }
return NULL;
}
@@ -1138,6 +1160,11 @@ static inline void set_page_memcg(struct page *page,
{
}
+static inline bool PageMemcgKmem(struct page *page)
+{
+ return false;
+}
+
static inline bool mem_cgroup_is_root(struct mem_cgroup *memcg)
{
return true;
diff --git a/include/linux/page-flags.h b/include/linux/page-flags.h
index 4f6ba9379112..fc0e1bd48e73 100644
--- a/include/linux/page-flags.h
+++ b/include/linux/page-flags.h
@@ -715,9 +715,8 @@ PAGEFLAG_FALSE(DoubleMap)
#define PAGE_MAPCOUNT_RESERVE -128
#define PG_buddy 0x00000080
#define PG_offline 0x00000100
-#define PG_kmemcg 0x00000200
-#define PG_table 0x00000400
-#define PG_guard 0x00000800
+#define PG_table 0x00000200
+#define PG_guard 0x00000400
#define PageType(page, flag) \
((page->page_type & (PAGE_TYPE_BASE | flag)) == PAGE_TYPE_BASE)
@@ -768,12 +767,6 @@ PAGE_TYPE_OPS(Buddy, buddy)
*/
PAGE_TYPE_OPS(Offline, offline)
-/*
- * If kmemcg is enabled, the buddy allocator will set PageKmemcg() on
- * pages allocated with __GFP_ACCOUNT. It gets cleared on page free.
- */
-PAGE_TYPE_OPS(Kmemcg, kmemcg)
-
/*
* Marks pages in use as page tables.
*/
diff --git a/mm/memcontrol.c b/mm/memcontrol.c
index 7e690424a917..ff9bb3e8333a 100644
--- a/mm/memcontrol.c
+++ b/mm/memcontrol.c
@@ -3090,8 +3090,8 @@ int __memcg_kmem_charge_page(struct page *page, gfp_t gfp, int order)
if (memcg && !mem_cgroup_is_root(memcg)) {
ret = __memcg_kmem_charge(memcg, gfp, 1 << order);
if (!ret) {
- set_page_memcg(page, memcg);
- __SetPageKmemcg(page);
+ page->memcg_data = (unsigned long)memcg |
+ MEMCG_DATA_KMEM;
return 0;
}
css_put(&memcg->css);
@@ -3116,10 +3116,6 @@ void __memcg_kmem_uncharge_page(struct page *page, int order)
__memcg_kmem_uncharge(memcg, nr_pages);
page->memcg_data = 0;
css_put(&memcg->css);
-
- /* slab pages do not have PageKmemcg flag set */
- if (PageKmemcg(page))
- __ClearPageKmemcg(page);
}
static bool consume_obj_stock(struct obj_cgroup *objcg, unsigned int nr_bytes)
@@ -6863,12 +6859,10 @@ static void uncharge_page(struct page *page, struct uncharge_gather *ug)
nr_pages = compound_nr(page);
ug->nr_pages += nr_pages;
- if (!PageKmemcg(page)) {
- ug->pgpgout++;
- } else {
+ if (PageMemcgKmem(page))
ug->nr_kmem += nr_pages;
- __ClearPageKmemcg(page);
- }
+ else
+ ug->pgpgout++;
ug->dummy_page = page;
page->memcg_data = 0;
diff --git a/mm/page_alloc.c b/mm/page_alloc.c
index e9f0fe4a143e..5ebd50183d93 100644
--- a/mm/page_alloc.c
+++ b/mm/page_alloc.c
@@ -1179,7 +1179,7 @@ static __always_inline bool free_pages_prepare(struct page *page,
* Do not let hwpoison pages hit pcplists/buddy
* Untie memcg state and reset page's owner
*/
- if (memcg_kmem_enabled() && PageKmemcg(page))
+ if (memcg_kmem_enabled() && PageMemcgKmem(page))
__memcg_kmem_uncharge_page(page, order);
reset_page_owner(page, order);
return false;
@@ -1209,7 +1209,7 @@ static __always_inline bool free_pages_prepare(struct page *page,
}
if (PageMappingFlags(page))
page->mapping = NULL;
- if (memcg_kmem_enabled() && PageKmemcg(page))
+ if (memcg_kmem_enabled() && PageMemcgKmem(page))
__memcg_kmem_uncharge_page(page, order);
if (check_free)
bad += check_free_page(page);
--
2.26.2
On Wed, Sep 30, 2020 at 05:27:10PM -0700, Roman Gushchin wrote:
> PageKmemcg flag is currently defined as a page type (like buddy,
> offline, table and guard). Semantically it means that the page
> was accounted as a kernel memory by the page allocator and has
> to be uncharged on the release.
>
> As a side effect of defining the flag as a page type, the accounted
> page can't be mapped to userspace (look at page_has_type() and
> comments above). In particular, this blocks the accounting of
> vmalloc-backed memory used by some bpf maps, because these maps
> do map the memory to userspace.
>
> One option is to fix it by complicating the access to page->mapcount,
> which provides some free bits for page->page_type.
>
> But it's way better to move this flag into page->memcg_data flags.
> Indeed, the flag makes no sense without enabled memory cgroups
> and memory cgroup pointer set in particular.
>
> This commit replaces PageKmemcg() and __SetPageKmemcg() with
> PageMemcgKmem() and an open-coded OR operation setting the memcg
> pointer with the MEMCG_DATA_KMEM bit. __ClearPageKmemcg() can be
> simple deleted, as the whole memcg_data is zeroed at once.
>
> As a bonus, on !CONFIG_MEMCG build the PageMemcgKmem() check will
> be compiled out.
>
> Signed-off-by: Roman Gushchin <[email protected]>
Acked-by: Johannes Weiner <[email protected]>
Hi.
On Wed, Sep 30, 2020 at 05:27:10PM -0700, Roman Gushchin <[email protected]> wrote:
> @@ -369,8 +371,12 @@ enum page_memcg_data_flags {
> */
> static inline struct mem_cgroup *page_memcg(struct page *page)
> {
> + unsigned long memcg_data = page->memcg_data;
> +
> VM_BUG_ON_PAGE(PageSlab(page), page);
> - return (struct mem_cgroup *)page->memcg_data;
> + VM_BUG_ON_PAGE(memcg_data & MEMCG_DATA_OBJCGS, page);
> +
> + return (struct mem_cgroup *)(memcg_data & ~MEMCG_DATA_FLAGS_MASK);
> }
Shouldn't this change go also into page_memcg_rcu()? (I don't think the
current single user (workingset_activation() would pass a non-slab
kernel page but for consistency sake.)
Alternatively, I'm thinking why (in its single use) is there
page_memcg_rcu() a separate function to page_memcg() (cross memcg page
migration?).
Regards,
Michal
On Thu, Oct 01, 2020 at 07:00:36PM +0200, Michal Koutn? wrote:
> Hi.
>
> On Wed, Sep 30, 2020 at 05:27:10PM -0700, Roman Gushchin <[email protected]> wrote:
> > @@ -369,8 +371,12 @@ enum page_memcg_data_flags {
> > */
> > static inline struct mem_cgroup *page_memcg(struct page *page)
> > {
> > + unsigned long memcg_data = page->memcg_data;
> > +
> > VM_BUG_ON_PAGE(PageSlab(page), page);
> > - return (struct mem_cgroup *)page->memcg_data;
> > + VM_BUG_ON_PAGE(memcg_data & MEMCG_DATA_OBJCGS, page);
> > +
> > + return (struct mem_cgroup *)(memcg_data & ~MEMCG_DATA_FLAGS_MASK);
> > }
> Shouldn't this change go also into page_memcg_rcu()? (I don't think the
> current single user (workingset_activation() would pass a non-slab
> kernel page but for consistency sake.)
+1
> Alternatively, I'm thinking why (in its single use) is there
> page_memcg_rcu() a separate function to page_memcg() (cross memcg page
> migration?).
It goes back to commit 55779ec759ccc3c12b917b3712a7716e1140c652.
The activation code is the only path where page migration is not
excluded. Because unlike with page state statistics, we don't really
mind a race when counting an activation event.
I do think there is a bug, though: mem_cgroup_move_account() should
use WRITE_ONCE() on page->mem_cgroup.
On Thu, Oct 01, 2020 at 07:00:36PM +0200, Michal Koutny wrote:
> Hi.
>
> On Wed, Sep 30, 2020 at 05:27:10PM -0700, Roman Gushchin <[email protected]> wrote:
> > @@ -369,8 +371,12 @@ enum page_memcg_data_flags {
> > */
> > static inline struct mem_cgroup *page_memcg(struct page *page)
> > {
> > + unsigned long memcg_data = page->memcg_data;
> > +
> > VM_BUG_ON_PAGE(PageSlab(page), page);
> > - return (struct mem_cgroup *)page->memcg_data;
> > + VM_BUG_ON_PAGE(memcg_data & MEMCG_DATA_OBJCGS, page);
> > +
> > + return (struct mem_cgroup *)(memcg_data & ~MEMCG_DATA_FLAGS_MASK);
> > }
> Shouldn't this change go also into page_memcg_rcu()? (I don't think the
> current single user (workingset_activation() would pass a non-slab
> kernel page but for consistency sake.)
Yes, a good idea, I will add it.
Thanks!
On Thu, Oct 01, 2020 at 01:27:13PM -0400, Johannes Weiner <[email protected]> wrote:
> The activation code is the only path where page migration is not
> excluded. Because unlike with page state statistics, we don't really
> mind a race when counting an activation event.
Thanks for the explanation. I see why the accessor trio is justified.
> I do think there is a bug, though: mem_cgroup_move_account() should
> use WRITE_ONCE() on page->mem_cgroup.
If this were a bug, wouldn't be the proper approach
rcu_assign_pointer()/rcu_dereference() pair?
Michal
On Fri, Oct 02, 2020 at 12:03:50PM +0200, Michal Koutn? wrote:
> On Thu, Oct 01, 2020 at 01:27:13PM -0400, Johannes Weiner <[email protected]> wrote:
> > I do think there is a bug, though: mem_cgroup_move_account() should
> > use WRITE_ONCE() on page->mem_cgroup.
> If this were a bug, wouldn't be the proper approach
> rcu_assign_pointer()/rcu_dereference() pair?
Yes, you're right.
I'll double check these code paths and follow up with a patch if
necessary.