[patch] perf/x86/intel/pt: cleanup error handling in pt_pmu_hw_init()

[patch] perf/x86/intel/pt: cleanup error handling in pt_pmu_hw_init()

[Dan Carpenter]

There is no need to free NULL pointers.

Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>

diff --git a/arch/x86/kernel/cpu/perf_event_intel_pt.c b/arch/x86/kernel/cpu/perf_event_intel_pt.c
index f5a3afc..c358877 100644
--- a/arch/x86/kernel/cpu/perf_event_intel_pt.c
+++ b/arch/x86/kernel/cpu/perf_event_intel_pt.c
@@ -135,12 +135,12 @@ static int __init pt_pmu_hw_init(void)
 	size = sizeof(struct attribute *) * (ARRAY_SIZE(pt_caps) + 1);
 	attrs = kzalloc(size, GFP_KERNEL);
 	if (!attrs)
-		goto err_attrs;
+		return -ENOMEM;
 
 	size = sizeof(struct dev_ext_attribute) * (ARRAY_SIZE(pt_caps) + 1);
 	de_attrs = kzalloc(size, GFP_KERNEL);
 	if (!de_attrs)
-		goto err_de_attrs;
+		goto err_attrs;
 
 	for (i = 0; i < ARRAY_SIZE(pt_caps); i++) {
 		de_attrs[i].attr.attr.name = pt_caps[i].name;
@@ -155,8 +155,6 @@ static int __init pt_pmu_hw_init(void)
 	pt_cap_group.attrs = attrs;
 	return 0;
 
-err_de_attrs:
-	kfree(de_attrs);
 err_attrs:
 	kfree(attrs);
 

Re: [patch] perf/x86/intel/pt: cleanup error handling in pt_pmu_hw_init()

[Ingo Molnar]

* Dan Carpenter <dan.carpenter@oracle.com> wrote:

> There is no need to free NULL pointers.
> 
> Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
> 
> diff --git a/arch/x86/kernel/cpu/perf_event_intel_pt.c b/arch/x86/kernel/cpu/perf_event_intel_pt.c
> index f5a3afc..c358877 100644
> --- a/arch/x86/kernel/cpu/perf_event_intel_pt.c
> +++ b/arch/x86/kernel/cpu/perf_event_intel_pt.c
> @@ -135,12 +135,12 @@ static int __init pt_pmu_hw_init(void)
>  	size = sizeof(struct attribute *) * (ARRAY_SIZE(pt_caps) + 1);
>  	attrs = kzalloc(size, GFP_KERNEL);
>  	if (!attrs)
> -		goto err_attrs;
> +		return -ENOMEM;

Please don't put stray return statements into functions, try to keep 
to clean (and singular) goto driven exit paths.

Thanks,

	Ingo

Re: [patch] perf/x86/intel/pt: cleanup error handling in pt_pmu_hw_init()

[Dan Carpenter]

On Thu, Apr 09, 2015 at 02:02:12PM +0200, Ingo Molnar wrote:
> 
> * Dan Carpenter <dan.carpenter@oracle.com> wrote:
> 
> > There is no need to free NULL pointers.
> > 
> > Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
> > 
> > diff --git a/arch/x86/kernel/cpu/perf_event_intel_pt.c b/arch/x86/kernel/cpu/perf_event_intel_pt.c
> > index f5a3afc..c358877 100644
> > --- a/arch/x86/kernel/cpu/perf_event_intel_pt.c
> > +++ b/arch/x86/kernel/cpu/perf_event_intel_pt.c
> > @@ -135,12 +135,12 @@ static int __init pt_pmu_hw_init(void)
> >  	size = sizeof(struct attribute *) * (ARRAY_SIZE(pt_caps) + 1);
> >  	attrs = kzalloc(size, GFP_KERNEL);
> >  	if (!attrs)
> > -		goto err_attrs;
> > +		return -ENOMEM;
> 
> Please don't put stray return statements into functions, try to keep
> to clean (and singular) goto driven exit paths.

There was one earlier already.

Also I have come to despise "err" labels.  They do one of three things:

1) Nothing.  These are a pointless interruption for people reading the
   code from top to bottom because you wonder, "How is goto err
   different from return -ENOMEM?".  You scroll down to the bottom and
   find that they are the same.  Now you have lost your place and your
   train of thought.

