| Message ID | 20231204182330.654255-1-irogers@google.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v1,1/2] perf metrics: Avoid segv if default metricgroup isn't set | expand |
On Mon, 4 Dec 2023, Ian Rogers wrote: > A metric is default by having "Default" within its groups. The default > metricgroup name needn't be set and this can result in segv in > default_metricgroup_cmp and perf_stat__print_shadow_stats_metricgroup > that assume it has a value when there is a Default metric group. To > avoid the segv initialize the value to "". > > Fixes: 1c0e47956a8e ("perf metrics: Sort the Default metricgroup") > Signed-off-by: Ian Rogers <irogers@google.com> Thanks! I was going to look for the bug but got pulled to other tasks. The patch looks good to me and I tested it successfully on AmpereOne. Reviewed-and-tested-by: Ilkka Koskinen <ilkka@os.amperecomputing.com> Cheers, Ilkka > --- > tools/perf/util/metricgroup.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/tools/perf/util/metricgroup.c b/tools/perf/util/metricgroup.c > index 0484736d9fe4..ca3e0404f187 100644 > --- a/tools/perf/util/metricgroup.c > +++ b/tools/perf/util/metricgroup.c > @@ -225,7 +225,7 @@ static struct metric *metric__new(const struct pmu_metric *pm, > > m->pmu = pm->pmu ?: "cpu"; > m->metric_name = pm->metric_name; > - m->default_metricgroup_name = pm->default_metricgroup_name; > + m->default_metricgroup_name = pm->default_metricgroup_name ?: ""; > m->modifier = NULL; > if (modifier) { > m->modifier = strdup(modifier); > -- > 2.43.0.rc2.451.g8631bc7472-goog > >
Hello, On Mon, Dec 4, 2023 at 2:45 PM Ilkka Koskinen <ilkka@os.amperecomputing.com> wrote: > > > > On Mon, 4 Dec 2023, Ian Rogers wrote: > > A metric is default by having "Default" within its groups. The default > > metricgroup name needn't be set and this can result in segv in > > default_metricgroup_cmp and perf_stat__print_shadow_stats_metricgroup > > that assume it has a value when there is a Default metric group. To > > avoid the segv initialize the value to "". > > > > Fixes: 1c0e47956a8e ("perf metrics: Sort the Default metricgroup") > > Signed-off-by: Ian Rogers <irogers@google.com> > > Thanks! I was going to look for the bug but got pulled to other > tasks. The patch looks good to me and I tested it successfully on > AmpereOne. > > Reviewed-and-tested-by: Ilkka Koskinen <ilkka@os.amperecomputing.com> Looks like it needs to go through perf-tools for v6.7. Ian, do you think this one is enough? Thanks, Namhyung
On Mon, Dec 4, 2023 at 7:33 PM Namhyung Kim <namhyung@kernel.org> wrote: > > Hello, > > On Mon, Dec 4, 2023 at 2:45 PM Ilkka Koskinen > <ilkka@os.amperecomputing.com> wrote: > > > > > > > > On Mon, 4 Dec 2023, Ian Rogers wrote: > > > A metric is default by having "Default" within its groups. The default > > > metricgroup name needn't be set and this can result in segv in > > > default_metricgroup_cmp and perf_stat__print_shadow_stats_metricgroup > > > that assume it has a value when there is a Default metric group. To > > > avoid the segv initialize the value to "". > > > > > > Fixes: 1c0e47956a8e ("perf metrics: Sort the Default metricgroup") > > > Signed-off-by: Ian Rogers <irogers@google.com> > > > > Thanks! I was going to look for the bug but got pulled to other > > tasks. The patch looks good to me and I tested it successfully on > > AmpereOne. > > > > Reviewed-and-tested-by: Ilkka Koskinen <ilkka@os.amperecomputing.com> > > Looks like it needs to go through perf-tools for v6.7. > Ian, do you think this one is enough? From a user's pov the json fix is nicer as otherwise perf stat output for the relevant metrics lacks a heading on the left. This fix is smaller. I'm easy about which to take :-) Thanks, Ian > Thanks, > Namhyung
Em Mon, Dec 04, 2023 at 02:44:54PM -0800, Ilkka Koskinen escreveu: > > > On Mon, 4 Dec 2023, Ian Rogers wrote: > > A metric is default by having "Default" within its groups. The default > > metricgroup name needn't be set and this can result in segv in > > default_metricgroup_cmp and perf_stat__print_shadow_stats_metricgroup > > that assume it has a value when there is a Default metric group. To > > avoid the segv initialize the value to "". > > > > Fixes: 1c0e47956a8e ("perf metrics: Sort the Default metricgroup") > > Signed-off-by: Ian Rogers <irogers@google.com> > > Thanks! I was going to look for the bug but got pulled to other tasks. The > patch looks good to me and I tested it successfully on AmpereOne. > > Reviewed-and-tested-by: Ilkka Koskinen <ilkka@os.amperecomputing.com> Thanks, applied to perf-tools-next. - Arnaldo
Em Mon, Dec 04, 2023 at 07:33:18PM -0800, Namhyung Kim escreveu: > On Mon, Dec 4, 2023 at 2:45 PM Ilkka Koskinen <ilkka@os.amperecomputing.com> wrote: > > On Mon, 4 Dec 2023, Ian Rogers wrote: > > > A metric is default by having "Default" within its groups. The default > > > metricgroup name needn't be set and this can result in segv in > > > default_metricgroup_cmp and perf_stat__print_shadow_stats_metricgroup > > > that assume it has a value when there is a Default metric group. To > > > avoid the segv initialize the value to "". > > > Fixes: 1c0e47956a8e ("perf metrics: Sort the Default metricgroup") > > > Signed-off-by: Ian Rogers <irogers@google.com> > > Thanks! I was going to look for the bug but got pulled to other > > tasks. The patch looks good to me and I tested it successfully on > > AmpereOne. > > Reviewed-and-tested-by: Ilkka Koskinen <ilkka@os.amperecomputing.com> > Looks like it needs to go through perf-tools for v6.7. > Ian, do you think this one is enough? So I had this on my local perf-tools-next, removed now, I also have some other fixes by Ian on the tmp.perf-tools-next, please let me know what you guys decide to have in perf-tools for v6.7 so that I can remove it from there. - Arnaldo
Hi Arnaldo, On Tue, Dec 5, 2023 at 7:51 AM Arnaldo Carvalho de Melo <acme@kernel.org> wrote: > > Em Mon, Dec 04, 2023 at 07:33:18PM -0800, Namhyung Kim escreveu: > > On Mon, Dec 4, 2023 at 2:45 PM Ilkka Koskinen <ilkka@os.amperecomputing.com> wrote: > > > On Mon, 4 Dec 2023, Ian Rogers wrote: > > > > A metric is default by having "Default" within its groups. The default > > > > metricgroup name needn't be set and this can result in segv in > > > > default_metricgroup_cmp and perf_stat__print_shadow_stats_metricgroup > > > > that assume it has a value when there is a Default metric group. To > > > > avoid the segv initialize the value to "". > > > > > Fixes: 1c0e47956a8e ("perf metrics: Sort the Default metricgroup") > > > > Signed-off-by: Ian Rogers <irogers@google.com> > > > > Thanks! I was going to look for the bug but got pulled to other > > > tasks. The patch looks good to me and I tested it successfully on > > > AmpereOne. > > > > Reviewed-and-tested-by: Ilkka Koskinen <ilkka@os.amperecomputing.com> > > > Looks like it needs to go through perf-tools for v6.7. > > Ian, do you think this one is enough? > > So I had this on my local perf-tools-next, removed now, I also have some > other fixes by Ian on the tmp.perf-tools-next, please let me know what > you guys decide to have in perf-tools for v6.7 so that I can remove it > from there. I think we need this one and the Ampere default-metric-group fix. https://lore.kernel.org/r/20231201021550.1109196-2-ilkka@os.amperecomputing.com/ Also perf list JSON fix can go to v6.7. https://lore.kernel.org/r/20231129213428.2227448-2-irogers@google.com/ Thanks, Namhyung
Em Tue, Dec 05, 2023 at 09:24:42AM -0800, Namhyung Kim escreveu: > Hi Arnaldo, > > On Tue, Dec 5, 2023 at 7:51 AM Arnaldo Carvalho de Melo <acme@kernel.org> wrote: > > > > Em Mon, Dec 04, 2023 at 07:33:18PM -0800, Namhyung Kim escreveu: > > > On Mon, Dec 4, 2023 at 2:45 PM Ilkka Koskinen <ilkka@os.amperecomputing.com> wrote: > > > > On Mon, 4 Dec 2023, Ian Rogers wrote: > > > > > A metric is default by having "Default" within its groups. The default > > > > > metricgroup name needn't be set and this can result in segv in > > > > > default_metricgroup_cmp and perf_stat__print_shadow_stats_metricgroup > > > > > that assume it has a value when there is a Default metric group. To > > > > > avoid the segv initialize the value to "". > > > > > > > Fixes: 1c0e47956a8e ("perf metrics: Sort the Default metricgroup") > > > > > Signed-off-by: Ian Rogers <irogers@google.com> > > > > > > Thanks! I was going to look for the bug but got pulled to other > > > > tasks. The patch looks good to me and I tested it successfully on > > > > AmpereOne. > > > > > > Reviewed-and-tested-by: Ilkka Koskinen <ilkka@os.amperecomputing.com> > > > > > Looks like it needs to go through perf-tools for v6.7. > > > Ian, do you think this one is enough? > > > > So I had this on my local perf-tools-next, removed now, I also have some > > other fixes by Ian on the tmp.perf-tools-next, please let me know what > > you guys decide to have in perf-tools for v6.7 so that I can remove it > > from there. > > I think we need this one and the Ampere default-metric-group fix. > > https://lore.kernel.org/r/20231201021550.1109196-2-ilkka@os.amperecomputing.com/ > > Also perf list JSON fix can go to v6.7. > > https://lore.kernel.org/r/20231129213428.2227448-2-irogers@google.com/ Ok, removed both, please augment the later description to: "perf list: Fix JSON segfault by setting the used skip_fuplicate_pmus callback" The original description was vague, improving it a bit like that helps when just looking at the output of "git log --oneline". Thanks, - Arnaldo
On Tue, Dec 5, 2023 at 10:48 AM Arnaldo Carvalho de Melo <acme@kernel.org> wrote: > > Em Tue, Dec 05, 2023 at 09:24:42AM -0800, Namhyung Kim escreveu: > > Hi Arnaldo, > > > > On Tue, Dec 5, 2023 at 7:51 AM Arnaldo Carvalho de Melo <acme@kernel.org> wrote: > > > > > > Em Mon, Dec 04, 2023 at 07:33:18PM -0800, Namhyung Kim escreveu: > > > > On Mon, Dec 4, 2023 at 2:45 PM Ilkka Koskinen <ilkka@os.amperecomputing.com> wrote: > > > > > On Mon, 4 Dec 2023, Ian Rogers wrote: > > > > > > A metric is default by having "Default" within its groups. The default > > > > > > metricgroup name needn't be set and this can result in segv in > > > > > > default_metricgroup_cmp and perf_stat__print_shadow_stats_metricgroup > > > > > > that assume it has a value when there is a Default metric group. To > > > > > > avoid the segv initialize the value to "". > > > > > > > > > Fixes: 1c0e47956a8e ("perf metrics: Sort the Default metricgroup") > > > > > > Signed-off-by: Ian Rogers <irogers@google.com> > > > > > > > > Thanks! I was going to look for the bug but got pulled to other > > > > > tasks. The patch looks good to me and I tested it successfully on > > > > > AmpereOne. > > > > > > > > Reviewed-and-tested-by: Ilkka Koskinen <ilkka@os.amperecomputing.com> > > > > > > > Looks like it needs to go through perf-tools for v6.7. > > > > Ian, do you think this one is enough? > > > > > > So I had this on my local perf-tools-next, removed now, I also have some > > > other fixes by Ian on the tmp.perf-tools-next, please let me know what > > > you guys decide to have in perf-tools for v6.7 so that I can remove it > > > from there. > > > > I think we need this one and the Ampere default-metric-group fix. > > > > https://lore.kernel.org/r/20231201021550.1109196-2-ilkka@os.amperecomputing.com/ > > > > Also perf list JSON fix can go to v6.7. > > > > https://lore.kernel.org/r/20231129213428.2227448-2-irogers@google.com/ > > Ok, removed both, please augment the later description to: > > "perf list: Fix JSON segfault by setting the used skip_fuplicate_pmus callback" > > The original description was vague, improving it a bit like that helps > when just looking at the output of "git log --oneline". Done and pushed to tmp.perf-tools! Thanks, Namhyung
On Tue, Dec 5, 2023 at 11:11 AM Namhyung Kim <namhyung@kernel.org> wrote: > > On Tue, Dec 5, 2023 at 10:48 AM Arnaldo Carvalho de Melo > <acme@kernel.org> wrote: > > > > Em Tue, Dec 05, 2023 at 09:24:42AM -0800, Namhyung Kim escreveu: > > > Hi Arnaldo, > > > > > > On Tue, Dec 5, 2023 at 7:51 AM Arnaldo Carvalho de Melo <acme@kernel.org> wrote: > > > > > > > > Em Mon, Dec 04, 2023 at 07:33:18PM -0800, Namhyung Kim escreveu: > > > > > On Mon, Dec 4, 2023 at 2:45 PM Ilkka Koskinen <ilkka@os.amperecomputing.com> wrote: > > > > > > On Mon, 4 Dec 2023, Ian Rogers wrote: > > > > > > > A metric is default by having "Default" within its groups. The default > > > > > > > metricgroup name needn't be set and this can result in segv in > > > > > > > default_metricgroup_cmp and perf_stat__print_shadow_stats_metricgroup > > > > > > > that assume it has a value when there is a Default metric group. To > > > > > > > avoid the segv initialize the value to "". > > > > > > > > > > > Fixes: 1c0e47956a8e ("perf metrics: Sort the Default metricgroup") > > > > > > > Signed-off-by: Ian Rogers <irogers@google.com> > > > > > > > > > > Thanks! I was going to look for the bug but got pulled to other > > > > > > tasks. The patch looks good to me and I tested it successfully on > > > > > > AmpereOne. > > > > > > > > > > Reviewed-and-tested-by: Ilkka Koskinen <ilkka@os.amperecomputing.com> > > > > > > > > > Looks like it needs to go through perf-tools for v6.7. > > > > > Ian, do you think this one is enough? > > > > > > > > So I had this on my local perf-tools-next, removed now, I also have some > > > > other fixes by Ian on the tmp.perf-tools-next, please let me know what > > > > you guys decide to have in perf-tools for v6.7 so that I can remove it > > > > from there. > > > > > > I think we need this one and the Ampere default-metric-group fix. > > > > > > https://lore.kernel.org/r/20231201021550.1109196-2-ilkka@os.amperecomputing.com/ > > > > > > Also perf list JSON fix can go to v6.7. > > > > > > https://lore.kernel.org/r/20231129213428.2227448-2-irogers@google.com/ > > > > Ok, removed both, please augment the later description to: > > > > "perf list: Fix JSON segfault by setting the used skip_fuplicate_pmus callback" > > > > The original description was vague, improving it a bit like that helps > > when just looking at the output of "git log --oneline". > > Done and pushed to tmp.perf-tools! Thanks Namhyung, there was a typo in Arnaldo's commit message "s/fuplicate/duplicate/" could you fix? Ian > Thanks, > Namhyung
On Tue, Dec 5, 2023 at 11:15 AM Ian Rogers <irogers@google.com> wrote: > > On Tue, Dec 5, 2023 at 11:11 AM Namhyung Kim <namhyung@kernel.org> wrote: > > > > On Tue, Dec 5, 2023 at 10:48 AM Arnaldo Carvalho de Melo > > <acme@kernel.org> wrote: > > > "perf list: Fix JSON segfault by setting the used skip_fuplicate_pmus callback" > > > > > > The original description was vague, improving it a bit like that helps > > > when just looking at the output of "git log --oneline". > > > > Done and pushed to tmp.perf-tools! > > Thanks Namhyung, there was a typo in Arnaldo's commit message > "s/fuplicate/duplicate/" could you fix? Oops, fixed now. Thanks for checking. Namhyung
diff --git a/tools/perf/util/metricgroup.c b/tools/perf/util/metricgroup.c index 0484736d9fe4..ca3e0404f187 100644 --- a/tools/perf/util/metricgroup.c +++ b/tools/perf/util/metricgroup.c @@ -225,7 +225,7 @@ static struct metric *metric__new(const struct pmu_metric *pm, m->pmu = pm->pmu ?: "cpu"; m->metric_name = pm->metric_name; - m->default_metricgroup_name = pm->default_metricgroup_name; + m->default_metricgroup_name = pm->default_metricgroup_name ?: ""; m->modifier = NULL; if (modifier) { m->modifier = strdup(modifier);
A metric is default by having "Default" within its groups. The default metricgroup name needn't be set and this can result in segv in default_metricgroup_cmp and perf_stat__print_shadow_stats_metricgroup that assume it has a value when there is a Default metric group. To avoid the segv initialize the value to "". Fixes: 1c0e47956a8e ("perf metrics: Sort the Default metricgroup") Signed-off-by: Ian Rogers <irogers@google.com> --- tools/perf/util/metricgroup.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)