* [RFC][PATCH 1/3] add cgroup_xxxx_disabled functions
2011-11-23 8:28 [RFC][PATCH 0/3] impelemnt cgroup_(subsys)_disabled in generic KAMEZAWA Hiroyuki
@ 2011-11-23 8:31 ` KAMEZAWA Hiroyuki
2011-11-23 8:32 ` [RFC][PATCH 2/3] use static_branch for cgroup_xxxx_disabled KAMEZAWA Hiroyuki
` (3 subsequent siblings)
4 siblings, 0 replies; 9+ messages in thread
From: KAMEZAWA Hiroyuki @ 2011-11-23 8:31 UTC (permalink / raw)
To: KAMEZAWA Hiroyuki
Cc: cgroups, linux-kernel@vger.kernel.org, Tejun Heo,
lizf@cn.fujitsu.com, hannes@cmpxchg.org, Michal Hocko,
bsingharora@gmail.com
>From 1a9e228c2a28bb918ea8cb9aecc11fd80444acaa Mon Sep 17 00:00:00 2001
From: KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com>
Date: Wed, 23 Nov 2011 15:36:22 +0900
Subject: [PATCH 1/3] add cgroup_xxxx_disabled() functions
Now, memory cgroup uses mem_cgroup_disabled() for checking
cgroup is disabled or not. This patch adds cgroup_xxxx_disabled()
functions for all subsys. After all works, mem_cgroup_disabled()
can be disappeared.
Signed-off-by: KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com>
---
include/linux/cgroup.h | 18 ++++++++++++++++++
include/linux/cgroup_subsys.h | 40 ++++++++++++++++++++++++++++++++++++++++
2 files changed, 58 insertions(+), 0 deletions(-)
diff --git a/include/linux/cgroup.h b/include/linux/cgroup.h
index 1b7f9d5..9eaa6fe 100644
--- a/include/linux/cgroup.h
+++ b/include/linux/cgroup.h
@@ -526,6 +526,16 @@ struct cgroup_subsys {
#include <linux/cgroup_subsys.h>
#undef SUBSYS
+#define SUBSYS(_x)\
+ static inline bool cgroup_ ## _x ## _disabled(void){\
+ return _x ## _subsys.disabled;}
+#define SUBSYS_UNDEFINED(_x)\
+ static inline bool cgroup_ ## _x ## _disabled(void){\
+ return true;}
+#include <linux/cgroup_subsys.h>
+#undef SUBSYS
+#undef SUBSYS_UNDEFINED
+
static inline struct cgroup_subsys_state *cgroup_subsys_state(
struct cgroup *cgrp, int subsys_id)
{
@@ -655,6 +665,14 @@ static inline int cgroup_attach_task_current_cg(struct task_struct *t)
return 0;
}
+#define SUBSYS(_x)
+#define SUBSYS_UNDEFINED(_x)\
+ static inline bool cgroup_ ## _x ## _disabled(void){\
+ return true;}
+#include <linux/cgroup_subsys.h>
+#undef SUBSYS
+#undef SUBSYS_UNDEFINED
+
#endif /* !CONFIG_CGROUPS */
#endif /* _LINUX_CGROUP_H */
diff --git a/include/linux/cgroup_subsys.h b/include/linux/cgroup_subsys.h
index ac663c1..f3eaee8 100644
--- a/include/linux/cgroup_subsys.h
+++ b/include/linux/cgroup_subsys.h
@@ -9,58 +9,98 @@
#ifdef CONFIG_CPUSETS
SUBSYS(cpuset)
+#else
+#ifdef SUBSYS_DEFINED
+SUBSYS_UNDEFINED(cpuset)
+#endif
#endif
/* */
#ifdef CONFIG_CGROUP_DEBUG
SUBSYS(debug)
+#else
+#ifdef SUBSYS_UNDEFINED
+SUBSYS_UNDEFINED(debug)
+#endif
#endif
/* */
#ifdef CONFIG_CGROUP_SCHED
SUBSYS(cpu_cgroup)
+#else
+#ifdef SUBSYS_UNDEFINED
+SUBSYS_UNDEFINED(cpu_cgroup)
+#endif
#endif
/* */
#ifdef CONFIG_CGROUP_CPUACCT
SUBSYS(cpuacct)
+#else
+#ifdef SUBSYS_UNDEFINED
+SUBSYS_UNDEFINED(cpuacct)
+#endif
#endif
/* */
#ifdef CONFIG_CGROUP_MEM_RES_CTLR
SUBSYS(mem_cgroup)
+#else
+#ifdef SUBSYS_UNDEFINED
+SUBSYS_UNDEFINED(mem_cgroup)
+#endif
#endif
/* */
#ifdef CONFIG_CGROUP_DEVICE
SUBSYS(devices)
+#else
+#ifdef SUBSYS_UNDEFINED
+SUBSYS_UNDEFINED(devices)
+#endif
#endif
/* */
#ifdef CONFIG_CGROUP_FREEZER
SUBSYS(freezer)
+#else
+#ifdef SUBSYS_UNDEFINED
+SUBSYS_UNDEFINED(freezer)
+#endif
#endif
/* */
#ifdef CONFIG_NET_CLS_CGROUP
SUBSYS(net_cls)
+#else
+#ifdef SUBSYS_DEFINED
+SUBSYS_UNDEFINED(net_cls)
+#endif
#endif
/* */
#ifdef CONFIG_BLK_CGROUP
SUBSYS(blkio)
+#else
+#ifdef SUBSYS_UNDEFINED
+SUBSYS_UNDEFINED(blkio)
+#endif
#endif
#ifdef CONFIG_CGROUP_PERF
SUBSYS(perf)
+#else
+#ifdef SUBSYS_UNDEFINED
+SUBSYS_UNDEFINED(perf)
+#endif
#endif
/* */
--
1.7.4.1
^ permalink raw reply related [flat|nested] 9+ messages in thread* [RFC][PATCH 2/3] use static_branch for cgroup_xxxx_disabled
2011-11-23 8:28 [RFC][PATCH 0/3] impelemnt cgroup_(subsys)_disabled in generic KAMEZAWA Hiroyuki
2011-11-23 8:31 ` [RFC][PATCH 1/3] add cgroup_xxxx_disabled functions KAMEZAWA Hiroyuki
@ 2011-11-23 8:32 ` KAMEZAWA Hiroyuki
2011-11-23 8:34 ` [RFC][PATCH 3/3] replace mem_cgroup_disabled KAMEZAWA Hiroyuki
` (2 subsequent siblings)
4 siblings, 0 replies; 9+ messages in thread
From: KAMEZAWA Hiroyuki @ 2011-11-23 8:32 UTC (permalink / raw)
To: KAMEZAWA Hiroyuki
Cc: cgroups, linux-kernel@vger.kernel.org, Tejun Heo,
lizf@cn.fujitsu.com, hannes@cmpxchg.org, Michal Hocko,
bsingharora@gmail.com
>From 7eae9a2735bf8df8a7e12f10dd7c27e729760196 Mon Sep 17 00:00:00 2001
From: KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com>
Date: Wed, 23 Nov 2011 15:53:18 +0900
Subject: [PATCH 2/3] use static_branch for cgroup_xxx_disabled
The behavior of cgroup_xxx_disable is determined at boot.
Then, using static_brach() will allow us binary patching rather
than checking global variable.
Signed-off-by: KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com>
---
include/linux/cgroup.h | 8 ++++++++
kernel/cgroup.c | 22 +++++++++++++++++++++-
2 files changed, 29 insertions(+), 1 deletions(-)
diff --git a/include/linux/cgroup.h b/include/linux/cgroup.h
index 9eaa6fe..511cb6a 100644
--- a/include/linux/cgroup.h
+++ b/include/linux/cgroup.h
@@ -16,6 +16,7 @@
#include <linux/prio_heap.h>
#include <linux/rwsem.h>
#include <linux/idr.h>
+#include <linux/jump_label.h>
#ifdef CONFIG_CGROUPS
@@ -526,9 +527,16 @@ struct cgroup_subsys {
#include <linux/cgroup_subsys.h>
#undef SUBSYS
+#ifdef CONFIG_JUMP_LABEL
+#define SUBSYS(_x)\
+ extern struct jump_label_key cgroup_ ## _x ## _disabled_key;\
+ static inline bool cgroup_ ## _x ## _disabled(void){\
+ return static_branch(&cgroup_ ## _x ## _disabled_key);}
+#else
#define SUBSYS(_x)\
static inline bool cgroup_ ## _x ## _disabled(void){\
return _x ## _subsys.disabled;}
+#endif
#define SUBSYS_UNDEFINED(_x)\
static inline bool cgroup_ ## _x ## _disabled(void){\
return true;}
diff --git a/kernel/cgroup.c b/kernel/cgroup.c
index d9d5648..28d4430 100644
--- a/kernel/cgroup.c
+++ b/kernel/cgroup.c
@@ -75,7 +75,7 @@ static DEFINE_MUTEX(cgroup_mutex);
static struct cgroup_subsys *subsys[CGROUP_SUBSYS_COUNT] = {
#include <linux/cgroup_subsys.h>
};
-
+#undef SUBSYS
#define MAX_CGROUP_ROOT_NAMELEN 64
/*
@@ -4776,6 +4776,25 @@ static void cgroup_release_agent(struct work_struct *work)
raw_spin_unlock(&release_list_lock);
mutex_unlock(&cgroup_mutex);
}
+#ifdef CONFIG_JUMP_LABEL
+#define SUBSYS(_x)\
+ struct jump_label_key cgroup_ ## _x ## _disable_key;
+#include <linux/cgroup_subsys.h>
+#undef SUBSYS
+
+static void cgroup_subsys_disable(void)
+{
+#define SUBSYS(_x)\
+ if ( _x ## _subsys.disabled)\
+ jump_label_inc(&cgroup_ ## _x ## _disable_key);
+#include <linux/cgroup_subsys.h>
+#undef SUBSYS
+}
+#else
+static void cgroup_subsys_disable(void)
+{
+}
+#endif
static int __init cgroup_disable(char *str)
{
@@ -4800,6 +4819,7 @@ static int __init cgroup_disable(char *str)
}
}
}
+ cgroup_subsys_disable();
return 1;
}
__setup("cgroup_disable=", cgroup_disable);
--
1.7.4.1
^ permalink raw reply related [flat|nested] 9+ messages in thread* [RFC][PATCH 3/3] replace mem_cgroup_disabled
2011-11-23 8:28 [RFC][PATCH 0/3] impelemnt cgroup_(subsys)_disabled in generic KAMEZAWA Hiroyuki
2011-11-23 8:31 ` [RFC][PATCH 1/3] add cgroup_xxxx_disabled functions KAMEZAWA Hiroyuki
2011-11-23 8:32 ` [RFC][PATCH 2/3] use static_branch for cgroup_xxxx_disabled KAMEZAWA Hiroyuki
@ 2011-11-23 8:34 ` KAMEZAWA Hiroyuki
2011-11-23 10:43 ` Glauber Costa
2011-11-23 10:32 ` [RFC][PATCH 0/3] impelemnt cgroup_(subsys)_disabled in generic Glauber Costa
2011-11-24 2:22 ` Li Zefan
4 siblings, 1 reply; 9+ messages in thread
From: KAMEZAWA Hiroyuki @ 2011-11-23 8:34 UTC (permalink / raw)
To: KAMEZAWA Hiroyuki
Cc: cgroups, linux-kernel@vger.kernel.org, Tejun Heo,
lizf@cn.fujitsu.com, hannes@cmpxchg.org, Michal Hocko,
bsingharora@gmail.com
I'll rebase this onto mmotm this is based on mainline git tree.
==
>From 2da35fd8eab3a8c2ca80d7aa5dfd4276a23ebf57 Mon Sep 17 00:00:00 2001
From: KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com>
Date: Wed, 23 Nov 2011 16:42:59 +0900
Subject: [PATCH 3/3] replace mem_cgroup_disabled().
cgroup provires cgroup_xxxx_disabled() functions for checking
subsys is diabled by boot option or not. Make use of it instead
of using private function.
Signed-off-by: KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com>
---
include/linux/memcontrol.h | 12 ------------
kernel/cgroup.c | 4 ++--
mm/memcontrol.c | 32 ++++++++++++++++----------------
mm/page_cgroup.c | 4 ++--
4 files changed, 20 insertions(+), 32 deletions(-)
diff --git a/include/linux/memcontrol.h b/include/linux/memcontrol.h
index b87068a..fa5712e 100644
--- a/include/linux/memcontrol.h
+++ b/include/linux/memcontrol.h
@@ -124,13 +124,6 @@ extern void mem_cgroup_print_oom_info(struct mem_cgroup *memcg,
extern int do_swap_account;
#endif
-static inline bool mem_cgroup_disabled(void)
-{
- if (mem_cgroup_subsys.disabled)
- return true;
- return false;
-}
-
void mem_cgroup_update_page_stat(struct page *page,
enum mem_cgroup_page_stat_item idx,
int val);
@@ -291,11 +284,6 @@ static inline void mem_cgroup_record_reclaim_priority(struct mem_cgroup *memcg,
{
}
-static inline bool mem_cgroup_disabled(void)
-{
- return true;
-}
-
static inline int
mem_cgroup_inactive_anon_is_low(struct mem_cgroup *memcg, struct zone *zone)
{
diff --git a/kernel/cgroup.c b/kernel/cgroup.c
index 28d4430..e5c33f5 100644
--- a/kernel/cgroup.c
+++ b/kernel/cgroup.c
@@ -4778,7 +4778,7 @@ static void cgroup_release_agent(struct work_struct *work)
}
#ifdef CONFIG_JUMP_LABEL
#define SUBSYS(_x)\
- struct jump_label_key cgroup_ ## _x ## _disable_key;
+ struct jump_label_key cgroup_ ## _x ## _disabled_key;
#include <linux/cgroup_subsys.h>
#undef SUBSYS
@@ -4786,7 +4786,7 @@ static void cgroup_subsys_disable(void)
{
#define SUBSYS(_x)\
if ( _x ## _subsys.disabled)\
- jump_label_inc(&cgroup_ ## _x ## _disable_key);
+ jump_label_inc(&cgroup_ ## _x ## _disabled_key);
#include <linux/cgroup_subsys.h>
#undef SUBSYS
}
diff --git a/mm/memcontrol.c b/mm/memcontrol.c
index 6aff93c..594af98 100644
--- a/mm/memcontrol.c
+++ b/mm/memcontrol.c
@@ -916,7 +916,7 @@ void mem_cgroup_del_lru_list(struct page *page, enum lru_list lru)
struct page_cgroup *pc;
struct mem_cgroup_per_zone *mz;
- if (mem_cgroup_disabled())
+ if (cgroup_mem_cgroup_disabled())
return;
pc = lookup_page_cgroup(page);
/* can happen while we handle swapcache. */
@@ -952,7 +952,7 @@ void mem_cgroup_rotate_reclaimable_page(struct page *page)
struct page_cgroup *pc;
enum lru_list lru = page_lru(page);
- if (mem_cgroup_disabled())
+ if (cgroup_mem_cgroup_disabled())
return;
pc = lookup_page_cgroup(page);
@@ -972,7 +972,7 @@ void mem_cgroup_rotate_lru_list(struct page *page, enum lru_list lru)
struct mem_cgroup_per_zone *mz;
struct page_cgroup *pc;
- if (mem_cgroup_disabled())
+ if (cgroup_mem_cgroup_disabled())
return;
pc = lookup_page_cgroup(page);
@@ -992,7 +992,7 @@ void mem_cgroup_add_lru_list(struct page *page, enum lru_list lru)
struct page_cgroup *pc;
struct mem_cgroup_per_zone *mz;
- if (mem_cgroup_disabled())
+ if (cgroup_mem_cgroup_disabled())
return;
pc = lookup_page_cgroup(page);
VM_BUG_ON(PageCgroupAcctLRU(pc));
@@ -1081,7 +1081,7 @@ static void mem_cgroup_lru_add_after_commit(struct page *page)
void mem_cgroup_move_lists(struct page *page,
enum lru_list from, enum lru_list to)
{
- if (mem_cgroup_disabled())
+ if (cgroup_mem_cgroup_disabled())
return;
mem_cgroup_del_lru_list(page, from);
mem_cgroup_add_lru_list(page, to);
@@ -1180,7 +1180,7 @@ mem_cgroup_get_reclaim_stat_from_page(struct page *page)
struct page_cgroup *pc;
struct mem_cgroup_per_zone *mz;
- if (mem_cgroup_disabled())
+ if (cgroup_mem_cgroup_disabled())
return NULL;
pc = lookup_page_cgroup(page);
@@ -2530,7 +2530,7 @@ void mem_cgroup_split_huge_fixup(struct page *head, struct page *tail)
struct page_cgroup *tail_pc = lookup_page_cgroup(tail);
unsigned long flags;
- if (mem_cgroup_disabled())
+ if (cgroup_mem_cgroup_disabled())
return;
/*
* We have no races with charge/uncharge but will have races with
@@ -2730,7 +2730,7 @@ static int mem_cgroup_charge_common(struct page *page, struct mm_struct *mm,
int mem_cgroup_newpage_charge(struct page *page,
struct mm_struct *mm, gfp_t gfp_mask)
{
- if (mem_cgroup_disabled())
+ if (cgroup_mem_cgroup_disabled())
return 0;
/*
* If already mapped, we don't have to account.
@@ -2773,7 +2773,7 @@ int mem_cgroup_cache_charge(struct page *page, struct mm_struct *mm,
struct mem_cgroup *memcg = NULL;
int ret;
- if (mem_cgroup_disabled())
+ if (cgroup_mem_cgroup_disabled())
return 0;
if (PageCompound(page))
return 0;
@@ -2823,7 +2823,7 @@ int mem_cgroup_try_charge_swapin(struct mm_struct *mm,
*ptr = NULL;
- if (mem_cgroup_disabled())
+ if (cgroup_mem_cgroup_disabled())
return 0;
if (!do_swap_account)
@@ -2853,7 +2853,7 @@ static void
__mem_cgroup_commit_charge_swapin(struct page *page, struct mem_cgroup *ptr,
enum charge_type ctype)
{
- if (mem_cgroup_disabled())
+ if (cgroup_mem_cgroup_disabled())
return;
if (!ptr)
return;
@@ -2903,7 +2903,7 @@ void mem_cgroup_commit_charge_swapin(struct page *page, struct mem_cgroup *ptr)
void mem_cgroup_cancel_charge_swapin(struct mem_cgroup *memcg)
{
- if (mem_cgroup_disabled())
+ if (cgroup_mem_cgroup_disabled())
return;
if (!memcg)
return;
@@ -2974,7 +2974,7 @@ __mem_cgroup_uncharge_common(struct page *page, enum charge_type ctype)
unsigned int nr_pages = 1;
struct page_cgroup *pc;
- if (mem_cgroup_disabled())
+ if (cgroup_mem_cgroup_disabled())
return NULL;
if (PageSwapCache(page))
@@ -3237,7 +3237,7 @@ int mem_cgroup_prepare_migration(struct page *page,
*ptr = NULL;
VM_BUG_ON(PageTransHuge(page));
- if (mem_cgroup_disabled())
+ if (cgroup_mem_cgroup_disabled())
return 0;
pc = lookup_page_cgroup(page);
@@ -3379,7 +3379,7 @@ static struct page_cgroup *lookup_page_cgroup_used(struct page *page)
bool mem_cgroup_bad_page_check(struct page *page)
{
- if (mem_cgroup_disabled())
+ if (cgroup_mem_cgroup_disabled())
return false;
return lookup_page_cgroup_used(page) != NULL;
@@ -4853,7 +4853,7 @@ static struct mem_cgroup *parent_mem_cgroup(struct mem_cgroup *memcg)
#ifdef CONFIG_CGROUP_MEM_RES_CTLR_SWAP
static void __init enable_swap_cgroup(void)
{
- if (!mem_cgroup_disabled() && really_do_swap_account)
+ if (!cgroup_mem_cgroup_disabled() && really_do_swap_account)
do_swap_account = 1;
}
#else
diff --git a/mm/page_cgroup.c b/mm/page_cgroup.c
index 2d123f9..a7bf01d 100644
--- a/mm/page_cgroup.c
+++ b/mm/page_cgroup.c
@@ -87,7 +87,7 @@ void __init page_cgroup_init_flatmem(void)
int nid, fail;
- if (mem_cgroup_disabled())
+ if (cgroup_mem_cgroup_disabled())
return;
for_each_online_node(nid) {
@@ -302,7 +302,7 @@ void __init page_cgroup_init(void)
unsigned long pfn;
int nid;
- if (mem_cgroup_disabled())
+ if (cgroup_mem_cgroup_disabled())
return;
for_each_node_state(nid, N_HIGH_MEMORY) {
--
1.7.4.1
^ permalink raw reply related [flat|nested] 9+ messages in thread* Re: [RFC][PATCH 3/3] replace mem_cgroup_disabled
2011-11-23 8:34 ` [RFC][PATCH 3/3] replace mem_cgroup_disabled KAMEZAWA Hiroyuki
@ 2011-11-23 10:43 ` Glauber Costa
2011-11-24 0:20 ` KAMEZAWA Hiroyuki
0 siblings, 1 reply; 9+ messages in thread
From: Glauber Costa @ 2011-11-23 10:43 UTC (permalink / raw)
To: KAMEZAWA Hiroyuki
Cc: cgroups, linux-kernel@vger.kernel.org, Tejun Heo,
lizf@cn.fujitsu.com, hannes@cmpxchg.org, Michal Hocko,
bsingharora@gmail.com
On 11/23/2011 06:34 AM, KAMEZAWA Hiroyuki wrote:
> I'll rebase this onto mmotm this is based on mainline git tree.
> ==
> From 2da35fd8eab3a8c2ca80d7aa5dfd4276a23ebf57 Mon Sep 17 00:00:00 2001
> From: KAMEZAWA Hiroyuki<kamezawa.hiroyu@jp.fujitsu.com>
> Date: Wed, 23 Nov 2011 16:42:59 +0900
> Subject: [PATCH 3/3] replace mem_cgroup_disabled().
>
> cgroup provires cgroup_xxxx_disabled() functions for checking
> subsys is diabled by boot option or not. Make use of it instead
> of using private function.
>
> Signed-off-by: KAMEZAWA Hiroyuki<kamezawa.hiroyu@jp.fujitsu.com>
> ---
> include/linux/memcontrol.h | 12 ------------
> kernel/cgroup.c | 4 ++--
> mm/memcontrol.c | 32 ++++++++++++++++----------------
> mm/page_cgroup.c | 4 ++--
> 4 files changed, 20 insertions(+), 32 deletions(-)
>
> diff --git a/kernel/cgroup.c b/kernel/cgroup.c
> index 28d4430..e5c33f5 100644
> --- a/kernel/cgroup.c
> +++ b/kernel/cgroup.c
> @@ -4778,7 +4778,7 @@ static void cgroup_release_agent(struct work_struct *work)
> }
> #ifdef CONFIG_JUMP_LABEL
> #define SUBSYS(_x)\
> - struct jump_label_key cgroup_ ## _x ## _disable_key;
> + struct jump_label_key cgroup_ ## _x ## _disabled_key;
> #include<linux/cgroup_subsys.h>
> #undef SUBSYS
I have the impression this is just churn. Can you call it disabled_key
from the beginning ?
> @@ -4786,7 +4786,7 @@ static void cgroup_subsys_disable(void)
> {
> #define SUBSYS(_x)\
> if ( _x ## _subsys.disabled)\
> - jump_label_inc(&cgroup_ ## _x ## _disable_key);
> + jump_label_inc(&cgroup_ ## _x ## _disabled_key);
> #include<linux/cgroup_subsys.h>
> #undef SUBSYS
> }
> diff --git a/mm/memcontrol.c b/mm/memcontrol.c
> index 6aff93c..594af98 100644
> --- a/mm/memcontrol.c
> +++ b/mm/memcontrol.c
> @@ -916,7 +916,7 @@ void mem_cgroup_del_lru_list(struct page *page, enum lru_list lru)
> struct page_cgroup *pc;
> struct mem_cgroup_per_zone *mz;
>
> - if (mem_cgroup_disabled())
> + if (cgroup_mem_cgroup_disabled())
> return;
> pc = lookup_page_cgroup(page);
> /* can happen while we handle swapcache. */
> @@ -952,7 +952,7 @@ void mem_cgroup_rotate_reclaimable_page(struct page *page)
> struct page_cgroup *pc;
> enum lru_list lru = page_lru(page);
>
> - if (mem_cgroup_disabled())
> + if (cgroup_mem_cgroup_disabled())
> return;
>
> pc = lookup_page_cgroup(page);
In many cases, this will be just a useless function call. Wouldn't it be
better if in the disabled case we would not even call those functions?
It may help some fast paths.
We could define inline versions in memcontrol.h in place of the function
signatures themselves, and have those to test for the static branch.
Maybe a macro can be used to make it less laborious?
^ permalink raw reply [flat|nested] 9+ messages in thread* Re: [RFC][PATCH 3/3] replace mem_cgroup_disabled
2011-11-23 10:43 ` Glauber Costa
@ 2011-11-24 0:20 ` KAMEZAWA Hiroyuki
0 siblings, 0 replies; 9+ messages in thread
From: KAMEZAWA Hiroyuki @ 2011-11-24 0:20 UTC (permalink / raw)
To: Glauber Costa
Cc: cgroups, linux-kernel@vger.kernel.org, Tejun Heo,
lizf@cn.fujitsu.com, hannes@cmpxchg.org, Michal Hocko,
bsingharora@gmail.com
On Wed, 23 Nov 2011 08:43:15 -0200
Glauber Costa <glommer@parallels.com> wrote:
> On 11/23/2011 06:34 AM, KAMEZAWA Hiroyuki wrote:
> > I'll rebase this onto mmotm this is based on mainline git tree.
> > ==
> > From 2da35fd8eab3a8c2ca80d7aa5dfd4276a23ebf57 Mon Sep 17 00:00:00 2001
> > From: KAMEZAWA Hiroyuki<kamezawa.hiroyu@jp.fujitsu.com>
> > Date: Wed, 23 Nov 2011 16:42:59 +0900
> > Subject: [PATCH 3/3] replace mem_cgroup_disabled().
> >
> > cgroup provires cgroup_xxxx_disabled() functions for checking
> > subsys is diabled by boot option or not. Make use of it instead
> > of using private function.
> >
> > Signed-off-by: KAMEZAWA Hiroyuki<kamezawa.hiroyu@jp.fujitsu.com>
> > ---
> > include/linux/memcontrol.h | 12 ------------
> > kernel/cgroup.c | 4 ++--
> > mm/memcontrol.c | 32 ++++++++++++++++----------------
> > mm/page_cgroup.c | 4 ++--
> > 4 files changed, 20 insertions(+), 32 deletions(-)
> >
>
> > diff --git a/kernel/cgroup.c b/kernel/cgroup.c
> > index 28d4430..e5c33f5 100644
> > --- a/kernel/cgroup.c
> > +++ b/kernel/cgroup.c
> > @@ -4778,7 +4778,7 @@ static void cgroup_release_agent(struct work_struct *work)
> > }
> > #ifdef CONFIG_JUMP_LABEL
> > #define SUBSYS(_x)\
> > - struct jump_label_key cgroup_ ## _x ## _disable_key;
> > + struct jump_label_key cgroup_ ## _x ## _disabled_key;
> > #include<linux/cgroup_subsys.h>
> > #undef SUBSYS
>
> I have the impression this is just churn. Can you call it disabled_key
> from the beginning ?
>
Ouch, I made a mistake at commiting and fix to previous commit sneaked
into here. Thank you for review, I'll do fix.
> > @@ -4786,7 +4786,7 @@ static void cgroup_subsys_disable(void)
> > {
> > #define SUBSYS(_x)\
> > if ( _x ## _subsys.disabled)\
> > - jump_label_inc(&cgroup_ ## _x ## _disable_key);
> > + jump_label_inc(&cgroup_ ## _x ## _disabled_key);
> > #include<linux/cgroup_subsys.h>
> > #undef SUBSYS
> > }
> > diff --git a/mm/memcontrol.c b/mm/memcontrol.c
> > index 6aff93c..594af98 100644
> > --- a/mm/memcontrol.c
> > +++ b/mm/memcontrol.c
> > @@ -916,7 +916,7 @@ void mem_cgroup_del_lru_list(struct page *page, enum lru_list lru)
> > struct page_cgroup *pc;
> > struct mem_cgroup_per_zone *mz;
> >
> > - if (mem_cgroup_disabled())
> > + if (cgroup_mem_cgroup_disabled())
> > return;
> > pc = lookup_page_cgroup(page);
> > /* can happen while we handle swapcache. */
> > @@ -952,7 +952,7 @@ void mem_cgroup_rotate_reclaimable_page(struct page *page)
> > struct page_cgroup *pc;
> > enum lru_list lru = page_lru(page);
> >
> > - if (mem_cgroup_disabled())
> > + if (cgroup_mem_cgroup_disabled())
> > return;
> >
> > pc = lookup_page_cgroup(page);
>
> In many cases, this will be just a useless function call. Wouldn't it be
> better if in the disabled case we would not even call those functions?
> It may help some fast paths.
>
Hm ? diassembled code is like this.
Dump of assembler code for function mem_cgroup_rotate_reclaimable_page:
0xffffffff8116c460 <+0>: push %rbp
0xffffffff8116c461 <+1>: mov %rsp,%rbp
0xffffffff8116c464 <+4>: sub $0x10,%rsp
0xffffffff8116c468 <+8>: mov %rbx,(%rsp)
0xffffffff8116c46c <+12>: mov %r12,0x8(%rsp)
0xffffffff8116c471 <+17>: callq 0xffffffff815c59c0
0xffffffff8116c476 <+22>: mov (%rdi),%rax
0xffffffff8116c479 <+25>: mov $0x4,%ebx
0xffffffff8116c47e <+30>: mov %rdi,%r12
0xffffffff8116c481 <+33>: test $0x100000,%eax
0xffffffff8116c486 <+38>: jne 0xffffffff8116c4a6 <mem_cgroup_rotate_reclaimable_page+70>
0xffffffff8116c488 <+40>: mov (%rdi),%rax
0xffffffff8116c48b <+43>: and $0x80000,%eax
0xffffffff8116c490 <+48>: cmp $0x1,%rax
0xffffffff8116c494 <+52>: mov (%rdi),%rax
0xffffffff8116c497 <+55>: sbb %ebx,%ebx
0xffffffff8116c499 <+57>: and $0x2,%ebx
0xffffffff8116c49c <+60>: and $0x40,%eax
0xffffffff8116c49f <+63>: cmp $0x1,%rax
0xffffffff8116c4a3 <+67>: sbb $0xffffffffffffffff,%ebx
0xffffffff8116c4a6 <+70>: jmpq 0xffffffff8116c4ab <mem_cgroup_rotate_reclaimable_page+75>
0xffffffff8116c4ab <+75>: mov %r12,%rdi
0xffffffff8116c4ae <+78>: callq 0xffffffff81173740 <lookup_page_cgroup>
0xffffffff8116c4a6 is the jump label I used.
jump labels in static inline functions seems to work as expected.
and no function calls. This seems good.
Thanks,
-Kame
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [RFC][PATCH 0/3] impelemnt cgroup_(subsys)_disabled in generic.
2011-11-23 8:28 [RFC][PATCH 0/3] impelemnt cgroup_(subsys)_disabled in generic KAMEZAWA Hiroyuki
` (2 preceding siblings ...)
2011-11-23 8:34 ` [RFC][PATCH 3/3] replace mem_cgroup_disabled KAMEZAWA Hiroyuki
@ 2011-11-23 10:32 ` Glauber Costa
2011-11-24 2:22 ` Li Zefan
4 siblings, 0 replies; 9+ messages in thread
From: Glauber Costa @ 2011-11-23 10:32 UTC (permalink / raw)
To: KAMEZAWA Hiroyuki
Cc: cgroups, linux-kernel@vger.kernel.org, Tejun Heo,
lizf@cn.fujitsu.com, hannes@cmpxchg.org, Michal Hocko,
bsingharora@gmail.com
On 11/23/2011 06:28 AM, KAMEZAWA Hiroyuki wrote:
>
> Now, memory cgroup has 'mem_cgroup_disabled()' in memcontrol.h
>
> I made a brief trial to use static_branch() for that function. At doing that,
> I thought it will be better to implement generic cgroup functions rather
> than having memory cgroup's its own one.
>
> This series consists of 3 patches
> 1 .... implement cgroup_xxxx_disabled() in generic.
> 2 .... use jump_label for cgroup_xxxx_disabled()
> 3 .... remove mem_cgroup_disabled() in memcontrol.c
>
> And I post this series for getting review/comments.
> I'm not sure patches for using jump_label is worth to be merged.
>
> I did a test to run a loop
> while(-) {
> mmap(1M)
> touch all pages
> munmap()
> }
>
> and measured performance score in ROOT cgroup. Here,
>
> (Before patch)
> 182,932,842,128 cycles # 0.000 GHz [33.33%]
> 192,711,643,877 instructions # 1.05 insns per cycle [49.99%]
> 761,483,416 cache-references [49.98%]
> 159,908 cache-misses # 0.021 % of all cache refs [50.00%]
> 33,253,084,874 branches [33.34%]
> 109,796,792 branch-misses # 0.33% of all branches [33.34%]
>
> 58.289265709 seconds time elapsed
>
> (After patch)
> Performance counter stats for './malloc 1':
>
> 183,068,407,487 cycles # 0.000 GHz [33.33%]
> 191,834,248,678 instructions # 1.05 insns per cycle [50.00%]
> 798,635,028 cache-references [49.98%]
> 95,562 cache-misses # 0.012 % of all cache refs [50.00%]
> 32,755,318,286 branches [33.34%]
> 77,774,624 branch-misses # 0.24% of all branches [33.34%]
>
> 58.332356996 seconds time elapsed
>
> There is no differece in 'time' ;)
> But I got an impression that 'branch' score gets better in several tests.
>
branch and cache misses are a lot smaller as well. I think this is a win.
^ permalink raw reply [flat|nested] 9+ messages in thread* Re: [RFC][PATCH 0/3] impelemnt cgroup_(subsys)_disabled in generic.
2011-11-23 8:28 [RFC][PATCH 0/3] impelemnt cgroup_(subsys)_disabled in generic KAMEZAWA Hiroyuki
` (3 preceding siblings ...)
2011-11-23 10:32 ` [RFC][PATCH 0/3] impelemnt cgroup_(subsys)_disabled in generic Glauber Costa
@ 2011-11-24 2:22 ` Li Zefan
2011-11-24 3:18 ` KAMEZAWA Hiroyuki
4 siblings, 1 reply; 9+ messages in thread
From: Li Zefan @ 2011-11-24 2:22 UTC (permalink / raw)
To: KAMEZAWA Hiroyuki
Cc: cgroups, linux-kernel@vger.kernel.org, Tejun Heo,
hannes@cmpxchg.org, Michal Hocko, bsingharora@gmail.com
KAMEZAWA Hiroyuki wrote:
>
> Now, memory cgroup has 'mem_cgroup_disabled()' in memcontrol.h
>
> I made a brief trial to use static_branch() for that function. At doing that,
> I thought it will be better to implement generic cgroup functions rather
> than having memory cgroup's its own one.
>
> This series consists of 3 patches
> 1 .... implement cgroup_xxxx_disabled() in generic.
> 2 .... use jump_label for cgroup_xxxx_disabled()
> 3 .... remove mem_cgroup_disabled() in memcontrol.c
>
> And I post this series for getting review/comments.
> I'm not sure patches for using jump_label is worth to be merged.
>
> I did a test to run a loop
> while(-) {
> mmap(1M)
> touch all pages
> munmap()
> }
>
> and measured performance score in ROOT cgroup. Here,
>
> (Before patch)
> 182,932,842,128 cycles # 0.000 GHz [33.33%]
> 192,711,643,877 instructions # 1.05 insns per cycle [49.99%]
> 761,483,416 cache-references [49.98%]
> 159,908 cache-misses # 0.021 % of all cache refs [50.00%]
> 33,253,084,874 branches [33.34%]
> 109,796,792 branch-misses # 0.33% of all branches [33.34%]
>
> 58.289265709 seconds time elapsed
>
> (After patch)
> Performance counter stats for './malloc 1':
>
> 183,068,407,487 cycles # 0.000 GHz [33.33%]
> 191,834,248,678 instructions # 1.05 insns per cycle [50.00%]
> 798,635,028 cache-references [49.98%]
> 95,562 cache-misses # 0.012 % of all cache refs [50.00%]
> 32,755,318,286 branches [33.34%]
> 77,774,624 branch-misses # 0.24% of all branches [33.34%]
>
> 58.332356996 seconds time elapsed
>
> There is no differece in 'time' ;)
> But I got an impression that 'branch' score gets better in several tests.
>
> Thanks,
> -Kame
>
> P.S. maybe I can replace 'do_swap_account' with jump_label, too.
>
The numbers sugguest using jump label is a win.
However I'm not quite convinced that we make it generic. The subsys.disabled flag
was introduced long ago for memcg, but yet it has no other users.
So maybe for now make use of jump label in memcg only? We probably still needs
a bit help from cgroup core, to provide a subsys->disable() callback.
--
Li Zefan
^ permalink raw reply [flat|nested] 9+ messages in thread* Re: [RFC][PATCH 0/3] impelemnt cgroup_(subsys)_disabled in generic.
2011-11-24 2:22 ` Li Zefan
@ 2011-11-24 3:18 ` KAMEZAWA Hiroyuki
0 siblings, 0 replies; 9+ messages in thread
From: KAMEZAWA Hiroyuki @ 2011-11-24 3:18 UTC (permalink / raw)
To: Li Zefan
Cc: cgroups, linux-kernel@vger.kernel.org, Tejun Heo,
hannes@cmpxchg.org, Michal Hocko, bsingharora@gmail.com
On Thu, 24 Nov 2011 10:22:13 +0800
Li Zefan <lizf@cn.fujitsu.com> wrote:
> KAMEZAWA Hiroyuki wrote:
> >
> > Now, memory cgroup has 'mem_cgroup_disabled()' in memcontrol.h
> >
> > I made a brief trial to use static_branch() for that function. At doing that,
> > I thought it will be better to implement generic cgroup functions rather
> > than having memory cgroup's its own one.
> >
> > This series consists of 3 patches
> > 1 .... implement cgroup_xxxx_disabled() in generic.
> > 2 .... use jump_label for cgroup_xxxx_disabled()
> > 3 .... remove mem_cgroup_disabled() in memcontrol.c
> >
> > And I post this series for getting review/comments.
> > I'm not sure patches for using jump_label is worth to be merged.
> >
> > I did a test to run a loop
> > while(-) {
> > mmap(1M)
> > touch all pages
> > munmap()
> > }
> >
> > and measured performance score in ROOT cgroup. Here,
> >
> > (Before patch)
> > 182,932,842,128 cycles # 0.000 GHz [33.33%]
> > 192,711,643,877 instructions # 1.05 insns per cycle [49.99%]
> > 761,483,416 cache-references [49.98%]
> > 159,908 cache-misses # 0.021 % of all cache refs [50.00%]
> > 33,253,084,874 branches [33.34%]
> > 109,796,792 branch-misses # 0.33% of all branches [33.34%]
> >
> > 58.289265709 seconds time elapsed
> >
> > (After patch)
> > Performance counter stats for './malloc 1':
> >
> > 183,068,407,487 cycles # 0.000 GHz [33.33%]
> > 191,834,248,678 instructions # 1.05 insns per cycle [50.00%]
> > 798,635,028 cache-references [49.98%]
> > 95,562 cache-misses # 0.012 % of all cache refs [50.00%]
> > 32,755,318,286 branches [33.34%]
> > 77,774,624 branch-misses # 0.24% of all branches [33.34%]
> >
> > 58.332356996 seconds time elapsed
> >
> > There is no differece in 'time' ;)
> > But I got an impression that 'branch' score gets better in several tests.
> >
> > Thanks,
> > -Kame
> >
> > P.S. maybe I can replace 'do_swap_account' with jump_label, too.
> >
>
> The numbers sugguest using jump label is a win.
>
> However I'm not quite convinced that we make it generic. The subsys.disabled flag
> was introduced long ago for memcg, but yet it has no other users.
>
We have generic cgroup_disable=xxxx boot options. But yes, it seems only memory cgroup
checks it in the hooks. (But I'm not sure it's not necessary to be checked..)
> So maybe for now make use of jump label in memcg only? We probably still needs
> a bit help from cgroup core, to provide a subsys->disable() callback.
>
Hm, ok. if this is not welcomed, I'll keep this only in memory cgroup and don't
touch cgroup.h.
Thank you for review.
Regards,
-Kame
^ permalink raw reply [flat|nested] 9+ messages in thread