2) One thing.  In this case the label is poorly named, better to say
   "goto unlock".

3) Everything.  This is a leading source of error handling bugs.  It
   looks like this.

err:
	kfree(foo);
	return ret;

Later we will change it so that foo is allocated with kmemdup_user()
instead of kmalloc() so now kfree() will oops trying to free the error
pointer.  I see so many one err bugs it's not even funny.

regards,
dan carpenter

Re: [patch] perf/x86/intel/pt: cleanup error handling in pt_pmu_hw_init()

[Peter Zijlstra]

On Thu, Apr 09, 2015 at 03:33:36PM +0300, Dan Carpenter wrote:
> Also I have come to despise "err" labels.  They do one of three things:
> 
> 1) Nothing.  These are a pointless interruption for people reading the
>    code from top to bottom because you wonder, "How is goto err
>    different from return -ENOMEM?".  You scroll down to the bottom and
>    find that they are the same.  Now you have lost your place and your
>    train of thought.
> 
> 2) One thing.  In this case the label is poorly named, better to say
>    "goto unlock".
> 
> 3) Everything.  This is a leading source of error handling bugs.  It
>    looks like this.
> 
> err:
> 	kfree(foo);
> 	return ret;

  4) unwind complex state; see for example copy_process(). You do not
  want to endlessly duplicate the increasing cleanup for every exit.

> Later we will change it so that foo is allocated with kmemdup_user()
> instead of kmalloc() so now kfree() will oops trying to free the error
> pointer.  

That's just poor engineering in the first place, if you change the alloc
site you had better also look for all the free sites. Error labels have
nothing to do with that, alloc/free can be otherwise separated and hard
to correlate, in fact error labels can be some of the easier places to
find.

> I see so many one err bugs it's not even funny

Well, that's a side effect of us not actually testing many of our error
paths :/

Re: [patch] perf/x86/intel/pt: cleanup error handling in pt_pmu_hw_init()

[Alexander Shishkin]

Dan Carpenter <dan.carpenter@oracle.com> writes:

> There is no need to free NULL pointers.
>
> Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>

Nice catch!

FWIW,

Acked-by: Alexander Shishkin <alexander.shishkin@linux.intel.com>

Re: [patch] perf/x86/intel/pt: cleanup error handling in pt_pmu_hw_init()

[Dan Carpenter]

On Thu, Apr 09, 2015 at 02:45:30PM +0200, Peter Zijlstra wrote:
> On Thu, Apr 09, 2015 at 03:33:36PM +0300, Dan Carpenter wrote:
> > Also I have come to despise "err" labels.  They do one of three things:
> > 
> > 1) Nothing.  These are a pointless interruption for people reading the
> >    code from top to bottom because you wonder, "How is goto err
> >    different from return -ENOMEM?".  You scroll down to the bottom and
> >    find that they are the same.  Now you have lost your place and your
> >    train of thought.
> > 
> > 2) One thing.  In this case the label is poorly named, better to say
> >    "goto unlock".
> > 
> > 3) Everything.  This is a leading source of error handling bugs.  It
> >    looks like this.
> > 
> > err:
> > 	kfree(foo);
> > 	return ret;
> 
>   4) unwind complex state; see for example copy_process(). You do not
>   want to endlessly duplicate the increasing cleanup for every exit.

I *like* using gotos for unwinding.  It's specifically the err:, out:,
and bail: type labels I object to.  The error handling in copy_process()
is pretty clean but the fork_out label is kind of useless.  Here is how
it's used.

kernel/fork.c
  1285          if (clone_flags & CLONE_SIGHAND) {
  1286                  if ((clone_flags & (CLONE_NEWUSER | CLONE_NEWPID)) ||
  1287                      (task_active_pid_ns(current) !  1288                                  current->nsproxy->pid_ns_for_children))
  1289                          return ERR_PTR(-EINVAL);
                                ^^^^^^^^^^^^^^^^^^^^^^^
This direct return is clear.

  1290          }
  1291  
  1292          retval = security_task_create(clone_flags);
  1293          if (retval)
  1294                  goto fork_out;
                        ^^^^^^^^^^^^^

