[PATCH] perf mem: add priv level filtering support

[PATCH] perf mem: add priv level filtering support

[Stephane Eranian]

perf mem: add priv level filtering support

This patch adds the -u -and -k options to perf to allow
filtering of load/store sampling based on priv levels.
This may not be supported by all HW platforms.

By default, loads/stores are sampled at both user and
kernel privilege levels.

To sample only at the user level:
$ perf mem -u -t load rec ......

To sample only at the kernel level:
$ perf mem -k -t load rec ......

Man page updated accordingly.

Signed-off-by: Stephane Eranian <eranian@google.com>
---

diff --git a/tools/perf/Documentation/perf-mem.txt b/tools/perf/Documentation/perf-mem.txt
index 888d511..4c4e405 100644
--- a/tools/perf/Documentation/perf-mem.txt
+++ b/tools/perf/Documentation/perf-mem.txt
@@ -43,6 +43,12 @@ OPTIONS
 	option can be passed in record mode. It will be interpreted the same way as perf
 	record.
 
+-k::
+	Only sample loads/stores at the user level (default: user + kernel)
+
+-u::
+	Only sample loads/stores at the kernel level (default: user + kernel)
+
 SEE ALSO
 --------
 linkperf:perf-record[1], linkperf:perf-report[1]
diff --git a/tools/perf/builtin-mem.c b/tools/perf/builtin-mem.c
index 706a1fa..8ac9d1e 100644
--- a/tools/perf/builtin-mem.c
+++ b/tools/perf/builtin-mem.c
@@ -9,13 +9,18 @@
 #define MEM_OPERATION_LOAD	"load"
 #define MEM_OPERATION_STORE	"store"
 
-static const char	*mem_operation		= MEM_OPERATION_LOAD;
+#define OP_LOAD		0x1
+#define OP_STORE	0x2
+
 
 struct perf_mem {
 	struct perf_tool	tool;
 	char const		*input_name;
 	bool			hide_unresolved;
+	const char		*mem_op;
 	bool			dump_raw;
+	bool			user;
+	bool			kernel;
 	const char		*cpu_list;
 	DECLARE_BITMAP(cpu_bitmap, MAX_NR_CPUS);
 };
@@ -25,35 +30,88 @@ static const char * const mem_usage[] = {
 	NULL
 };
 
-static int __cmd_record(int argc, const char **argv)
+static inline const char *get_plm(struct perf_mem *mem)
+{
+	const char *plm = "";
+
+	if (mem->user && !mem->kernel) {
+		plm = "u";
+	} else if (!mem->user && mem->kernel) {
+		plm = "k";
+	}
+	return plm;
+}
+
+static int __cmd_record(int argc, const char **argv, struct perf_mem *mem)
 {
 	int rec_argc, i = 0, j;
 	const char **rec_argv;
-	char event[64];
-	int ret;
+	char *str;
+	int mode = 0;
+	int ki, ret;
+
+
+	if (!strcmp(mem->mem_op, MEM_OPERATION_STORE))
+		mode |= OP_STORE;
+	else if (!strcmp(mem->mem_op, MEM_OPERATION_LOAD))
+		mode |= OP_LOAD;
+	else  {
+		fprintf(stderr, "unknown sampling mode: %s\n", mem->mem_op);
+		return -1;
+	}
 
-	rec_argc = argc + 4;
+	rec_argc = argc + 6;
 	rec_argv = calloc(rec_argc + 1, sizeof(char *));
 	if (!rec_argv)
 		return -1;
 
 	rec_argv[i++] = strdup("record");
-	if (!strcmp(mem_operation, MEM_OPERATION_LOAD))
-		rec_argv[i++] = strdup("-W");
+
 	rec_argv[i++] = strdup("-d");
-	rec_argv[i++] = strdup("-e");
 
-	if (strcmp(mem_operation, MEM_OPERATION_LOAD))
-		sprintf(event, "cpu/mem-stores/pp");
-	else
-		sprintf(event, "cpu/mem-loads/pp");
+        if (mode & OP_LOAD) {
+                rec_argv[i++] = strdup("-W");
 
-	rec_argv[i++] = strdup(event);
-	for (j = 1; j < argc; j++, i++)
-		rec_argv[i] = argv[j];
+                rec_argv[i++] = strdup("-e");
+
+                str = malloc(strlen("cpu/mem-loads/pp") + 1 + 1);
+                if (!str) {
+                        ki = i;
+                        ret = -1;
+                        goto end;
+                }
+                sprintf(str, "cpu/mem-loads/%spp", get_plm(mem));
+                rec_argv[i++] = str;
+        }
+
+        if (mode & OP_STORE) {
+                rec_argv[i++] = strdup("-e");
+
+                str = malloc(strlen("cpu/mem-stores/pp") + 1 + 1);
+                if (!str) {
+                        ki = i;
+                        ret = -1;
+                        goto end;
+                }
+                sprintf(str, "cpu/mem-stores/%spp", get_plm(mem));
+                rec_argv[i++] = str;
+        }
+
+        /* arguments after i are not malloc'd */
+        ki = i;
 
-	ret = cmd_record(i, rec_argv, NULL);
+	for (j = 1; j < argc; j++, ki++)
+		rec_argv[ki] = argv[j];
+
+	ret = cmd_record(ki, rec_argv, NULL);
+
+end:
+	/*
+	 * XXX: free rec_argv[] entries, difficult because
+	 * cmd_record() drops some of them...
+	 */
 	free(rec_argv);
+
 	return ret;
 }
 
