[patch] perf_counter : add new parameter to set cpu affinity

[patch] perf_counter : add new parameter to set cpu affinity

[Erdem Aktas]

    new parameter to set cpu affinity
    when tracing the cache related performance counters, task migration
    might cause unpredicted cache events. We can use the cpu affinity to
    limit which cpus can be used to migrate the current task. I am
    suggesting -c parameter as in
    ./perf stat -c 1 -- ls -al
    ./perf stat -c 1,2 -- ls -al
    ./perf stat -c 1-3 -- ls -al

    Signed-off-by: eaktas <eaktas1@gmail.com>

diff --git a/tools/perf/builtin-stat.c b/tools/perf/builtin-stat.c
index 27921a8..f55923c 100644
--- a/tools/perf/builtin-stat.c
+++ b/tools/perf/builtin-stat.c
@@ -37,6 +37,7 @@
  * Released under the GPL v2. (and only v2, not any later version)
  */

+#define _GNU_SOURCE
 #include "perf.h"
 #include "builtin.h"
 #include "util/util.h"
@@ -45,6 +46,7 @@

 #include <sys/prctl.h>
 #include <math.h>
+#include <sched.h>

 static struct perf_counter_attr default_attrs[] = {

@@ -242,6 +244,10 @@ static int run_perf_stat(int argc __used, const
char **argv)
 		if (read(go_pipe[0], &buf, 1) == -1)
 			perror("unable to read pipe");

+		if (enableaffinity)
+			sched_setaffinity(pid, sizeof(affinity_mask),
+				&affinity_mask);
+
 		execvp(argv[0], (char **)argv);

 		perror(argv[0]);
@@ -504,6 +510,9 @@ static const struct option options[] = {
 		    "repeat command and print average + stddev (max: 100)"),
 	OPT_BOOLEAN('n', "null", &null_run,
 		    "null run - dont start any counters"),
+	OPT_CALLBACK('c', "cpus", NULL, "n",
+		     "Set cpu affinity i.e: -c 1,3,5-8",
+		     set_cpu_affinity),
 	OPT_END()
 };

@@ -511,6 +520,7 @@ int cmd_stat(int argc, const char **argv, const
char *prefix __used)
 {
 	int status;

+  enableaffinity = 0;
 	argc = parse_options(argc, argv, options, stat_usage, 0);
 	if (!argc)
 		usage_with_options(stat_usage, options);
diff --git a/tools/perf/util/parse-events.c b/tools/perf/util/parse-events.c
index 5184959..8a90117 100644
--- a/tools/perf/util/parse-events.c
+++ b/tools/perf/util/parse-events.c
@@ -1,4 +1,5 @@

+#define _GNU_SOURCE
 #include "../perf.h"
 #include "util.h"
 #include "parse-options.h"
@@ -9,6 +10,8 @@
 extern char *strcasestr(const char *haystack, const char *needle);

 int					nr_counters;
+cpu_set_t affinity_mask;
+int enableaffinity;

 struct perf_counter_attr		attrs[MAX_COUNTERS];

@@ -474,3 +477,49 @@ void print_events(void)

 	exit(129);
 }
+
+/*
+ * Set cpu affinity
+ * Sets which cpus this task can be assigned.
+ * usage can be 1   1,2   1,2,4-8
+ */
+int set_cpu_affinity(const struct option *opt, const char *str, int unset)
+{
+	char *str_dup, *token, *paramlist[30];
+	int paramcount = 0;
+
+	/*I need this dumy if statement, otherwise the compiler gives error */
+	if (opt == NULL && unset == 0) {
+	}
+	str_dup = strdup(str);
+	token = strtok(str_dup, ",");
+	CPU_ZERO(&affinity_mask);
+	while (token) {
+		paramlist[paramcount++] = token;
+		token = strtok(NULL, ",");
+	}
+	while (paramcount > 0) {
+		int number;
+
+		token = strtok(paramlist[--paramcount], "-");
+		number = -1;
+		while (token != NULL) {
+			if (number != -1) {
+				int number2;
+				number2 = atoi(token);
+				while (number < number2) {
+					CPU_SET(++number, &affinity_mask);
+				}
+			} else {
+				number = atoi(token);
+				CPU_SET(number, &affinity_mask);
+			}
+			token = strtok(NULL, "-");
+		}
+	}
+	free(str_dup);
+	enableaffinity = 1;
+	return 0;
+}
+
+
diff --git a/tools/perf/util/parse-events.h b/tools/perf/util/parse-events.h
index e3d5529..c94f0ac 100644
--- a/tools/perf/util/parse-events.h
+++ b/tools/perf/util/parse-events.h
@@ -3,6 +3,12 @@
  * Parse symbolic events/counts passed in as options:
  */

+#include <sched.h>
+
+extern cpu_set_t affinity_mask;
+
+extern int			enableaffinity;
+
 extern int			nr_counters;

 extern struct perf_counter_attr attrs[MAX_COUNTERS];
@@ -15,3 +21,5 @@ extern int parse_events(const struct option *opt,
const char *str, int unset);

 extern void print_events(void);

+extern int set_cpu_affinity(const struct option *opt,
+						const char *str, int unset);
diff --git a/tools/perf/util/util.h b/tools/perf/util/util.h
index b4be607..6db141d 100644
--- a/tools/perf/util/util.h
+++ b/tools/perf/util/util.h
@@ -44,6 +44,7 @@
 #define _XOPEN_SOURCE_EXTENDED 1 /* AIX 5.3L needs this */
 #endif
 #define _ALL_SOURCE 1
