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

Stephane Eranian
2013-03-05T08:34:00Z

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
>