@@ -171,7 +229,7 @@ static int report_events(int argc, const char **argv, struct perf_mem *mem)
 	 * there is no weight (cost) associated with stores, so don't print
 	 * the column
 	 */
-	if (strcmp(mem_operation, MEM_OPERATION_LOAD))
+	if (strcmp(mem->mem_op, MEM_OPERATION_LOAD))
 		rep_argv[i++] = strdup("--sort=mem,sym,dso,symbol_daddr,"
 				       "dso_daddr,tlb,locked");
 
@@ -199,7 +257,7 @@ int cmd_mem(int argc, const char **argv, const char *prefix __maybe_unused)
 		.input_name		 = "perf.data",
 	};
 	const struct option mem_options[] = {
-	OPT_STRING('t', "type", &mem_operation,
+	OPT_STRING('t', "type", &mem.mem_op,
 		   "type", "memory operations(load/store)"),
 	OPT_BOOLEAN('D', "dump-raw-samples", &mem.dump_raw,
 		    "dump raw samples in ASCII"),
@@ -213,13 +271,18 @@ int cmd_mem(int argc, const char **argv, const char *prefix __maybe_unused)
 		   "separator",
 		   "separator for columns, no spaces will be added"
 		   " between columns '.' is reserved."),
+	OPT_BOOLEAN('u', "user-level", &mem.user,
+		    "include user-level accesses"),
+	OPT_BOOLEAN('k', "kernel-level", &mem.kernel,
+		    "include kernel-level accesses"),
 	OPT_END()
 	};
 
 	argc = parse_options(argc, argv, mem_options, mem_usage,
 			     PARSE_OPT_STOP_AT_NON_OPTION);
 
-	if (!argc || !(strncmp(argv[0], "rec", 3) || mem_operation))
+	if (!argc || !(strncmp(argv[0], "rec", 3)
+		       || strncmp(argv[0], "rep", 3)))
 		usage_with_options(mem_usage, mem_options);
 
 	if (!mem.input_name || !strlen(mem.input_name)) {
@@ -228,9 +291,12 @@ int cmd_mem(int argc, const char **argv, const char *prefix __maybe_unused)
 		else
 			mem.input_name = "perf.data";
 	}
+	/* default to load only, some processors only support loads */
+	if (!mem.mem_op)
+		mem.mem_op = MEM_OPERATION_LOAD;
 
 	if (!strncmp(argv[0], "rec", 3))
-		return __cmd_record(argc, argv);
+		return __cmd_record(argc, argv, &mem);
 	else if (!strncmp(argv[0], "rep", 3))
 		return report_events(argc, argv, &mem);
 	else

Re: [PATCH] perf mem: add priv level filtering support

[David Ahern]

On 8/26/13 7:11 AM, Stephane Eranian wrote:
> perf mem: add priv level filtering support
>
> This patch adds the -u -and -k options to perf to allow
> filtering of load/store sampling based on priv levels.
> This may not be supported by all HW platforms.
>
> By default, loads/stores are sampled at both user and
> kernel privilege levels.
>
> To sample only at the user level:
> $ perf mem -u -t load rec ......
>
> To sample only at the kernel level:
> $ perf mem -k -t load rec ......
>
> Man page updated accordingly.
>
> Signed-off-by: Stephane Eranian <eranian@google.com>
> ---
>
> diff --git a/tools/perf/Documentation/perf-mem.txt b/tools/perf/Documentation/perf-mem.txt
> index 888d511..4c4e405 100644
> --- a/tools/perf/Documentation/perf-mem.txt
> +++ b/tools/perf/Documentation/perf-mem.txt
> @@ -43,6 +43,12 @@ OPTIONS
>   	option can be passed in record mode. It will be interpreted the same way as perf
>   	record.
>
> +-k::
> +	Only sample loads/stores at the user level (default: user + kernel)
> +
> +-u::
> +	Only sample loads/stores at the kernel level (default: user + kernel)

