2022-08-30 06:02:19

by Kairui Song

[permalink] [raw]
Subject: [PATCH 0/2] mm: memcontrol: cleanup and optimize for accounting params

From: Kairui Song <[email protected]>

Patch 1/2 changes the behavior of kmem accounting a bit, making
it either globally enabled or globally disabled by boot params and
no longer affected by the creation of the first non-root cgroup.
This might be a bit arguable though.

Patch 2/2 optimizes some hot paths by making cgroup_memory_noswap a
static key, benchmark shows swap paths now have a ~4% lower overhead.

Kairui Song (2):
mm: memcontrol: remove mem_cgroup_kmem_disabled
mm: memcontrol: make cgroup_memory_noswap a static key

include/linux/memcontrol.h | 8 +------
mm/memcontrol.c | 45 +++++++++++++++++++++++---------------
mm/percpu.c | 2 +-
mm/slab_common.c | 2 +-
4 files changed, 30 insertions(+), 27 deletions(-)

--
2.35.2


2022-08-30 06:03:19

by Kairui Song

[permalink] [raw]
Subject: [PATCH 2/2] mm: memcontrol: make cgroup_memory_noswap a static key

From: Kairui Song <[email protected]>

cgroup_memory_noswap is used in many hot path, so make it a static key
to lower the kernel overhead.

Using 8G of ZRAM as SWAP, benchmark using `perf stat -d -d -d --repeat 100`
with the following code snip in a non-root cgroup:

#include <stdio.h>
#include <string.h>
#include <linux/mman.h>
#include <sys/mman.h>
#define MB 1024UL * 1024UL
int main(int argc, char **argv){
void *p = mmap(NULL, 8000 * MB, PROT_READ | PROT_WRITE,
MAP_PRIVATE | MAP_ANONYMOUS, -1, 0);
memset(p, 0xff, 8000 * MB);
madvise(p, 8000 * MB, MADV_PAGEOUT);
memset(p, 0xff, 8000 * MB);
return 0;
}

Before:
7,021.43 msec task-clock # 0.967 CPUs utilized ( +- 0.03% )
4,010 context-switches # 573.853 /sec ( +- 0.01% )
0 cpu-migrations # 0.000 /sec
2,052,057 page-faults # 293.661 K/sec ( +- 0.00% )
12,616,546,027 cycles # 1.805 GHz ( +- 0.06% ) (39.92%)
156,823,666 stalled-cycles-frontend # 1.25% frontend cycles idle ( +- 0.10% ) (40.25%)
310,130,812 stalled-cycles-backend # 2.47% backend cycles idle ( +- 4.39% ) (40.73%)
18,692,516,591 instructions # 1.49 insn per cycle
# 0.01 stalled cycles per insn ( +- 0.04% ) (40.75%)
4,907,447,976 branches # 702.283 M/sec ( +- 0.05% ) (40.30%)
13,002,578 branch-misses # 0.26% of all branches ( +- 0.08% ) (40.48%)
7,069,786,296 L1-dcache-loads # 1.012 G/sec ( +- 0.03% ) (40.32%)
649,385,847 L1-dcache-load-misses # 9.13% of all L1-dcache accesses ( +- 0.07% ) (40.10%)
1,485,448,688 L1-icache-loads # 212.576 M/sec ( +- 0.15% ) (39.49%)
31,628,457 L1-icache-load-misses # 2.13% of all L1-icache accesses ( +- 0.40% ) (39.57%)
6,667,311 dTLB-loads # 954.129 K/sec ( +- 0.21% ) (39.50%)
5,668,555 dTLB-load-misses # 86.40% of all dTLB cache accesses ( +- 0.12% ) (39.03%)
765 iTLB-loads # 109.476 /sec ( +- 21.81% ) (39.44%)
4,370,351 iTLB-load-misses # 214320.09% of all iTLB cache accesses ( +- 1.44% ) (39.86%)
149,207,254 L1-dcache-prefetches # 21.352 M/sec ( +- 0.13% ) (40.27%)

7.25869 +- 0.00203 seconds time elapsed ( +- 0.03% )

After:
6,576.16 msec task-clock # 0.953 CPUs utilized ( +- 0.10% )
4,020 context-switches # 605.595 /sec ( +- 0.01% )
0 cpu-migrations # 0.000 /sec
2,052,056 page-faults # 309.133 K/sec ( +- 0.00% )
11,967,619,180 cycles # 1.803 GHz ( +- 0.36% ) (38.76%)
161,259,240 stalled-cycles-frontend # 1.38% frontend cycles idle ( +- 0.27% ) (36.58%)
253,605,302 stalled-cycles-backend # 2.16% backend cycles idle ( +- 4.45% ) (34.78%)
19,328,171,892 instructions # 1.65 insn per cycle
# 0.01 stalled cycles per insn ( +- 0.10% ) (31.46%)
5,213,967,902 branches # 785.461 M/sec ( +- 0.18% ) (30.68%)
12,385,170 branch-misses # 0.24% of all branches ( +- 0.26% ) (34.13%)
7,271,687,822 L1-dcache-loads # 1.095 G/sec ( +- 0.12% ) (35.29%)
649,873,045 L1-dcache-load-misses # 8.93% of all L1-dcache accesses ( +- 0.11% ) (41.41%)
1,950,037,608 L1-icache-loads # 293.764 M/sec ( +- 0.33% ) (43.11%)
31,365,566 L1-icache-load-misses # 1.62% of all L1-icache accesses ( +- 0.39% ) (45.89%)
6,767,809 dTLB-loads # 1.020 M/sec ( +- 0.47% ) (48.42%)
6,339,590 dTLB-load-misses # 95.43% of all dTLB cache accesses ( +- 0.50% ) (46.60%)
736 iTLB-loads # 110.875 /sec ( +- 1.79% ) (48.60%)
4,314,836 iTLB-load-misses # 518653.73% of all iTLB cache accesses ( +- 0.63% ) (42.91%)
144,950,156 L1-dcache-prefetches # 21.836 M/sec ( +- 0.37% ) (41.39%)

6.89935 +- 0.00703 seconds time elapsed ( +- 0.10% )

The performance is clearly better.

Signed-off-by: Kairui Song <[email protected]>
---
mm/memcontrol.c | 27 +++++++++++++++++++--------
1 file changed, 19 insertions(+), 8 deletions(-)

diff --git a/mm/memcontrol.c b/mm/memcontrol.c
index 20e26ccd7dddc..8ea5589345a14 100644
--- a/mm/memcontrol.c
+++ b/mm/memcontrol.c
@@ -90,9 +90,18 @@ static bool cgroup_memory_nokmem __initdata;

