[PATCH 1/2] perf: Reset detached siblings' group_flags

[PATCH 1/2] perf: Reset detached siblings' group_flags

[Namhyung Kim]

From: Namhyung Kim <namhyung.kim@lge.com>

Currently if a group_leader event is deleted, the sibling events are
upgraded to singleton events of a same group list.  At this time, the
siblings inherit the leader's group_flags.

However, if the group has mixed hw/sw events the leader's group_flag
does not contain PERF_GROUP_SOFTWARE so sibling sw events will miss
the flag also.  Fix it.

Cc: Jiri Olsa <jolsa@redhat.com>
Cc: Frederic Weisbecker <fweisbec@gmail.com>
Signed-off-by: Namhyung Kim <namhyung@kernel.org>
---
 kernel/events/core.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/kernel/events/core.c b/kernel/events/core.c
index 5c75791d7269..007dfe846d4d 100644
--- a/kernel/events/core.c
+++ b/kernel/events/core.c
@@ -1107,8 +1107,9 @@ static void perf_group_detach(struct perf_event *event)
 			list_move_tail(&sibling->group_entry, list);
 		sibling->group_leader = sibling;
 
-		/* Inherit group flags from the previous leader */
-		sibling->group_flags = event->group_flags;
+		/* Reset group flags for each siblings */
+		sibling->group_flags = is_software_event(sibling) ?
+						PERF_GROUP_SOFTWARE : 0;
 	}
 
 out:
-- 
1.7.11.7

[PATCH 2/2] perf: Fix mixed hw/sw event group initialization

[Namhyung Kim]

From: Namhyung Kim <namhyung.kim@lge.com>

There's a problem with mixed hw/sw group when the leader is a software
event.  For instance:

  $ perf stat -e '{task-clock,cycles,faults}' sleep 1

   Performance counter stats for 'sleep 1':

            0.273436 task-clock                #    0.000 CPUs utilized
             962,965 cycles                    #    3.522 GHz
     <not supported> faults

         1.000804279 seconds time elapsed

Jiri's patch 0231bb533675 ("perf: Fix event group context move") fixed
a part of problem but there's a devil still..

The problem arose when a sw event is added to already moved (to hw
context) group whose leader also is a sw event.  In the above example

 1. task-clock (sw event) is a group leader (has PERF_GROUP_SOFTWARE)
 2. cycles (hw event) is added, so the leader moved to the hw context
 3. faults (sw event) is added but the leader also is a sw event
 4. after find_get_context(), ctx is not same as leader->ctx since the
    leader had moved to the hw context (-EINVAL)

Fix it by adding new PERF_GROUP_MIXED flag and use leader's ctx->pmu
if it's set.

  $ perf -state -e '{task-clock,cycles,faults}' sleep 1

   Performance counter stats for 'sleep 1':

            0.670405 task-clock                #    0.001 CPUs utilized
             933,264 cycles                    #    1.392 GHz
                 176 faults                    #    0.263 M/sec

         1.001506178 seconds time elapsed

Reported-by: Andreas Hollmann <hollmann@in.tum.de>
Cc: Jiri Olsa <jolsa@redhat.com>
Cc: Vince Weaver <vince@deater.net>
Cc: Frederic Weisbecker <fweisbec@gmail.com>
Signed-off-by: Namhyung Kim <namhyung@kernel.org>
---
 include/linux/perf_event.h |  1 +
 kernel/events/core.c       | 37 ++++++++++++++++++++++---------------
 2 files changed, 23 insertions(+), 15 deletions(-)