Are the descriptions backwards? In the commit message yuo have -u means 
user level and -k means kernel level; the help message here seems backwards.

David


> +
>   SEE ALSO
>   --------
>   linkperf:perf-record[1], linkperf:perf-report[1]
> diff --git a/tools/perf/builtin-mem.c b/tools/perf/builtin-mem.c
> index 706a1fa..8ac9d1e 100644
> --- a/tools/perf/builtin-mem.c
> +++ b/tools/perf/builtin-mem.c
> @@ -9,13 +9,18 @@
>   #define MEM_OPERATION_LOAD	"load"
>   #define MEM_OPERATION_STORE	"store"
>
> -static const char	*mem_operation		= MEM_OPERATION_LOAD;
> +#define OP_LOAD		0x1
> +#define OP_STORE	0x2
> +
>
>   struct perf_mem {
>   	struct perf_tool	tool;
>   	char const		*input_name;
>   	bool			hide_unresolved;
> +	const char		*mem_op;
>   	bool			dump_raw;
> +	bool			user;
> +	bool			kernel;
>   	const char		*cpu_list;
>   	DECLARE_BITMAP(cpu_bitmap, MAX_NR_CPUS);
>   };
> @@ -25,35 +30,88 @@ static const char * const mem_usage[] = {
>   	NULL
>   };
>
> -static int __cmd_record(int argc, const char **argv)
> +static inline const char *get_plm(struct perf_mem *mem)
> +{
> +	const char *plm = "";
> +
> +	if (mem->user && !mem->kernel) {
> +		plm = "u";
> +	} else if (!mem->user && mem->kernel) {
> +		plm = "k";
> +	}
> +	return plm;
> +}
> +
> +static int __cmd_record(int argc, const char **argv, struct perf_mem *mem)
>   {
>   	int rec_argc, i = 0, j;
>   	const char **rec_argv;
> -	char event[64];
> -	int ret;
> +	char *str;
> +	int mode = 0;
> +	int ki, ret;
> +
> +
> +	if (!strcmp(mem->mem_op, MEM_OPERATION_STORE))
> +		mode |= OP_STORE;
> +	else if (!strcmp(mem->mem_op, MEM_OPERATION_LOAD))
> +		mode |= OP_LOAD;
> +	else  {
> +		fprintf(stderr, "unknown sampling mode: %s\n", mem->mem_op);
> +		return -1;
> +	}
>
> -	rec_argc = argc + 4;
> +	rec_argc = argc + 6;
>   	rec_argv = calloc(rec_argc + 1, sizeof(char *));
>   	if (!rec_argv)
>   		return -1;
>
>   	rec_argv[i++] = strdup("record");
> -	if (!strcmp(mem_operation, MEM_OPERATION_LOAD))
> -		rec_argv[i++] = strdup("-W");
> +
>   	rec_argv[i++] = strdup("-d");
> -	rec_argv[i++] = strdup("-e");
>
> -	if (strcmp(mem_operation, MEM_OPERATION_LOAD))
> -		sprintf(event, "cpu/mem-stores/pp");
> -	else
> -		sprintf(event, "cpu/mem-loads/pp");
> +        if (mode & OP_LOAD) {
> +                rec_argv[i++] = strdup("-W");
>
> -	rec_argv[i++] = strdup(event);
> -	for (j = 1; j < argc; j++, i++)
> -		rec_argv[i] = argv[j];
> +                rec_argv[i++] = strdup("-e");
> +
> +                str = malloc(strlen("cpu/mem-loads/pp") + 1 + 1);
> +                if (!str) {
> +                        ki = i;
> +                        ret = -1;
> +                        goto end;
> +                }
> +                sprintf(str, "cpu/mem-loads/%spp", get_plm(mem));
> +                rec_argv[i++] = str;
> +        }
> +
> +        if (mode & OP_STORE) {
> +                rec_argv[i++] = strdup("-e");
> +
> +                str = malloc(strlen("cpu/mem-stores/pp") + 1 + 1);
> +                if (!str) {
> +                        ki = i;
> +                        ret = -1;
> +                        goto end;
> +                }
> +                sprintf(str, "cpu/mem-stores/%spp", get_plm(mem));
> +                rec_argv[i++] = str;
> +        }
> +
> +        /* arguments after i are not malloc'd */
> +        ki = i;
>
> -	ret = cmd_record(i, rec_argv, NULL);
> +	for (j = 1; j < argc; j++, ki++)
> +		rec_argv[ki] = argv[j];
> +
> +	ret = cmd_record(ki, rec_argv, NULL);
> +
> +end:
> +	/*
> +	 * XXX: free rec_argv[] entries, difficult because
> +	 * cmd_record() drops some of them...
> +	 */
>   	free(rec_argv);
> +
>   	return ret;
>   }
>
> @@ -171,7 +229,7 @@ static int report_events(int argc, const char **argv, struct perf_mem *mem)
>   	 * there is no weight (cost) associated with stores, so don't print
>   	 * the column
>   	 */
> -	if (strcmp(mem_operation, MEM_OPERATION_LOAD))
> +	if (strcmp(mem->mem_op, MEM_OPERATION_LOAD))
>   		rep_argv[i++] = strdup("--sort=mem,sym,dso,symbol_daddr,"
>   				       "dso_daddr,tlb,locked");
>
> @@ -199,7 +257,7 @@ int cmd_mem(int argc, const char **argv, const char *prefix __maybe_unused)
>   		.input_name		 = "perf.data",
>   	};
>   	const struct option mem_options[] = {
> -	OPT_STRING('t', "type", &mem_operation,
> +	OPT_STRING('t', "type", &mem.mem_op,
>   		   "type", "memory operations(load/store)"),
>   	OPT_BOOLEAN('D', "dump-raw-samples", &mem.dump_raw,
>   		    "dump raw samples in ASCII"),
> @@ -213,13 +271,18 @@ int cmd_mem(int argc, const char **argv, const char *prefix __maybe_unused)
>   		   "separator",
>   		   "separator for columns, no spaces will be added"
>   		   " between columns '.' is reserved."),
> +	OPT_BOOLEAN('u', "user-level", &mem.user,
> +		    "include user-level accesses"),
> +	OPT_BOOLEAN('k', "kernel-level", &mem.kernel,
> +		    "include kernel-level accesses"),
>   	OPT_END()
>   	};
>
>   	argc = parse_options(argc, argv, mem_options, mem_usage,
>   			     PARSE_OPT_STOP_AT_NON_OPTION);
>
> -	if (!argc || !(strncmp(argv[0], "rec", 3) || mem_operation))
> +	if (!argc || !(strncmp(argv[0], "rec", 3)
> +		       || strncmp(argv[0], "rep", 3)))
>   		usage_with_options(mem_usage, mem_options);
>
>   	if (!mem.input_name || !strlen(mem.input_name)) {
> @@ -228,9 +291,12 @@ int cmd_mem(int argc, const char **argv, const char *prefix __maybe_unused)
>   		else
>   			mem.input_name = "perf.data";
>   	}
> +	/* default to load only, some processors only support loads */
> +	if (!mem.mem_op)
> +		mem.mem_op = MEM_OPERATION_LOAD;
>
>   	if (!strncmp(argv[0], "rec", 3))
> -		return __cmd_record(argc, argv);
> +		return __cmd_record(argc, argv, &mem);
>   	else if (!strncmp(argv[0], "rep", 3))
>   		return report_events(argc, argv, &mem);
>   	else
>

