* [patch] mm, memcg: avoid unnecessary function call when memcg is disabled
@ 2012-11-20 1:44 David Rientjes
2012-11-20 7:07 ` Michal Hocko
[not found] ` <alpine.DEB.2.00.1211191741060.24618-X6Q0R45D7oAcqpCFd4KODRPsWskHk0ljAL8bYrjMMd8@public.gmane.org>
0 siblings, 2 replies; 12+ messages in thread
From: David Rientjes @ 2012-11-20 1:44 UTC (permalink / raw)
To: Andrew Morton
Cc: Johannes Weiner, Michal Hocko, KAMEZAWA Hiroyuki, Hugh Dickins,
linux-kernel-u79uwXL29TY76Z2rM5mHXA,
cgroups-u79uwXL29TY76Z2rM5mHXA, linux-mm-Bw31MaZKKs3YtjvyW6yDsg
While profiling numa/core v16 with cgroup_disable=memory on the command
line, I noticed mem_cgroup_count_vm_event() still showed up as high as
0.60% in perftop.
This occurs because the function is called extremely often even when memcg
is disabled.
To fix this, inline the check for mem_cgroup_disabled() so we avoid the
unnecessary function call if memcg is disabled.
Signed-off-by: David Rientjes <rientjes-hpIqsD4AKlfQT0dZR+AlfA@public.gmane.org>
---
include/linux/memcontrol.h | 9 ++++++++-
mm/memcontrol.c | 9 ++++-----
2 files changed, 12 insertions(+), 6 deletions(-)
diff --git a/include/linux/memcontrol.h b/include/linux/memcontrol.h
--- a/include/linux/memcontrol.h
+++ b/include/linux/memcontrol.h
@@ -181,7 +181,14 @@ unsigned long mem_cgroup_soft_limit_reclaim(struct zone *zone, int order,
gfp_t gfp_mask,
unsigned long *total_scanned);
-void mem_cgroup_count_vm_event(struct mm_struct *mm, enum vm_event_item idx);
+void __mem_cgroup_count_vm_event(struct mm_struct *mm, enum vm_event_item idx);
+static inline void mem_cgroup_count_vm_event(struct mm_struct *mm,
+ enum vm_event_item idx)
+{
+ if (mem_cgroup_disabled() || !mm)
+ return;
+ __mem_cgroup_count_vm_event(mm, idx);
+}
#ifdef CONFIG_TRANSPARENT_HUGEPAGE
void mem_cgroup_split_huge_fixup(struct page *head);
#endif
diff --git a/mm/memcontrol.c b/mm/memcontrol.c
--- a/mm/memcontrol.c
+++ b/mm/memcontrol.c
@@ -59,6 +59,8 @@
#include <trace/events/vmscan.h>
struct cgroup_subsys mem_cgroup_subsys __read_mostly;
+EXPORT_SYMBOL(mem_cgroup_subsys);
+
#define MEM_CGROUP_RECLAIM_RETRIES 5
static struct mem_cgroup *root_mem_cgroup __read_mostly;
@@ -1015,13 +1017,10 @@ void mem_cgroup_iter_break(struct mem_cgroup *root,
iter != NULL; \
iter = mem_cgroup_iter(NULL, iter, NULL))
-void mem_cgroup_count_vm_event(struct mm_struct *mm, enum vm_event_item idx)
+void __mem_cgroup_count_vm_event(struct mm_struct *mm, enum vm_event_item idx)
{
struct mem_cgroup *memcg;
- if (!mm)
- return;
-
rcu_read_lock();
memcg = mem_cgroup_from_task(rcu_dereference(mm->owner));
if (unlikely(!memcg))
@@ -1040,7 +1039,7 @@ void mem_cgroup_count_vm_event(struct mm_struct *mm, enum vm_event_item idx)
out:
rcu_read_unlock();
}
-EXPORT_SYMBOL(mem_cgroup_count_vm_event);
+EXPORT_SYMBOL(__mem_cgroup_count_vm_event);
/**
* mem_cgroup_zone_lruvec - get the lru list vector for a zone and memcg
^ permalink raw reply [flat|nested] 12+ messages in thread* Re: [patch] mm, memcg: avoid unnecessary function call when memcg is disabled 2012-11-20 1:44 [patch] mm, memcg: avoid unnecessary function call when memcg is disabled David Rientjes @ 2012-11-20 7:07 ` Michal Hocko [not found] ` <alpine.DEB.2.00.1211191741060.24618-X6Q0R45D7oAcqpCFd4KODRPsWskHk0ljAL8bYrjMMd8@public.gmane.org> 1 sibling, 0 replies; 12+ messages in thread From: Michal Hocko @ 2012-11-20 7:07 UTC (permalink / raw) To: David Rientjes Cc: Andrew Morton, Johannes Weiner, KAMEZAWA Hiroyuki, Hugh Dickins, linux-kernel, cgroups, linux-mm On Mon 19-11-12 17:44:34, David Rientjes wrote: > While profiling numa/core v16 with cgroup_disable=memory on the command > line, I noticed mem_cgroup_count_vm_event() still showed up as high as > 0.60% in perftop. > > This occurs because the function is called extremely often even when memcg > is disabled. > > To fix this, inline the check for mem_cgroup_disabled() so we avoid the > unnecessary function call if memcg is disabled. > > Signed-off-by: David Rientjes <rientjes@google.com> Acked-by: Michal Hocko <mhocko@suse.cz> Thanks! > --- > include/linux/memcontrol.h | 9 ++++++++- > mm/memcontrol.c | 9 ++++----- > 2 files changed, 12 insertions(+), 6 deletions(-) > > diff --git a/include/linux/memcontrol.h b/include/linux/memcontrol.h > --- a/include/linux/memcontrol.h > +++ b/include/linux/memcontrol.h > @@ -181,7 +181,14 @@ unsigned long mem_cgroup_soft_limit_reclaim(struct zone *zone, int order, > gfp_t gfp_mask, > unsigned long *total_scanned); > > -void mem_cgroup_count_vm_event(struct mm_struct *mm, enum vm_event_item idx); > +void __mem_cgroup_count_vm_event(struct mm_struct *mm, enum vm_event_item idx); > +static inline void mem_cgroup_count_vm_event(struct mm_struct *mm, > + enum vm_event_item idx) > +{ > + if (mem_cgroup_disabled() || !mm) > + return; > + __mem_cgroup_count_vm_event(mm, idx); > +} > #ifdef CONFIG_TRANSPARENT_HUGEPAGE > void mem_cgroup_split_huge_fixup(struct page *head); > #endif > diff --git a/mm/memcontrol.c b/mm/memcontrol.c > --- a/mm/memcontrol.c > +++ b/mm/memcontrol.c > @@ -59,6 +59,8 @@ > #include <trace/events/vmscan.h> > > struct cgroup_subsys mem_cgroup_subsys __read_mostly; > +EXPORT_SYMBOL(mem_cgroup_subsys); > + > #define MEM_CGROUP_RECLAIM_RETRIES 5 > static struct mem_cgroup *root_mem_cgroup __read_mostly; > > @@ -1015,13 +1017,10 @@ void mem_cgroup_iter_break(struct mem_cgroup *root, > iter != NULL; \ > iter = mem_cgroup_iter(NULL, iter, NULL)) > > -void mem_cgroup_count_vm_event(struct mm_struct *mm, enum vm_event_item idx) > +void __mem_cgroup_count_vm_event(struct mm_struct *mm, enum vm_event_item idx) > { > struct mem_cgroup *memcg; > > - if (!mm) > - return; > - > rcu_read_lock(); > memcg = mem_cgroup_from_task(rcu_dereference(mm->owner)); > if (unlikely(!memcg)) > @@ -1040,7 +1039,7 @@ void mem_cgroup_count_vm_event(struct mm_struct *mm, enum vm_event_item idx) > out: > rcu_read_unlock(); > } > -EXPORT_SYMBOL(mem_cgroup_count_vm_event); > +EXPORT_SYMBOL(__mem_cgroup_count_vm_event); > > /** > * mem_cgroup_zone_lruvec - get the lru list vector for a zone and memcg > -- > To unsubscribe from this list: send the line "unsubscribe cgroups" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html -- Michal Hocko SUSE Labs -- To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to majordomo@kvack.org. For more info on Linux MM, see: http://www.linux-mm.org/ . Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a> ^ permalink raw reply [flat|nested] 12+ messages in thread
[parent not found: <alpine.DEB.2.00.1211191741060.24618-X6Q0R45D7oAcqpCFd4KODRPsWskHk0ljAL8bYrjMMd8@public.gmane.org>]
* Re: [patch] mm, memcg: avoid unnecessary function call when memcg is disabled [not found] ` <alpine.DEB.2.00.1211191741060.24618-X6Q0R45D7oAcqpCFd4KODRPsWskHk0ljAL8bYrjMMd8@public.gmane.org> @ 2012-11-20 4:23 ` Kamezawa Hiroyuki [not found] ` <50AB05B4.4000303-+CUm20s59erQFUHtdCDX3A@public.gmane.org> 2012-11-20 17:27 ` Johannes Weiner 2012-11-20 21:49 ` Andrew Morton 2 siblings, 1 reply; 12+ messages in thread From: Kamezawa Hiroyuki @ 2012-11-20 4:23 UTC (permalink / raw) To: David Rientjes Cc: Andrew Morton, Johannes Weiner, Michal Hocko, Hugh Dickins, linux-kernel-u79uwXL29TY76Z2rM5mHXA, cgroups-u79uwXL29TY76Z2rM5mHXA, linux-mm-Bw31MaZKKs3YtjvyW6yDsg (2012/11/20 10:44), David Rientjes wrote: > While profiling numa/core v16 with cgroup_disable=memory on the command > line, I noticed mem_cgroup_count_vm_event() still showed up as high as > 0.60% in perftop. > > This occurs because the function is called extremely often even when memcg > is disabled. > > To fix this, inline the check for mem_cgroup_disabled() so we avoid the > unnecessary function call if memcg is disabled. > > Signed-off-by: David Rientjes <rientjes-hpIqsD4AKlfQT0dZR+AlfA@public.gmane.org> Acked-By: KAMEZAWA Hiroyuki <kamezawa.hiroyu-+CUm20s59erQFUHtdCDX3A@public.gmane.org> ^ permalink raw reply [flat|nested] 12+ messages in thread
[parent not found: <50AB05B4.4000303-+CUm20s59erQFUHtdCDX3A@public.gmane.org>]
* Re: [patch] mm, memcg: avoid unnecessary function call when memcg is disabled [not found] ` <50AB05B4.4000303-+CUm20s59erQFUHtdCDX3A@public.gmane.org> @ 2012-11-20 8:34 ` Glauber Costa 0 siblings, 0 replies; 12+ messages in thread From: Glauber Costa @ 2012-11-20 8:34 UTC (permalink / raw) To: Kamezawa Hiroyuki Cc: David Rientjes, Andrew Morton, Johannes Weiner, Michal Hocko, Hugh Dickins, linux-kernel-u79uwXL29TY76Z2rM5mHXA, cgroups-u79uwXL29TY76Z2rM5mHXA, linux-mm-Bw31MaZKKs3YtjvyW6yDsg On 11/20/2012 08:23 AM, Kamezawa Hiroyuki wrote: > (2012/11/20 10:44), David Rientjes wrote: >> While profiling numa/core v16 with cgroup_disable=memory on the command >> line, I noticed mem_cgroup_count_vm_event() still showed up as high as >> 0.60% in perftop. >> >> This occurs because the function is called extremely often even when >> memcg >> is disabled. >> >> To fix this, inline the check for mem_cgroup_disabled() so we avoid the >> unnecessary function call if memcg is disabled. >> >> Signed-off-by: David Rientjes <rientjes-hpIqsD4AKlfQT0dZR+AlfA@public.gmane.org> > > Acked-By: KAMEZAWA Hiroyuki <kamezawa.hiroyu-+CUm20s59erQFUHtdCDX3A@public.gmane.org> > I am fine with this as well. Acked-by: Glauber Costa <glommer-bzQdu9zFT3WakBO8gow8eQ@public.gmane.org> ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [patch] mm, memcg: avoid unnecessary function call when memcg is disabled [not found] ` <alpine.DEB.2.00.1211191741060.24618-X6Q0R45D7oAcqpCFd4KODRPsWskHk0ljAL8bYrjMMd8@public.gmane.org> 2012-11-20 4:23 ` Kamezawa Hiroyuki @ 2012-11-20 17:27 ` Johannes Weiner 2012-11-20 21:49 ` Andrew Morton 2 siblings, 0 replies; 12+ messages in thread From: Johannes Weiner @ 2012-11-20 17:27 UTC (permalink / raw) To: David Rientjes Cc: Andrew Morton, Michal Hocko, KAMEZAWA Hiroyuki, Hugh Dickins, linux-kernel-u79uwXL29TY76Z2rM5mHXA, cgroups-u79uwXL29TY76Z2rM5mHXA, linux-mm-Bw31MaZKKs3YtjvyW6yDsg On Mon, Nov 19, 2012 at 05:44:34PM -0800, David Rientjes wrote: > While profiling numa/core v16 with cgroup_disable=memory on the command > line, I noticed mem_cgroup_count_vm_event() still showed up as high as > 0.60% in perftop. > > This occurs because the function is called extremely often even when memcg > is disabled. > > To fix this, inline the check for mem_cgroup_disabled() so we avoid the > unnecessary function call if memcg is disabled. > > Signed-off-by: David Rientjes <rientjes-hpIqsD4AKlfQT0dZR+AlfA@public.gmane.org> Acked-by: Johannes Weiner <hannes-druUgvl0LCNAfugRpC6u6w@public.gmane.org> ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [patch] mm, memcg: avoid unnecessary function call when memcg is disabled [not found] ` <alpine.DEB.2.00.1211191741060.24618-X6Q0R45D7oAcqpCFd4KODRPsWskHk0ljAL8bYrjMMd8@public.gmane.org> 2012-11-20 4:23 ` Kamezawa Hiroyuki 2012-11-20 17:27 ` Johannes Weiner @ 2012-11-20 21:49 ` Andrew Morton [not found] ` <20121120134932.055bc192.akpm-de/tnXTf+JLsfHDXvbKv3WD2FQJk+8+b@public.gmane.org> 2 siblings, 1 reply; 12+ messages in thread From: Andrew Morton @ 2012-11-20 21:49 UTC (permalink / raw) To: David Rientjes Cc: Johannes Weiner, Michal Hocko, KAMEZAWA Hiroyuki, Hugh Dickins, linux-kernel-u79uwXL29TY76Z2rM5mHXA, cgroups-u79uwXL29TY76Z2rM5mHXA, linux-mm-Bw31MaZKKs3YtjvyW6yDsg On Mon, 19 Nov 2012 17:44:34 -0800 (PST) David Rientjes <rientjes-hpIqsD4AKlfQT0dZR+AlfA@public.gmane.org> wrote: > While profiling numa/core v16 with cgroup_disable=memory on the command > line, I noticed mem_cgroup_count_vm_event() still showed up as high as > 0.60% in perftop. > > This occurs because the function is called extremely often even when memcg > is disabled. > > To fix this, inline the check for mem_cgroup_disabled() so we avoid the > unnecessary function call if memcg is disabled. > > ... > > diff --git a/include/linux/memcontrol.h b/include/linux/memcontrol.h > --- a/include/linux/memcontrol.h > +++ b/include/linux/memcontrol.h > @@ -181,7 +181,14 @@ unsigned long mem_cgroup_soft_limit_reclaim(struct zone *zone, int order, > gfp_t gfp_mask, > unsigned long *total_scanned); > > -void mem_cgroup_count_vm_event(struct mm_struct *mm, enum vm_event_item idx); > +void __mem_cgroup_count_vm_event(struct mm_struct *mm, enum vm_event_item idx); > +static inline void mem_cgroup_count_vm_event(struct mm_struct *mm, > + enum vm_event_item idx) > +{ > + if (mem_cgroup_disabled() || !mm) > + return; > + __mem_cgroup_count_vm_event(mm, idx); > +} Does the !mm case occur frequently enough to justify inlining it, or should that test remain out-of-line? ^ permalink raw reply [flat|nested] 12+ messages in thread
[parent not found: <20121120134932.055bc192.akpm-de/tnXTf+JLsfHDXvbKv3WD2FQJk+8+b@public.gmane.org>]
* Re: [patch] mm, memcg: avoid unnecessary function call when memcg is disabled [not found] ` <20121120134932.055bc192.akpm-de/tnXTf+JLsfHDXvbKv3WD2FQJk+8+b@public.gmane.org> @ 2012-11-21 1:02 ` Kamezawa Hiroyuki 2012-11-21 2:48 ` [patch] mm, memcg: avoid unnecessary function call when memcg is disabled fix David Rientjes 2012-11-21 8:35 ` [patch] mm, memcg: avoid unnecessary function call when memcg is disabled Michal Hocko 1 sibling, 1 reply; 12+ messages in thread From: Kamezawa Hiroyuki @ 2012-11-21 1:02 UTC (permalink / raw) To: Andrew Morton Cc: David Rientjes, Johannes Weiner, Michal Hocko, Hugh Dickins, linux-kernel-u79uwXL29TY76Z2rM5mHXA, cgroups-u79uwXL29TY76Z2rM5mHXA, linux-mm-Bw31MaZKKs3YtjvyW6yDsg (2012/11/21 6:49), Andrew Morton wrote: > On Mon, 19 Nov 2012 17:44:34 -0800 (PST) > David Rientjes <rientjes-hpIqsD4AKlfQT0dZR+AlfA@public.gmane.org> wrote: > >> While profiling numa/core v16 with cgroup_disable=memory on the command >> line, I noticed mem_cgroup_count_vm_event() still showed up as high as >> 0.60% in perftop. >> >> This occurs because the function is called extremely often even when memcg >> is disabled. >> >> To fix this, inline the check for mem_cgroup_disabled() so we avoid the >> unnecessary function call if memcg is disabled. >> >> ... >> >> diff --git a/include/linux/memcontrol.h b/include/linux/memcontrol.h >> --- a/include/linux/memcontrol.h >> +++ b/include/linux/memcontrol.h >> @@ -181,7 +181,14 @@ unsigned long mem_cgroup_soft_limit_reclaim(struct zone *zone, int order, >> gfp_t gfp_mask, >> unsigned long *total_scanned); >> >> -void mem_cgroup_count_vm_event(struct mm_struct *mm, enum vm_event_item idx); >> +void __mem_cgroup_count_vm_event(struct mm_struct *mm, enum vm_event_item idx); >> +static inline void mem_cgroup_count_vm_event(struct mm_struct *mm, >> + enum vm_event_item idx) >> +{ >> + if (mem_cgroup_disabled() || !mm) >> + return; >> + __mem_cgroup_count_vm_event(mm, idx); >> +} > > Does the !mm case occur frequently enough to justify inlining it, or > should that test remain out-of-line? > I think this should be out-of-line. Thanks, -Kame ^ permalink raw reply [flat|nested] 12+ messages in thread
* [patch] mm, memcg: avoid unnecessary function call when memcg is disabled fix 2012-11-21 1:02 ` Kamezawa Hiroyuki @ 2012-11-21 2:48 ` David Rientjes [not found] ` <alpine.DEB.2.00.1211201847450.2278-X6Q0R45D7oAcqpCFd4KODRPsWskHk0ljAL8bYrjMMd8@public.gmane.org> 0 siblings, 1 reply; 12+ messages in thread From: David Rientjes @ 2012-11-21 2:48 UTC (permalink / raw) To: Kamezawa Hiroyuki, Andrew Morton Cc: Johannes Weiner, Michal Hocko, Hugh Dickins, linux-kernel, cgroups, linux-mm Move the check for !mm out of line as suggested by Andrew. Signed-off-by: David Rientjes <rientjes@google.com> --- include/linux/memcontrol.h | 2 +- mm/memcontrol.c | 3 +++ 2 files changed, 4 insertions(+), 1 deletion(-) diff --git a/include/linux/memcontrol.h b/include/linux/memcontrol.h --- a/include/linux/memcontrol.h +++ b/include/linux/memcontrol.h @@ -185,7 +185,7 @@ void __mem_cgroup_count_vm_event(struct mm_struct *mm, enum vm_event_item idx); static inline void mem_cgroup_count_vm_event(struct mm_struct *mm, enum vm_event_item idx) { - if (mem_cgroup_disabled() || !mm) + if (mem_cgroup_disabled()) return; __mem_cgroup_count_vm_event(mm, idx); } diff --git a/mm/memcontrol.c b/mm/memcontrol.c --- a/mm/memcontrol.c +++ b/mm/memcontrol.c @@ -1021,6 +1021,9 @@ void __mem_cgroup_count_vm_event(struct mm_struct *mm, enum vm_event_item idx) { struct mem_cgroup *memcg; + if (!mm) + return; + rcu_read_lock(); memcg = mem_cgroup_from_task(rcu_dereference(mm->owner)); if (unlikely(!memcg)) -- To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to majordomo@kvack.org. For more info on Linux MM, see: http://www.linux-mm.org/ . Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a> ^ permalink raw reply [flat|nested] 12+ messages in thread
[parent not found: <alpine.DEB.2.00.1211201847450.2278-X6Q0R45D7oAcqpCFd4KODRPsWskHk0ljAL8bYrjMMd8@public.gmane.org>]
* Re: [patch] mm, memcg: avoid unnecessary function call when memcg is disabled fix [not found] ` <alpine.DEB.2.00.1211201847450.2278-X6Q0R45D7oAcqpCFd4KODRPsWskHk0ljAL8bYrjMMd8@public.gmane.org> @ 2012-11-21 4:11 ` Kamezawa Hiroyuki 0 siblings, 0 replies; 12+ messages in thread From: Kamezawa Hiroyuki @ 2012-11-21 4:11 UTC (permalink / raw) To: David Rientjes Cc: Andrew Morton, Johannes Weiner, Michal Hocko, Hugh Dickins, linux-kernel-u79uwXL29TY76Z2rM5mHXA, cgroups-u79uwXL29TY76Z2rM5mHXA, linux-mm-Bw31MaZKKs3YtjvyW6yDsg (2012/11/21 11:48), David Rientjes wrote: > Move the check for !mm out of line as suggested by Andrew. > > Signed-off-by: David Rientjes <rientjes-hpIqsD4AKlfQT0dZR+AlfA@public.gmane.org> Thank you very much ! Acked-by: KAMEZAWA Hiroyuki <kamezawa.hiroyu-+CUm20s59erQFUHtdCDX3A@public.gmane.org> > --- > include/linux/memcontrol.h | 2 +- > mm/memcontrol.c | 3 +++ > 2 files changed, 4 insertions(+), 1 deletion(-) > > diff --git a/include/linux/memcontrol.h b/include/linux/memcontrol.h > --- a/include/linux/memcontrol.h > +++ b/include/linux/memcontrol.h > @@ -185,7 +185,7 @@ void __mem_cgroup_count_vm_event(struct mm_struct *mm, enum vm_event_item idx); > static inline void mem_cgroup_count_vm_event(struct mm_struct *mm, > enum vm_event_item idx) > { > - if (mem_cgroup_disabled() || !mm) > + if (mem_cgroup_disabled()) > return; > __mem_cgroup_count_vm_event(mm, idx); > } > diff --git a/mm/memcontrol.c b/mm/memcontrol.c > --- a/mm/memcontrol.c > +++ b/mm/memcontrol.c > @@ -1021,6 +1021,9 @@ void __mem_cgroup_count_vm_event(struct mm_struct *mm, enum vm_event_item idx) > { > struct mem_cgroup *memcg; > > + if (!mm) > + return; > + > rcu_read_lock(); > memcg = mem_cgroup_from_task(rcu_dereference(mm->owner)); > if (unlikely(!memcg)) > ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [patch] mm, memcg: avoid unnecessary function call when memcg is disabled [not found] ` <20121120134932.055bc192.akpm-de/tnXTf+JLsfHDXvbKv3WD2FQJk+8+b@public.gmane.org> 2012-11-21 1:02 ` Kamezawa Hiroyuki @ 2012-11-21 8:35 ` Michal Hocko 2012-11-28 23:29 ` Hugh Dickins 1 sibling, 1 reply; 12+ messages in thread From: Michal Hocko @ 2012-11-21 8:35 UTC (permalink / raw) To: Andrew Morton, Ying Han Cc: David Rientjes, Johannes Weiner, KAMEZAWA Hiroyuki, Hugh Dickins, linux-kernel-u79uwXL29TY76Z2rM5mHXA, cgroups-u79uwXL29TY76Z2rM5mHXA, linux-mm-Bw31MaZKKs3YtjvyW6yDsg On Tue 20-11-12 13:49:32, Andrew Morton wrote: > On Mon, 19 Nov 2012 17:44:34 -0800 (PST) > David Rientjes <rientjes-hpIqsD4AKlfQT0dZR+AlfA@public.gmane.org> wrote: > > > While profiling numa/core v16 with cgroup_disable=memory on the command > > line, I noticed mem_cgroup_count_vm_event() still showed up as high as > > 0.60% in perftop. > > > > This occurs because the function is called extremely often even when memcg > > is disabled. > > > > To fix this, inline the check for mem_cgroup_disabled() so we avoid the > > unnecessary function call if memcg is disabled. > > > > ... > > > > diff --git a/include/linux/memcontrol.h b/include/linux/memcontrol.h > > --- a/include/linux/memcontrol.h > > +++ b/include/linux/memcontrol.h > > @@ -181,7 +181,14 @@ unsigned long mem_cgroup_soft_limit_reclaim(struct zone *zone, int order, > > gfp_t gfp_mask, > > unsigned long *total_scanned); > > > > -void mem_cgroup_count_vm_event(struct mm_struct *mm, enum vm_event_item idx); > > +void __mem_cgroup_count_vm_event(struct mm_struct *mm, enum vm_event_item idx); > > +static inline void mem_cgroup_count_vm_event(struct mm_struct *mm, > > + enum vm_event_item idx) > > +{ > > + if (mem_cgroup_disabled() || !mm) > > + return; > > + __mem_cgroup_count_vm_event(mm, idx); > > +} > > Does the !mm case occur frequently enough to justify inlining it, or > should that test remain out-of-line? Now that you've asked about it I started looking around and I cannot see how mm can ever be NULL. The condition is there since the very beginning (456f998e memcg: add the pagefault count into memcg stats) but all the callers are page fault handlers and those shouldn't have mm==NULL. Or is there anything obvious I am missing? Ying, the whole thread starts https://lkml.org/lkml/2012/11/19/545 but the primary question is why we need !mm test for mem_cgroup_count_vm_event at all. Thanks! -- Michal Hocko SUSE Labs ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [patch] mm, memcg: avoid unnecessary function call when memcg is disabled 2012-11-21 8:35 ` [patch] mm, memcg: avoid unnecessary function call when memcg is disabled Michal Hocko @ 2012-11-28 23:29 ` Hugh Dickins [not found] ` <alpine.LNX.2.00.1211281509560.15410-fupSdm12i1nKWymIFiNcPA@public.gmane.org> 0 siblings, 1 reply; 12+ messages in thread From: Hugh Dickins @ 2012-11-28 23:29 UTC (permalink / raw) To: Michal Hocko Cc: Andrew Morton, Ying Han, David Rientjes, Johannes Weiner, KAMEZAWA Hiroyuki, linux-kernel, cgroups, linux-mm On Wed, 21 Nov 2012, Michal Hocko wrote: > On Tue 20-11-12 13:49:32, Andrew Morton wrote: > > On Mon, 19 Nov 2012 17:44:34 -0800 (PST) > > David Rientjes <rientjes@google.com> wrote: > > > > > While profiling numa/core v16 with cgroup_disable=memory on the command > > > line, I noticed mem_cgroup_count_vm_event() still showed up as high as > > > 0.60% in perftop. > > > > > > This occurs because the function is called extremely often even when memcg > > > is disabled. > > > > > > To fix this, inline the check for mem_cgroup_disabled() so we avoid the > > > unnecessary function call if memcg is disabled. > > > > > > ... > > > > > > diff --git a/include/linux/memcontrol.h b/include/linux/memcontrol.h > > > --- a/include/linux/memcontrol.h > > > +++ b/include/linux/memcontrol.h > > > @@ -181,7 +181,14 @@ unsigned long mem_cgroup_soft_limit_reclaim(struct zone *zone, int order, > > > gfp_t gfp_mask, > > > unsigned long *total_scanned); > > > > > > -void mem_cgroup_count_vm_event(struct mm_struct *mm, enum vm_event_item idx); > > > +void __mem_cgroup_count_vm_event(struct mm_struct *mm, enum vm_event_item idx); > > > +static inline void mem_cgroup_count_vm_event(struct mm_struct *mm, > > > + enum vm_event_item idx) > > > +{ > > > + if (mem_cgroup_disabled() || !mm) > > > + return; > > > + __mem_cgroup_count_vm_event(mm, idx); > > > +} > > > > Does the !mm case occur frequently enough to justify inlining it, or > > should that test remain out-of-line? > > Now that you've asked about it I started looking around and I cannot see > how mm can ever be NULL. The condition is there since the very beginning > (456f998e memcg: add the pagefault count into memcg stats) but all the > callers are page fault handlers and those shouldn't have mm==NULL. > Or is there anything obvious I am missing? > > Ying, the whole thread starts https://lkml.org/lkml/2012/11/19/545 but > the primary question is why we need !mm test for mem_cgroup_count_vm_event > at all. Here's a guess: as Ying's 456f998e patch started out in akpm's tree, shmem.c was calling mem_cgroup_count_vm_event(current->mm, PGMAJFAULT). Then I insisted that was inconsistent with how we usually account when one task touches another's address space, and rearranged it to work on vma->vm_mm instead. Done the original way, if the touching task were a kernel daemon (KSM's ksmd comes to my mind), then the current->mm could well have been NULL. I agree with you that it looks redundant now. Hugh -- To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to majordomo@kvack.org. For more info on Linux MM, see: http://www.linux-mm.org/ . Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a> ^ permalink raw reply [flat|nested] 12+ messages in thread
[parent not found: <alpine.LNX.2.00.1211281509560.15410-fupSdm12i1nKWymIFiNcPA@public.gmane.org>]
* [PATCH] memcg: do not check for mm in mem_cgroup_count_vm_event disabled [not found] ` <alpine.LNX.2.00.1211281509560.15410-fupSdm12i1nKWymIFiNcPA@public.gmane.org> @ 2012-11-29 13:28 ` Michal Hocko 0 siblings, 0 replies; 12+ messages in thread From: Michal Hocko @ 2012-11-29 13:28 UTC (permalink / raw) To: Andrew Morton Cc: Hugh Dickins, Ying Han, David Rientjes, Johannes Weiner, KAMEZAWA Hiroyuki, linux-kernel-u79uwXL29TY76Z2rM5mHXA, cgroups-u79uwXL29TY76Z2rM5mHXA, linux-mm-Bw31MaZKKs3YtjvyW6yDsg On Wed 28-11-12 15:29:30, Hugh Dickins wrote: > On Wed, 21 Nov 2012, Michal Hocko wrote: > > On Tue 20-11-12 13:49:32, Andrew Morton wrote: > > > On Mon, 19 Nov 2012 17:44:34 -0800 (PST) > > > David Rientjes <rientjes-hpIqsD4AKlfQT0dZR+AlfA@public.gmane.org> wrote: [...] > > > > -void mem_cgroup_count_vm_event(struct mm_struct *mm, enum vm_event_item idx); > > > > +void __mem_cgroup_count_vm_event(struct mm_struct *mm, enum vm_event_item idx); > > > > +static inline void mem_cgroup_count_vm_event(struct mm_struct *mm, > > > > + enum vm_event_item idx) > > > > +{ > > > > + if (mem_cgroup_disabled() || !mm) > > > > + return; > > > > + __mem_cgroup_count_vm_event(mm, idx); > > > > +} > > > > > > Does the !mm case occur frequently enough to justify inlining it, or > > > should that test remain out-of-line? > > > > Now that you've asked about it I started looking around and I cannot see > > how mm can ever be NULL. The condition is there since the very beginning > > (456f998e memcg: add the pagefault count into memcg stats) but all the > > callers are page fault handlers and those shouldn't have mm==NULL. > > Or is there anything obvious I am missing? > > > > Ying, the whole thread starts https://lkml.org/lkml/2012/11/19/545 but > > the primary question is why we need !mm test for mem_cgroup_count_vm_event > > at all. > > Here's a guess: as Ying's 456f998e patch started out in akpm's tree, > shmem.c was calling mem_cgroup_count_vm_event(current->mm, PGMAJFAULT). > > Then I insisted that was inconsistent with how we usually account when > one task touches another's address space, and rearranged it to work on > vma->vm_mm instead. Thanks Hugh! > Done the original way, if the touching task were a kernel daemon (KSM's > ksmd comes to my mind), then the current->mm could well have been NULL. > > I agree with you that it looks redundant now. Andrew could you please pick this up? --- From 619b1ab26c3e96944f6c60256cf7920671bafa5b Mon Sep 17 00:00:00 2001 From: Michal Hocko <mhocko-AlSwsSmVLrQ@public.gmane.org> Date: Thu, 29 Nov 2012 14:20:58 +0100 Subject: [PATCH] memcg: do not check for mm in mem_cgroup_count_vm_event mm given to mem_cgroup_count_vm_event cannot be NULL because the function is either called from the page fault path or vma->vm_mm is used. So the check can be dropped. The check has been introduced by 456f998e (memcg: add the pagefault count into memcg stats) because the originally proposed patch used current->mm for shmem but this has been changed to vma->vm_mm later on without the check being removed (thanks to Hugh for this recollection). Signed-off-by: Michal Hocko <mhocko-AlSwsSmVLrQ@public.gmane.org> --- include/linux/memcontrol.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/include/linux/memcontrol.h b/include/linux/memcontrol.h index 095d2b4..0108a56 100644 --- a/include/linux/memcontrol.h +++ b/include/linux/memcontrol.h @@ -188,7 +188,7 @@ void __mem_cgroup_count_vm_event(struct mm_struct *mm, enum vm_event_item idx); static inline void mem_cgroup_count_vm_event(struct mm_struct *mm, enum vm_event_item idx) { - if (mem_cgroup_disabled() || !mm) + if (mem_cgroup_disabled()) return; __mem_cgroup_count_vm_event(mm, idx); } -- 1.7.10.4 -- Michal Hocko SUSE Labs ^ permalink raw reply related [flat|nested] 12+ messages in thread
end of thread, other threads:[~2012-11-29 13:28 UTC | newest]
Thread overview: 12+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2012-11-20 1:44 [patch] mm, memcg: avoid unnecessary function call when memcg is disabled David Rientjes
2012-11-20 7:07 ` Michal Hocko
[not found] ` <alpine.DEB.2.00.1211191741060.24618-X6Q0R45D7oAcqpCFd4KODRPsWskHk0ljAL8bYrjMMd8@public.gmane.org>
2012-11-20 4:23 ` Kamezawa Hiroyuki
[not found] ` <50AB05B4.4000303-+CUm20s59erQFUHtdCDX3A@public.gmane.org>
2012-11-20 8:34 ` Glauber Costa
2012-11-20 17:27 ` Johannes Weiner
2012-11-20 21:49 ` Andrew Morton
[not found] ` <20121120134932.055bc192.akpm-de/tnXTf+JLsfHDXvbKv3WD2FQJk+8+b@public.gmane.org>
2012-11-21 1:02 ` Kamezawa Hiroyuki
2012-11-21 2:48 ` [patch] mm, memcg: avoid unnecessary function call when memcg is disabled fix David Rientjes
[not found] ` <alpine.DEB.2.00.1211201847450.2278-X6Q0R45D7oAcqpCFd4KODRPsWskHk0ljAL8bYrjMMd8@public.gmane.org>
2012-11-21 4:11 ` Kamezawa Hiroyuki
2012-11-21 8:35 ` [patch] mm, memcg: avoid unnecessary function call when memcg is disabled Michal Hocko
2012-11-28 23:29 ` Hugh Dickins
[not found] ` <alpine.LNX.2.00.1211281509560.15410-fupSdm12i1nKWymIFiNcPA@public.gmane.org>
2012-11-29 13:28 ` [PATCH] memcg: do not check for mm in mem_cgroup_count_vm_event disabled Michal Hocko
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).