lkml.org 
[lkml]   [2020]   [Oct]   [14]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [PATCH v5 4/4] mm: convert page kmemcg type to a page memcg flag
    On Fri 02-10-20 10:25:59, 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.

    Completely agreed. Also this concept is already used for anon/ksm
    which set flags on the same pointer as the object. Having that
    outsourced to elsewhere is really harder to follow.

    > 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 <guro@fb.com>
    > Acked-by: Johannes Weiner <hannes@cmpxchg.org>

    Acked-by: Michal Hocko <mhocko@suse.com>

    > ---
    > cover.txt | 4 ++--
    > include/linux/memcontrol.h | 37 +++++++++++++++++++++++++++++++++----
    > include/linux/page-flags.h | 11 ++---------
    > mm/memcontrol.c | 16 +++++-----------
    > mm/page_alloc.c | 4 ++--
    > 5 files changed, 44 insertions(+), 28 deletions(-)
    >
    > diff --git a/cover.txt b/cover.txt
    > index 32460cd3316e..cf23753cad15 100644
    > --- a/cover.txt
    > +++ b/cover.txt
    > @@ -15,10 +15,10 @@ their memory can't be accounted because of this implementation detail.
    >
    > This patchset removes this limitation by moving the PageKmemcg flag
    > into one of the free bits of the page->mem_cgroup pointer. Also it
    > -formalizes all accesses to the page->mem_cgroup and page->obj_cgroups
    > +formalizes accesses to the page->mem_cgroup and page->obj_cgroups
    > using new helpers, adds several checks and removes a couple of obsolete
    > functions. As the result the code became more robust with fewer
    > -open-coded bits tricks.
    > +open-coded bit tricks.
    >
    > v5:
    > - added ~MEMCG_DATA_FLAGS_MASK to page_memcg_rcu(), by Michal
    > diff --git a/include/linux/memcontrol.h b/include/linux/memcontrol.h
    > index 99a4841d658b..7c9d43476166 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);
    > }
    >
    > /*
    > @@ -387,7 +393,8 @@ static inline struct mem_cgroup *page_memcg_rcu(struct page *page)
    > VM_BUG_ON_PAGE(PageSlab(page), page);
    > WARN_ON_ONCE(!rcu_read_lock_held());
    >
    > - return (struct mem_cgroup *)READ_ONCE(page->memcg_data);
    > + return (struct mem_cgroup *)(READ_ONCE(page->memcg_data) &
    > + ~MEMCG_DATA_FLAGS_MASK);
    > }
    >
    > /*
    > @@ -416,7 +423,21 @@ 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);
    > +}
    > +
    > +/*
    > + * 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
    > @@ -435,6 +456,7 @@ 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_KMEM, page);
    >
    > return (struct obj_cgroup **)(memcg_data & ~MEMCG_DATA_FLAGS_MASK);
    > }
    > @@ -454,6 +476,8 @@ static inline struct obj_cgroup **page_objcgs_check(struct page *page)
    > if (!memcg_data || !(memcg_data & MEMCG_DATA_OBJCGS))
    > return NULL;
    >
    > + VM_BUG_ON_PAGE(memcg_data & MEMCG_DATA_KMEM, page);
    > +
    > return (struct obj_cgroup **)(memcg_data & ~MEMCG_DATA_FLAGS_MASK);
    > }
    >
    > @@ -1109,6 +1133,11 @@ static inline struct mem_cgroup *page_memcg_check(struct page *page)
    > return NULL;
    > }
    >
    > +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 a8eb20283dcd..e3fc240101ff 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) {
    > - page->memcg_data = (unsigned long)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

    --
    Michal Hocko
    SUSE Labs

    \
     
     \ /
      Last update: 2020-10-14 16:40    [W:2.390 / U:0.016 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site