Re: [PATCH] perf mem: add priv level filtering support

[Stephane Eranian]

On Tue, Aug 27, 2013 at 12:21 AM, David Ahern <dsahern@gmail.com> wrote:
>
> On 8/26/13 7:11 AM, Stephane Eranian wrote:
>>
>> perf mem: add priv level filtering support
>>
>> This patch adds the -u -and -k options to perf to allow
>> filtering of load/store sampling based on priv levels.
>> This may not be supported by all HW platforms.
>>
>> By default, loads/stores are sampled at both user and
>> kernel privilege levels.
>>
>> To sample only at the user level:
>> $ perf mem -u -t load rec ......
>>
>> To sample only at the kernel level:
>> $ perf mem -k -t load rec ......
>>
>> Man page updated accordingly.
>>
>> Signed-off-by: Stephane Eranian <eranian@google.com>
>> ---
>>
>> diff --git a/tools/perf/Documentation/perf-mem.txt b/tools/perf/Documentation/perf-mem.txt
>> index 888d511..4c4e405 100644
>> --- a/tools/perf/Documentation/perf-mem.txt
>> +++ b/tools/perf/Documentation/perf-mem.txt
>> @@ -43,6 +43,12 @@ OPTIONS
>>         option can be passed in record mode. It will be interpreted the same way as perf
>>         record.
>>
>> +-k::
>> +       Only sample loads/stores at the user level (default: user + kernel)
>> +
>> +-u::
>> +       Only sample loads/stores at the kernel level (default: user + kernel)
>
>
> Are the descriptions backwards? In the commit message yuo have -u means user level and -k means kernel level; the help message here seems backwards.
>
You are right. Let me resubmit....
Thanks.