This does the same thing but it's more ambigous.  You have to scroll
down 350 lines to find out what it does.

> 
> > Later we will change it so that foo is allocated with kmemdup_user()
> > instead of kmalloc() so now kfree() will oops trying to free the error
> > pointer.  
> 
> That's just poor engineering in the first place, if you change the alloc
> site you had better also look for all the free sites. Error labels have
> nothing to do with that, alloc/free can be otherwise separated and hard
> to correlate, in fact error labels can be some of the easier places to
> find.

A more typical example is:

err:
	mega_free_function(foo);
	return ret;

But inside mega_free_function() function it dereferences foo->bar which
is still NULL.  The problem with one err bugs is that you're merging
a lot of error paths together and then separating them out using if
statements and it's easy to make a mistake.  If you unwind like:

err_free_bar:
	kfree(foo->bar);
err_free_foo:
	kfree(foo);
	return ret;

That is less error prone.

regards,
dan carpenter

Re: [patch] perf/x86/intel/pt: cleanup error handling in pt_pmu_hw_init()

[Ingo Molnar]

* Dan Carpenter <dan.carpenter@oracle.com> wrote:

> On Thu, Apr 09, 2015 at 02:45:30PM +0200, Peter Zijlstra wrote:
> > On Thu, Apr 09, 2015 at 03:33:36PM +0300, Dan Carpenter wrote:
> > > Also I have come to despise "err" labels.  They do one of three things:
> > > 
> > > 1) Nothing.  These are a pointless interruption for people reading the
> > >    code from top to bottom because you wonder, "How is goto err
> > >    different from return -ENOMEM?".  You scroll down to the bottom and
> > >    find that they are the same.  Now you have lost your place and your
> > >    train of thought.
> > > 
> > > 2) One thing.  In this case the label is poorly named, better to say
> > >    "goto unlock".
> > > 
> > > 3) Everything.  This is a leading source of error handling bugs.  It
> > >    looks like this.
> > > 
> > > err:
> > > 	kfree(foo);
> > > 	return ret;
> > 
> >   4) unwind complex state; see for example copy_process(). You do not
> >   want to endlessly duplicate the increasing cleanup for every exit.
> 
> I *like* using gotos for unwinding.  It's specifically the err:, out:,
> and bail: type labels I object to. [...]

Agreed, and I'd suggest you don't use poorly named and poorly 
constructed labels.

> [...]  If you unwind like:
> 
> err_free_bar:
> 	kfree(foo->bar);
> err_free_foo:
> 	kfree(foo);
> 	return ret;
> 
> That is less error prone.

That's how I name and structure unwind labels as well, and my 
suggestion is to use something similar here in this code too, in 
arch/x86/kernel/cpu/perf_event_intel_pt.c.

Agreed?

Thanks,

	Ingo

Re: [patch] perf/x86/intel/pt: cleanup error handling in pt_pmu_hw_init()

[Dan Carpenter]

On Thu, Apr 09, 2015 at 04:54:40PM +0200, Ingo Molnar wrote:
> > [...]  If you unwind like:
> > 
> > err_free_bar:
> > 	kfree(foo->bar);
> > err_free_foo:
> > 	kfree(foo);
> > 	return ret;
> > 
> > That is less error prone.
> 
> That's how I name and structure unwind labels as well, and my 
> suggestion is to use something similar here in this code too, in 
> arch/x86/kernel/cpu/perf_event_intel_pt.c.
> 
> Agreed?

I don't understand what you want me to do here.  I think you are saying
I should do this:

err_attrs:
	kfree(attrs);
err:
	return ret;

That's not the style that the rest of this file uses.  Every function
uses direct returns where possible except pt_event_add() and that
function seems buggy.

arch/x86/kernel/cpu/perf_event_intel_pt.c
  1000  
  1001          if (mode & PERF_EF_START) {
  1002                  pt_event_start(event, 0);
  1003                  if (hwc->state = PERF_HES_STOPPED) {
  1004                          pt_event_del(event, 0);
  1005                          ret = -EBUSY;
                                ^^^^^^^^^^^^
We set "ret" here but then return zero.

  1006                  }
  1007          } else {
  1008                  hwc->state = PERF_HES_STOPPED;
                        ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

Shouldn't we set "ret" here?

  1009          }
  1010  
  1011          ret = 0;
  1012  out:
  1013  
  1014          if (ret)
  1015                  hwc->state = PERF_HES_STOPPED;
  1016  
  1017          return ret;
  1018  }