+#undef _GNU_SOURCE
 #define _GNU_SOURCE 1
 #define _BSD_SOURCE 1

Re: [patch] perf_counter : add new parameter to set cpu affinity

[Ingo Molnar]

* Erdem Aktas <eaktas1@gmail.com> wrote:

>     new parameter to set cpu affinity
>     when tracing the cache related performance counters, task migration
>     might cause unpredicted cache events. We can use the cpu affinity to
>     limit which cpus can be used to migrate the current task. I am
>     suggesting -c parameter as in
>     ./perf stat -c 1 -- ls -al
>     ./perf stat -c 1,2 -- ls -al
>     ./perf stat -c 1-3 -- ls -al
> 
>     Signed-off-by: eaktas <eaktas1@gmail.com>

thanks Erdem, this patch looks really useful.

Note, the first patch is line-wrapped: please take a look at 
Documentation/email-clients.txt to set up your mailer properly for 
plain-text patches.

The code looks fine, with a few minor things that need to be 
fixed before i can apply it:

> @@ -511,6 +520,7 @@ int cmd_stat(int argc, const char **argv, const
> char *prefix __used)
>  {
>  	int status;
> 
> +  enableaffinity = 0;

please use a tab, not two spaces.

Also, please rename enableaffinity to affinity_enabled - that is 
more readable.

Note, this initialization is probably not needed at all, as the 
variable is global and gets initialized to zero already.

> @@ -9,6 +10,8 @@
>  extern char *strcasestr(const char *haystack, const char *needle);
> 
>  int					nr_counters;
> +cpu_set_t affinity_mask;
> +int enableaffinity;

Please fix the style here, to match the style of existing 
variables:

int					nr_counters;
cpu_set_t				affinity_mask;
int					affinity_enabled;


> 
>  struct perf_counter_attr		attrs[MAX_COUNTERS];
> 
> @@ -474,3 +477,49 @@ void print_events(void)
> 
>  	exit(129);
>  }
> +
> +/*
> + * Set cpu affinity
> + * Sets which cpus this task can be assigned.
> + * usage can be 1   1,2   1,2,4-8
> + */
> +int set_cpu_affinity(const struct option *opt, const char *str, int unset)
> +{
> +	char *str_dup, *token, *paramlist[30];
> +	int paramcount = 0;
> +
> +	/*I need this dumy if statement, otherwise the compiler gives error */
> +	if (opt == NULL && unset == 0) {
> +	}

please add __used tags to 'opt' and 'unset' - that is how we 
annotate unused parameters. (then you can remove this hack)

> +	str_dup = strdup(str);
> +	token = strtok(str_dup, ",");
> +	CPU_ZERO(&affinity_mask);
> +	while (token) {
> +		paramlist[paramcount++] = token;
> +		token = strtok(NULL, ",");
> +	}

this will overflow the paramlist[] array if we have more than 30 
elements in the parameter string.

Please add a PARAM_LIMIT define and bail out with an error if that 
ever gets hit.

> +	while (paramcount > 0) {
> +		int number;
> +
> +		token = strtok(paramlist[--paramcount], "-");
> +		number = -1;
> +		while (token != NULL) {
> +			if (number != -1) {
> +				int number2;
> +				number2 = atoi(token);

you can initialize number2 in the line of definition.

Also, please put an extra newline after local variable definitions.

> +				while (number < number2) {
> +					CPU_SET(++number, &affinity_mask);
> +				}
> +			} else {
> +				number = atoi(token);
> +				CPU_SET(number, &affinity_mask);
> +			}
> +			token = strtok(NULL, "-");
> +		}
> +	}
> +	free(str_dup);
> +	enableaffinity = 1;
> +	return 0;
> +}
> +
> +
> diff --git a/tools/perf/util/parse-events.h b/tools/perf/util/parse-events.h
> index e3d5529..c94f0ac 100644
> --- a/tools/perf/util/parse-events.h
> +++ b/tools/perf/util/parse-events.h
> @@ -3,6 +3,12 @@
>   * Parse symbolic events/counts passed in as options:
>   */
> 
> +#include <sched.h>
> +
> +extern cpu_set_t affinity_mask;
> +
> +extern int			enableaffinity;
> +
>  extern int			nr_counters;


please align this consistently too.

> 
>  extern struct perf_counter_attr attrs[MAX_COUNTERS];
> @@ -15,3 +21,5 @@ extern int parse_events(const struct option *opt,
> const char *str, int unset);
> 
>  extern void print_events(void);
> 
> +extern int set_cpu_affinity(const struct option *opt,
> +						const char *str, int unset);
> diff --git a/tools/perf/util/util.h b/tools/perf/util/util.h
> index b4be607..6db141d 100644
> --- a/tools/perf/util/util.h
> +++ b/tools/perf/util/util.h
> @@ -44,6 +44,7 @@
>  #define _XOPEN_SOURCE_EXTENDED 1 /* AIX 5.3L needs this */
>  #endif
>  #define _ALL_SOURCE 1
> +#undef _GNU_SOURCE
>  #define _GNU_SOURCE 1
>  #define _BSD_SOURCE 1

Why do we undefine _GNU_SOURCE just to define it again in (some) .c 
files?

	Ingo