>
> David
>
>
>
>> +
>>   SEE ALSO
>>   --------
>>   linkperf:perf-record[1], linkperf:perf-report[1]
>> diff --git a/tools/perf/builtin-mem.c b/tools/perf/builtin-mem.c
>> index 706a1fa..8ac9d1e 100644
>> --- a/tools/perf/builtin-mem.c
>> +++ b/tools/perf/builtin-mem.c
>> @@ -9,13 +9,18 @@
>>   #define MEM_OPERATION_LOAD    "load"
>>   #define MEM_OPERATION_STORE   "store"
>>
>> -static const char      *mem_operation          = MEM_OPERATION_LOAD;
>> +#define OP_LOAD                0x1
>> +#define OP_STORE       0x2
>> +
>>
>>   struct perf_mem {
>>         struct perf_tool        tool;
>>         char const              *input_name;
>>         bool                    hide_unresolved;
>> +       const char              *mem_op;
>>         bool                    dump_raw;
>> +       bool                    user;
>> +       bool                    kernel;
>>         const char              *cpu_list;
>>         DECLARE_BITMAP(cpu_bitmap, MAX_NR_CPUS);
>>   };
>> @@ -25,35 +30,88 @@ static const char * const mem_usage[] = {
>>         NULL
>>   };
>>
>> -static int __cmd_record(int argc, const char **argv)
>> +static inline const char *get_plm(struct perf_mem *mem)
>> +{
>> +       const char *plm = "";
>> +
>> +       if (mem->user && !mem->kernel) {
>> +               plm = "u";
>> +       } else if (!mem->user && mem->kernel) {
>> +               plm = "k";
>> +       }
>> +       return plm;
>> +}
>> +
>> +static int __cmd_record(int argc, const char **argv, struct perf_mem *mem)
>>   {
>>         int rec_argc, i = 0, j;
>>         const char **rec_argv;
>> -       char event[64];
>> -       int ret;
>> +       char *str;
>> +       int mode = 0;
>> +       int ki, ret;
>> +
>> +
>> +       if (!strcmp(mem->mem_op, MEM_OPERATION_STORE))
>> +               mode |= OP_STORE;
>> +       else if (!strcmp(mem->mem_op, MEM_OPERATION_LOAD))
>> +               mode |= OP_LOAD;
>> +       else  {
>> +               fprintf(stderr, "unknown sampling mode: %s\n", mem->mem_op);
>> +               return -1;
>> +       }
>>
>> -       rec_argc = argc + 4;
>> +       rec_argc = argc + 6;
>>         rec_argv = calloc(rec_argc + 1, sizeof(char *));
>>         if (!rec_argv)
>>                 return -1;
>>
>>         rec_argv[i++] = strdup("record");
>> -       if (!strcmp(mem_operation, MEM_OPERATION_LOAD))
>> -               rec_argv[i++] = strdup("-W");
>> +
>>         rec_argv[i++] = strdup("-d");
>> -       rec_argv[i++] = strdup("-e");
>>
>> -       if (strcmp(mem_operation, MEM_OPERATION_LOAD))
>> -               sprintf(event, "cpu/mem-stores/pp");
>> -       else
>> -               sprintf(event, "cpu/mem-loads/pp");
>> +        if (mode & OP_LOAD) {
>> +                rec_argv[i++] = strdup("-W");
>>
>> -       rec_argv[i++] = strdup(event);
>> -       for (j = 1; j < argc; j++, i++)
>> -               rec_argv[i] = argv[j];
>> +                rec_argv[i++] = strdup("-e");
>> +
>> +                str = malloc(strlen("cpu/mem-loads/pp") + 1 + 1);
>> +                if (!str) {
>> +                        ki = i;
>> +                        ret = -1;
>> +                        goto end;
>> +                }
>> +                sprintf(str, "cpu/mem-loads/%spp", get_plm(mem));
>> +                rec_argv[i++] = str;
>> +        }
>> +
>> +        if (mode & OP_STORE) {
>> +                rec_argv[i++] = strdup("-e");
>> +
>> +                str = malloc(strlen("cpu/mem-stores/pp") + 1 + 1);
>> +                if (!str) {
>> +                        ki = i;
>> +                        ret = -1;
>> +                        goto end;
>> +                }
>> +                sprintf(str, "cpu/mem-stores/%spp", get_plm(mem));
>> +                rec_argv[i++] = str;
>> +        }
>> +
>> +        /* arguments after i are not malloc'd */
>> +        ki = i;
>>
>> -       ret = cmd_record(i, rec_argv, NULL);
>> +       for (j = 1; j < argc; j++, ki++)
>> +               rec_argv[ki] = argv[j];
>> +
>> +       ret = cmd_record(ki, rec_argv, NULL);
>> +
>> +end:
>> +       /*
>> +        * XXX: free rec_argv[] entries, difficult because
>> +        * cmd_record() drops some of them...
>> +        */
>>         free(rec_argv);
>> +
>>         return ret;
>>   }
>>
>> @@ -171,7 +229,7 @@ static int report_events(int argc, const char **argv, struct perf_mem *mem)
>>          * there is no weight (cost) associated with stores, so don't print
>>          * the column
>>          */
>> -       if (strcmp(mem_operation, MEM_OPERATION_LOAD))
>> +       if (strcmp(mem->mem_op, MEM_OPERATION_LOAD))
>>                 rep_argv[i++] = strdup("--sort=mem,sym,dso,symbol_daddr,"
>>                                        "dso_daddr,tlb,locked");
>>
>> @@ -199,7 +257,7 @@ int cmd_mem(int argc, const char **argv, const char *prefix __maybe_unused)
>>                 .input_name              = "perf.data",
>>         };
>>         const struct option mem_options[] = {
>> -       OPT_STRING('t', "type", &mem_operation,
>> +       OPT_STRING('t', "type", &mem.mem_op,
>>                    "type", "memory operations(load/store)"),
>>         OPT_BOOLEAN('D', "dump-raw-samples", &mem.dump_raw,
>>                     "dump raw samples in ASCII"),
>> @@ -213,13 +271,18 @@ int cmd_mem(int argc, const char **argv, const char *prefix __maybe_unused)
>>                    "separator",
>>                    "separator for columns, no spaces will be added"
>>                    " between columns '.' is reserved."),
>> +       OPT_BOOLEAN('u', "user-level", &mem.user,
>> +                   "include user-level accesses"),
>> +       OPT_BOOLEAN('k', "kernel-level", &mem.kernel,
>> +                   "include kernel-level accesses"),
>>         OPT_END()
>>         };
>>
>>         argc = parse_options(argc, argv, mem_options, mem_usage,
>>                              PARSE_OPT_STOP_AT_NON_OPTION);
>>
>> -       if (!argc || !(strncmp(argv[0], "rec", 3) || mem_operation))
>> +       if (!argc || !(strncmp(argv[0], "rec", 3)
>> +                      || strncmp(argv[0], "rep", 3)))
>>                 usage_with_options(mem_usage, mem_options);
>>
>>         if (!mem.input_name || !strlen(mem.input_name)) {
>> @@ -228,9 +291,12 @@ int cmd_mem(int argc, const char **argv, const char *prefix __maybe_unused)
>>                 else
>>                         mem.input_name = "perf.data";
>>         }
>> +       /* default to load only, some processors only support loads */
>> +       if (!mem.mem_op)
>> +               mem.mem_op = MEM_OPERATION_LOAD;
>>
>>         if (!strncmp(argv[0], "rec", 3))
>> -               return __cmd_record(argc, argv);
>> +               return __cmd_record(argc, argv, &mem);
>>         else if (!strncmp(argv[0], "rep", 3))
>>                 return report_events(argc, argv, &mem);
>>         else
>>
>