regards,
dan carpenter

Re: [patch] perf/x86/intel/pt: cleanup error handling in pt_pmu_hw_init()

[Ingo Molnar]

* Dan Carpenter <dan.carpenter@oracle.com> wrote:

> On Thu, Apr 09, 2015 at 04:54:40PM +0200, Ingo Molnar wrote:
> > > [...]  If you unwind like:
> > > 
> > > err_free_bar:
> > > 	kfree(foo->bar);
> > > err_free_foo:
> > > 	kfree(foo);
> > > 	return ret;
> > > 
> > > That is less error prone.
> > 
> > That's how I name and structure unwind labels as well, and my 
> > suggestion is to use something similar here in this code too, in 
> > arch/x86/kernel/cpu/perf_event_intel_pt.c.
> > 
> > Agreed?
> 
> I don't understand what you want me to do here.  I think you are saying
> I should do this:
> 
> err_attrs:
> 	kfree(attrs);
> err:
> 	return ret;

Yeah, that looks cleaner.

> 
> That's not the style that the rest of this file uses. [...]

That might have slipped through review, but it's not a reason to 
continue the bad practice.

Thanks,

	Ingo

Re: [patch] perf/x86/intel/pt: cleanup error handling in pt_pmu_hw_init()

[Dan Carpenter]

I used to use out: and err: labels but for the past few years my job has
been to look at error handling every day.  I now believe very strongly
that do-nothing gotos are harmful.  It's going to be my contribution to
computer engineering to persuade people to hate exit labels.

https://plus.google.com/106378716002406849458/posts/dnanfhQ4mHQ

1) "goto err;" is annoying to read because the name is meaningless.

2) Exit labels bad associations because code which uses err: or out:
   labels is often buggy as we have seen in pt_event_add().

3) They don't do anything for return in the middle bugs.  They only
   discourage people from returning at the start of the function where
   direct returns are fine.

   In theory, one place where they might be useful is if we added
   locking to a function later on.  First of all, when we add locking no
   one ever changes the out: label to out_unlock:.  It never happens.

   Also missing unlocks on error paths is sort of a sloppy/newbie
   mistake.  I have looked through the git log and newbies are just as
   likely to get locking wrong when there is an exit label as when there
   is not.

   https://plus.google.com/106378716002406849458/posts/DfuAkt8szf2

4) Exit labels introduce a whole new class of "forgot to set the error
   code" bugs which don't happen with direct returns.

regards,
dan carpenter

Re: [patch] perf/x86/intel/pt: cleanup error handling in pt_pmu_hw_init()

[Ingo Molnar]

* Dan Carpenter <dan.carpenter@oracle.com> wrote:

> I used to use out: and err: labels but for the past few years my job 
> has been to look at error handling every day.  I now believe very 
> strongly that do-nothing gotos are harmful.  It's going to be my 
> contribution to computer engineering to persuade people to hate exit 
> labels.

So I ended up cleaning up that function myself:

static int __init pt_pmu_hw_init(void)
{
	struct dev_ext_attribute *de_attrs;
	struct attribute **attrs;
	size_t size;
	int ret;
	long i;

	attrs = NULL;
	ret = -ENODEV;
	if (!test_cpu_cap(&boot_cpu_data, X86_FEATURE_INTEL_PT))
		goto fail;

	for (i = 0; i < PT_CPUID_LEAVES; i++) {
		cpuid_count(20, i,
			    &pt_pmu.caps[CR_EAX + i*4],
			    &pt_pmu.caps[CR_EBX + i*4],
			    &pt_pmu.caps[CR_ECX + i*4],
			    &pt_pmu.caps[CR_EDX + i*4]);
	}

	ret = -ENOMEM;
	size = sizeof(struct attribute *) * (ARRAY_SIZE(pt_caps)+1);
	attrs = kzalloc(size, GFP_KERNEL);
	if (!attrs)
		goto fail;

	size = sizeof(struct dev_ext_attribute) * (ARRAY_SIZE(pt_caps)+1);
	de_attrs = kzalloc(size, GFP_KERNEL);
	if (!de_attrs)
		goto fail;

	for (i = 0; i < ARRAY_SIZE(pt_caps); i++) {
		struct dev_ext_attribute *de_attr = de_attrs + i;

		de_attr->attr.attr.name = pt_caps[i].name;

		sysfs_attr_init(&de_attrs->attr.attr);

		de_attr->attr.attr.mode		= S_IRUGO;
		de_attr->attr.show		= pt_cap_show;
		de_attr->var			= (void *)i;

		attrs[i] = &de_attr->attr.attr;
	}

	pt_cap_group.attrs = attrs;

	return 0;

fail:
	kfree(attrs);

	return ret;
}

