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

[Stephane Eranian]

On Tue, Mar 5, 2013 at 4:38 AM, Li Zefan <lizefan@xxxxxxxxxx> 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@xxxxxxxxxx>
> ---
>  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
>
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/