Re: [PATCH] perf mem: add priv level filtering support

[Arnaldo Carvalho de Melo]

Em Mon, Aug 26, 2013 at 04:21:44PM -0600, David Ahern escreveu:
> On 8/26/13 7:11 AM, Stephane Eranian wrote:

> >This patch adds the -u -and -k options to perf to allow
> >filtering of load/store sampling based on priv levels.
> >This may not be supported by all HW platforms.


> >+++ b/tools/perf/Documentation/perf-mem.txt
> >+-k::
> >+	Only sample loads/stores at the user level (default: user + kernel)
> >+-u::
> >+	Only sample loads/stores at the kernel level (default: user + kernel)
 
> Are the descriptions backwards? In the commit message yuo have -u
> means user level and -k means kernel level; the help message here
> seems backwards.

Looks like it is reversed, yes.

> >+	OPT_BOOLEAN('u', "user-level", &mem.user, "include user-level accesses"),
> >+	OPT_BOOLEAN('k', "kernel-level", &mem.kernel, "include kernel-level accesses"),

And its not clear to say that using -u will _exclude_ kernel samples,
just that it includes user samples, perf top has:

    -K, --hide_kernel_symbols	hide kernel symbols
    -U, --hide_user_symbols	hide user symbols

So perhaps we should change both to (and add this to 'report' as well):

    -U, --hide_kernel_symbols	hide kernel symbols
    -K, --hide_user_symbols	hide user symbols