diff --git a/include/linux/perf_event.h b/include/linux/perf_event.h
index e47ee462c2f2..001a3b64fe61 100644
--- a/include/linux/perf_event.h
+++ b/include/linux/perf_event.h
@@ -285,6 +285,7 @@ typedef void (*perf_overflow_handler_t)(struct perf_event *,
 
 enum perf_group_flag {
 	PERF_GROUP_SOFTWARE		= 0x1,
+	PERF_GROUP_MIXED		= 0x2,
 };
 
 #define SWEVENT_HLIST_BITS		8
diff --git a/kernel/events/core.c b/kernel/events/core.c
index 007dfe846d4d..06266d5ed500 100644
--- a/kernel/events/core.c
+++ b/kernel/events/core.c
@@ -6441,6 +6441,8 @@ out:
  * @pid:		target pid
  * @cpu:		target cpu
  * @group_fd:		group leader event fd
+ * @flags:		flags which controls the meaning of arguments.
+ * 			see PERF_FLAG_*
  */
 SYSCALL_DEFINE5(perf_event_open,
 		struct perf_event_attr __user *, attr_uptr,
@@ -6536,26 +6538,30 @@ SYSCALL_DEFINE5(perf_event_open,
 	 */
 	pmu = event->pmu;
 
-	if (group_leader &&
-	    (is_software_event(event) != is_software_event(group_leader))) {
-		if (is_software_event(event)) {
-			/*
-			 * If event and group_leader are not both a software
-			 * event, and event is, then group leader is not.
-			 *
-			 * Allow the addition of software events to !software
-			 * groups, this is safe because software events never
-			 * fail to schedule.
-			 */
-			pmu = group_leader->pmu;
-		} else if (is_software_event(group_leader) &&
-			   (group_leader->group_flags & PERF_GROUP_SOFTWARE)) {
+	if (group_leader) {
+		if (group_leader->group_flags & PERF_GROUP_SOFTWARE) {
 			/*
 			 * In case the group is a pure software group, and we
 			 * try to add a hardware event, move the whole group to
 			 * the hardware context.
 			 */
-			move_group = 1;
+			if (!is_software_event(event))
+				move_group = 1;
+		} else if (group_leader->group_flags & PERF_GROUP_MIXED) {
+			/*
+			 * The group leader was moved on to a hardware context,
+			 * so move this event also.
+			 */
+			if (is_software_event(event))
+				pmu = group_leader->ctx->pmu;
+		} else if (!is_software_event(group_leader)) {
+			/*
+			 * Allow the addition of software events to !software
+			 * groups, this is safe because software events never
+			 * fail to schedule.
+			 */
+			if (is_software_event(event))
+				pmu = group_leader->pmu;
 		}
 	}
 
@@ -6650,6 +6656,7 @@ SYSCALL_DEFINE5(perf_event_open,
 			perf_install_in_context(ctx, sibling, event->cpu);
 			get_ctx(ctx);
 		}
+		group_leader->group_flags = PERF_GROUP_MIXED;
 	}
 
 	perf_install_in_context(ctx, event, event->cpu);
-- 
1.7.11.7

Re: [PATCH 2/2] perf: Fix mixed hw/sw event group initialization

[Peter Zijlstra]

On Thu, 2013-03-07 at 13:19 +0900, Namhyung Kim wrote:
> From: Namhyung Kim <namhyung.kim@lge.com>
> 
> There's a problem with mixed hw/sw group when the leader is a software
> event.  For instance:

> Jiri's patch 0231bb533675 ("perf: Fix event group context move") fixed
> a part of problem but there's a devil still..
> 
> The problem arose when a sw event is added to already moved (to hw
> context) group whose leader also is a sw event.  In the above example
> 
>  1. task-clock (sw event) is a group leader (has PERF_GROUP_SOFTWARE)
>  2. cycles (hw event) is added, so the leader moved to the hw context
>  3. faults (sw event) is added but the leader also is a sw event
>  4. after find_get_context(), ctx is not same as leader->ctx since the
>     leader had moved to the hw context (-EINVAL)
> 
> Fix it by adding new PERF_GROUP_MIXED flag and use leader's ctx->pmu
> if it's set.

> Reported-by: Andreas Hollmann <hollmann@in.tum.de>
> Cc: Jiri Olsa <jolsa@redhat.com>
> Cc: Vince Weaver <vince@deater.net>
> Cc: Frederic Weisbecker <fweisbec@gmail.com>
> Signed-off-by: Namhyung Kim <namhyung@kernel.org>
> ---
>  include/linux/perf_event.h |  1 +
>  kernel/events/core.c       | 37 ++++++++++++++++++++++---------------
>  2 files changed, 23 insertions(+), 15 deletions(-)
> 
> diff --git a/include/linux/perf_event.h b/include/linux/perf_event.h
> index e47ee462c2f2..001a3b64fe61 100644
> --- a/include/linux/perf_event.h
> +++ b/include/linux/perf_event.h
> @@ -285,6 +285,7 @@ typedef void (*perf_overflow_handler_t)(struct perf_event *,
>  
>  enum perf_group_flag {
>  	PERF_GROUP_SOFTWARE		= 0x1,
> +	PERF_GROUP_MIXED		= 0x2,
>  };
>  
>  #define SWEVENT_HLIST_BITS		8
> diff --git a/kernel/events/core.c b/kernel/events/core.c
> index 007dfe846d4d..06266d5ed500 100644
> --- a/kernel/events/core.c
> +++ b/kernel/events/core.c
> @@ -6441,6 +6441,8 @@ out:
>   * @pid:		target pid
>   * @cpu:		target cpu
>   * @group_fd:		group leader event fd
> + * @flags:		flags which controls the meaning of arguments.
> + * 			see PERF_FLAG_*
>   */
>  SYSCALL_DEFINE5(perf_event_open,
>  		struct perf_event_attr __user *, attr_uptr,
> @@ -6536,26 +6538,30 @@ SYSCALL_DEFINE5(perf_event_open,
>  	 */
>  	pmu = event->pmu;
>  
> -	if (group_leader &&
> -	    (is_software_event(event) != is_software_event(group_leader))) {
> -		if (is_software_event(event)) {
> -			/*
> -			 * If event and group_leader are not both a software
> -			 * event, and event is, then group leader is not.
> -			 *
> -			 * Allow the addition of software events to !software
> -			 * groups, this is safe because software events never
> -			 * fail to schedule.
> -			 */
> -			pmu = group_leader->pmu;
> -		} else if (is_software_event(group_leader) &&
> -			   (group_leader->group_flags & PERF_GROUP_SOFTWARE)) {
> +	if (group_leader) {
> +		if (group_leader->group_flags & PERF_GROUP_SOFTWARE) {
>  			/*
>  			 * In case the group is a pure software group, and we
>  			 * try to add a hardware event, move the whole group to
>  			 * the hardware context.
>  			 */
> -			move_group = 1;
> +			if (!is_software_event(event))
> +				move_group = 1;
> +		} else if (group_leader->group_flags & PERF_GROUP_MIXED) {
> +			/*
> +			 * The group leader was moved on to a hardware context,
> +			 * so move this event also.
> +			 */
> +			if (is_software_event(event))
> +				pmu = group_leader->ctx->pmu;
> +		} else if (!is_software_event(group_leader)) {
> +			/*
> +			 * Allow the addition of software events to !software
> +			 * groups, this is safe because software events never
> +			 * fail to schedule.
> +			 */
> +			if (is_software_event(event))
> +				pmu = group_leader->pmu;
>  		}
>  	}
>  
> @@ -6650,6 +6656,7 @@ SYSCALL_DEFINE5(perf_event_open,
>  			perf_install_in_context(ctx, sibling, event->cpu);
>  			get_ctx(ctx);
>  		}
> +		group_leader->group_flags = PERF_GROUP_MIXED;
>  	}
>  
>  	perf_install_in_context(ctx, event, event->cpu);


This seems reasonable, but I think the perf_group_detach thing needs to
migrate events between pmu's as well to complete the whole mess.

Re: [PATCH 2/2] perf: Fix mixed hw/sw event group initialization

[Namhyung Kim]

Hi Peter,

On Mon, 11 Mar 2013 11:01:14 +0100, Peter Zijlstra wrote:
> On Thu, 2013-03-07 at 13:19 +0900, Namhyung Kim wrote:
>> From: Namhyung Kim <namhyung.kim@lge.com>
>> 
>> There's a problem with mixed hw/sw group when the leader is a software
>> event.  For instance:
>
>> Jiri's patch 0231bb533675 ("perf: Fix event group context move") fixed
>> a part of problem but there's a devil still..
>> 
>> The problem arose when a sw event is added to already moved (to hw
>> context) group whose leader also is a sw event.  In the above example
>> 
>>  1. task-clock (sw event) is a group leader (has PERF_GROUP_SOFTWARE)
>>  2. cycles (hw event) is added, so the leader moved to the hw context
>>  3. faults (sw event) is added but the leader also is a sw event
>>  4. after find_get_context(), ctx is not same as leader->ctx since the
>>     leader had moved to the hw context (-EINVAL)
>> 
>> Fix it by adding new PERF_GROUP_MIXED flag and use leader's ctx->pmu
>> if it's set.
>
>> Reported-by: Andreas Hollmann <hollmann@in.tum.de>
>> Cc: Jiri Olsa <jolsa@redhat.com>
>> Cc: Vince Weaver <vince@deater.net>
>> Cc: Frederic Weisbecker <fweisbec@gmail.com>
>> Signed-off-by: Namhyung Kim <namhyung@kernel.org>
>> ---
>>  include/linux/perf_event.h |  1 +
>>  kernel/events/core.c       | 37 ++++++++++++++++++++++---------------
>>  2 files changed, 23 insertions(+), 15 deletions(-)
>> 
>> diff --git a/include/linux/perf_event.h b/include/linux/perf_event.h
>> index e47ee462c2f2..001a3b64fe61 100644
>> --- a/include/linux/perf_event.h
>> +++ b/include/linux/perf_event.h
>> @@ -285,6 +285,7 @@ typedef void (*perf_overflow_handler_t)(struct perf_event *,
>>  
>>  enum perf_group_flag {
>>  	PERF_GROUP_SOFTWARE		= 0x1,
>> +	PERF_GROUP_MIXED		= 0x2,
>>  };
>>  
>>  #define SWEVENT_HLIST_BITS		8
>> diff --git a/kernel/events/core.c b/kernel/events/core.c
>> index 007dfe846d4d..06266d5ed500 100644
>> --- a/kernel/events/core.c
>> +++ b/kernel/events/core.c
>> @@ -6441,6 +6441,8 @@ out:
>>   * @pid:		target pid
>>   * @cpu:		target cpu
>>   * @group_fd:		group leader event fd
>> + * @flags:		flags which controls the meaning of arguments.
>> + * 			see PERF_FLAG_*
>>   */
>>  SYSCALL_DEFINE5(perf_event_open,
>>  		struct perf_event_attr __user *, attr_uptr,
>> @@ -6536,26 +6538,30 @@ SYSCALL_DEFINE5(perf_event_open,
>>  	 */
>>  	pmu = event->pmu;
>>  
>> -	if (group_leader &&
>> -	    (is_software_event(event) != is_software_event(group_leader))) {
>> -		if (is_software_event(event)) {
>> -			/*
>> -			 * If event and group_leader are not both a software
>> -			 * event, and event is, then group leader is not.
>> -			 *
>> -			 * Allow the addition of software events to !software
>> -			 * groups, this is safe because software events never
>> -			 * fail to schedule.
>> -			 */
>> -			pmu = group_leader->pmu;
>> -		} else if (is_software_event(group_leader) &&
>> -			   (group_leader->group_flags & PERF_GROUP_SOFTWARE)) {
>> +	if (group_leader) {
>> +		if (group_leader->group_flags & PERF_GROUP_SOFTWARE) {
>>  			/*
>>  			 * In case the group is a pure software group, and we
>>  			 * try to add a hardware event, move the whole group to
>>  			 * the hardware context.
>>  			 */
>> -			move_group = 1;
>> +			if (!is_software_event(event))
>> +				move_group = 1;
>> +		} else if (group_leader->group_flags & PERF_GROUP_MIXED) {
>> +			/*
>> +			 * The group leader was moved on to a hardware context,
>> +			 * so move this event also.
>> +			 */
>> +			if (is_software_event(event))
>> +				pmu = group_leader->ctx->pmu;
>> +		} else if (!is_software_event(group_leader)) {
>> +			/*
>> +			 * Allow the addition of software events to !software
>> +			 * groups, this is safe because software events never
>> +			 * fail to schedule.
>> +			 */
>> +			if (is_software_event(event))
>> +				pmu = group_leader->pmu;
>>  		}
>>  	}
>>  
>> @@ -6650,6 +6656,7 @@ SYSCALL_DEFINE5(perf_event_open,
>>  			perf_install_in_context(ctx, sibling, event->cpu);
>>  			get_ctx(ctx);
>>  		}
>> +		group_leader->group_flags = PERF_GROUP_MIXED;
>>  	}
>>  
>>  	perf_install_in_context(ctx, event, event->cpu);
>
>
> This seems reasonable, but I think the perf_group_detach thing needs to
> migrate events between pmu's as well to complete the whole mess.

Hmm.. it seems that it needs whole perf_{remove_from,install_in}_context
step to migrate them, right?  Looks like not a simple job for me. :(

Any advice?

Thanks,
Namhyung

[PATCH] perf tests: Add automated test for mixed type event groups

[Jiri Olsa]

On Thu, Mar 07, 2013 at 01:19:50PM +0900, Namhyung Kim wrote:
> From: Namhyung Kim <namhyung.kim@lge.com>
> 
> There's a problem with mixed hw/sw group when the leader is a software
> event.  For instance:
> 
>   $ perf stat -e '{task-clock,cycles,faults}' sleep 1
> 
>    Performance counter stats for 'sleep 1':
> 
>             0.273436 task-clock                #    0.000 CPUs utilized
>              962,965 cycles                    #    3.522 GHz
>      <not supported> faults
> 
>          1.000804279 seconds time elapsed
> 

as discussed on irc, here's automated test for this,
feel free to change it in any way

jirka


---
Adding automated test for mixed type event groups.

Signed-off-by: Jiri Olsa <jolsa@redhat.com>
---
 tools/perf/Makefile             |    1 +
 tools/perf/tests/builtin-test.c |    4 ++
 tools/perf/tests/mixed-groups.c |  104 +++++++++++++++++++++++++++++++++++++++
 tools/perf/tests/tests.h        |    1 +
 4 files changed, 110 insertions(+), 0 deletions(-)
 create mode 100644 tools/perf/tests/mixed-groups.c

diff --git a/tools/perf/Makefile b/tools/perf/Makefile
index a2108ca..6eb5930 100644
--- a/tools/perf/Makefile
+++ b/tools/perf/Makefile
@@ -497,6 +497,7 @@ LIB_OBJS += $(OUTPUT)tests/evsel-tp-sched.o
 LIB_OBJS += $(OUTPUT)tests/pmu.o
 LIB_OBJS += $(OUTPUT)tests/hists_link.o
 LIB_OBJS += $(OUTPUT)tests/python-use.o
+LIB_OBJS += $(OUTPUT)tests/mixed-groups.o
 
 BUILTIN_OBJS += $(OUTPUT)builtin-annotate.o
 BUILTIN_OBJS += $(OUTPUT)builtin-bench.o
diff --git a/tools/perf/tests/builtin-test.c b/tools/perf/tests/builtin-test.c
index acb98e0..aaec91d 100644
--- a/tools/perf/tests/builtin-test.c
+++ b/tools/perf/tests/builtin-test.c
@@ -78,6 +78,10 @@ static struct test {
 		.func = test__python_use,
 	},
 	{
+		.desc = "Test event mixed groups",
+		.func = test__mixed_groups,
+	},
+	{
 		.func = NULL,
 	},
 };
diff --git a/tools/perf/tests/mixed-groups.c b/tools/perf/tests/mixed-groups.c
new file mode 100644
index 0000000..fe55354
--- /dev/null
+++ b/tools/perf/tests/mixed-groups.c
@@ -0,0 +1,104 @@
+
+#include <linux/compiler.h>
+#include "tests.h"
+#include "debug.h"
+
+#define EVENTS 4
+static int ret;
+
+static int event(int hw, int group)
+{
+	struct perf_event_attr pe;
+	u32 type = hw ? PERF_TYPE_HARDWARE : PERF_TYPE_SOFTWARE;
+	u64 config = hw ? PERF_COUNT_HW_CPU_CYCLES : PERF_COUNT_SW_CPU_CLOCK;
+	int fd;
+
+	memset(&pe, 0, sizeof(struct perf_event_attr));
+	pe.type   = type;
+	pe.config = config;
+	pe.size   = sizeof(struct perf_event_attr);
+
+	pe.disabled = group == -1 ? 1 : 0;
+	pe.exclude_hv = 1;
+
+	fd = sys_perf_event_open(&pe, 0, -1, group, 0);
+
+	pr_debug("%s(%d) ", hw ? "H" : "S", fd);
+
+	return fd;
+}
+
+static long long count(int fd)
+{
+	long long c;
+	int err;
+
+	if (fd < 0)
+		return 0;
+
+	err = read(fd, &c, sizeof(c));
+	if (err != sizeof(c)) {
+		pr_err("failed to read: %d\n", err);
+		ret = TEST_FAIL;
+	}
+
+	return c;
+}
+
+static void __test__mixed_groups(int events)
+{
+#define group fd[0]
+	int fd[EVENTS] = { -1 };
+	int i;
+
+	for (i = 0; i < EVENTS; i++) {
+		int hw = (1 << i) & events;
+
+		fd[i] = event(hw, group);
+		if (!hw && fd[i] < 0) {
+			pr_debug("\n");
+			ret = TEST_FAIL;
+			goto out;
+		}
+	}
+
+	pr_debug("\n  counts: ");
+
+	ioctl(group, PERF_EVENT_IOC_ENABLE, 0);
+
+	while(i++);
+
+	ioctl(group, PERF_EVENT_IOC_DISABLE, 0);
+
+	for (i = 0; i < EVENTS; i++) {
+		int hw = (1 << i) & events;
+		long long c;
+
+		c = count(fd[i]);
+
+		pr_debug("%d(%lld) ", fd[i], c);
+		if (!hw && !c) {
+			pr_debug("no count for SW event");
+			ret = TEST_FAIL;
+			break;
+		}
+	}
+
+	pr_debug("\n");
+
+ out:
+	for (i = 0; i < EVENTS; i++)
+		close(fd[i]);
+#undef group
+}
+
+int test__mixed_groups(void)
+{
+	int i;
+
+	for (i = 0; i < (1 << EVENTS); i++)
+		__test__mixed_groups(i);
+
+	pr_debug("ret %d\n", ret);
+	return ret;
+}
diff --git a/tools/perf/tests/tests.h b/tools/perf/tests/tests.h
index 5de0be1..c4a2f05 100644
--- a/tools/perf/tests/tests.h
+++ b/tools/perf/tests/tests.h
@@ -23,5 +23,6 @@ int test__dso_data(void);
 int test__parse_events(void);
 int test__hists_link(void);
 int test__python_use(void);
+int test__mixed_groups(void);
 
 #endif /* TESTS_H */
-- 
1.7.7.6

Re: [PATCH 1/2] perf: Reset detached siblings' group_flags

[Peter Zijlstra]

On Thu, 2013-03-07 at 13:19 +0900, Namhyung Kim wrote:
> From: Namhyung Kim <namhyung.kim@lge.com>
> 
> Currently if a group_leader event is deleted, the sibling events are
> upgraded to singleton events of a same group list.  At this time, the
> siblings inherit the leader's group_flags.
> 
> However, if the group has mixed hw/sw events the leader's group_flag
> does not contain PERF_GROUP_SOFTWARE so sibling sw events will miss
> the flag also.  Fix it.
> 
> Cc: Jiri Olsa <jolsa@redhat.com>
> Cc: Frederic Weisbecker <fweisbec@gmail.com>
> Signed-off-by: Namhyung Kim <namhyung@kernel.org>
> ---
>  kernel/events/core.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/kernel/events/core.c b/kernel/events/core.c
> index 5c75791d7269..007dfe846d4d 100644
> --- a/kernel/events/core.c
> +++ b/kernel/events/core.c
> @@ -1107,8 +1107,9 @@ static void perf_group_detach(struct perf_event *event)
>  			list_move_tail(&sibling->group_entry, list);
>  		sibling->group_leader = sibling;
>  
> -		/* Inherit group flags from the previous leader */
> -		sibling->group_flags = event->group_flags;
> +		/* Reset group flags for each siblings */
> +		sibling->group_flags = is_software_event(sibling) ?
> +						PERF_GROUP_SOFTWARE : 0;
>  	}


In such a case, does the event not continue to live on the hw pmu? That
is, after this patch we'll have a software event on the hardware pmu
list with PERF_GROUP_SOFTWARE set, right? Seems odd.

Re: [PATCH 1/2] perf: Reset detached siblings' group_flags

[Namhyung Kim]

On Mon, 11 Mar 2013 10:59:44 +0100, Peter Zijlstra wrote:
> On Thu, 2013-03-07 at 13:19 +0900, Namhyung Kim wrote:
>> From: Namhyung Kim <namhyung.kim@lge.com>
>> 
>> Currently if a group_leader event is deleted, the sibling events are
>> upgraded to singleton events of a same group list.  At this time, the
>> siblings inherit the leader's group_flags.
>> 
>> However, if the group has mixed hw/sw events the leader's group_flag
>> does not contain PERF_GROUP_SOFTWARE so sibling sw events will miss
>> the flag also.  Fix it.
>> 
>> Cc: Jiri Olsa <jolsa@redhat.com>
>> Cc: Frederic Weisbecker <fweisbec@gmail.com>
>> Signed-off-by: Namhyung Kim <namhyung@kernel.org>
>> ---
>>  kernel/events/core.c | 5 +++--
>>  1 file changed, 3 insertions(+), 2 deletions(-)
>> 
>> diff --git a/kernel/events/core.c b/kernel/events/core.c
>> index 5c75791d7269..007dfe846d4d 100644
>> --- a/kernel/events/core.c
>> +++ b/kernel/events/core.c
>> @@ -1107,8 +1107,9 @@ static void perf_group_detach(struct perf_event *event)
>>  			list_move_tail(&sibling->group_entry, list);
>>  		sibling->group_leader = sibling;
>>  
>> -		/* Inherit group flags from the previous leader */
>> -		sibling->group_flags = event->group_flags;
>> +		/* Reset group flags for each siblings */
>> +		sibling->group_flags = is_software_event(sibling) ?
>> +						PERF_GROUP_SOFTWARE : 0;
>>  	}
>
>
> In such a case, does the event not continue to live on the hw pmu? That
> is, after this patch we'll have a software event on the hardware pmu
> list with PERF_GROUP_SOFTWARE set, right? Seems odd.

Right.

Just curious that dropping this one and applying just patch 2 will set
the PERF_GROUP_MIXED for them and it'll fix the mixed group problem
without migrating those events.  What do you think about this approach?

Thanks,
Namhyung

Re: [PATCH 1/2] perf: Reset detached siblings' group_flags

[Namhyung Kim]

While looking at the code again, I found that the detached siblings
should call perf_event__header_size() for themselves in the loop since
after moving them to the list the previous leader will have empty
sibling_list.  Something like below is needed IMHO:


diff --git a/kernel/events/core.c b/kernel/events/core.c
index dc96fee942a6..53ac21cc4012 100644
--- a/kernel/events/core.c
+++ b/kernel/events/core.c
@@ -1100,6 +1100,13 @@ static void perf_group_detach(struct perf_event *event)
 
 		/* Inherit group flags from the previous leader */
 		sibling->group_flags = event->group_flags;
+
+		/*
+		 * The leader will forget about siblings from now on.
+		 * So below loop to update header size of each sibling won't
+		 * work for this case. Do it directly.
+		 */
+		perf_event__header_size(sibling);
 	}
 
 out: