Re: [PATCH] perf: remove include of cgroup.h from perf_event.h

Tejun Heo
2013-03-05T17:36:28Z

(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