* [PATCH] perf: remove include of cgroup.h from perf_event.h @ 2013-03-05 3:38 Li Zefan 2013-03-05 8:33 ` Stephane Eranian 2013-03-06 14:43 ` [tip:perf/core] perf: Remove " tip-bot for Li Zefan 0 siblings, 2 replies; 7+ messages in thread From: Li Zefan @ 2013-03-05 3:38 UTC (permalink / raw) To: Ingo Molnar; +Cc: LKML, Cgroups, Peter Zijlstra, Stephane Eranian, Tejun Heo Move struct perf_cgroup_info and perf_cgroup to kernel/perf/core.c, and then we can remove include of cgroup.h. Signed-off-by: Li Zefan <lizefan@huawei.com> --- include/linux/perf_event.h | 18 +----------------- kernel/events/core.c | 15 +++++++++++++++ 2 files changed, 16 insertions(+), 17 deletions(-) diff --git a/include/linux/perf_event.h b/include/linux/perf_event.h index e47ee46..8737e1c 100644 --- a/include/linux/perf_event.h +++ b/include/linux/perf_event.h @@ -21,7 +21,6 @@ */ #ifdef CONFIG_PERF_EVENTS -# include <linux/cgroup.h> # include <asm/perf_event.h> # include <asm/local64.h> #endif @@ -299,22 +298,7 @@ struct swevent_hlist { #define PERF_ATTACH_GROUP 0x02 #define PERF_ATTACH_TASK 0x04 -#ifdef CONFIG_CGROUP_PERF -/* - * perf_cgroup_info keeps track of time_enabled for a cgroup. - * This is a per-cpu dynamically allocated data structure. - */ -struct perf_cgroup_info { - u64 time; - u64 timestamp; -}; - -struct perf_cgroup { - struct cgroup_subsys_state css; - struct perf_cgroup_info *info; /* timing info, one per cpu */ -}; -#endif - +struct perf_cgroup; struct ring_buffer; /** diff --git a/kernel/events/core.c b/kernel/events/core.c index b0cd865..5976a2a 100644 --- a/kernel/events/core.c +++ b/kernel/events/core.c @@ -37,6 +37,7 @@ #include <linux/ftrace_event.h> #include <linux/hw_breakpoint.h> #include <linux/mm_types.h> +#include <linux/cgroup.h> #include "internal.h" @@ -234,6 +235,20 @@ static void perf_ctx_unlock(struct perf_cpu_context *cpuctx, #ifdef CONFIG_CGROUP_PERF /* + * perf_cgroup_info keeps track of time_enabled for a cgroup. + * This is a per-cpu dynamically allocated data structure. + */ +struct perf_cgroup_info { + u64 time; + u64 timestamp; +}; + +struct perf_cgroup { + struct cgroup_subsys_state css; + struct perf_cgroup_info *info; +}; + +/* * Must ensure cgroup is pinned (css_get) before calling * this function. In other words, we cannot call this function * if there is no cgroup event for the current CPU context. -- 1.8.0.2 ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH] perf: remove include of cgroup.h from perf_event.h 2013-03-05 3:38 [PATCH] perf: remove include of cgroup.h from perf_event.h Li Zefan @ 2013-03-05 8:33 ` Stephane Eranian 2013-03-05 10:37 ` Li Zefan 2013-03-06 10:31 ` Ingo Molnar 2013-03-06 14:43 ` [tip:perf/core] perf: Remove " tip-bot for Li Zefan 1 sibling, 2 replies; 7+ messages in thread From: Stephane Eranian @ 2013-03-05 8:33 UTC (permalink / raw) To: Li Zefan; +Cc: Ingo Molnar, LKML, Cgroups, Peter Zijlstra, Tejun Heo On Tue, Mar 5, 2013 at 4:38 AM, Li Zefan <lizefan@huawei.com> wrote: > Move struct perf_cgroup_info and perf_cgroup to kernel/perf/core.c, > and then we can remove include of cgroup.h. > > Signed-off-by: Li Zefan <lizefan@huawei.com> > --- > include/linux/perf_event.h | 18 +----------------- > kernel/events/core.c | 15 +++++++++++++++ > 2 files changed, 16 insertions(+), 17 deletions(-) > > diff --git a/include/linux/perf_event.h b/include/linux/perf_event.h > index e47ee46..8737e1c 100644 > --- a/include/linux/perf_event.h > +++ b/include/linux/perf_event.h > @@ -21,7 +21,6 @@ > */ > > #ifdef CONFIG_PERF_EVENTS > -# include <linux/cgroup.h> > # include <asm/perf_event.h> > # include <asm/local64.h> > #endif > @@ -299,22 +298,7 @@ struct swevent_hlist { > #define PERF_ATTACH_GROUP 0x02 > #define PERF_ATTACH_TASK 0x04 > > -#ifdef CONFIG_CGROUP_PERF > -/* > - * perf_cgroup_info keeps track of time_enabled for a cgroup. > - * This is a per-cpu dynamically allocated data structure. > - */ > -struct perf_cgroup_info { > - u64 time; > - u64 timestamp; > -}; > - > -struct perf_cgroup { > - struct cgroup_subsys_state css; > - struct perf_cgroup_info *info; /* timing info, one per cpu */ > -}; > -#endif > - > +struct perf_cgroup; The problem is that you have struct perf_cgroup in the struct perf_event structure. Today, this field is not referenced outside of kernel/events/core.c But it is available outside this file. If someday the field is reference, your changes will have to do reverted. So I am wondering what is the point of the change right now? > struct ring_buffer; > > /** > diff --git a/kernel/events/core.c b/kernel/events/core.c > index b0cd865..5976a2a 100644 > --- a/kernel/events/core.c > +++ b/kernel/events/core.c > @@ -37,6 +37,7 @@ > #include <linux/ftrace_event.h> > #include <linux/hw_breakpoint.h> > #include <linux/mm_types.h> > +#include <linux/cgroup.h> > > #include "internal.h" > > @@ -234,6 +235,20 @@ static void perf_ctx_unlock(struct perf_cpu_context *cpuctx, > #ifdef CONFIG_CGROUP_PERF > > /* > + * perf_cgroup_info keeps track of time_enabled for a cgroup. > + * This is a per-cpu dynamically allocated data structure. > + */ > +struct perf_cgroup_info { > + u64 time; > + u64 timestamp; > +}; > + > +struct perf_cgroup { > + struct cgroup_subsys_state css; > + struct perf_cgroup_info *info; > +}; > + > +/* > * Must ensure cgroup is pinned (css_get) before calling > * this function. In other words, we cannot call this function > * if there is no cgroup event for the current CPU context. > -- > 1.8.0.2 > ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] perf: remove include of cgroup.h from perf_event.h 2013-03-05 8:33 ` Stephane Eranian @ 2013-03-05 10:37 ` Li Zefan 2013-03-05 17:36 ` Tejun Heo 2013-03-06 10:31 ` Ingo Molnar 1 sibling, 1 reply; 7+ messages in thread From: Li Zefan @ 2013-03-05 10:37 UTC (permalink / raw) To: Stephane Eranian; +Cc: Ingo Molnar, LKML, Cgroups, Peter Zijlstra, Tejun Heo On 2013/3/5 16:33, Stephane Eranian wrote: > On Tue, Mar 5, 2013 at 4:38 AM, Li Zefan <lizefan@huawei.com> wrote: >> Move struct perf_cgroup_info and perf_cgroup to kernel/perf/core.c, >> and then we can remove include of cgroup.h. >> >> Signed-off-by: Li Zefan <lizefan@huawei.com> >> --- >> include/linux/perf_event.h | 18 +----------------- >> kernel/events/core.c | 15 +++++++++++++++ >> 2 files changed, 16 insertions(+), 17 deletions(-) >> >> diff --git a/include/linux/perf_event.h b/include/linux/perf_event.h >> index e47ee46..8737e1c 100644 >> --- a/include/linux/perf_event.h >> +++ b/include/linux/perf_event.h >> @@ -21,7 +21,6 @@ >> */ >> >> #ifdef CONFIG_PERF_EVENTS >> -# include <linux/cgroup.h> >> # include <asm/perf_event.h> >> # include <asm/local64.h> >> #endif >> @@ -299,22 +298,7 @@ struct swevent_hlist { >> #define PERF_ATTACH_GROUP 0x02 >> #define PERF_ATTACH_TASK 0x04 >> >> -#ifdef CONFIG_CGROUP_PERF >> -/* >> - * perf_cgroup_info keeps track of time_enabled for a cgroup. >> - * This is a per-cpu dynamically allocated data structure. >> - */ >> -struct perf_cgroup_info { >> - u64 time; >> - u64 timestamp; >> -}; >> - >> -struct perf_cgroup { >> - struct cgroup_subsys_state css; >> - struct perf_cgroup_info *info; /* timing info, one per cpu */ >> -}; >> -#endif >> - >> +struct perf_cgroup; > > The problem is that you have struct perf_cgroup in the struct perf_event > structure. Today, this field is not referenced outside of kernel/events/core.c > But it is available outside this file. If someday the field is reference, your > changes will have to do reverted. So I am wondering what is the point > of the change right now? > I touch cgroup.h quite frequently, so I'd like to reduce the compile time caused by cgroup.h changes, so I've made a few patches to remove cgroup.h from some header files. It's better to expose as less interfaces as possible, and if someone wants to export someting he should think about if it's the right thing to do. Like we often make static functions usable to external users, and also often do the reverse thing, this patch is nothing special. ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] perf: remove include of cgroup.h from perf_event.h 2013-03-05 10:37 ` Li Zefan @ 2013-03-05 17:36 ` Tejun Heo 0 siblings, 0 replies; 7+ messages in thread From: Tejun Heo @ 2013-03-05 17:36 UTC (permalink / raw) To: Li Zefan Cc: Stephane Eranian, Ingo Molnar, LKML, Cgroups, Peter Zijlstra, Steven Rostedt, Frederic Weisbecker (cc'ing Steven and Frederic and quoting whole body) Hello, guys. On Tue, Mar 05, 2013 at 06:37:12PM +0800, Li Zefan wrote: > On 2013/3/5 16:33, Stephane Eranian wrote: > > On Tue, Mar 5, 2013 at 4:38 AM, Li Zefan <lizefan@huawei.com> wrote: > >> Move struct perf_cgroup_info and perf_cgroup to kernel/perf/core.c, > >> and then we can remove include of cgroup.h. > >> > >> Signed-off-by: Li Zefan <lizefan@huawei.com> > >> --- > >> include/linux/perf_event.h | 18 +----------------- > >> kernel/events/core.c | 15 +++++++++++++++ > >> 2 files changed, 16 insertions(+), 17 deletions(-) > >> > >> diff --git a/include/linux/perf_event.h b/include/linux/perf_event.h > >> index e47ee46..8737e1c 100644 > >> --- a/include/linux/perf_event.h > >> +++ b/include/linux/perf_event.h > >> @@ -21,7 +21,6 @@ > >> */ > >> > >> #ifdef CONFIG_PERF_EVENTS > >> -# include <linux/cgroup.h> > >> # include <asm/perf_event.h> > >> # include <asm/local64.h> > >> #endif > >> @@ -299,22 +298,7 @@ struct swevent_hlist { > >> #define PERF_ATTACH_GROUP 0x02 > >> #define PERF_ATTACH_TASK 0x04 > >> > >> -#ifdef CONFIG_CGROUP_PERF > >> -/* > >> - * perf_cgroup_info keeps track of time_enabled for a cgroup. > >> - * This is a per-cpu dynamically allocated data structure. > >> - */ > >> -struct perf_cgroup_info { > >> - u64 time; > >> - u64 timestamp; > >> -}; > >> - > >> -struct perf_cgroup { > >> - struct cgroup_subsys_state css; > >> - struct perf_cgroup_info *info; /* timing info, one per cpu */ > >> -}; > >> -#endif > >> - > >> +struct perf_cgroup; > > > > The problem is that you have struct perf_cgroup in the struct perf_event > > structure. Today, this field is not referenced outside of kernel/events/core.c > > But it is available outside this file. If someday the field is reference, your > > changes will have to do reverted. So I am wondering what is the point > > of the change right now? > > > > I touch cgroup.h quite frequently, so I'd like to reduce the compile time caused > by cgroup.h changes, so I've made a few patches to remove cgroup.h from some > header files. > > It's better to expose as less interfaces as possible, and if someone wants to > export someting he should think about if it's the right thing to do. > > Like we often make static functions usable to external users, and also often do the > reverse thing, this patch is nothing special. I don't care either way. Steven, Frederic, Ingo? Thanks. -- tejun ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] perf: remove include of cgroup.h from perf_event.h 2013-03-05 8:33 ` Stephane Eranian 2013-03-05 10:37 ` Li Zefan @ 2013-03-06 10:31 ` Ingo Molnar 2013-03-06 11:20 ` Stephane Eranian 1 sibling, 1 reply; 7+ messages in thread From: Ingo Molnar @ 2013-03-06 10:31 UTC (permalink / raw) To: Stephane Eranian; +Cc: Li Zefan, LKML, Cgroups, Peter Zijlstra, Tejun Heo * Stephane Eranian <eranian@google.com> wrote: > > - * This is a per-cpu dynamically allocated data structure. > > - */ > > -struct perf_cgroup_info { > > - u64 time; > > - u64 timestamp; > > -}; > > - > > -struct perf_cgroup { > > - struct cgroup_subsys_state css; > > - struct perf_cgroup_info *info; /* timing info, one per cpu */ > > -}; > > -#endif > > - > > +struct perf_cgroup; > > The problem is that you have struct perf_cgroup in the struct perf_event > structure. Today, this field is not referenced outside of kernel/events/core.c But > it is available outside this file. If someday the field is reference, your changes > will have to do reverted. So I am wondering what is the point of the change right > now? It's standard practice to not define the type for task_struct or other kernel subsystems. For example slab caches can be created via kmem_cache_create() anywhere in the tree, but the internal structure is only known to the SLAB subsystem. Thanks, Ingo ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] perf: remove include of cgroup.h from perf_event.h 2013-03-06 10:31 ` Ingo Molnar @ 2013-03-06 11:20 ` Stephane Eranian 0 siblings, 0 replies; 7+ messages in thread From: Stephane Eranian @ 2013-03-06 11:20 UTC (permalink / raw) To: Ingo Molnar; +Cc: Li Zefan, LKML, Cgroups, Peter Zijlstra, Tejun Heo On Wed, Mar 6, 2013 at 11:31 AM, Ingo Molnar <mingo@kernel.org> wrote: > > * Stephane Eranian <eranian@google.com> wrote: > >> > - * This is a per-cpu dynamically allocated data structure. >> > - */ >> > -struct perf_cgroup_info { >> > - u64 time; >> > - u64 timestamp; >> > -}; >> > - >> > -struct perf_cgroup { >> > - struct cgroup_subsys_state css; >> > - struct perf_cgroup_info *info; /* timing info, one per cpu */ >> > -}; >> > -#endif >> > - >> > +struct perf_cgroup; >> >> The problem is that you have struct perf_cgroup in the struct perf_event >> structure. Today, this field is not referenced outside of kernel/events/core.c But >> it is available outside this file. If someday the field is reference, your changes >> will have to do reverted. So I am wondering what is the point of the change right >> now? > > It's standard practice to not define the type for task_struct or other kernel > subsystems. > > For example slab caches can be created via kmem_cache_create() anywhere in the tree, > but the internal structure is only known to the SLAB subsystem. > I am fine with the change. ^ permalink raw reply [flat|nested] 7+ messages in thread
* [tip:perf/core] perf: Remove include of cgroup.h from perf_event.h 2013-03-05 3:38 [PATCH] perf: remove include of cgroup.h from perf_event.h Li Zefan 2013-03-05 8:33 ` Stephane Eranian @ 2013-03-06 14:43 ` tip-bot for Li Zefan 1 sibling, 0 replies; 7+ messages in thread From: tip-bot for Li Zefan @ 2013-03-06 14:43 UTC (permalink / raw) To: linux-tip-commits Cc: linux-kernel, eranian, hpa, mingo, peterz, tj, tglx, lizefan Commit-ID: 877c685607925238e302cd3aa38788dca6c1b226 Gitweb: http://git.kernel.org/tip/877c685607925238e302cd3aa38788dca6c1b226 Author: Li Zefan <lizefan@huawei.com> AuthorDate: Tue, 5 Mar 2013 11:38:08 +0800 Committer: Ingo Molnar <mingo@kernel.org> CommitDate: Wed, 6 Mar 2013 11:32:56 +0100 perf: Remove include of cgroup.h from perf_event.h Move struct perf_cgroup_info and perf_cgroup to kernel/perf/core.c, and then we can remove include of cgroup.h. Signed-off-by: Li Zefan <lizefan@huawei.com> Cc: Peter Zijlstra <peterz@infradead.org> Cc: Stephane Eranian <eranian@google.com> Cc: Tejun Heo <tj@kernel.org> Link: http://lkml.kernel.org/r/513568A0.6020804@huawei.com Signed-off-by: Ingo Molnar <mingo@kernel.org> --- include/linux/perf_event.h | 18 +----------------- kernel/events/core.c | 15 +++++++++++++++ 2 files changed, 16 insertions(+), 17 deletions(-) diff --git a/include/linux/perf_event.h b/include/linux/perf_event.h index e47ee46..8737e1c 100644 --- a/include/linux/perf_event.h +++ b/include/linux/perf_event.h @@ -21,7 +21,6 @@ */ #ifdef CONFIG_PERF_EVENTS -# include <linux/cgroup.h> # include <asm/perf_event.h> # include <asm/local64.h> #endif @@ -299,22 +298,7 @@ struct swevent_hlist { #define PERF_ATTACH_GROUP 0x02 #define PERF_ATTACH_TASK 0x04 -#ifdef CONFIG_CGROUP_PERF -/* - * perf_cgroup_info keeps track of time_enabled for a cgroup. - * This is a per-cpu dynamically allocated data structure. - */ -struct perf_cgroup_info { - u64 time; - u64 timestamp; -}; - -struct perf_cgroup { - struct cgroup_subsys_state css; - struct perf_cgroup_info *info; /* timing info, one per cpu */ -}; -#endif - +struct perf_cgroup; struct ring_buffer; /** diff --git a/kernel/events/core.c b/kernel/events/core.c index b0cd865..5976a2a 100644 --- a/kernel/events/core.c +++ b/kernel/events/core.c @@ -37,6 +37,7 @@ #include <linux/ftrace_event.h> #include <linux/hw_breakpoint.h> #include <linux/mm_types.h> +#include <linux/cgroup.h> #include "internal.h" @@ -234,6 +235,20 @@ static void perf_ctx_unlock(struct perf_cpu_context *cpuctx, #ifdef CONFIG_CGROUP_PERF /* + * perf_cgroup_info keeps track of time_enabled for a cgroup. + * This is a per-cpu dynamically allocated data structure. + */ +struct perf_cgroup_info { + u64 time; + u64 timestamp; +}; + +struct perf_cgroup { + struct cgroup_subsys_state css; + struct perf_cgroup_info *info; +}; + +/* * Must ensure cgroup is pinned (css_get) before calling * this function. In other words, we cannot call this function * if there is no cgroup event for the current CPU context. ^ permalink raw reply related [flat|nested] 7+ messages in thread
end of thread, other threads:[~2013-03-06 14:44 UTC | newest] Thread overview: 7+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2013-03-05 3:38 [PATCH] perf: remove include of cgroup.h from perf_event.h Li Zefan 2013-03-05 8:33 ` Stephane Eranian 2013-03-05 10:37 ` Li Zefan 2013-03-05 17:36 ` Tejun Heo 2013-03-06 10:31 ` Ingo Molnar 2013-03-06 11:20 ` Stephane Eranian 2013-03-06 14:43 ` [tip:perf/core] perf: Remove " tip-bot for Li Zefan
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox