Possible overwriting of errno

Possible overwriting of errno

[Emil Berg]

Hi!

I'm getting the error "Failed to start threads: File exists" from perf, probably EEXIST.

I just want to discuss the changes to tools/perf/builtin-record.c made Feb 10 2022 with:
perf record: Start threads in the beginning of trace streaming
SHA: 3217e9fecf118d5dcabdd68d91e0c6afcb4c3e1b

At line 2014 pthread_create() is run and on line 2017 strerror(errno) is printed. Between line 2014 and 2017 record__terminate_thread() is run.

I just think record__terminate_thread() run in-between looks like it may overwrite errno, thus messing up the error message. To be clear I think the error message should come from failure of thread creation and not from failure of thread termination. Can someone enlighten me here?

if (pthread_create(&handle, &attrs, record__thread, &thread_data[t])) {
    for (tt = 1; tt < t; tt++)
        record__terminate_thread(&thread_data[t]);
    pr_err("Failed to start threads: %s\n", strerror(errno));

/Emil Berg

Re: Possible overwriting of errno

[Jiri Olsa]

On Wed, Jun 29, 2022 at 05:31:42AM +0000, Emil Berg wrote:
> Hi!
> 
> I'm getting the error "Failed to start threads: File exists" from perf, probably EEXIST.
> 
> I just want to discuss the changes to tools/perf/builtin-record.c made Feb 10 2022 with:
> perf record: Start threads in the beginning of trace streaming
> SHA: 3217e9fecf118d5dcabdd68d91e0c6afcb4c3e1b
> 
> At line 2014 pthread_create() is run and on line 2017 strerror(errno) is printed. Between line 2014 and 2017 record__terminate_thread() is run.
> 
> I just think record__terminate_thread() run in-between looks like it may overwrite errno, thus messing up the error message. To be clear I think the error message should come from failure of thread creation and not from failure of thread termination. Can someone enlighten me here?
> 
> if (pthread_create(&handle, &attrs, record__thread, &thread_data[t])) {
>     for (tt = 1; tt < t; tt++)
>         record__terminate_thread(&thread_data[t]);
>     pr_err("Failed to start threads: %s\n", strerror(errno));
> 
> /Emil Berg

yea, seems wrong.. could you try patch below?

thanks,
jirka


---
diff --git a/tools/perf/builtin-record.c b/tools/perf/builtin-record.c
index cf5c5379ceaa..158bb0f293d2 100644
--- a/tools/perf/builtin-record.c
+++ b/tools/perf/builtin-record.c
@@ -2103,11 +2103,11 @@ static int record__start_threads(struct record *rec)
 					    MMAP_CPU_MASK_BYTES(&(thread_data[t].mask->affinity)),
 					    (cpu_set_t *)(thread_data[t].mask->affinity.bits));
 #endif
-		if (pthread_create(&handle, &attrs, record__thread, &thread_data[t])) {
+		ret = pthread_create(&handle, &attrs, record__thread, &thread_data[t]);
+		if (ret) {
 			for (tt = 1; tt < t; tt++)
 				record__terminate_thread(&thread_data[t]);
-			pr_err("Failed to start threads: %s\n", strerror(errno));
-			ret = -1;
+			pr_err("Failed to start threads: %s\n", strerror(ret));
 			goto out_err;
 		}
 

RE: Possible overwriting of errno

[Emil Berg]

That's nice. Right now it's a bit cumbersome to test this patch since I'm running perf via vtune in a container...

> -----Original Message-----
> From: Jiri Olsa <olsajiri@gmail.com>
> Sent: den 29 juni 2022 11:45
> To: Emil Berg <emil.berg@ericsson.com>
> Cc: linux-perf-users@vger.kernel.org
> Subject: Re: Possible overwriting of errno
> 
> On Wed, Jun 29, 2022 at 05:31:42AM +0000, Emil Berg wrote:
> > Hi!
> >
> > I'm getting the error "Failed to start threads: File exists" from perf,
> probably EEXIST.
> >
> > I just want to discuss the changes to tools/perf/builtin-record.c made Feb
> 10 2022 with:
> > perf record: Start threads in the beginning of trace streaming
> > SHA: 3217e9fecf118d5dcabdd68d91e0c6afcb4c3e1b
> >
> > At line 2014 pthread_create() is run and on line 2017 strerror(errno) is
> printed. Between line 2014 and 2017 record__terminate_thread() is run.
> >
> > I just think record__terminate_thread() run in-between looks like it may
> overwrite errno, thus messing up the error message. To be clear I think the
> error message should come from failure of thread creation and not from
> failure of thread termination. Can someone enlighten me here?
> >
> > if (pthread_create(&handle, &attrs, record__thread, &thread_data[t])) {
> >     for (tt = 1; tt < t; tt++)
> >         record__terminate_thread(&thread_data[t]);
> >     pr_err("Failed to start threads: %s\n", strerror(errno));
> >
> > /Emil Berg
> 
> yea, seems wrong.. could you try patch below?
> 
> thanks,
> jirka
> 
> 
> ---
> diff --git a/tools/perf/builtin-record.c b/tools/perf/builtin-record.c index
> cf5c5379ceaa..158bb0f293d2 100644
> --- a/tools/perf/builtin-record.c
> +++ b/tools/perf/builtin-record.c
> @@ -2103,11 +2103,11 @@ static int record__start_threads(struct record
> *rec)
> 
> MMAP_CPU_MASK_BYTES(&(thread_data[t].mask->affinity)),
> 
> (cpu_set_t *)(thread_data[t].mask->affinity.bits));
>  #endif
> -		if (pthread_create(&handle, &attrs,
> record__thread, &thread_data[t])) {
> +		ret = pthread_create(&handle, &attrs,
> record__thread, &thread_data[t]);
> +		if (ret) {
>  			for (tt = 1; tt < t; tt++)
> 
> 	record__terminate_thread(&thread_data[t]);
> -			pr_err("Failed to start threads:
> %s\n", strerror(errno));
> -			ret = -1;
> +			pr_err("Failed to start threads:
> %s\n", strerror(ret));
>  			goto out_err;
>  		}
> 

RE: Possible overwriting of errno

[Emil Berg]

Although it's been a while I still think this is a bug. Assuming any posix function, e.g. close() in record__terminate_thread(), can modify errno, it seems like this is an issue which may produce confusing error messages. Will there be a fix for this?

See below.

> -----Original Message-----
> From: Emil Berg
> Sent: den 29 juni 2022 07:32
> To: linux-perf-users@vger.kernel.org
> Subject: Possible overwriting of errno
> 
> Hi!
> 
> I'm getting the error "Failed to start threads: File exists" from perf, probably
> EEXIST.
> 
> I just want to discuss the changes to tools/perf/builtin-record.c made Feb 10
> 2022 with:
> perf record: Start threads in the beginning of trace streaming
> SHA: 3217e9fecf118d5dcabdd68d91e0c6afcb4c3e1b
> 
> At line 2014 pthread_create() is run and on line 2017 strerror(errno) is
> printed. Between line 2014 and 2017 record__terminate_thread() is run.
> 
> I just think record__terminate_thread() run in-between looks like it may
> overwrite errno, thus messing up the error message. To be clear I think the
> error message should come from failure of thread creation and not from
> failure of thread termination. Can someone enlighten me here?
> 
> if (pthread_create(&handle, &attrs, record__thread, &thread_data[t])) {
>     for (tt = 1; tt < t; tt++)
>         record__terminate_thread(&thread_data[t]);
>     pr_err("Failed to start threads: %s\n", strerror(errno));
> 
> /Emil Berg