[PATCH] perf: ignore exited thread when synthesize thread map

[PATCH] perf: ignore exited thread when synthesize thread map

[Cruz Zhao]

When synthesize thread map, some threads in thread map may have
already exited, so that __event__synthesize_thread() returns -1
and the synthesis breaks. However, It will not have any effect
if we just ignore the exited thread. So just ignore it and continue.

Signed-off-by: Cruz Zhao <CruzZhao@linux.alibaba.com>
---
 tools/perf/util/synthetic-events.c | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

diff --git a/tools/perf/util/synthetic-events.c b/tools/perf/util/synthetic-events.c
index a0579c7d7b9e..43ad2298a933 100644
--- a/tools/perf/util/synthetic-events.c
+++ b/tools/perf/util/synthetic-events.c
@@ -866,14 +866,16 @@ int perf_event__synthesize_thread_map(struct perf_tool *tool,
 
 	err = 0;
 	for (thread = 0; thread < threads->nr; ++thread) {
+		/*
+		 * We may race with exiting thread, so don't stop just because
+		 * one thread couldn't be synthesized.
+		 */
 		if (__event__synthesize_thread(comm_event, mmap_event,
 					       fork_event, namespaces_event,
 					       perf_thread_map__pid(threads, thread), 0,
 					       process, tool, machine,
-					       needs_mmap, mmap_data)) {
-			err = -1;
-			break;
-		}
+					       needs_mmap, mmap_data))
+			continue;
 
 		/*
 		 * comm.pid is set to thread group id by
-- 
2.39.3

Re: [PATCH] perf: ignore exited thread when synthesize thread map

[Namhyung Kim]

Hello,

On Tue, Nov 21, 2023 at 6:22 PM Cruz Zhao <CruzZhao@linux.alibaba.com> wrote:
>
> When synthesize thread map, some threads in thread map may have
> already exited, so that __event__synthesize_thread() returns -1
> and the synthesis breaks. However, It will not have any effect
> if we just ignore the exited thread. So just ignore it and continue.

Looks ok.  But I guess you want to do the same for the leader
thread below as well.

Thanks,
Namhyung

>
> Signed-off-by: Cruz Zhao <CruzZhao@linux.alibaba.com>
> ---
>  tools/perf/util/synthetic-events.c | 10 ++++++----
>  1 file changed, 6 insertions(+), 4 deletions(-)
>
> diff --git a/tools/perf/util/synthetic-events.c b/tools/perf/util/synthetic-events.c
> index a0579c7d7b9e..43ad2298a933 100644
> --- a/tools/perf/util/synthetic-events.c
> +++ b/tools/perf/util/synthetic-events.c
> @@ -866,14 +866,16 @@ int perf_event__synthesize_thread_map(struct perf_tool *tool,
>
>         err = 0;
>         for (thread = 0; thread < threads->nr; ++thread) {
> +               /*
> +                * We may race with exiting thread, so don't stop just because
> +                * one thread couldn't be synthesized.
> +                */
>                 if (__event__synthesize_thread(comm_event, mmap_event,
>                                                fork_event, namespaces_event,
>                                                perf_thread_map__pid(threads, thread), 0,
>                                                process, tool, machine,
> -                                              needs_mmap, mmap_data)) {
> -                       err = -1;
> -                       break;
> -               }
> +                                              needs_mmap, mmap_data))
> +                       continue;
>
>                 /*
>                  * comm.pid is set to thread group id by
> --
> 2.39.3
>

Re: [PATCH] perf: ignore exited thread when synthesize thread map

[cruzzhao]

在 2023/11/23 上午5:05, Namhyung Kim 写道:
> Hello,
> 
> On Tue, Nov 21, 2023 at 6:22 PM Cruz Zhao <CruzZhao@linux.alibaba.com> wrote:
>>
>> When synthesize thread map, some threads in thread map may have
>> already exited, so that __event__synthesize_thread() returns -1
>> and the synthesis breaks. However, It will not have any effect
>> if we just ignore the exited thread. So just ignore it and continue.
> 
> Looks ok.  But I guess you want to do the same for the leader
> thread below as well.
> 
> Thanks,
> Namhyung
> 

With my testcase, no error is returned even if we don't do the same for
the leader thread blow. Well, I'll check whether the logic is still
correct if we do so.

Many thanks for reviewing.

Best,
Cruz Zhao

Re: [PATCH] perf: ignore exited thread when synthesize thread map

[Ian Rogers]

On Mon, Nov 27, 2023 at 10:23 PM cruzzhao <cruzzhao@linux.alibaba.com> wrote:
>
>
>
> 在 2023/11/23 上午5:05, Namhyung Kim 写道:
> > Hello,
> >
> > On Tue, Nov 21, 2023 at 6:22 PM Cruz Zhao <CruzZhao@linux.alibaba.com> wrote:
> >>
> >> When synthesize thread map, some threads in thread map may have
> >> already exited, so that __event__synthesize_thread() returns -1
> >> and the synthesis breaks. However, It will not have any effect
> >> if we just ignore the exited thread. So just ignore it and continue.
> >
> > Looks ok.  But I guess you want to do the same for the leader
> > thread below as well.
> >
> > Thanks,
> > Namhyung
> >
>
> With my testcase, no error is returned even if we don't do the same for
> the leader thread blow. Well, I'll check whether the logic is still
> correct if we do so.
>
> Many thanks for reviewing.

Thanks for looking at this. Could you share the test? It looks like
the thread be removed from the thread map to avoid potential future
broken accesses like below:
https://git.kernel.org/pub/scm/linux/kernel/git/perf/perf-tools-next.git/tree/tools/perf/util/synthetic-events.c?h=perf-tools-next#n887

Some of the race will hopefully get narrowed by switching to a less
memory intense readdir:
https://lore.kernel.org/lkml/20231127220902.1315692-7-irogers@google.com/

Threads racing is an issue in this example:
```
$ sudo perf top --stdio -u `whoami`
Error:
The sys_perf_event_open() syscall returned with 3 (No such process)
for event (cycles:P).
/bin/dmesg | grep -i perf may provide additional information.
```

Generally the races are covered by the dummy event that gathers
sideband data like thread creation and exits, which is created prior
to synthesis. It would be nice to have a better threading abstraction
to avoid these races.

Thanks,
Ian

> Best,
> Cruz Zhao

Re: [PATCH] perf: ignore exited thread when synthesize thread map

[Ian Rogers]

On Tue, Nov 28, 2023 at 9:12 AM Ian Rogers <irogers@google.com> wrote:
>
> On Mon, Nov 27, 2023 at 10:23 PM cruzzhao <cruzzhao@linux.alibaba.com> wrote:
> >
> >
> >
> > 在 2023/11/23 上午5:05, Namhyung Kim 写道:
> > > Hello,
> > >
> > > On Tue, Nov 21, 2023 at 6:22 PM Cruz Zhao <CruzZhao@linux.alibaba.com> wrote:
> > >>
> > >> When synthesize thread map, some threads in thread map may have
> > >> already exited, so that __event__synthesize_thread() returns -1
> > >> and the synthesis breaks. However, It will not have any effect
> > >> if we just ignore the exited thread. So just ignore it and continue.
> > >
> > > Looks ok.  But I guess you want to do the same for the leader
> > > thread below as well.
> > >
> > > Thanks,
> > > Namhyung
> > >
> >
> > With my testcase, no error is returned even if we don't do the same for
> > the leader thread blow. Well, I'll check whether the logic is still
> > correct if we do so.
> >
> > Many thanks for reviewing.
>
> Thanks for looking at this. Could you share the test? It looks like
> the thread be removed from the thread map to avoid potential future
> broken accesses like below:
> https://git.kernel.org/pub/scm/linux/kernel/git/perf/perf-tools-next.git/tree/tools/perf/util/synthetic-events.c?h=perf-tools-next#n887
>
> Some of the race will hopefully get narrowed by switching to a less
> memory intense readdir:
> https://lore.kernel.org/lkml/20231127220902.1315692-7-irogers@google.com/
>
> Threads racing is an issue in this example:
> ```
> $ sudo perf top --stdio -u `whoami`
> Error:
> The sys_perf_event_open() syscall returned with 3 (No such process)
> for event (cycles:P).
> /bin/dmesg | grep -i perf may provide additional information.
> ```
>
> Generally the races are covered by the dummy event that gathers
> sideband data like thread creation and exits, which is created prior
> to synthesis. It would be nice to have a better threading abstraction
> to avoid these races.
>
> Thanks,
> Ian

Fwiw, we hit more of these issues when running the test suite in
parallel. I posted changes to do that along with a similar fix:
https://lore.kernel.org/lkml/20231201235031.475293-1-irogers@google.com/

Thanks,
Ian

> > Best,
> > Cruz Zhao