[PATCH] perf tools: Clarify error when running "perf lock record"

[PATCH] perf tools: Clarify error when running "perf lock record"

[Rodrigo Campos]

If you have the config options enabled in your kernel, you still need to be root
to run perf lock. And is kind of misleading when you have that options enabled
and the error says that they might not be.

So, this patch just adds to the error that it should be run as root. Although we
can probably check if it's running as root or not, adding it to the error
message is really simple and should be enough.

Signed-off-by: Rodrigo Campos <rodrigo@sdfg.com.ar>
---
Please keep me in Cc: since I'm not subscribed
---
 tools/perf/builtin-lock.c |    4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/tools/perf/builtin-lock.c b/tools/perf/builtin-lock.c
index 76543a4..f07b318 100644
--- a/tools/perf/builtin-lock.c
+++ b/tools/perf/builtin-lock.c
@@ -886,8 +886,8 @@ static int __cmd_record(int argc, const char **argv)
 	for (i = 0; i < ARRAY_SIZE(lock_tracepoints); i++) {
 		if (!is_valid_tracepoint(lock_tracepoints[i].name)) {
 				pr_err("tracepoint %s is not enabled. "
-				       "Are CONFIG_LOCKDEP and CONFIG_LOCK_STAT enabled?\n",
-				       lock_tracepoints[i].name);
+				       "Are CONFIG_LOCKDEP and CONFIG_LOCK_STAT enabled "
+				       "and running as root?\n", lock_tracepoints[i].name);
 				return 1;
 		}
 	}
-- 
1.7.10.4

Re: [PATCH] perf tools: Clarify error when running "perf lock record"

[David Ahern]

On 9/5/13 2:32 AM, Rodrigo Campos wrote:
> If you have the config options enabled in your kernel, you still need to be root
> to run perf lock. And is kind of misleading when you have that options enabled
> and the error says that they might not be.
>
> So, this patch just adds to the error that it should be run as root. Although we
> can probably check if it's running as root or not, adding it to the error
> message is really simple and should be enough.
>
> Signed-off-by: Rodrigo Campos <rodrigo@sdfg.com.ar>
> ---
> Please keep me in Cc: since I'm not subscribed
> ---
>   tools/perf/builtin-lock.c |    4 ++--
>   1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/tools/perf/builtin-lock.c b/tools/perf/builtin-lock.c
> index 76543a4..f07b318 100644
> --- a/tools/perf/builtin-lock.c
> +++ b/tools/perf/builtin-lock.c
> @@ -886,8 +886,8 @@ static int __cmd_record(int argc, const char **argv)
>   	for (i = 0; i < ARRAY_SIZE(lock_tracepoints); i++) {
>   		if (!is_valid_tracepoint(lock_tracepoints[i].name)) {
>   				pr_err("tracepoint %s is not enabled. "
> -				       "Are CONFIG_LOCKDEP and CONFIG_LOCK_STAT enabled?\n",
> -				       lock_tracepoints[i].name);
> +				       "Are CONFIG_LOCKDEP and CONFIG_LOCK_STAT enabled "
> +				       "and running as root?\n", lock_tracepoints[i].name);
>   				return 1;
>   		}
>   	}
>

you can run perf-lock as non-root with certain settings. It would be 
better to discriminate debugfs access errors (insufficient privilege). 
That means improving is_valid_tracepoint and handling the errors in 
perf-lock.

David

Re: [PATCH] perf tools: Clarify error when running "perf lock record"

[Rodrigo Campos]

On Thu, Sep 05, 2013 at 08:29:10AM -0600, David Ahern wrote:
> On 9/5/13 2:32 AM, Rodrigo Campos wrote:
> >If you have the config options enabled in your kernel, you still need to be root
> >to run perf lock. And is kind of misleading when you have that options enabled
> >and the error says that they might not be.
> >
> >So, this patch just adds to the error that it should be run as root. Although we
> >can probably check if it's running as root or not, adding it to the error
> >message is really simple and should be enough.
> >
> >Signed-off-by: Rodrigo Campos <rodrigo@sdfg.com.ar>
> >---
> >Please keep me in Cc: since I'm not subscribed
> >---
> >  tools/perf/builtin-lock.c |    4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> >
> >diff --git a/tools/perf/builtin-lock.c b/tools/perf/builtin-lock.c
> >index 76543a4..f07b318 100644
> >--- a/tools/perf/builtin-lock.c
> >+++ b/tools/perf/builtin-lock.c
> >@@ -886,8 +886,8 @@ static int __cmd_record(int argc, const char **argv)
> >  	for (i = 0; i < ARRAY_SIZE(lock_tracepoints); i++) {
> >  		if (!is_valid_tracepoint(lock_tracepoints[i].name)) {
> >  				pr_err("tracepoint %s is not enabled. "
> >-				       "Are CONFIG_LOCKDEP and CONFIG_LOCK_STAT enabled?\n",
> >-				       lock_tracepoints[i].name);
> >+				       "Are CONFIG_LOCKDEP and CONFIG_LOCK_STAT enabled "
> >+				       "and running as root?\n", lock_tracepoints[i].name);
> >  				return 1;
> >  		}
> >  	}
> >
> 
> you can run perf-lock as non-root with certain settings. It would be

Ohh, I didn't know. Sorry.

> better to discriminate debugfs access errors (insufficient
> privilege). That means improving is_valid_tracepoint and handling
> the errors in perf-lock.

IMHO suggesting to check the privileges should be enough for most cases and is
simpler that checking all that. I mean, for me it's not worth the effort, but
maybe I'm too lazy. Probably it's quite simple too... :-)




Thanks,
Rodrigo