/* Whether the swap controller is active */
#ifdef CONFIG_MEMCG_SWAP
-static bool cgroup_memory_noswap __ro_after_init;
+static bool cgroup_memory_noswap __initdata;
+
+static DEFINE_STATIC_KEY_FALSE(memcg_swap_enabled_key);
+static inline bool memcg_swap_enabled(void)
+{
+ return static_branch_likely(&memcg_swap_enabled_key);
+}
#else
-#define cgroup_memory_noswap 1
+static inline bool memcg_swap_enabled(void)
+{
+ return false;
+}
#endif

#ifdef CONFIG_CGROUP_WRITEBACK
@@ -102,7 +111,7 @@ static DECLARE_WAIT_QUEUE_HEAD(memcg_cgwb_frn_waitq);
/* Whether legacy memory+swap accounting is active */
static bool do_memsw_account(void)
{
- return !cgroup_subsys_on_dfl(memory_cgrp_subsys) && !cgroup_memory_noswap;
+ return !cgroup_subsys_on_dfl(memory_cgrp_subsys) && memcg_swap_enabled();
}

#define THRESHOLDS_EVENTS_TARGET 128
@@ -7264,7 +7273,7 @@ void mem_cgroup_swapout(struct folio *folio, swp_entry_t entry)
if (!mem_cgroup_is_root(memcg))
page_counter_uncharge(&memcg->memory, nr_entries);