Note the fundamentally robust failure handling logic: all places can 
do a 'goto fail' and it will just clean itself up. Since this is init 
code, the kfree(NULL) is a non-issue.

Should this be extended in the future, the 'fail' path can be extended 
indefinitely to clean up other resources as well.

Let me react to your list of concerns:

> https://plus.google.com/106378716002406849458/posts/dnanfhQ4mHQ
> 
> 1) "goto err;" is annoying to read because the name is meaningless.

Agreed, 'goto fail' is more meaningful.

> 2) Exit labels bad associations because code which uses err: or out:
>    labels is often buggy as we have seen in pt_event_add().

Not if the cleanup path is robustly constructed. Multiple labels can 
also be used:

  - if performance is of concern
  - if we need to unroll layers of taken locks

> 3) They don't do anything for return in the middle bugs. [...]

Of course they do: if the standard pattern is to use goto labels then 
a stray 'return' sticks out like a sore thumb.

>                                                    [...]  They only
>    discourage people from returning at the start of the function where
>    direct returns are fine.

I disagree - I always notice middle returns during review.

>    In theory, one place where they might be useful is if we added
>    locking to a function later on.  First of all, when we add locking no
>    one ever changes the out: label to out_unlock:.  It never happens.

I frequently do that in code I maintain, but if it's missing anywhere 
I'll take patches gladly - control flow readability, especially as it 
relates to exception/error handling, is one of my pet peeves.

>    Also missing unlocks on error paths is sort of a sloppy/newbie
>    mistake.  I have looked through the git log and newbies are just as
>    likely to get locking wrong when there is an exit label as when there
>    is not.
> 
>    https://plus.google.com/106378716002406849458/posts/DfuAkt8szf2

The main point isn't to discourage newbies from making newbie 
mistakes.

The point is to make code more naturally readable/maintainable and 
extensible in the long run.

> 4) Exit labels introduce a whole new class of "forgot to set the error
>    code" bugs which don't happen with direct returns.

Well, that's not really a concern as long as you use robust 
techniques:

 - initialize the return code before any attempt that might fail.
 - update the return code accordingly

The worst that can happen in that case is to return say -ENODEV 
instead of -ENOMEM which isn't a serious bug in any case, and it 
typically sticks out during review as well.

Plus what you have not considered, direct returns may have some code 
generation costs as well.

For example branch predictions: forward labels are more likely to 
result in default-untaken forward branches - while direct returns are 
more likely to result in a RET instruction which is suboptimal.

( Which is obviously not a concern in init code - but it's of 
  relevence in hotter codepaths. )

Thanks,

	Ingo

Re: [patch] perf/x86/intel/pt: cleanup error handling in pt_pmu_hw_init()

[Julia Lawall]

> > 2) Exit labels bad associations because code which uses err: or out:
> >    labels is often buggy as we have seen in pt_event_add().
> 
> Not if the cleanup path is robustly constructed. Multiple labels can 
> also be used:
> 
>   - if performance is of concern
>   - if we need to unroll layers of taken locks

A cascade of labels helps the reader understand what goes with what, in
what order, because functions are not called when they are not needed.

julia

Re: [patch] perf/x86/intel/pt: cleanup error handling in pt_pmu_hw_init()

[Ingo Molnar]

* Julia Lawall <julia.lawall@lip6.fr> wrote:

> > > 2) Exit labels bad associations because code which uses err: or out:
> > >    labels is often buggy as we have seen in pt_event_add().
> > 
> > Not if the cleanup path is robustly constructed. Multiple labels can 
> > also be used:
> > 
> >   - if performance is of concern
> >   - if we need to unroll layers of taken locks
> 
> A cascade of labels helps the reader understand what goes with what, 
> in what order, because functions are not called when they are not 
> needed.

I certainly agree for complex init/teardown sequences that are 
non-trivial to clean up.

For the specific case of kfree() in init paths it's also a valid model 
to just auto-cleanup everything in the failure path, to simplify 
things. (which I did here.)

Thanks,

	Ingo

[tip:perf/core] perf/x86/intel/pt: Clean up the control flow in pt_pmu_hw_init()

[tip-bot for Ingo Molnar]

Commit-ID:  066450be419fa48007a9f29e19828f2a86198754
Gitweb:     http://git.kernel.org/tip/066450be419fa48007a9f29e19828f2a86198754
Author:     Ingo Molnar <mingo@kernel.org>
AuthorDate: Sun, 12 Apr 2015 11:11:21 +0200
Committer:  Ingo Molnar <mingo@kernel.org>
CommitDate: Sun, 12 Apr 2015 11:21:15 +0200

perf/x86/intel/pt: Clean up the control flow in pt_pmu_hw_init()

Dan Carpenter pointed out that the control flow in pt_pmu_hw_init()
is a bit messy: for example the kfree(de_attrs) is entirely
superfluous.

Another problem is the inconsistent mixing of label based and
direct return error handling.

Add modern, label based error handling instead and clean up the code
a bit as well.

Note that we'll still do a kfree(NULL) in the normal case - this does
not matter as this is an init path and kfree() returns early if it
sees a NULL.

Reported-by: Dan Carpenter <dan.carpenter@oracle.com>
Cc: Alexander Shishkin <alexander.shishkin@linux.intel.com>
Cc: Arnaldo Carvalho de Melo <acme@kernel.org>
Cc: Paul Mackerras <paulus@samba.org>
Cc: Peter Zijlstra <a.p.zijlstra@chello.nl>
Link: http://lkml.kernel.org/r/20150409090805.GG17605@mwanda
Signed-off-by: Ingo Molnar <mingo@kernel.org>
---
 arch/x86/kernel/cpu/perf_event_intel_pt.c | 53 +++++++++++++++++--------------
 1 file changed, 30 insertions(+), 23 deletions(-)

diff --git a/arch/x86/kernel/cpu/perf_event_intel_pt.c b/arch/x86/kernel/cpu/perf_event_intel_pt.c
index f5a3afc..f277064 100644
--- a/arch/x86/kernel/cpu/perf_event_intel_pt.c
+++ b/arch/x86/kernel/cpu/perf_event_intel_pt.c
@@ -119,48 +119,55 @@ static int __init pt_pmu_hw_init(void)
 	struct dev_ext_attribute *de_attrs;
 	struct attribute **attrs;
 	size_t size;
+	int ret;
 	long i;
 