To state that:

	perf top -K
and
	perf top --hide_user_symbols

Are equivalent and asks for 'kernel only' samples, like it seems its the
intent (filtering) of Stephane here, and seems to clarify things?

- Arnaldo

Re: [PATCH] perf mem: add priv level filtering support

[Stephane Eranian]

On Wed, Aug 28, 2013 at 3:27 PM, Arnaldo Carvalho de Melo
<acme@redhat.com> wrote:
> Em Mon, Aug 26, 2013 at 04:21:44PM -0600, David Ahern escreveu:
>> On 8/26/13 7:11 AM, Stephane Eranian wrote:
>
>> >This patch adds the -u -and -k options to perf to allow
>> >filtering of load/store sampling based on priv levels.
>> >This may not be supported by all HW platforms.
>
>
>> >+++ b/tools/perf/Documentation/perf-mem.txt
>> >+-k::
>> >+    Only sample loads/stores at the user level (default: user + kernel)
>> >+-u::
>> >+    Only sample loads/stores at the kernel level (default: user + kernel)
>
>> Are the descriptions backwards? In the commit message yuo have -u
>> means user level and -k means kernel level; the help message here
>> seems backwards.
>
> Looks like it is reversed, yes.
>
>> >+    OPT_BOOLEAN('u', "user-level", &mem.user, "include user-level accesses"),
>> >+    OPT_BOOLEAN('k', "kernel-level", &mem.kernel, "include kernel-level accesses"),
>
> And its not clear to say that using -u will _exclude_ kernel samples,
> just that it includes user samples, perf top has:
>
>     -K, --hide_kernel_symbols   hide kernel symbols
>     -U, --hide_user_symbols     hide user symbols
>
> So perhaps we should change both to (and add this to 'report' as well):
>
>     -U, --hide_kernel_symbols   hide kernel symbols
>     -K, --hide_user_symbols     hide user symbols
>

Well, I don't know what perf top does here but I don't want to hide
the samples. I simply don't want to collect them (do not appear
in the perf.data file). If that's what is happening in perf top, then
I'll be glad to use the same options.


> To state that:
>
>         perf top -K
> and
>         perf top --hide_user_symbols
>
> Are equivalent and asks for 'kernel only' samples, like it seems its the
> intent (filtering) of Stephane here, and seems to clarify things?
>
> - Arnaldo

Re: [PATCH] perf mem: add priv level filtering support

[Stephane Eranian]

Arnaldo,

So I just checked perf top and those 2 options are
doing user level filtering of the samples. This is different
from what I want which is hardware level filtering if avail.



On Wed, Aug 28, 2013 at 3:38 PM, Stephane Eranian <eranian@google.com> wrote:
> On Wed, Aug 28, 2013 at 3:27 PM, Arnaldo Carvalho de Melo
> <acme@redhat.com> wrote:
>> Em Mon, Aug 26, 2013 at 04:21:44PM -0600, David Ahern escreveu:
>>> On 8/26/13 7:11 AM, Stephane Eranian wrote:
>>
>>> >This patch adds the -u -and -k options to perf to allow
>>> >filtering of load/store sampling based on priv levels.
>>> >This may not be supported by all HW platforms.
>>
>>
>>> >+++ b/tools/perf/Documentation/perf-mem.txt
>>> >+-k::
>>> >+    Only sample loads/stores at the user level (default: user + kernel)
>>> >+-u::
>>> >+    Only sample loads/stores at the kernel level (default: user + kernel)
>>
>>> Are the descriptions backwards? In the commit message yuo have -u
>>> means user level and -k means kernel level; the help message here
>>> seems backwards.
>>
>> Looks like it is reversed, yes.
>>
>>> >+    OPT_BOOLEAN('u', "user-level", &mem.user, "include user-level accesses"),
>>> >+    OPT_BOOLEAN('k', "kernel-level", &mem.kernel, "include kernel-level accesses"),
>>
>> And its not clear to say that using -u will _exclude_ kernel samples,
>> just that it includes user samples, perf top has:
>>
>>     -K, --hide_kernel_symbols   hide kernel symbols
>>     -U, --hide_user_symbols     hide user symbols
>>
>> So perhaps we should change both to (and add this to 'report' as well):
>>
>>     -U, --hide_kernel_symbols   hide kernel symbols
>>     -K, --hide_user_symbols     hide user symbols
>>
>
> Well, I don't know what perf top does here but I don't want to hide
> the samples. I simply don't want to collect them (do not appear
> in the perf.data file). If that's what is happening in perf top, then
> I'll be glad to use the same options.
>
>
>> To state that:
>>
>>         perf top -K
>> and
>>         perf top --hide_user_symbols
>>
>> Are equivalent and asks for 'kernel only' samples, like it seems its the
>> intent (filtering) of Stephane here, and seems to clarify things?
>>
>> - Arnaldo