- if (!cgroup_memory_noswap && memcg != swap_memcg) {
+ if (memcg_swap_enabled() && memcg != swap_memcg) {
if (!mem_cgroup_is_root(swap_memcg))
page_counter_charge(&swap_memcg->memsw, nr_entries);
page_counter_uncharge(&memcg->memsw, nr_entries);
@@ -7316,7 +7325,7 @@ int __mem_cgroup_try_charge_swap(struct folio *folio, swp_entry_t entry)

memcg = mem_cgroup_id_get_online(memcg);

- if (!cgroup_memory_noswap && !mem_cgroup_is_root(memcg) &&
+ if (memcg_swap_enabled() && !mem_cgroup_is_root(memcg) &&
!page_counter_try_charge(&memcg->swap, nr_pages, &counter)) {
memcg_memory_event(memcg, MEMCG_SWAP_MAX);
memcg_memory_event(memcg, MEMCG_SWAP_FAIL);
@@ -7348,7 +7357,7 @@ void __mem_cgroup_uncharge_swap(swp_entry_t entry, unsigned int nr_pages)
rcu_read_lock();
memcg = mem_cgroup_from_id(id);
if (memcg) {
- if (!cgroup_memory_noswap && !mem_cgroup_is_root(memcg)) {
+ if (memcg_swap_enabled() && !mem_cgroup_is_root(memcg)) {
if (cgroup_subsys_on_dfl(memory_cgrp_subsys))
page_counter_uncharge(&memcg->swap, nr_pages);
else
@@ -7364,7 +7373,7 @@ long mem_cgroup_get_nr_swap_pages(struct mem_cgroup *memcg)
{
long nr_swap_pages = get_nr_swap_pages();

- if (cgroup_memory_noswap || !cgroup_subsys_on_dfl(memory_cgrp_subsys))
+ if (!memcg_swap_enabled() || !cgroup_subsys_on_dfl(memory_cgrp_subsys))
return nr_swap_pages;
for (; memcg != root_mem_cgroup; memcg = parent_mem_cgroup(memcg))
nr_swap_pages = min_t(long, nr_swap_pages,
@@ -7381,7 +7390,7 @@ bool mem_cgroup_swap_full(struct page *page)

if (vm_swap_full())
return true;
- if (cgroup_memory_noswap || !cgroup_subsys_on_dfl(memory_cgrp_subsys))
+ if (!memcg_swap_enabled() || !cgroup_subsys_on_dfl(memory_cgrp_subsys))
return false;

memcg = page_memcg(page);
@@ -7689,6 +7698,8 @@ static int __init mem_cgroup_swap_init(void)
if (cgroup_memory_noswap)
return 0;

+ static_branch_enable(&memcg_swap_enabled_key);
+
WARN_ON(cgroup_add_dfl_cftypes(&memory_cgrp_subsys, swap_files));
WARN_ON(cgroup_add_legacy_cftypes(&memory_cgrp_subsys, memsw_files));
#if defined(CONFIG_MEMCG_KMEM) && defined(CONFIG_ZSWAP)
--
2.35.2

2022-08-30 06:09:48

by Kairui Song

[permalink] [raw]
Subject: [PATCH 1/2] mm: memcontrol: remove mem_cgroup_kmem_disabled

From: Kairui Song <[email protected]>

There are currently two helpers for checking if cgroup kmem
accounting is enabled:

- mem_cgroup_kmem_disabled
- memcg_kmem_enabled

mem_cgroup_kmem_disabled is a simple helper that returns true if
cgroup.memory=nokmem is specified, otherwise returns false.

memcg_kmem_enabled is a bit different, it returns true if
cgroup.memory=nokmem is not specified and there is at least one
non-root cgroup ever created. And once there is any non-root memcg
created, it won't go back to return false again.

This may help improve performance for some corner use cases where
the user enables memory cgroup and kmem accounting globally but never
create any cgroup.

Considering that corner case is rare, especially nowadays cgroup is
widely used as a standard way to organize services. And the "once
enabled never disable" behavior is kind of strange. This commit simplifies
the behavior of memcg_kmem_enabled, making it simply the opposite of
mem_cgroup_kmem_disabled, always true if cgroup.memory=nokmem is
not specified. So mem_cgroup_kmem_disabled can be dropped.

This simplifies the code, and besides, memcg_kmem_enabled makes use
of static key so it has a lower overhead.

Signed-off-by: Kairui Song <[email protected]>
---
include/linux/memcontrol.h | 8 +-------
mm/memcontrol.c | 17 +++++++----------
mm/percpu.c | 2 +-
mm/slab_common.c | 2 +-
4 files changed, 10 insertions(+), 19 deletions(-)

diff --git a/include/linux/memcontrol.h b/include/linux/memcontrol.h
index 6257867fbf953..9c08464ed6b46 100644
--- a/include/linux/memcontrol.h
+++ b/include/linux/memcontrol.h
@@ -1731,7 +1731,6 @@ static inline void set_shrinker_bit(struct mem_cgroup *memcg,
#endif

#ifdef CONFIG_MEMCG_KMEM
-bool mem_cgroup_kmem_disabled(void);
int __memcg_kmem_charge_page(struct page *page, gfp_t gfp, int order);
void __memcg_kmem_uncharge_page(struct page *page, int order);

@@ -1779,7 +1778,7 @@ static inline void count_objcg_event(struct obj_cgroup *objcg,
{
struct mem_cgroup *memcg;

- if (mem_cgroup_kmem_disabled())
+ if (!memcg_kmem_enabled())
return;

rcu_read_lock();
@@ -1825,11 +1824,6 @@ static inline struct mem_cgroup *mem_cgroup_or_root(struct mem_cgroup *memcg)
return memcg ? memcg : root_mem_cgroup;
}
#else
-static inline bool mem_cgroup_kmem_disabled(void)
-{
- return true;
-}
-
static inline int memcg_kmem_charge_page(struct page *page, gfp_t gfp,
int order)
{
diff --git a/mm/memcontrol.c b/mm/memcontrol.c
index b69979c9ced5c..20e26ccd7dddc 100644
--- a/mm/memcontrol.c
+++ b/mm/memcontrol.c
@@ -86,7 +86,7 @@ EXPORT_PER_CPU_SYMBOL_GPL(int_active_memcg);
static bool cgroup_memory_nosocket __ro_after_init;

/* Kernel memory accounting disabled? */
-static bool cgroup_memory_nokmem __ro_after_init;
+static bool cgroup_memory_nokmem __initdata;

/* Whether the swap controller is active */
#ifdef CONFIG_MEMCG_SWAP
@@ -255,11 +255,6 @@ struct mem_cgroup *vmpressure_to_memcg(struct vmpressure *vmpr)
#ifdef CONFIG_MEMCG_KMEM
static DEFINE_SPINLOCK(objcg_lock);

-bool mem_cgroup_kmem_disabled(void)
-{
- return cgroup_memory_nokmem;
-}
-
static void obj_cgroup_uncharge_pages(struct obj_cgroup *objcg,
unsigned int nr_pages);

@@ -3667,7 +3662,7 @@ static int memcg_online_kmem(struct mem_cgroup *memcg)
{
struct obj_cgroup *objcg;

- if (mem_cgroup_kmem_disabled())
+ if (!memcg_kmem_enabled())
return 0;

if (unlikely(mem_cgroup_is_root(memcg)))
@@ -3680,8 +3675,6 @@ static int memcg_online_kmem(struct mem_cgroup *memcg)
objcg->memcg = memcg;
rcu_assign_pointer(memcg->objcg, objcg);

- static_branch_enable(&memcg_kmem_enabled_key);
-
memcg->kmemcg_id = memcg->id.id;

return 0;
@@ -3691,7 +3684,7 @@ static void memcg_offline_kmem(struct mem_cgroup *memcg)
{
struct mem_cgroup *parent;

- if (mem_cgroup_kmem_disabled())
+ if (!memcg_kmem_enabled())
return;

if (unlikely(mem_cgroup_is_root(memcg)))
@@ -7153,6 +7146,10 @@ static int __init cgroup_memory(char *s)
if (!strcmp(token, "nokmem"))
cgroup_memory_nokmem = true;
}
+
+ if (!cgroup_memory_nokmem)
+ static_branch_enable(&memcg_kmem_enabled_key);
+
return 1;
}
__setup("cgroup.memory=", cgroup_memory);
diff --git a/mm/percpu.c b/mm/percpu.c
index 27697b2429c2e..c62d6e98f7d20 100644
--- a/mm/percpu.c
+++ b/mm/percpu.c
@@ -1467,7 +1467,7 @@ static struct pcpu_chunk *pcpu_alloc_chunk(gfp_t gfp)
goto md_blocks_fail;

#ifdef CONFIG_MEMCG_KMEM
- if (!mem_cgroup_kmem_disabled()) {
+ if (memcg_kmem_enabled()) {
chunk->obj_cgroups =
pcpu_mem_zalloc(pcpu_chunk_map_bits(chunk) *
sizeof(struct obj_cgroup *), gfp);
diff --git a/mm/slab_common.c b/mm/slab_common.c
index 17996649cfe3e..bbdc0fe3c5e34 100644
--- a/mm/slab_common.c
+++ b/mm/slab_common.c
@@ -829,7 +829,7 @@ new_kmalloc_cache(int idx, enum kmalloc_cache_type type, slab_flags_t flags)
if (type == KMALLOC_RECLAIM) {
flags |= SLAB_RECLAIM_ACCOUNT;
} else if (IS_ENABLED(CONFIG_MEMCG_KMEM) && (type == KMALLOC_CGROUP)) {
- if (mem_cgroup_kmem_disabled()) {
+ if (!memcg_kmem_enabled()) {
kmalloc_caches[type][idx] = kmalloc_caches[KMALLOC_NORMAL][idx];
return;
}
--
2.35.2

2022-08-30 07:01:07

by Michal Hocko

[permalink] [raw]
Subject: Re: [PATCH 1/2] mm: memcontrol: remove mem_cgroup_kmem_disabled

On Tue 30-08-22 13:59:48, Kairui Song wrote:
> From: Kairui Song <[email protected]>
>
> There are currently two helpers for checking if cgroup kmem
> accounting is enabled:
>
> - mem_cgroup_kmem_disabled
> - memcg_kmem_enabled

Yes, this is a bit confusing indeed!

> mem_cgroup_kmem_disabled is a simple helper that returns true if
> cgroup.memory=nokmem is specified, otherwise returns false.
>
> memcg_kmem_enabled is a bit different, it returns true if
> cgroup.memory=nokmem is not specified and there is at least one
> non-root cgroup ever created. And once there is any non-root memcg
> created, it won't go back to return false again.
>
> This may help improve performance for some corner use cases where
> the user enables memory cgroup and kmem accounting globally but never
> create any cgroup.
>
> Considering that corner case is rare, especially nowadays cgroup is
> widely used as a standard way to organize services.

Is it really that rare? Most configurations would use a default setup, so
both MEMCG enabled and without nokmem on cmd line yet the memory
controller is not enabled in their setups.

> And the "once
> enabled never disable" behavior is kind of strange. This commit simplifies
> the behavior of memcg_kmem_enabled, making it simply the opposite of
> mem_cgroup_kmem_disabled, always true if cgroup.memory=nokmem is
> not specified. So mem_cgroup_kmem_disabled can be dropped.
>
> This simplifies the code, and besides, memcg_kmem_enabled makes use
> of static key so it has a lower overhead.

I agree that this is slightly confusing and undocumented. The first step
would be finding out why we need both outside of the memcg proper.

E.g. it doesn't make much sense to me that count_objcg_event uses the
command line variant when it should be using the dynamic (and more
optimized no branch) variant.

On the other hand pcpu_alloc_chunk seems to be different because it can
be called before the controller is enabled but maybe we do not need to
waste memory before that? Similarly new_kmalloc_cache. I suspect these
are mostly to simplify the code and reduce special casing.
>
> Signed-off-by: Kairui Song <[email protected]>
> ---
> include/linux/memcontrol.h | 8 +-------
> mm/memcontrol.c | 17 +++++++----------
> mm/percpu.c | 2 +-
> mm/slab_common.c | 2 +-
> 4 files changed, 10 insertions(+), 19 deletions(-)

I do not think that saving 9 LOC and sacrifice optimization that might
be useful is a good justification.

--
Michal Hocko
SUSE Labs

2022-08-30 07:24:46

by Michal Hocko

[permalink] [raw]
Subject: Re: [PATCH 1/2] mm: memcontrol: remove mem_cgroup_kmem_disabled

On Tue 30-08-22 15:06:57, Kairui Song wrote:
> Michal Hocko <[email protected]> 于2022年8月30日周二 14:45写道:
> >
> > On Tue 30-08-22 13:59:48, Kairui Song wrote:
> > > From: Kairui Song <[email protected]>
> > >
> > > There are currently two helpers for checking if cgroup kmem
> > > accounting is enabled:
> > >
> > > - mem_cgroup_kmem_disabled
> > > - memcg_kmem_enabled
> >
> > Yes, this is a bit confusing indeed!
> >
> > > mem_cgroup_kmem_disabled is a simple helper that returns true if
> > > cgroup.memory=nokmem is specified, otherwise returns false.
> > >
> > > memcg_kmem_enabled is a bit different, it returns true if
> > > cgroup.memory=nokmem is not specified and there is at least one
> > > non-root cgroup ever created. And once there is any non-root memcg
> > > created, it won't go back to return false again.
> > >
> > > This may help improve performance for some corner use cases where
> > > the user enables memory cgroup and kmem accounting globally but never
> > > create any cgroup.
> > >
> > > Considering that corner case is rare, especially nowadays cgroup is
> > > widely used as a standard way to organize services.
> >
> > Is it really that rare? Most configurations would use a default setup, so
> > both MEMCG enabled and without nokmem on cmd line yet the memory
> > controller is not enabled in their setups.
>
> Actually I don't have too much confidence saying that as well... but
> AFAIK, almost all distros will create a few sub cgroup on boot by the
> init (eg. openrc, finit, systemd).

Yeah, but do they enable the memory controller as well? Unless I am
missing something this will require at least one memcg enabled cgroup to
be created.

--
Michal Hocko
SUSE Labs

2022-08-30 07:56:56

by Michal Hocko

[permalink] [raw]
Subject: Re: [PATCH 2/2] mm: memcontrol: make cgroup_memory_noswap a static key

On Tue 30-08-22 13:59:49, Kairui Song wrote:
> From: Kairui Song <[email protected]>
>
> cgroup_memory_noswap is used in many hot path, so make it a static key
> to lower the kernel overhead.
>
> Using 8G of ZRAM as SWAP, benchmark using `perf stat -d -d -d --repeat 100`
> with the following code snip in a non-root cgroup:
>
> #include <stdio.h>
> #include <string.h>
> #include <linux/mman.h>
> #include <sys/mman.h>
> #define MB 1024UL * 1024UL
> int main(int argc, char **argv){
> void *p = mmap(NULL, 8000 * MB, PROT_READ | PROT_WRITE,
> MAP_PRIVATE | MAP_ANONYMOUS, -1, 0);
> memset(p, 0xff, 8000 * MB);
> madvise(p, 8000 * MB, MADV_PAGEOUT);
> memset(p, 0xff, 8000 * MB);
> return 0;
> }
>
> Before:
> 7,021.43 msec task-clock # 0.967 CPUs utilized ( +- 0.03% )
> 4,010 context-switches # 573.853 /sec ( +- 0.01% )
> 0 cpu-migrations # 0.000 /sec
> 2,052,057 page-faults # 293.661 K/sec ( +- 0.00% )
> 12,616,546,027 cycles # 1.805 GHz ( +- 0.06% ) (39.92%)
> 156,823,666 stalled-cycles-frontend # 1.25% frontend cycles idle ( +- 0.10% ) (40.25%)
> 310,130,812 stalled-cycles-backend # 2.47% backend cycles idle ( +- 4.39% ) (40.73%)
> 18,692,516,591 instructions # 1.49 insn per cycle
> # 0.01 stalled cycles per insn ( +- 0.04% ) (40.75%)
> 4,907,447,976 branches # 702.283 M/sec ( +- 0.05% ) (40.30%)
> 13,002,578 branch-misses # 0.26% of all branches ( +- 0.08% ) (40.48%)
> 7,069,786,296 L1-dcache-loads # 1.012 G/sec ( +- 0.03% ) (40.32%)
> 649,385,847 L1-dcache-load-misses # 9.13% of all L1-dcache accesses ( +- 0.07% ) (40.10%)
> 1,485,448,688 L1-icache-loads # 212.576 M/sec ( +- 0.15% ) (39.49%)
> 31,628,457 L1-icache-load-misses # 2.13% of all L1-icache accesses ( +- 0.40% ) (39.57%)
> 6,667,311 dTLB-loads # 954.129 K/sec ( +- 0.21% ) (39.50%)
> 5,668,555 dTLB-load-misses # 86.40% of all dTLB cache accesses ( +- 0.12% ) (39.03%)
> 765 iTLB-loads # 109.476 /sec ( +- 21.81% ) (39.44%)
> 4,370,351 iTLB-load-misses # 214320.09% of all iTLB cache accesses ( +- 1.44% ) (39.86%)
> 149,207,254 L1-dcache-prefetches # 21.352 M/sec ( +- 0.13% ) (40.27%)
>
> 7.25869 +- 0.00203 seconds time elapsed ( +- 0.03% )
>
> After:
> 6,576.16 msec task-clock # 0.953 CPUs utilized ( +- 0.10% )
> 4,020 context-switches # 605.595 /sec ( +- 0.01% )
> 0 cpu-migrations # 0.000 /sec
> 2,052,056 page-faults # 309.133 K/sec ( +- 0.00% )
> 11,967,619,180 cycles # 1.803 GHz ( +- 0.36% ) (38.76%)
> 161,259,240 stalled-cycles-frontend # 1.38% frontend cycles idle ( +- 0.27% ) (36.58%)
> 253,605,302 stalled-cycles-backend # 2.16% backend cycles idle ( +- 4.45% ) (34.78%)
> 19,328,171,892 instructions # 1.65 insn per cycle
> # 0.01 stalled cycles per insn ( +- 0.10% ) (31.46%)
> 5,213,967,902 branches # 785.461 M/sec ( +- 0.18% ) (30.68%)
> 12,385,170 branch-misses # 0.24% of all branches ( +- 0.26% ) (34.13%)
> 7,271,687,822 L1-dcache-loads # 1.095 G/sec ( +- 0.12% ) (35.29%)
> 649,873,045 L1-dcache-load-misses # 8.93% of all L1-dcache accesses ( +- 0.11% ) (41.41%)
> 1,950,037,608 L1-icache-loads # 293.764 M/sec ( +- 0.33% ) (43.11%)
> 31,365,566 L1-icache-load-misses # 1.62% of all L1-icache accesses ( +- 0.39% ) (45.89%)
> 6,767,809 dTLB-loads # 1.020 M/sec ( +- 0.47% ) (48.42%)
> 6,339,590 dTLB-load-misses # 95.43% of all dTLB cache accesses ( +- 0.50% ) (46.60%)
> 736 iTLB-loads # 110.875 /sec ( +- 1.79% ) (48.60%)
> 4,314,836 iTLB-load-misses # 518653.73% of all iTLB cache accesses ( +- 0.63% ) (42.91%)
> 144,950,156 L1-dcache-prefetches # 21.836 M/sec ( +- 0.37% ) (41.39%)
>
> 6.89935 +- 0.00703 seconds time elapsed ( +- 0.10% )

Do you happen to have a perf profile before and after to see which of
the paths really benefits from this?

> The performance is clearly better.
>
> Signed-off-by: Kairui Song <[email protected]>

Anyway, this looks good to me. I like memcg_swap_enabled() better than
!cgroup_memory_noswap. The double negative was quite confusing.

Acked-by: Michal Hocko <[email protected]>

Thanks!

> ---
> mm/memcontrol.c | 27 +++++++++++++++++++--------
> 1 file changed, 19 insertions(+), 8 deletions(-)
>
> diff --git a/mm/memcontrol.c b/mm/memcontrol.c
> index 20e26ccd7dddc..8ea5589345a14 100644
> --- a/mm/memcontrol.c
> +++ b/mm/memcontrol.c
> @@ -90,9 +90,18 @@ static bool cgroup_memory_nokmem __initdata;
>
> /* Whether the swap controller is active */
> #ifdef CONFIG_MEMCG_SWAP
> -static bool cgroup_memory_noswap __ro_after_init;
> +static bool cgroup_memory_noswap __initdata;
> +
> +static DEFINE_STATIC_KEY_FALSE(memcg_swap_enabled_key);
> +static inline bool memcg_swap_enabled(void)
> +{
> + return static_branch_likely(&memcg_swap_enabled_key);
> +}
> #else
> -#define cgroup_memory_noswap 1
> +static inline bool memcg_swap_enabled(void)
> +{
> + return false;
> +}
> #endif
>
> #ifdef CONFIG_CGROUP_WRITEBACK
> @@ -102,7 +111,7 @@ static DECLARE_WAIT_QUEUE_HEAD(memcg_cgwb_frn_waitq);
> /* Whether legacy memory+swap accounting is active */
> static bool do_memsw_account(void)
> {
> - return !cgroup_subsys_on_dfl(memory_cgrp_subsys) && !cgroup_memory_noswap;
> + return !cgroup_subsys_on_dfl(memory_cgrp_subsys) && memcg_swap_enabled();
> }
>
> #define THRESHOLDS_EVENTS_TARGET 128
> @@ -7264,7 +7273,7 @@ void mem_cgroup_swapout(struct folio *folio, swp_entry_t entry)
> if (!mem_cgroup_is_root(memcg))
> page_counter_uncharge(&memcg->memory, nr_entries);
>
> - if (!cgroup_memory_noswap && memcg != swap_memcg) {
> + if (memcg_swap_enabled() && memcg != swap_memcg) {
> if (!mem_cgroup_is_root(swap_memcg))
> page_counter_charge(&swap_memcg->memsw, nr_entries);
> page_counter_uncharge(&memcg->memsw, nr_entries);
> @@ -7316,7 +7325,7 @@ int __mem_cgroup_try_charge_swap(struct folio *folio, swp_entry_t entry)
>
> memcg = mem_cgroup_id_get_online(memcg);
>
> - if (!cgroup_memory_noswap && !mem_cgroup_is_root(memcg) &&
> + if (memcg_swap_enabled() && !mem_cgroup_is_root(memcg) &&
> !page_counter_try_charge(&memcg->swap, nr_pages, &counter)) {
> memcg_memory_event(memcg, MEMCG_SWAP_MAX);
> memcg_memory_event(memcg, MEMCG_SWAP_FAIL);
> @@ -7348,7 +7357,7 @@ void __mem_cgroup_uncharge_swap(swp_entry_t entry, unsigned int nr_pages)
> rcu_read_lock();
> memcg = mem_cgroup_from_id(id);
> if (memcg) {
> - if (!cgroup_memory_noswap && !mem_cgroup_is_root(memcg)) {
> + if (memcg_swap_enabled() && !mem_cgroup_is_root(memcg)) {
> if (cgroup_subsys_on_dfl(memory_cgrp_subsys))
> page_counter_uncharge(&memcg->swap, nr_pages);
> else
> @@ -7364,7 +7373,7 @@ long mem_cgroup_get_nr_swap_pages(struct mem_cgroup *memcg)
> {
> long nr_swap_pages = get_nr_swap_pages();
>
> - if (cgroup_memory_noswap || !cgroup_subsys_on_dfl(memory_cgrp_subsys))
> + if (!memcg_swap_enabled() || !cgroup_subsys_on_dfl(memory_cgrp_subsys))
> return nr_swap_pages;
> for (; memcg != root_mem_cgroup; memcg = parent_mem_cgroup(memcg))
> nr_swap_pages = min_t(long, nr_swap_pages,
> @@ -7381,7 +7390,7 @@ bool mem_cgroup_swap_full(struct page *page)
>
> if (vm_swap_full())
> return true;
> - if (cgroup_memory_noswap || !cgroup_subsys_on_dfl(memory_cgrp_subsys))
> + if (!memcg_swap_enabled() || !cgroup_subsys_on_dfl(memory_cgrp_subsys))
> return false;
>
> memcg = page_memcg(page);
> @@ -7689,6 +7698,8 @@ static int __init mem_cgroup_swap_init(void)
> if (cgroup_memory_noswap)
> return 0;
>
> + static_branch_enable(&memcg_swap_enabled_key);
> +
> WARN_ON(cgroup_add_dfl_cftypes(&memory_cgrp_subsys, swap_files));
> WARN_ON(cgroup_add_legacy_cftypes(&memory_cgrp_subsys, memsw_files));
> #if defined(CONFIG_MEMCG_KMEM) && defined(CONFIG_ZSWAP)
> --
> 2.35.2

--
Michal Hocko
SUSE Labs

2022-08-30 07:58:37

by Kairui Song

[permalink] [raw]
Subject: Re: [PATCH 1/2] mm: memcontrol: remove mem_cgroup_kmem_disabled

Michal Hocko <[email protected]> 于2022年8月30日周二 15:12写道:
>
> On Tue 30-08-22 15:06:57, Kairui Song wrote:
> > Michal Hocko <[email protected]> 于2022年8月30日周二 14:45写道:
> > >
> > > On Tue 30-08-22 13:59:48, Kairui Song wrote:
> > > > From: Kairui Song <[email protected]>
> > > >
> > > > There are currently two helpers for checking if cgroup kmem
> > > > accounting is enabled:
> > > >
> > > > - mem_cgroup_kmem_disabled
> > > > - memcg_kmem_enabled
> > >
> > > Yes, this is a bit confusing indeed!
> > >
> > > > mem_cgroup_kmem_disabled is a simple helper that returns true if
> > > > cgroup.memory=nokmem is specified, otherwise returns false.
> > > >
> > > > memcg_kmem_enabled is a bit different, it returns true if
> > > > cgroup.memory=nokmem is not specified and there is at least one
> > > > non-root cgroup ever created. And once there is any non-root memcg
> > > > created, it won't go back to return false again.
> > > >
> > > > This may help improve performance for some corner use cases where
> > > > the user enables memory cgroup and kmem accounting globally but never
> > > > create any cgroup.
> > > >
> > > > Considering that corner case is rare, especially nowadays cgroup is
> > > > widely used as a standard way to organize services.
> > >
> > > Is it really that rare? Most configurations would use a default setup, so
> > > both MEMCG enabled and without nokmem on cmd line yet the memory
> > > controller is not enabled in their setups.
> >
> > Actually I don't have too much confidence saying that as well... but
> > AFAIK, almost all distros will create a few sub cgroup on boot by the
> > init (eg. openrc, finit, systemd).
>
> Yeah, but do they enable the memory controller as well? Unless I am
> missing something this will require at least one memcg enabled cgroup to
> be created.

Systemd enable memory controller by default since ver 238 from 2018,
but I'm not sure about the others.
Now I think I was wrong about the assumption, will be sure to do more
homework next time.

And thanks for the review!

2022-08-30 08:07:39

by Kairui Song

[permalink] [raw]
Subject: Re: [PATCH 1/2] mm: memcontrol: remove mem_cgroup_kmem_disabled

Michal Hocko <[email protected]> 于2022年8月30日周二 14:45写道:
>
> On Tue 30-08-22 13:59:48, Kairui Song wrote:
> > From: Kairui Song <[email protected]>
> >
> > There are currently two helpers for checking if cgroup kmem
> > accounting is enabled:
> >
> > - mem_cgroup_kmem_disabled
> > - memcg_kmem_enabled
>
> Yes, this is a bit confusing indeed!
>
> > mem_cgroup_kmem_disabled is a simple helper that returns true if
> > cgroup.memory=nokmem is specified, otherwise returns false.
> >
> > memcg_kmem_enabled is a bit different, it returns true if
> > cgroup.memory=nokmem is not specified and there is at least one
> > non-root cgroup ever created. And once there is any non-root memcg
> > created, it won't go back to return false again.
> >
> > This may help improve performance for some corner use cases where
> > the user enables memory cgroup and kmem accounting globally but never
> > create any cgroup.
> >
> > Considering that corner case is rare, especially nowadays cgroup is
> > widely used as a standard way to organize services.
>
> Is it really that rare? Most configurations would use a default setup, so
> both MEMCG enabled and without nokmem on cmd line yet the memory
> controller is not enabled in their setups.

Actually I don't have too much confidence saying that as well... but
AFAIK, almost all distros will create a few sub cgroup on boot by the
init (eg. openrc, finit, systemd).
Maybe it's not that rare indeed.

>
> > And the "once
> > enabled never disable" behavior is kind of strange. This commit simplifies
> > the behavior of memcg_kmem_enabled, making it simply the opposite of
> > mem_cgroup_kmem_disabled, always true if cgroup.memory=nokmem is
> > not specified. So mem_cgroup_kmem_disabled can be dropped.
> >
> > This simplifies the code, and besides, memcg_kmem_enabled makes use
> > of static key so it has a lower overhead.
>
> I agree that this is slightly confusing and undocumented. The first step
> would be finding out why we need both outside of the memcg proper.
>
> E.g. it doesn't make much sense to me that count_objcg_event uses the
> command line variant when it should be using the dynamic (and more
> optimized no branch) variant.
>
> On the other hand pcpu_alloc_chunk seems to be different because it can
> be called before the controller is enabled but maybe we do not need to
> waste memory before that? Similarly new_kmalloc_cache. I suspect these
> are mostly to simplify the code and reduce special casing.

Yes, that's very insightful, let me tidy up the code and logic behind
and send a V2 later.

2022-08-30 09:12:31

by Kairui Song

[permalink] [raw]
Subject: Re: [PATCH 2/2] mm: memcontrol: make cgroup_memory_noswap a static key

Michal Hocko <[email protected]> 于2022年8月30日周二 15:01写道:
>
> On Tue 30-08-22 13:59:49, Kairui Song wrote:
> > From: Kairui Song <[email protected]>
> >
> > cgroup_memory_noswap is used in many hot path, so make it a static key
> > to lower the kernel overhead.
> >
> > Using 8G of ZRAM as SWAP, benchmark using `perf stat -d -d -d --repeat 100`
> > with the following code snip in a non-root cgroup:
> >
> > #include <stdio.h>
> > #include <string.h>
> > #include <linux/mman.h>
> > #include <sys/mman.h>
> > #define MB 1024UL * 1024UL
> > int main(int argc, char **argv){
> > void *p = mmap(NULL, 8000 * MB, PROT_READ | PROT_WRITE,
> > MAP_PRIVATE | MAP_ANONYMOUS, -1, 0);
> > memset(p, 0xff, 8000 * MB);
> > madvise(p, 8000 * MB, MADV_PAGEOUT);
> > memset(p, 0xff, 8000 * MB);
> > return 0;
> > }
> >
> > Before:
> > 7,021.43 msec task-clock # 0.967 CPUs utilized ( +- 0.03% )
> > 4,010 context-switches # 573.853 /sec ( +- 0.01% )
> > 0 cpu-migrations # 0.000 /sec
> > 2,052,057 page-faults # 293.661 K/sec ( +- 0.00% )
> > 12,616,546,027 cycles # 1.805 GHz ( +- 0.06% ) (39.92%)
> > 156,823,666 stalled-cycles-frontend # 1.25% frontend cycles idle ( +- 0.10% ) (40.25%)
> > 310,130,812 stalled-cycles-backend # 2.47% backend cycles idle ( +- 4.39% ) (40.73%)
> > 18,692,516,591 instructions # 1.49 insn per cycle
> > # 0.01 stalled cycles per insn ( +- 0.04% ) (40.75%)
> > 4,907,447,976 branches # 702.283 M/sec ( +- 0.05% ) (40.30%)
> > 13,002,578 branch-misses # 0.26% of all branches ( +- 0.08% ) (40.48%)
> > 7,069,786,296 L1-dcache-loads # 1.012 G/sec ( +- 0.03% ) (40.32%)
> > 649,385,847 L1-dcache-load-misses # 9.13% of all L1-dcache accesses ( +- 0.07% ) (40.10%)
> > 1,485,448,688 L1-icache-loads # 212.576 M/sec ( +- 0.15% ) (39.49%)
> > 31,628,457 L1-icache-load-misses # 2.13% of all L1-icache accesses ( +- 0.40% ) (39.57%)
> > 6,667,311 dTLB-loads # 954.129 K/sec ( +- 0.21% ) (39.50%)
> > 5,668,555 dTLB-load-misses # 86.40% of all dTLB cache accesses ( +- 0.12% ) (39.03%)
> > 765 iTLB-loads # 109.476 /sec ( +- 21.81% ) (39.44%)
> > 4,370,351 iTLB-load-misses # 214320.09% of all iTLB cache accesses ( +- 1.44% ) (39.86%)
> > 149,207,254 L1-dcache-prefetches # 21.352 M/sec ( +- 0.13% ) (40.27%)
> >
> > 7.25869 +- 0.00203 seconds time elapsed ( +- 0.03% )
> >
> > After:
> > 6,576.16 msec task-clock # 0.953 CPUs utilized ( +- 0.10% )
> > 4,020 context-switches # 605.595 /sec ( +- 0.01% )
> > 0 cpu-migrations # 0.000 /sec
> > 2,052,056 page-faults # 309.133 K/sec ( +- 0.00% )
> > 11,967,619,180 cycles # 1.803 GHz ( +- 0.36% ) (38.76%)
> > 161,259,240 stalled-cycles-frontend # 1.38% frontend cycles idle ( +- 0.27% ) (36.58%)
> > 253,605,302 stalled-cycles-backend # 2.16% backend cycles idle ( +- 4.45% ) (34.78%)
> > 19,328,171,892 instructions # 1.65 insn per cycle
> > # 0.01 stalled cycles per insn ( +- 0.10% ) (31.46%)
> > 5,213,967,902 branches # 785.461 M/sec ( +- 0.18% ) (30.68%)
> > 12,385,170 branch-misses # 0.24% of all branches ( +- 0.26% ) (34.13%)
> > 7,271,687,822 L1-dcache-loads # 1.095 G/sec ( +- 0.12% ) (35.29%)
> > 649,873,045 L1-dcache-load-misses # 8.93% of all L1-dcache accesses ( +- 0.11% ) (41.41%)
> > 1,950,037,608 L1-icache-loads # 293.764 M/sec ( +- 0.33% ) (43.11%)
> > 31,365,566 L1-icache-load-misses # 1.62% of all L1-icache accesses ( +- 0.39% ) (45.89%)
> > 6,767,809 dTLB-loads # 1.020 M/sec ( +- 0.47% ) (48.42%)
> > 6,339,590 dTLB-load-misses # 95.43% of all dTLB cache accesses ( +- 0.50% ) (46.60%)
> > 736 iTLB-loads # 110.875 /sec ( +- 1.79% ) (48.60%)
> > 4,314,836 iTLB-load-misses # 518653.73% of all iTLB cache accesses ( +- 0.63% ) (42.91%)
> > 144,950,156 L1-dcache-prefetches # 21.836 M/sec ( +- 0.37% ) (41.39%)
> >
> > 6.89935 +- 0.00703 seconds time elapsed ( +- 0.10% )
>
> Do you happen to have a perf profile before and after to see which of
> the paths really benefits from this?

No I don't have a clear profile data about which path benefit the most.
The performance benchmark result can be stably reproduced, but perf
record & report & diff doesn't seems too helpful, as I can't see a
significant change of any single symbols.

There are quite a few callers of memcg_swap_enabled and
do_memsw_account (which also calls memcg_swap_enabled), to me, it
seems multiple pieces of optimization caused an overall improvement.
And a lower overhead for the branch predictor may also help in
general.

Any other suggestion about how to collect such data?

2022-08-30 11:17:24

by Michal Hocko

[permalink] [raw]
Subject: Re: [PATCH 2/2] mm: memcontrol: make cgroup_memory_noswap a static key

On Tue 30-08-22 16:50:38, Kairui Song wrote:
> Michal Hocko <[email protected]> 于2022年8月30日周二 15:01写道:
> >
> > On Tue 30-08-22 13:59:49, Kairui Song wrote:
> > > From: Kairui Song <[email protected]>
> > >
> > > cgroup_memory_noswap is used in many hot path, so make it a static key
> > > to lower the kernel overhead.
> > >
> > > Using 8G of ZRAM as SWAP, benchmark using `perf stat -d -d -d --repeat 100`
> > > with the following code snip in a non-root cgroup:
> > >
> > > #include <stdio.h>
> > > #include <string.h>
> > > #include <linux/mman.h>
> > > #include <sys/mman.h>
> > > #define MB 1024UL * 1024UL
> > > int main(int argc, char **argv){
> > > void *p = mmap(NULL, 8000 * MB, PROT_READ | PROT_WRITE,
> > > MAP_PRIVATE | MAP_ANONYMOUS, -1, 0);
> > > memset(p, 0xff, 8000 * MB);
> > > madvise(p, 8000 * MB, MADV_PAGEOUT);
> > > memset(p, 0xff, 8000 * MB);
> > > return 0;
> > > }
> > >
> > > Before:
> > > 7,021.43 msec task-clock # 0.967 CPUs utilized ( +- 0.03% )
> > > 4,010 context-switches # 573.853 /sec ( +- 0.01% )
> > > 0 cpu-migrations # 0.000 /sec
> > > 2,052,057 page-faults # 293.661 K/sec ( +- 0.00% )
> > > 12,616,546,027 cycles # 1.805 GHz ( +- 0.06% ) (39.92%)
> > > 156,823,666 stalled-cycles-frontend # 1.25% frontend cycles idle ( +- 0.10% ) (40.25%)
> > > 310,130,812 stalled-cycles-backend # 2.47% backend cycles idle ( +- 4.39% ) (40.73%)
> > > 18,692,516,591 instructions # 1.49 insn per cycle
> > > # 0.01 stalled cycles per insn ( +- 0.04% ) (40.75%)
> > > 4,907,447,976 branches # 702.283 M/sec ( +- 0.05% ) (40.30%)
> > > 13,002,578 branch-misses # 0.26% of all branches ( +- 0.08% ) (40.48%)
> > > 7,069,786,296 L1-dcache-loads # 1.012 G/sec ( +- 0.03% ) (40.32%)
> > > 649,385,847 L1-dcache-load-misses # 9.13% of all L1-dcache accesses ( +- 0.07% ) (40.10%)
> > > 1,485,448,688 L1-icache-loads # 212.576 M/sec ( +- 0.15% ) (39.49%)
> > > 31,628,457 L1-icache-load-misses # 2.13% of all L1-icache accesses ( +- 0.40% ) (39.57%)
> > > 6,667,311 dTLB-loads # 954.129 K/sec ( +- 0.21% ) (39.50%)
> > > 5,668,555 dTLB-load-misses # 86.40% of all dTLB cache accesses ( +- 0.12% ) (39.03%)
> > > 765 iTLB-loads # 109.476 /sec ( +- 21.81% ) (39.44%)
> > > 4,370,351 iTLB-load-misses # 214320.09% of all iTLB cache accesses ( +- 1.44% ) (39.86%)
> > > 149,207,254 L1-dcache-prefetches # 21.352 M/sec ( +- 0.13% ) (40.27%)
> > >
> > > 7.25869 +- 0.00203 seconds time elapsed ( +- 0.03% )
> > >
> > > After:
> > > 6,576.16 msec task-clock # 0.953 CPUs utilized ( +- 0.10% )
> > > 4,020 context-switches # 605.595 /sec ( +- 0.01% )
> > > 0 cpu-migrations # 0.000 /sec
> > > 2,052,056 page-faults # 309.133 K/sec ( +- 0.00% )
> > > 11,967,619,180 cycles # 1.803 GHz ( +- 0.36% ) (38.76%)
> > > 161,259,240 stalled-cycles-frontend # 1.38% frontend cycles idle ( +- 0.27% ) (36.58%)
> > > 253,605,302 stalled-cycles-backend # 2.16% backend cycles idle ( +- 4.45% ) (34.78%)
> > > 19,328,171,892 instructions # 1.65 insn per cycle
> > > # 0.01 stalled cycles per insn ( +- 0.10% ) (31.46%)
> > > 5,213,967,902 branches # 785.461 M/sec ( +- 0.18% ) (30.68%)
> > > 12,385,170 branch-misses # 0.24% of all branches ( +- 0.26% ) (34.13%)
> > > 7,271,687,822 L1-dcache-loads # 1.095 G/sec ( +- 0.12% ) (35.29%)
> > > 649,873,045 L1-dcache-load-misses # 8.93% of all L1-dcache accesses ( +- 0.11% ) (41.41%)
> > > 1,950,037,608 L1-icache-loads # 293.764 M/sec ( +- 0.33% ) (43.11%)
> > > 31,365,566 L1-icache-load-misses # 1.62% of all L1-icache accesses ( +- 0.39% ) (45.89%)
> > > 6,767,809 dTLB-loads # 1.020 M/sec ( +- 0.47% ) (48.42%)
> > > 6,339,590 dTLB-load-misses # 95.43% of all dTLB cache accesses ( +- 0.50% ) (46.60%)
> > > 736 iTLB-loads # 110.875 /sec ( +- 1.79% ) (48.60%)
> > > 4,314,836 iTLB-load-misses # 518653.73% of all iTLB cache accesses ( +- 0.63% ) (42.91%)
> > > 144,950,156 L1-dcache-prefetches # 21.836 M/sec ( +- 0.37% ) (41.39%)
> > >
> > > 6.89935 +- 0.00703 seconds time elapsed ( +- 0.10% )
> >
> > Do you happen to have a perf profile before and after to see which of
> > the paths really benefits from this?
>
> No I don't have a clear profile data about which path benefit the most.
> The performance benchmark result can be stably reproduced, but perf
> record & report & diff doesn't seems too helpful, as I can't see a
> significant change of any single symbols.

This is a good information on its own as it suggests that the overhead
is spilled over multiple places rather than a single hot spot. Good to
have in the changelog.

--
Michal Hocko
SUSE Labs

2022-08-30 18:11:30

by kernel test robot

[permalink] [raw]
Subject: Re: [PATCH 1/2] mm: memcontrol: remove mem_cgroup_kmem_disabled

Hi Kairui,

Thank you for the patch! Yet something to improve:

[auto build test ERROR on akpm-mm/mm-everything]

url: https://github.com/intel-lab-lkp/linux/commits/Kairui-Song/mm-memcontrol-cleanup-and-optimize-for-accounting-params/20220830-140150
base: https://git.kernel.org/pub/scm/linux/kernel/git/akpm/mm.git mm-everything
config: x86_64-randconfig-a004
compiler: gcc-11 (Debian 11.3.0-5) 11.3.0
reproduce (this is a W=1 build):
# https://github.com/intel-lab-lkp/linux/commit/45d0812323db1fbf1751cbd9d112f72f151ca3c6
git remote add linux-review https://github.com/intel-lab-lkp/linux
git fetch --no-tags linux-review Kairui-Song/mm-memcontrol-cleanup-and-optimize-for-accounting-params/20220830-140150
git checkout 45d0812323db1fbf1751cbd9d112f72f151ca3c6
# save the config file
mkdir build_dir && cp config build_dir/.config
make W=1 O=build_dir ARCH=x86_64 SHELL=/bin/bash

If you fix the issue, kindly add following tag where applicable
Reported-by: kernel test robot <[email protected]>

All errors (new ones prefixed by >>):

In file included from include/linux/static_key.h:1,
from arch/x86/include/asm/nospec-branch.h:6,
from arch/x86/include/asm/irqflags.h:9,
from include/linux/irqflags.h:16,
from include/linux/rcupdate.h:26,
from include/linux/rculist.h:11,
from include/linux/pid.h:5,
from include/linux/sched.h:14,
from include/linux/cgroup.h:12,
from include/linux/memcontrol.h:13,
from mm/memcontrol.c:29:
mm/memcontrol.c: In function 'cgroup_memory':
>> mm/memcontrol.c:7182:39: error: 'memcg_kmem_enabled_key' undeclared (first use in this function); did you mean 'memcg_kmem_enabled'?
7182 | static_branch_enable(&memcg_kmem_enabled_key);
| ^~~~~~~~~~~~~~~~~~~~~~
include/linux/jump_label.h:517:69: note: in definition of macro 'static_branch_enable'
517 | #define static_branch_enable(x) static_key_enable(&(x)->key)
| ^
mm/memcontrol.c:7182:39: note: each undeclared identifier is reported only once for each function it appears in
7182 | static_branch_enable(&memcg_kmem_enabled_key);
| ^~~~~~~~~~~~~~~~~~~~~~
include/linux/jump_label.h:517:69: note: in definition of macro 'static_branch_enable'
517 | #define static_branch_enable(x) static_key_enable(&(x)->key)
| ^


vim +7182 mm/memcontrol.c

7167
7168 static int __init cgroup_memory(char *s)
7169 {
7170 char *token;
7171
7172 while ((token = strsep(&s, ",")) != NULL) {
7173 if (!*token)
7174 continue;
7175 if (!strcmp(token, "nosocket"))
7176 cgroup_memory_nosocket = true;
7177 if (!strcmp(token, "nokmem"))
7178 cgroup_memory_nokmem = true;
7179 }
7180
7181 if (!cgroup_memory_nokmem)
> 7182 static_branch_enable(&memcg_kmem_enabled_key);
7183
7184 return 1;
7185 }
7186 __setup("cgroup.memory=", cgroup_memory);
7187

--
0-DAY CI Kernel Test Service
https://01.org/lkp


Attachments:
(No filename) (3.43 kB)
config (138.41 kB)
Download all attachments