-	if (test_cpu_cap(&boot_cpu_data, X86_FEATURE_INTEL_PT)) {
-		for (i = 0; i < PT_CPUID_LEAVES; i++)
-			cpuid_count(20, i,
-				    &pt_pmu.caps[CR_EAX + i * 4],
-				    &pt_pmu.caps[CR_EBX + i * 4],
-				    &pt_pmu.caps[CR_ECX + i * 4],
-				    &pt_pmu.caps[CR_EDX + i * 4]);
-	} else {
-		return -ENODEV;
+	attrs = NULL;
+	ret = -ENODEV;
+	if (!test_cpu_cap(&boot_cpu_data, X86_FEATURE_INTEL_PT))
+		goto fail;
+
+	for (i = 0; i < PT_CPUID_LEAVES; i++) {
+		cpuid_count(20, i,
+			    &pt_pmu.caps[CR_EAX + i*4],
+			    &pt_pmu.caps[CR_EBX + i*4],
+			    &pt_pmu.caps[CR_ECX + i*4],
+			    &pt_pmu.caps[CR_EDX + i*4]);
 	}
 
-	size = sizeof(struct attribute *) * (ARRAY_SIZE(pt_caps) + 1);
+	ret = -ENOMEM;
+	size = sizeof(struct attribute *) * (ARRAY_SIZE(pt_caps)+1);
 	attrs = kzalloc(size, GFP_KERNEL);
 	if (!attrs)
-		goto err_attrs;
+		goto fail;
 
-	size = sizeof(struct dev_ext_attribute) * (ARRAY_SIZE(pt_caps) + 1);
+	size = sizeof(struct dev_ext_attribute) * (ARRAY_SIZE(pt_caps)+1);
 	de_attrs = kzalloc(size, GFP_KERNEL);
 	if (!de_attrs)
-		goto err_de_attrs;
+		goto fail;
 
 	for (i = 0; i < ARRAY_SIZE(pt_caps); i++) {
-		de_attrs[i].attr.attr.name = pt_caps[i].name;
+		struct dev_ext_attribute *de_attr = de_attrs + i;
+
+		de_attr->attr.attr.name = pt_caps[i].name;
+
+		sysfs_attr_init(&de_attrs->attr.attr);
 
-		sysfs_attr_init(&de_attrs[i].attr.attr);
-		de_attrs[i].attr.attr.mode = S_IRUGO;
-		de_attrs[i].attr.show = pt_cap_show;
-		de_attrs[i].var = (void *)i;
-		attrs[i] = &de_attrs[i].attr.attr;
+		de_attr->attr.attr.mode		= S_IRUGO;
+		de_attr->attr.show		= pt_cap_show;
+		de_attr->var			= (void *)i;
+
+		attrs[i] = &de_attr->attr.attr;
 	}
 
 	pt_cap_group.attrs = attrs;
+
 	return 0;
 
-err_de_attrs:
-	kfree(de_attrs);
-err_attrs:
+fail:
 	kfree(attrs);
 
-	return -ENOMEM;
+	return ret;
 }
 
 #define PT_CONFIG_MASK (RTIT_CTL_TSC_EN | RTIT_CTL_DISRETC)

Re: [patch] perf/x86/intel/pt: cleanup error handling in pt_pmu_hw_init()

[Alexander Shishkin]

Dan Carpenter <dan.carpenter@oracle.com> writes:

> That's not the style that the rest of this file uses.  Every function
> uses direct returns where possible except pt_event_add() and that
> function seems buggy.

Indeed.

> arch/x86/kernel/cpu/perf_event_intel_pt.c
>   1000  
>   1001          if (mode & PERF_EF_START) {
>   1002                  pt_event_start(event, 0);
>   1003                  if (hwc->state = PERF_HES_STOPPED) {
>   1004                          pt_event_del(event, 0);
>   1005                          ret = -EBUSY;
>                                 ^^^^^^^^^^^^
> We set "ret" here but then return zero.
>
>   1006                  }
>   1007          } else {
>   1008                  hwc->state = PERF_HES_STOPPED;
>                         ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
>
> Shouldn't we set "ret" here?

No, or we'll end up returning -EBUSY where we should return zero for
snapshot counters. It can be done above the quoted if statement.

How does the following look to you?

From 726515f8bbef2ca02c495695b9451533d1bc6207 Mon Sep 17 00:00:00 2001
From: Alexander Shishkin <alexander.shishkin@linux.intel.com>
Date: Wed, 15 Apr 2015 12:56:52 +0300
Subject: [PATCH] perf/x86/intel/pt: cleanup error paths in pt_event_add()

pt_event_add() ends up returning 0 instead of -EBUSY in case of failure
to start the newly added event. This is a result of complex handling of
its return code.

This patch makes the return code handling of pt_event_add() more obvious
and fixes the mentioned bug.

Signed-off-by: Alexander Shishkin <alexander.shishkin@linux.intel.com>
---
 arch/x86/kernel/cpu/perf_event_intel_pt.c | 16 ++++++++--------
 1 file changed, 8 insertions(+), 8 deletions(-)

diff --git a/arch/x86/kernel/cpu/perf_event_intel_pt.c b/arch/x86/kernel/cpu/perf_event_intel_pt.c
index b58ba99cf8..b61a337856 100644
--- a/arch/x86/kernel/cpu/perf_event_intel_pt.c
+++ b/arch/x86/kernel/cpu/perf_event_intel_pt.c
@@ -1110,15 +1110,17 @@ static int pt_event_add(struct perf_event *event, int mode)
 	struct pt_buffer *buf;
 	struct pt *pt = this_cpu_ptr(&pt_ctx);
 	struct hw_perf_event *hwc = &event->hw;
-	int ret = -EBUSY;
+	int ret = 0;
 
-	if (pt->handle.event)
-		goto out;
+	if (pt->handle.event) {
+		ret = -EBUSY;
+		goto out_stop;
+	}
 
 	buf = perf_aux_output_begin(&pt->handle, event);
 	if (!buf) {
 		ret = -EINVAL;
-		goto out;
+		goto out_stop;
 	}
 
 	pt_buffer_reset_offsets(buf, pt->handle.head);
@@ -1126,7 +1128,7 @@ static int pt_event_add(struct perf_event *event, int mode)
 		ret = pt_buffer_reset_markers(buf, &pt->handle);
 		if (ret) {
 			perf_aux_output_end(&pt->handle, 0, true);
-			goto out;
+			goto out_stop;
 		}
 	}
 