Re: [PATCH] perf mem: add priv level filtering support

[Arnaldo Carvalho de Melo]

Em Wed, Aug 28, 2013 at 03:38:28PM +0200, Stephane Eranian escreveu:
> On Wed, Aug 28, 2013 at 3:27 PM, Arnaldo Carvalho de Melo <acme@redhat.com> wrote:
> > So perhaps we should change both to (and add this to 'report' as well):
> >
> >     -U, --hide_kernel_symbols   hide kernel symbols
> >     -K, --hide_user_symbols     hide user symbols
> >
> 
> Well, I don't know what perf top does here but I don't want to hide
> the samples. I simply don't want to collect them (do not appear
> in the perf.data file). If that's what is happening in perf top, then
> I'll be glad to use the same options.

Indeed, its for different purposes, 'perf top' when used with one of
those options will still collect samples for all priv levels and will
just toggle a flag to not zap the ones asked not to show when decaying
the samples.

When the user presses 'U' or 'K' on the UI, the flags gets toggled and
samples start being considered/zapped.

But my worry here is about consistency accross tools for the single
letter options, so perhaps if you could use:

     -U		collect only user level samples
     -K		collect only kernel level samples

I think it would stay consistent and clear, what do you think?

- Arnaldo

Re: [PATCH] perf mem: add priv level filtering support

[Sukadev Bhattiprolu]

Arnaldo Carvalho de Melo [acme@redhat.com] wrote:
| Em Wed, Aug 28, 2013 at 03:38:28PM +0200, Stephane Eranian escreveu:
| > On Wed, Aug 28, 2013 at 3:27 PM, Arnaldo Carvalho de Melo <acme@redhat.com> wrote:
| > > So perhaps we should change both to (and add this to 'report' as well):
| > >
| > >     -U, --hide_kernel_symbols   hide kernel symbols
| > >     -K, --hide_user_symbols     hide user symbols
| > >
| > 
| > Well, I don't know what perf top does here but I don't want to hide
| > the samples. I simply don't want to collect them (do not appear
| > in the perf.data file). If that's what is happening in perf top, then
| > I'll be glad to use the same options.
| 
| Indeed, its for different purposes, 'perf top' when used with one of
| those options will still collect samples for all priv levels and will
| just toggle a flag to not zap the ones asked not to show when decaying
| the samples.
| 
| When the user presses 'U' or 'K' on the UI, the flags gets toggled and
| samples start being considered/zapped.
| 
| But my worry here is about consistency accross tools for the single
| letter options, so perhaps if you could use:
| 
|      -U		collect only user level samples
|      -K		collect only kernel level samples
| 
| I think it would stay consistent and clear, what do you think?

But, we use lower case qualifiers :u, :k to select user or kernel mode
monitoring. 

	perf record -e cycles		# both kernel and user
	perf record -e cycles:u ...	# just user

(tools/perf/util/parse-events.c:

        struct event_modifier {
                int eu;
                int ek;
                int eh;
                int eH;
                int eG;
                int precise;
                int exclude_GH;
        };

Will we ever need hypervisor and host monitoring for 'perf mem' ?

Or can we add a '-e' option to 'perf mem' so user can specify the events
and qualfiers same as they do for 'perf record' ?

	perf mem -e mem-loads:u record .....

(this would of course expose the mem-loads and mem-stores events to
the user)

Sukadev

Re: [PATCH] perf mem: add priv level filtering support

[Andi Kleen]

> But my worry here is about consistency accross tools for the single
> letter options, so perhaps if you could use:
> 
>      -U		collect only user level samples
>      -K		collect only kernel level samples

Support for this would be nice for perf stat too, to use with 
the implicit events (using by -d, soon -T etc.)

-Andi

-- 
ak@linux.intel.com -- Speaking for myself only