diff mbox series

[4/4] perf stat: Do not use the same cgroup more than once

Message ID 20230104064402.1551516-5-namhyung@kernel.org
State New
Headers
Series perf bpf_counter: A set of random fixes (v1) |

Commit Message

Namhyung Kim Jan. 4, 2023, 6:44 a.m. UTC 
  The --for-each-cgroup can have the same cgroup multiple times, but it makes
bpf counters confused (since they have the same cgroup id), and the last
cgroup events are counted only.  Let's check the cgroup name before adding
a new entry.

Before:
  $ sudo ./perf stat -a --bpf-counters --for-each-cgroup /,/ sleep 1

   Performance counter stats for 'system wide':

       <not counted> msec cpu-clock                        /
       <not counted>      context-switches                 /
       <not counted>      cpu-migrations                   /
       <not counted>      page-faults                      /
       <not counted>      cycles                           /
       <not counted>      instructions                     /
       <not counted>      branches                         /
       <not counted>      branch-misses                    /
            8,016.04 msec cpu-clock                        /                #    7.998 CPUs utilized
               6,152      context-switches                 /                #  767.461 /sec
                 250      cpu-migrations                   /                #   31.187 /sec
                 442      page-faults                      /                #   55.139 /sec
         613,111,487      cycles                           /                #    0.076 GHz
         280,599,604      instructions                     /                #    0.46  insn per cycle
          57,692,724      branches                         /                #    7.197 M/sec
           3,385,168      branch-misses                    /                #    5.87% of all branches

         1.002220125 seconds time elapsed

After:
  $ sudo ./perf stat -a --bpf-counters --for-each-cgroup /,/  sleep 1

   Performance counter stats for 'system wide':

            8,013.38 msec cpu-clock                        /                #    7.998 CPUs utilized
               6,859      context-switches                 /                #  855.944 /sec
                 334      cpu-migrations                   /                #   41.680 /sec
                 345      page-faults                      /                #   43.053 /sec
         782,326,119      cycles                           /                #    0.098 GHz
         471,645,724      instructions                     /                #    0.60  insn per cycle
          94,963,430      branches                         /                #   11.851 M/sec
           3,685,511      branch-misses                    /                #    3.88% of all branches

         1.001864539 seconds time elapsed

Fixes: bb1c15b60b981 ("perf stat: Support regex pattern in --for-each-cgroup")
Signed-off-by: Namhyung Kim <namhyung@kernel.org>
---
 tools/perf/util/cgroup.c | 23 ++++++++++++++++++-----
 1 file changed, 18 insertions(+), 5 deletions(-)

Comments

Arnaldo Carvalho de Melo Jan. 4, 2023, 2:21 p.m. UTC | #1 
Em Tue, Jan 03, 2023 at 10:44:02PM -0800, Namhyung Kim escreveu:
> The --for-each-cgroup can have the same cgroup multiple times, but it makes
> bpf counters confused (since they have the same cgroup id), and the last
> cgroup events are counted only.  Let's check the cgroup name before adding
> a new entry.
> 
> Before:
>   $ sudo ./perf stat -a --bpf-counters --for-each-cgroup /,/ sleep 1
> 
>    Performance counter stats for 'system wide':
> 
>        <not counted> msec cpu-clock                        /
>        <not counted>      context-switches                 /
>        <not counted>      cpu-migrations                   /
>        <not counted>      page-faults                      /
>        <not counted>      cycles                           /
>        <not counted>      instructions                     /
>        <not counted>      branches                         /
>        <not counted>      branch-misses                    /
>             8,016.04 msec cpu-clock                        /                #    7.998 CPUs utilized
>                6,152      context-switches                 /                #  767.461 /sec
>                  250      cpu-migrations                   /                #   31.187 /sec
>                  442      page-faults                      /                #   55.139 /sec
>          613,111,487      cycles                           /                #    0.076 GHz
>          280,599,604      instructions                     /                #    0.46  insn per cycle
>           57,692,724      branches                         /                #    7.197 M/sec
>            3,385,168      branch-misses                    /                #    5.87% of all branches
> 
>          1.002220125 seconds time elapsed
> 
> After:
>   $ sudo ./perf stat -a --bpf-counters --for-each-cgroup /,/  sleep 1
> 
>    Performance counter stats for 'system wide':
> 
>             8,013.38 msec cpu-clock                        /                #    7.998 CPUs utilized
>                6,859      context-switches                 /                #  855.944 /sec
>                  334      cpu-migrations                   /                #   41.680 /sec
>                  345      page-faults                      /                #   43.053 /sec
>          782,326,119      cycles                           /                #    0.098 GHz
>          471,645,724      instructions                     /                #    0.60  insn per cycle
>           94,963,430      branches                         /                #   11.851 M/sec
>            3,685,511      branch-misses                    /                #    3.88% of all branches
> 
>          1.001864539 seconds time elapsed
> 
> Fixes: bb1c15b60b981 ("perf stat: Support regex pattern in --for-each-cgroup")
> Signed-off-by: Namhyung Kim <namhyung@kernel.org>

