perf: remove include of cgroup.h from perf_event.h

PATCH In review Li Zefan 5 participants 7 messages Mar 5, 2013 – Mar 6, 2013

Subsystems (2)

perf events cgroup

lore

Share & tools

lore

Li Zefan

author

2013-03-05 03:38 UTC #0 lore raw

PATCH 1/1 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 <[email protected]>

---
 include/linux/perf_event.h | 18 +-----------------
 kernel/events/core.c       | 15 +++++++++++++++
 2 files changed, 16 insertions(+), 17 deletions(-)

include/linux/perf_event.h +1 ~~-17~~

index e47ee46..8737e1c 100644
@@ -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;
 
 /**

kernel/events/core.c +15 -0

index b0cd865..5976a2a 100644
@@ -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.

Signature · 1 line

1.8.0.2

Stephane Eranian

2013-03-05 08:33 UTC #1 lore raw

Quoted 32 lines · Li Zefan

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 <[email protected]>

---
 include/linux/perf_event.h | 18 +-----------------
 kernel/events/core.c       | 15 +++++++++++++++
 2 files changed, 16 insertions(+), 17 deletions(-)

include/linux/perf_event.h +0 -8

index e47ee46..8737e1c 100644
@@ -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;

include/linux/perf_event.h struct swevent_hlist { · line 301

Quoted 10 lines

include/linux/perf_event.h +1 -9

-       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?

Quoted 39 lines

include/linux/perf_event.h +0 -0

 struct ring_buffer;
 
 /**

kernel/events/core.c +15 -1

index b0cd865..5976a2a 100644
@@ -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

Li Zefan

author

2013-03-05 10:37 UTC #2 lore raw

Quoted 41 lines · Stephane Eranian

Quoted 40 lines · Li Zefan

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 <[email protected]>

---
 include/linux/perf_event.h | 18 +-----------------
 kernel/events/core.c       | 15 +++++++++++++++
 2 files changed, 16 insertions(+), 17 deletions(-)

include/linux/perf_event.h +0 ~~-16~~

index e47ee46..8737e1c 100644
@@ -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

Quoted 9 lines

include/linux/perf_event.h struct swevent_hlist { · line 301
Quoted 2 lines
include/linux/perf_event.h +1 -1
-
+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.

Tejun Heo

maintainer

2013-03-05 17:36 UTC #3 lore raw

(cc'ing Steven and Frederic and quoting whole body)

Hello, guys.

Quoted 56 lines · Li Zefan

Quoted 41 lines · Stephane Eranian
Quoted 40 lines · Li Zefan
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 <[email protected]>
---
 include/linux/perf_event.h | 18 +-----------------
 kernel/events/core.c       | 15 +++++++++++++++
 2 files changed, 16 insertions(+), 17 deletions(-)
include/linux/perf_event.h +0 ~~-16~~
index e47ee46..8737e1c 100644
@@ -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
Quoted 9 lines
include/linux/perf_event.h struct swevent_hlist { · line 301
Quoted 2 lines
include/linux/perf_event.h +1 -1
-
+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.

Quoted 5 lines

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.

Signature · 1 line

tejun

Ingo Molnar

2013-03-06 10:31 UTC #4 lore raw

In reply to Stephane Eranian

Quoted 12 lines · Stephane Eranian

Quoted 12 lines

- * This is a per-cpu dynamically allocated data structure.
- */

include/linux/perf_event.h +0 ~~-10~~

-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

Quoted 8 lines

Review
Quoted 2 lines
(quoted diff) +1 -1
-
+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

Stephane Eranian

2013-03-06 11:20 UTC #5 lore raw

Quoted 24 lines · Ingo Molnar

Quoted 12 lines · Stephane Eranian
Quoted 12 lines
- * This is a per-cpu dynamically allocated data structure.
- */
include/linux/perf_event.h +0 ~~-10~~
-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
Quoted 8 lines
Review
Quoted 2 lines
(quoted diff) +1 -1
-
+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?

Quoted 6 lines

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.

tip-bot for Li Zefan

merge bot

2013-03-06 14:44 UTC #6 lore raw

[tip:perf/core] perf: Remove include of cgroup.h from perf_event.h

In reply to Li Zefan

perf: Remove include of cgroup.h from perf_event.h

Commit-ID:  877c685607925238e302cd3aa38788dca6c1b226
Gitweb:     http://git.kernel.org/tip/877c685607925238e302cd3aa38788dca6c1b226
Author:     Li Zefan <[email protected]>
AuthorDate: Tue, 5 Mar 2013 11:38:08 +0800
Committer:  Ingo Molnar <[email protected]>
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 <[email protected]>

Cc Peter Zijlstra <[email protected]>

Cc Stephane Eranian <[email protected]>

Cc Tejun Heo <[email protected]>

Link http://lkml.kernel.org/r/[email protected] open here

Signed-off-by Ingo Molnar <[email protected]>

---
 include/linux/perf_event.h | 18 +-----------------
 kernel/events/core.c       | 15 +++++++++++++++
 2 files changed, 16 insertions(+), 17 deletions(-)

include/linux/perf_event.h +1 ~~-17~~

index e47ee46..8737e1c 100644
@@ -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;
 
 /**

kernel/events/core.c +15 -0

index b0cd865..5976a2a 100644
@@ -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.