[PATCH v1] perf header: Fix missing PMU caps

[PATCH v1] perf header: Fix missing PMU caps

[Ian Rogers]

PMU caps are written as HEADER_PMU_CAPS or for the special case of the
PMU "cpu" as HEADER_CPU_PMU_CAPS. As the PMU "cpu" is special, and not
any "core" PMU, the logic had become broken and core PMUs not called
"cpu" were not having their caps written. This affects ARM and s390
non-hybrid PMUs.

Simplify the PMU caps writing logic to scan one fewer time and to be
more explicit in its behavior.

Reported-by: Wei Li <liwei391@huawei.com>
Fixes: 178ddf3bad98 ("perf header: Avoid hybrid PMU list in write_pmu_caps")
Signed-off-by: Ian Rogers <irogers@google.com>
---
 tools/perf/util/header.c | 31 ++++++++++++++++---------------
 1 file changed, 16 insertions(+), 15 deletions(-)

diff --git a/tools/perf/util/header.c b/tools/perf/util/header.c
index 52fbf526fe74..13c71d28e0eb 100644
--- a/tools/perf/util/header.c
+++ b/tools/perf/util/header.c
@@ -1605,8 +1605,15 @@ static int write_pmu_caps(struct feat_fd *ff,
 	int ret;
 
 	while ((pmu = perf_pmus__scan(pmu))) {
-		if (!pmu->name || !strcmp(pmu->name, "cpu") ||
-		    perf_pmu__caps_parse(pmu) <= 0)
+		if (!strcmp(pmu->name, "cpu")) {
+			/*
+			 * The "cpu" PMU is special and covered by
+			 * HEADER_CPU_PMU_CAPS. Note, core PMUs are
+			 * counted/written here for ARM, s390 and Intel hybrid.
+			 */
+			continue;
+		}
+		if (perf_pmu__caps_parse(pmu) <= 0)
 			continue;
 		nr_pmu++;
 	}
@@ -1619,23 +1626,17 @@ static int write_pmu_caps(struct feat_fd *ff,
 		return 0;
 
 	/*
-	 * Write hybrid pmu caps first to maintain compatibility with
-	 * older perf tool.
+	 * Note older perf tools assume core PMUs come first, this is a property
+	 * of perf_pmus__scan.
 	 */
-	if (perf_pmus__num_core_pmus() > 1) {
-		pmu = NULL;
-		while ((pmu = perf_pmus__scan_core(pmu))) {
-			ret = __write_pmu_caps(ff, pmu, true);
-			if (ret < 0)
-				return ret;
-		}
-	}
-
 	pmu = NULL;
 	while ((pmu = perf_pmus__scan(pmu))) {
-		if (pmu->is_core || !pmu->nr_caps)
+		if (!strcmp(pmu->name, "cpu")) {
+			/* Skip as above. */
+			continue;
+		}
+		if (perf_pmu__caps_parse(pmu) <= 0)
 			continue;
-
 		ret = __write_pmu_caps(ff, pmu, true);
 		if (ret < 0)
 			return ret;
-- 
2.42.0.rc1.204.g551eb34607-goog

Re: [PATCH v1] perf header: Fix missing PMU caps

[liwei (GF)]

Hi Ian:

On 2023/8/19 1:19, Ian Rogers wrote:
> PMU caps are written as HEADER_PMU_CAPS or for the special case of the
> PMU "cpu" as HEADER_CPU_PMU_CAPS. As the PMU "cpu" is special, and not
> any "core" PMU, the logic had become broken and core PMUs not called
> "cpu" were not having their caps written. This affects ARM and s390
> non-hybrid PMUs.
> 
> Simplify the PMU caps writing logic to scan one fewer time and to be
> more explicit in its behavior.
> 
> Reported-by: Wei Li <liwei391@huawei.com>
> Fixes: 178ddf3bad98 ("perf header: Avoid hybrid PMU list in write_pmu_caps")
> Signed-off-by: Ian Rogers <irogers@google.com>
> ---
>  tools/perf/util/header.c | 31 ++++++++++++++++---------------
>  1 file changed, 16 insertions(+), 15 deletions(-)
> 
> diff --git a/tools/perf/util/header.c b/tools/perf/util/header.c
> index 52fbf526fe74..13c71d28e0eb 100644
> --- a/tools/perf/util/header.c
> +++ b/tools/perf/util/header.c
> @@ -1605,8 +1605,15 @@ static int write_pmu_caps(struct feat_fd *ff,
>  	int ret;
>  
>  	while ((pmu = perf_pmus__scan(pmu))) {
> -		if (!pmu->name || !strcmp(pmu->name, "cpu") ||
> -		    perf_pmu__caps_parse(pmu) <= 0)
> +		if (!strcmp(pmu->name, "cpu")) {

So you removed the check of 'pmu->name', does this check really redundant? since
we can find such checks in many places in the perf code. If not, i think it is
necessary for strcmp().

> +			/*
> +			 * The "cpu" PMU is special and covered by
> +			 * HEADER_CPU_PMU_CAPS. Note, core PMUs are
> +			 * counted/written here for ARM, s390 and Intel hybrid.
> +			 */
> +			continue;
> +		}
> +		if (perf_pmu__caps_parse(pmu) <= 0)
>  			continue;
>  		nr_pmu++;
>  	}
> @@ -1619,23 +1626,17 @@ static int write_pmu_caps(struct feat_fd *ff,
>  		return 0;
>  
>  	/*
> -	 * Write hybrid pmu caps first to maintain compatibility with
> -	 * older perf tool.
> +	 * Note older perf tools assume core PMUs come first, this is a property
> +	 * of perf_pmus__scan.
>  	 */
> -	if (perf_pmus__num_core_pmus() > 1) {
> -		pmu = NULL;
> -		while ((pmu = perf_pmus__scan_core(pmu))) {
> -			ret = __write_pmu_caps(ff, pmu, true);
> -			if (ret < 0)
> -				return ret;
> -		}
> -	}
> -
>  	pmu = NULL;
>  	while ((pmu = perf_pmus__scan(pmu))) {
> -		if (pmu->is_core || !pmu->nr_caps)
> +		if (!strcmp(pmu->name, "cpu")) {

same here

Thanks,
Wei

> +			/* Skip as above. */
> +			continue;
> +		}
> +		if (perf_pmu__caps_parse(pmu) <= 0)
>  			continue;
> -
>  		ret = __write_pmu_caps(ff, pmu, true);
>  		if (ret < 0)
>  			return ret;

Re: [PATCH v1] perf header: Fix missing PMU caps

[Arnaldo Carvalho de Melo]

Em Sat, Aug 19, 2023 at 12:16:09PM +0800, liwei (GF) escreveu:
> Hi Ian:
> 
> On 2023/8/19 1:19, Ian Rogers wrote:
> > PMU caps are written as HEADER_PMU_CAPS or for the special case of the
> > PMU "cpu" as HEADER_CPU_PMU_CAPS. As the PMU "cpu" is special, and not
> > any "core" PMU, the logic had become broken and core PMUs not called
> > "cpu" were not having their caps written. This affects ARM and s390
> > non-hybrid PMUs.
> > 
> > Simplify the PMU caps writing logic to scan one fewer time and to be
> > more explicit in its behavior.
> > 
> > Reported-by: Wei Li <liwei391@huawei.com>
> > Fixes: 178ddf3bad98 ("perf header: Avoid hybrid PMU list in write_pmu_caps")
> > Signed-off-by: Ian Rogers <irogers@google.com>
> > ---
> >  tools/perf/util/header.c | 31 ++++++++++++++++---------------
> >  1 file changed, 16 insertions(+), 15 deletions(-)
> > 
> > diff --git a/tools/perf/util/header.c b/tools/perf/util/header.c
> > index 52fbf526fe74..13c71d28e0eb 100644
> > --- a/tools/perf/util/header.c
> > +++ b/tools/perf/util/header.c
> > @@ -1605,8 +1605,15 @@ static int write_pmu_caps(struct feat_fd *ff,
> >  	int ret;
> >  
> >  	while ((pmu = perf_pmus__scan(pmu))) {
> > -		if (!pmu->name || !strcmp(pmu->name, "cpu") ||
> > -		    perf_pmu__caps_parse(pmu) <= 0)
> > +		if (!strcmp(pmu->name, "cpu")) {
> 
> So you removed the check of 'pmu->name', does this check really redundant? since
> we can find such checks in many places in the perf code. If not, i think it is
> necessary for strcmp().

Indeed, when sorting in tools/perf/util/pmus.c in cmp_sevent() we have:

        /* Order by PMU name. */
        if (as->pmu != bs->pmu) {
                a_pmu_name = a_pmu_name ?: (as->pmu->name ?: "");
                b_pmu_name = b_pmu_name ?: (bs->pmu->name ?: "");
                ret = strcmp(a_pmu_name, b_pmu_name);
                if (ret)
                        return ret;
        }


And even if in this specific case, for some reason, we could guarantee
that pmu->name isn't NULL, then removing that check should be best left
for a separate patch with an explanation as to why that is safe.

Having it as:

 	while ((pmu = perf_pmus__scan(pmu))) {
-		if (!pmu->name || !strcmp(pmu->name, "cpu") ||
-		    perf_pmu__caps_parse(pmu) <= 0)
+		if (!pmu->name || !strcmp(pmu->name, "cpu")) {

even eases a bit reviewing, as we see we're just removing that
perf_pmu__caps_parse(pmu) line.

Ian?

- Arnaldo

 
> > +			/*
> > +			 * The "cpu" PMU is special and covered by
> > +			 * HEADER_CPU_PMU_CAPS. Note, core PMUs are
> > +			 * counted/written here for ARM, s390 and Intel hybrid.
> > +			 */
> > +			continue;
> > +		}
> > +		if (perf_pmu__caps_parse(pmu) <= 0)
> >  			continue;
> >  		nr_pmu++;
> >  	}
> > @@ -1619,23 +1626,17 @@ static int write_pmu_caps(struct feat_fd *ff,
> >  		return 0;
> >  
> >  	/*
> > -	 * Write hybrid pmu caps first to maintain compatibility with
> > -	 * older perf tool.
> > +	 * Note older perf tools assume core PMUs come first, this is a property
> > +	 * of perf_pmus__scan.
> >  	 */
> > -	if (perf_pmus__num_core_pmus() > 1) {
> > -		pmu = NULL;
> > -		while ((pmu = perf_pmus__scan_core(pmu))) {
> > -			ret = __write_pmu_caps(ff, pmu, true);
> > -			if (ret < 0)
> > -				return ret;
> > -		}
> > -	}
> > -
> >  	pmu = NULL;
> >  	while ((pmu = perf_pmus__scan(pmu))) {
> > -		if (pmu->is_core || !pmu->nr_caps)
> > +		if (!strcmp(pmu->name, "cpu")) {
> 
> same here
> 
> Thanks,
> Wei
> 
> > +			/* Skip as above. */
> > +			continue;
> > +		}
> > +		if (perf_pmu__caps_parse(pmu) <= 0)
> >  			continue;
> > -
> >  		ret = __write_pmu_caps(ff, pmu, true);
> >  		if (ret < 0)
> >  			return ret;

-- 

- Arnaldo

Re: [PATCH v1] perf header: Fix missing PMU caps

[Ian Rogers]

On Mon, Aug 21, 2023 at 6:16 AM Arnaldo Carvalho de Melo
<acme@kernel.org> wrote:
>
> Em Sat, Aug 19, 2023 at 12:16:09PM +0800, liwei (GF) escreveu:
> > Hi Ian:
> >
> > On 2023/8/19 1:19, Ian Rogers wrote:
> > > PMU caps are written as HEADER_PMU_CAPS or for the special case of the
> > > PMU "cpu" as HEADER_CPU_PMU_CAPS. As the PMU "cpu" is special, and not
> > > any "core" PMU, the logic had become broken and core PMUs not called
> > > "cpu" were not having their caps written. This affects ARM and s390
> > > non-hybrid PMUs.
> > >
> > > Simplify the PMU caps writing logic to scan one fewer time and to be
> > > more explicit in its behavior.
> > >
> > > Reported-by: Wei Li <liwei391@huawei.com>
> > > Fixes: 178ddf3bad98 ("perf header: Avoid hybrid PMU list in write_pmu_caps")
> > > Signed-off-by: Ian Rogers <irogers@google.com>
> > > ---
> > >  tools/perf/util/header.c | 31 ++++++++++++++++---------------
> > >  1 file changed, 16 insertions(+), 15 deletions(-)
> > >
> > > diff --git a/tools/perf/util/header.c b/tools/perf/util/header.c
> > > index 52fbf526fe74..13c71d28e0eb 100644
> > > --- a/tools/perf/util/header.c
> > > +++ b/tools/perf/util/header.c
> > > @@ -1605,8 +1605,15 @@ static int write_pmu_caps(struct feat_fd *ff,
> > >     int ret;
> > >
> > >     while ((pmu = perf_pmus__scan(pmu))) {
> > > -           if (!pmu->name || !strcmp(pmu->name, "cpu") ||
> > > -               perf_pmu__caps_parse(pmu) <= 0)
> > > +           if (!strcmp(pmu->name, "cpu")) {
> >
> > So you removed the check of 'pmu->name', does this check really redundant? since
> > we can find such checks in many places in the perf code. If not, i think it is
> > necessary for strcmp().
>
> Indeed, when sorting in tools/perf/util/pmus.c in cmp_sevent() we have:
>
>         /* Order by PMU name. */
>         if (as->pmu != bs->pmu) {
>                 a_pmu_name = a_pmu_name ?: (as->pmu->name ?: "");
>                 b_pmu_name = b_pmu_name ?: (bs->pmu->name ?: "");
>                 ret = strcmp(a_pmu_name, b_pmu_name);
>                 if (ret)
>                         return ret;
>         }
>
>
> And even if in this specific case, for some reason, we could guarantee
> that pmu->name isn't NULL, then removing that check should be best left
> for a separate patch with an explanation as to why that is safe.
>
> Having it as:
>
>         while ((pmu = perf_pmus__scan(pmu))) {
> -               if (!pmu->name || !strcmp(pmu->name, "cpu") ||
> -                   perf_pmu__caps_parse(pmu) <= 0)
> +               if (!pmu->name || !strcmp(pmu->name, "cpu")) {
>
> even eases a bit reviewing, as we see we're just removing that
> perf_pmu__caps_parse(pmu) line.
>
> Ian?

The pmu name is initialized with:

https://git.kernel.org/pub/scm/linux/kernel/git/perf/perf-tools-next.git/tree/tools/perf/util/pmu.c?h=perf-tools-next#n1001
pmu->name = strdup(name);
if (!pmu->name)
goto err;

so name can't be NULL, strdup of NULL is segv, as if it were pmu would
be NULL. I'll clean this up in an additional patch on top of this one.

Thanks,
Ian

> - Arnaldo
>
>
> > > +                   /*
> > > +                    * The "cpu" PMU is special and covered by
> > > +                    * HEADER_CPU_PMU_CAPS. Note, core PMUs are
> > > +                    * counted/written here for ARM, s390 and Intel hybrid.
> > > +                    */
> > > +                   continue;
> > > +           }
> > > +           if (perf_pmu__caps_parse(pmu) <= 0)
> > >                     continue;
> > >             nr_pmu++;
> > >     }
> > > @@ -1619,23 +1626,17 @@ static int write_pmu_caps(struct feat_fd *ff,
> > >             return 0;
> > >
> > >     /*
> > > -    * Write hybrid pmu caps first to maintain compatibility with
> > > -    * older perf tool.
> > > +    * Note older perf tools assume core PMUs come first, this is a property
> > > +    * of perf_pmus__scan.
> > >      */
> > > -   if (perf_pmus__num_core_pmus() > 1) {
> > > -           pmu = NULL;
> > > -           while ((pmu = perf_pmus__scan_core(pmu))) {
> > > -                   ret = __write_pmu_caps(ff, pmu, true);
> > > -                   if (ret < 0)
> > > -                           return ret;
> > > -           }
> > > -   }
> > > -
> > >     pmu = NULL;
> > >     while ((pmu = perf_pmus__scan(pmu))) {
> > > -           if (pmu->is_core || !pmu->nr_caps)
> > > +           if (!strcmp(pmu->name, "cpu")) {
> >
> > same here
> >
> > Thanks,
> > Wei
> >
> > > +                   /* Skip as above. */
> > > +                   continue;
> > > +           }
> > > +           if (perf_pmu__caps_parse(pmu) <= 0)
> > >                     continue;
> > > -
> > >             ret = __write_pmu_caps(ff, pmu, true);
> > >             if (ret < 0)
> > >                     return ret;
>
> --
>
> - Arnaldo

Re: [PATCH v1] perf header: Fix missing PMU caps

[Arnaldo Carvalho de Melo]

Em Thu, Aug 24, 2023 at 03:54:00PM -0700, Ian Rogers escreveu:
> On Mon, Aug 21, 2023 at 6:16 AM Arnaldo Carvalho de Melo
> <acme@kernel.org> wrote:
> >
> > Em Sat, Aug 19, 2023 at 12:16:09PM +0800, liwei (GF) escreveu:
> > > Hi Ian:
> > >
> > > On 2023/8/19 1:19, Ian Rogers wrote:
> > > > PMU caps are written as HEADER_PMU_CAPS or for the special case of the
> > > > PMU "cpu" as HEADER_CPU_PMU_CAPS. As the PMU "cpu" is special, and not
> > > > any "core" PMU, the logic had become broken and core PMUs not called
> > > > "cpu" were not having their caps written. This affects ARM and s390
> > > > non-hybrid PMUs.
> > > >
> > > > Simplify the PMU caps writing logic to scan one fewer time and to be
> > > > more explicit in its behavior.
> > > >
> > > > Reported-by: Wei Li <liwei391@huawei.com>
> > > > Fixes: 178ddf3bad98 ("perf header: Avoid hybrid PMU list in write_pmu_caps")
> > > > Signed-off-by: Ian Rogers <irogers@google.com>
> > > > ---
> > > >  tools/perf/util/header.c | 31 ++++++++++++++++---------------
> > > >  1 file changed, 16 insertions(+), 15 deletions(-)
> > > >
> > > > diff --git a/tools/perf/util/header.c b/tools/perf/util/header.c
> > > > index 52fbf526fe74..13c71d28e0eb 100644
> > > > --- a/tools/perf/util/header.c
> > > > +++ b/tools/perf/util/header.c
> > > > @@ -1605,8 +1605,15 @@ static int write_pmu_caps(struct feat_fd *ff,
> > > >     int ret;
> > > >
> > > >     while ((pmu = perf_pmus__scan(pmu))) {
> > > > -           if (!pmu->name || !strcmp(pmu->name, "cpu") ||
> > > > -               perf_pmu__caps_parse(pmu) <= 0)
> > > > +           if (!strcmp(pmu->name, "cpu")) {
> > >
> > > So you removed the check of 'pmu->name', does this check really redundant? since
> > > we can find such checks in many places in the perf code. If not, i think it is
> > > necessary for strcmp().
> >
> > Indeed, when sorting in tools/perf/util/pmus.c in cmp_sevent() we have:
> >
> >         /* Order by PMU name. */
> >         if (as->pmu != bs->pmu) {
> >                 a_pmu_name = a_pmu_name ?: (as->pmu->name ?: "");
> >                 b_pmu_name = b_pmu_name ?: (bs->pmu->name ?: "");
> >                 ret = strcmp(a_pmu_name, b_pmu_name);
> >                 if (ret)
> >                         return ret;
> >         }
> >
> >
> > And even if in this specific case, for some reason, we could guarantee
> > that pmu->name isn't NULL, then removing that check should be best left
> > for a separate patch with an explanation as to why that is safe.
> >
> > Having it as:
> >
> >         while ((pmu = perf_pmus__scan(pmu))) {
> > -               if (!pmu->name || !strcmp(pmu->name, "cpu") ||
> > -                   perf_pmu__caps_parse(pmu) <= 0)
> > +               if (!pmu->name || !strcmp(pmu->name, "cpu")) {
> >
> > even eases a bit reviewing, as we see we're just removing that
> > perf_pmu__caps_parse(pmu) line.
> >
> > Ian?
> 
> The pmu name is initialized with:
> 
> https://git.kernel.org/pub/scm/linux/kernel/git/perf/perf-tools-next.git/tree/tools/perf/util/pmu.c?h=perf-tools-next#n1001
> pmu->name = strdup(name);
> if (!pmu->name)
> goto err;
> 
> so name can't be NULL, strdup of NULL is segv, as if it were pmu would
> be NULL. I'll clean this up in an additional patch on top of this one.

Ok, perhaps at some point we can introduce a perf_pmu__name_is(pmu,
"cpu") and then have an assert for it not being NULL, as for some time
those tests were performed.

But thanks for checking, I'll apply the patches.

- Arnaldo

 
> Thanks,
> Ian
> 
> > - Arnaldo
> >
> >
> > > > +                   /*
> > > > +                    * The "cpu" PMU is special and covered by
> > > > +                    * HEADER_CPU_PMU_CAPS. Note, core PMUs are
> > > > +                    * counted/written here for ARM, s390 and Intel hybrid.
> > > > +                    */
> > > > +                   continue;
> > > > +           }
> > > > +           if (perf_pmu__caps_parse(pmu) <= 0)
> > > >                     continue;
> > > >             nr_pmu++;
> > > >     }
> > > > @@ -1619,23 +1626,17 @@ static int write_pmu_caps(struct feat_fd *ff,
> > > >             return 0;
> > > >
> > > >     /*
> > > > -    * Write hybrid pmu caps first to maintain compatibility with
> > > > -    * older perf tool.
> > > > +    * Note older perf tools assume core PMUs come first, this is a property
> > > > +    * of perf_pmus__scan.
> > > >      */
> > > > -   if (perf_pmus__num_core_pmus() > 1) {
> > > > -           pmu = NULL;
> > > > -           while ((pmu = perf_pmus__scan_core(pmu))) {
> > > > -                   ret = __write_pmu_caps(ff, pmu, true);
> > > > -                   if (ret < 0)
> > > > -                           return ret;
> > > > -           }
> > > > -   }
> > > > -
> > > >     pmu = NULL;
> > > >     while ((pmu = perf_pmus__scan(pmu))) {
> > > > -           if (pmu->is_core || !pmu->nr_caps)
> > > > +           if (!strcmp(pmu->name, "cpu")) {
> > >
> > > same here
> > >
> > > Thanks,
> > > Wei
> > >
> > > > +                   /* Skip as above. */
> > > > +                   continue;
> > > > +           }
> > > > +           if (perf_pmu__caps_parse(pmu) <= 0)
> > > >                     continue;
> > > > -
> > > >             ret = __write_pmu_caps(ff, pmu, true);
> > > >             if (ret < 0)
> > > >                     return ret;
> >
> > --
> >
> > - Arnaldo

-- 

- Arnaldo