Tested and appied.

- Arnaldo

> ---
>  tools/perf/util/cgroup.c | 23 ++++++++++++++++++-----
>  1 file changed, 18 insertions(+), 5 deletions(-)
> 
> diff --git a/tools/perf/util/cgroup.c b/tools/perf/util/cgroup.c
> index e99b41f9be45..cd978c240e0d 100644
> --- a/tools/perf/util/cgroup.c
> +++ b/tools/perf/util/cgroup.c
> @@ -224,6 +224,19 @@ static int add_cgroup_name(const char *fpath, const struct stat *sb __maybe_unus
>  	return 0;
>  }
>  
> +static int check_and_add_cgroup_name(const char *fpath)
> +{
> +	struct cgroup_name *cn;
> +
> +	list_for_each_entry(cn, &cgroup_list, list) {
> +		if (!strcmp(cn->name, fpath))
> +			return 0;
> +	}
> +
> +	/* pretend if it's added by ftw() */
> +	return add_cgroup_name(fpath, NULL, FTW_D, NULL);
> +}
> +
>  static void release_cgroup_list(void)
>  {
>  	struct cgroup_name *cn;
> @@ -242,7 +255,7 @@ static int list_cgroups(const char *str)
>  	struct cgroup_name *cn;
>  	char *s;
>  
> -	/* use given name as is - for testing purpose */
> +	/* use given name as is when no regex is given */
>  	for (;;) {
>  		p = strchr(str, ',');
>  		e = p ? p : eos;
> @@ -253,13 +266,13 @@ static int list_cgroups(const char *str)
>  			s = strndup(str, e - str);
>  			if (!s)
>  				return -1;
> -			/* pretend if it's added by ftw() */
> -			ret = add_cgroup_name(s, NULL, FTW_D, NULL);
> +
> +			ret = check_and_add_cgroup_name(s);
>  			free(s);
> -			if (ret)
> +			if (ret < 0)
>  				return -1;
>  		} else {
> -			if (add_cgroup_name("", NULL, FTW_D, NULL) < 0)
> +			if (check_and_add_cgroup_name("/") < 0)
>  				return -1;
>  		}
>  
> -- 
> 2.39.0.314.g84b9a713c41-goog
diff mbox series

Patch

diff --git a/tools/perf/util/cgroup.c b/tools/perf/util/cgroup.c
index e99b41f9be45..cd978c240e0d 100644
--- a/tools/perf/util/cgroup.c
+++ b/tools/perf/util/cgroup.c
@@ -224,6 +224,19 @@  static int add_cgroup_name(const char *fpath, const struct stat *sb __maybe_unus
 	return 0;
 }
 
+static int check_and_add_cgroup_name(const char *fpath)
+{
+	struct cgroup_name *cn;
+
+	list_for_each_entry(cn, &cgroup_list, list) {
+		if (!strcmp(cn->name, fpath))
+			return 0;
+	}
+
+	/* pretend if it's added by ftw() */
+	return add_cgroup_name(fpath, NULL, FTW_D, NULL);
+}
+
 static void release_cgroup_list(void)
 {
 	struct cgroup_name *cn;
@@ -242,7 +255,7 @@  static int list_cgroups(const char *str)
 	struct cgroup_name *cn;
 	char *s;
 
-	/* use given name as is - for testing purpose */
+	/* use given name as is when no regex is given */
 	for (;;) {
 		p = strchr(str, ',');
 		e = p ? p : eos;
@@ -253,13 +266,13 @@  static int list_cgroups(const char *str)
 			s = strndup(str, e - str);
 			if (!s)
 				return -1;
-			/* pretend if it's added by ftw() */
-			ret = add_cgroup_name(s, NULL, FTW_D, NULL);
+
+			ret = check_and_add_cgroup_name(s);
 			free(s);
-			if (ret)
+			if (ret < 0)
 				return -1;
 		} else {
-			if (add_cgroup_name("", NULL, FTW_D, NULL) < 0)
+			if (check_and_add_cgroup_name("/") < 0)
 				return -1;
 		}