From: He Zhe <[email protected]>
The buffer containing string used to set cpumask is overwritten by end of
string later in cpu_map__snprint_mask due to not enough memory space, when
there is only one cpu. And thus causes the following failure.
$ perf ftrace ls
failed to reset ftrace
This patch fixes the calculation of cpumask string size.
Signed-off-by: He Zhe <[email protected]>
---
tools/perf/builtin-ftrace.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/tools/perf/builtin-ftrace.c b/tools/perf/builtin-ftrace.c
index 66d5a66..0193128 100644
--- a/tools/perf/builtin-ftrace.c
+++ b/tools/perf/builtin-ftrace.c
@@ -173,7 +173,7 @@ static int set_tracing_cpumask(struct cpu_map *cpumap)
int last_cpu;
last_cpu = cpu_map__cpu(cpumap, cpumap->nr - 1);
- mask_size = (last_cpu + 3) / 4 + 1;
+ mask_size = last_cpu / 4 + 2; /* one more byte for EOS */
mask_size += last_cpu / 32; /* ',' is needed for every 32th cpus */
cpumask = malloc(mask_size);
--
2.7.4
On Fri, Aug 02, 2019 at 04:29:51PM +0800, [email protected] wrote:
> From: He Zhe <[email protected]>
>
> The buffer containing string used to set cpumask is overwritten by end of
> string later in cpu_map__snprint_mask due to not enough memory space, when
> there is only one cpu. And thus causes the following failure.
>
> $ perf ftrace ls
> failed to reset ftrace
>
> This patch fixes the calculation of cpumask string size.
>
> Signed-off-by: He Zhe <[email protected]>
> ---
> tools/perf/builtin-ftrace.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/tools/perf/builtin-ftrace.c b/tools/perf/builtin-ftrace.c
> index 66d5a66..0193128 100644
> --- a/tools/perf/builtin-ftrace.c
> +++ b/tools/perf/builtin-ftrace.c
> @@ -173,7 +173,7 @@ static int set_tracing_cpumask(struct cpu_map *cpumap)
> int last_cpu;
>
> last_cpu = cpu_map__cpu(cpumap, cpumap->nr - 1);
> - mask_size = (last_cpu + 3) / 4 + 1;
> + mask_size = last_cpu / 4 + 2; /* one more byte for EOS */
> mask_size += last_cpu / 32; /* ',' is needed for every 32th cpus */
ugh.. why do we care about last_cpu value in here at all?
feels like using static buffer would be more reasonable
jirka
>
> cpumask = malloc(mask_size);
> --
> 2.7.4
>
On 8/2/19 9:06 PM, Jiri Olsa wrote:
> On Fri, Aug 02, 2019 at 04:29:51PM +0800, [email protected] wrote:
>> From: He Zhe <[email protected]>
>>
>> The buffer containing string used to set cpumask is overwritten by end of
>> string later in cpu_map__snprint_mask due to not enough memory space, when
>> there is only one cpu. And thus causes the following failure.
>>
>> $ perf ftrace ls
>> failed to reset ftrace
>>
>> This patch fixes the calculation of cpumask string size.
>>
>> Signed-off-by: He Zhe <[email protected]>
>> ---
>> tools/perf/builtin-ftrace.c | 2 +-
>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/tools/perf/builtin-ftrace.c b/tools/perf/builtin-ftrace.c
>> index 66d5a66..0193128 100644
>> --- a/tools/perf/builtin-ftrace.c
>> +++ b/tools/perf/builtin-ftrace.c
>> @@ -173,7 +173,7 @@ static int set_tracing_cpumask(struct cpu_map *cpumap)
>> int last_cpu;
>>
>> last_cpu = cpu_map__cpu(cpumap, cpumap->nr - 1);
>> - mask_size = (last_cpu + 3) / 4 + 1;
>> + mask_size = last_cpu / 4 + 2; /* one more byte for EOS */
>> mask_size += last_cpu / 32; /* ',' is needed for every 32th cpus */
> ugh.. why do we care about last_cpu value in here at all?
>
> feels like using static buffer would be more reasonable
Thanks, and yes, a static buffer would be easy to handle. A 2KB buffer is enough
to cover 8196 cpus, the maximum numbers of cpus we can run with for now.
Let's see if there is any other concerns.
Zhe
>
> jirka
>
>>
>> cpumask = malloc(mask_size);
>> --
>> 2.7.4
>>
Em Fri, Aug 02, 2019 at 04:29:51PM +0800, [email protected] escreveu:
> From: He Zhe <[email protected]>
>
> The buffer containing string used to set cpumask is overwritten by end of
> string later in cpu_map__snprint_mask due to not enough memory space, when
> there is only one cpu. And thus causes the following failure.
>
> $ perf ftrace ls
> failed to reset ftrace
>
> This patch fixes the calculation of cpumask string size.
Please next time add this as well:
Fixes: dc23103278c5 ("perf ftrace: Add support for -a and -C option")
Applied,
- Arnaldo
> Signed-off-by: He Zhe <[email protected]>
> ---
> tools/perf/builtin-ftrace.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/tools/perf/builtin-ftrace.c b/tools/perf/builtin-ftrace.c
> index 66d5a66..0193128 100644
> --- a/tools/perf/builtin-ftrace.c
> +++ b/tools/perf/builtin-ftrace.c
> @@ -173,7 +173,7 @@ static int set_tracing_cpumask(struct cpu_map *cpumap)
> int last_cpu;
>
> last_cpu = cpu_map__cpu(cpumap, cpumap->nr - 1);
> - mask_size = (last_cpu + 3) / 4 + 1;
> + mask_size = last_cpu / 4 + 2; /* one more byte for EOS */
> mask_size += last_cpu / 32; /* ',' is needed for every 32th cpus */
>
> cpumask = malloc(mask_size);
> --
> 2.7.4
--
- Arnaldo
Commit-ID: cf30ae726c011e0372fd4c2d588466c8b50a8907
Gitweb: https://git.kernel.org/tip/cf30ae726c011e0372fd4c2d588466c8b50a8907
Author: He Zhe <[email protected]>
AuthorDate: Fri, 2 Aug 2019 16:29:51 +0800
Committer: Arnaldo Carvalho de Melo <[email protected]>
CommitDate: Thu, 8 Aug 2019 15:41:10 -0300
perf ftrace: Fix failure to set cpumask when only one cpu is present
The buffer containing the string used to set cpumask is overwritten at
the end of the string later in cpu_map__snprint_mask due to not enough
memory space, when there is only one cpu.
And thus causes the following failure:
$ perf ftrace ls
failed to reset ftrace
$
This patch fixes the calculation of the cpumask string size.
Signed-off-by: He Zhe <[email protected]>
Tested-by: Arnaldo Carvalho de Melo <[email protected]>
Cc: Alexander Shishkin <[email protected]>
Cc: Alexey Budankov <[email protected]>
Cc: Jiri Olsa <[email protected]>
Cc: Kan Liang <[email protected]>
Cc: Namhyung Kim <[email protected]>
Cc: Peter Zijlstra <[email protected]>
Cc: Stephane Eranian <[email protected]>
Fixes: dc23103278c5 ("perf ftrace: Add support for -a and -C option")
Link: http://lkml.kernel.org/r/[email protected]
Signed-off-by: Arnaldo Carvalho de Melo <[email protected]>
---
tools/perf/builtin-ftrace.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/tools/perf/builtin-ftrace.c b/tools/perf/builtin-ftrace.c
index 66d5a6658daf..019312810405 100644
--- a/tools/perf/builtin-ftrace.c
+++ b/tools/perf/builtin-ftrace.c
@@ -173,7 +173,7 @@ static int set_tracing_cpumask(struct cpu_map *cpumap)
int last_cpu;
last_cpu = cpu_map__cpu(cpumap, cpumap->nr - 1);
- mask_size = (last_cpu + 3) / 4 + 1;
+ mask_size = last_cpu / 4 + 2; /* one more byte for EOS */
mask_size += last_cpu / 32; /* ',' is needed for every 32th cpus */
cpumask = malloc(mask_size);