@@ -1140,9 +1142,7 @@ static int pt_event_add(struct perf_event *event, int mode)
 		hwc->state = PERF_HES_STOPPED;
 	}
 
-	ret = 0;
-out:
-
+out_stop:
 	if (ret)
 		hwc->state = PERF_HES_STOPPED;
 
-- 
2.1.4

Re: [patch] perf/x86/intel/pt: cleanup error handling in pt_pmu_hw_init()

[Dan Carpenter]

On Wed, Apr 15, 2015 at 01:10:17PM +0300, Alexander Shishkin wrote:
> Dan Carpenter <dan.carpenter@oracle.com> writes:
> 
> > That's not the style that the rest of this file uses.  Every function
> > uses direct returns where possible except pt_event_add() and that
> > function seems buggy.
> 
> Indeed.
> 
> > arch/x86/kernel/cpu/perf_event_intel_pt.c
> >   1000  
> >   1001          if (mode & PERF_EF_START) {
> >   1002                  pt_event_start(event, 0);
> >   1003                  if (hwc->state = PERF_HES_STOPPED) {
> >   1004                          pt_event_del(event, 0);
> >   1005                          ret = -EBUSY;
> >                                 ^^^^^^^^^^^^
> > We set "ret" here but then return zero.
> >
> >   1006                  }
> >   1007          } else {
> >   1008                  hwc->state = PERF_HES_STOPPED;
> >                         ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> >
> > Shouldn't we set "ret" here?
> 
> No, or we'll end up returning -EBUSY where we should return zero for
> snapshot counters. It can be done above the quoted if statement.
> 
> How does the following look to you?
> 

Are you sure you want to know??  :P

> @@ -1140,9 +1142,7 @@ static int pt_event_add(struct perf_event *event, int mode)
>  		hwc->state = PERF_HES_STOPPED;
>  	}
>  
> -	ret = 0;
> -out:
> -
> +out_stop:
>  	if (ret)
>  		hwc->state = PERF_HES_STOPPED;

Of course, I would prefer:

	return 0;

out_stop:
	hwc->state = PERF_HES_STOPPED;

	return ret;

But your version is also fine.

Reviewed-by: Dan Carpenter <dan.carpenter@oracle.com>

regards,
dan carpenter

Re: [patch] perf/x86/intel/pt: cleanup error handling in pt_pmu_hw_init()

[Alexander Shishkin]

Dan Carpenter <dan.carpenter@oracle.com> writes:

> On Wed, Apr 15, 2015 at 01:10:17PM +0300, Alexander Shishkin wrote:
>> How does the following look to you?
>> 
>
> Are you sure you want to know??  :P

Why would I ask if I didn't?

> Of course, I would prefer:
>
> 	return 0;
>
> out_stop:
> 	hwc->state = PERF_HES_STOPPED;
>
> 	return ret;
>
> But your version is also fine.
>
> Reviewed-by: Dan Carpenter <dan.carpenter@oracle.com>

Thanks,
--
Alex