2010-12-05 10:54:09

by Jovi Zhang

[permalink] [raw]
Subject: [PATCH] perf_event: fix error handling path

fix error handling path

Signed-off-by: Jovi Zhang <[email protected]>
kernel/perf_event.c | 2 --
1 files changed, 0 insertions(+), 2 deletions(-)

diff --git a/kernel/perf_event.c b/kernel/perf_event.c
index cb6c0d2..62f9e9d 100644
--- a/kernel/perf_event.c
+++ b/kernel/perf_event.c
@@ -1918,8 +1918,6 @@ static int get_callchain_buffers(void)
}

err = alloc_callchain_buffers();
- if (err)
- release_callchain_buffers();
exit:
mutex_unlock(&callchain_mutex);


2010-12-05 12:29:05

by Thiago Farina

[permalink] [raw]
Subject: Re: [PATCH] perf_event: fix error handling path

On Sat, Dec 4, 2010 at 1:19 AM, <[email protected]> wrote:
> fix error handling path
>
> Signed-off-by: Jovi Zhang <[email protected]>
>  kernel/perf_event.c |    2 --
>  1 files changed, 0 insertions(+), 2 deletions(-)
>
> diff --git a/kernel/perf_event.c b/kernel/perf_event.c
> index cb6c0d2..62f9e9d 100644
> --- a/kernel/perf_event.c
> +++ b/kernel/perf_event.c
> @@ -1918,8 +1918,6 @@ static int get_callchain_buffers(void)
>        }
>
>        err = alloc_callchain_buffers();
> -       if (err)
> -               release_callchain_buffers();

Care to explain in the change log message? As I reader, is not clear
to me what is wrong with this.

2010-12-06 01:59:39

by Jovi Zhang

[permalink] [raw]
Subject: Re: [PATCH] perf_event: fix error handling path

On Sun, Dec 5, 2010 at 8:29 PM, Thiago Farina <[email protected]> wrote:
>
> On Sat, Dec 4, 2010 at 1:19 AM,  <[email protected]> wrote:
> > fix error handling path
> >
> > Signed-off-by: Jovi Zhang <[email protected]>
> >  kernel/perf_event.c |    2 --
> >  1 files changed, 0 insertions(+), 2 deletions(-)
> >
> > diff --git a/kernel/perf_event.c b/kernel/perf_event.c
> > index cb6c0d2..62f9e9d 100644
> > --- a/kernel/perf_event.c
> > +++ b/kernel/perf_event.c
> > @@ -1918,8 +1918,6 @@ static int get_callchain_buffers(void)
> >        }
> >
> >        err = alloc_callchain_buffers();
> > -       if (err)
> > -               release_callchain_buffers();
>
> Care to explain in the change log message? As I reader, is not clear
> to me what is wrong with this.

Sorry, the description should be as:
fix error handling path. alloc_callchain_buffers() can return -ENOMEM,
in this time callchain_cpus_entries maybe is NULL, It will oops if
invoke release_callchain_buffers() when callchain_cpus_entries is
NULL.

2010-12-08 01:52:07

by Jovi Zhang

[permalink] [raw]
Subject: Re: [PATCH] perf_event: fix error handling path

On Mon, Dec 6, 2010 at 9:59 AM, jovi zhang <[email protected]> wrote:
> On Sun, Dec 5, 2010 at 8:29 PM, Thiago Farina <[email protected]> wrote:
>>
>> On Sat, Dec 4, 2010 at 1:19 AM,  <[email protected]> wrote:
>> > fix error handling path
>> >
>> > Signed-off-by: Jovi Zhang <[email protected]>
>> >  kernel/perf_event.c |    2 --
>> >  1 files changed, 0 insertions(+), 2 deletions(-)
>> >
>> > diff --git a/kernel/perf_event.c b/kernel/perf_event.c
>> > index cb6c0d2..62f9e9d 100644
>> > --- a/kernel/perf_event.c
>> > +++ b/kernel/perf_event.c
>> > @@ -1918,8 +1918,6 @@ static int get_callchain_buffers(void)
>> >        }
>> >
>> >        err = alloc_callchain_buffers();
>> > -       if (err)
>> > -               release_callchain_buffers();
>>
>> Care to explain in the change log message? As I reader, is not clear
>> to me what is wrong with this.
>
> Sorry, the description should be as:
> fix error handling path. alloc_callchain_buffers() can return -ENOMEM,
> in this time callchain_cpus_entries maybe is NULL, It will oops if
> invoke release_callchain_buffers() when callchain_cpus_entries is
> NULL.
>
I hope my understanding is right, is it?

2010-12-09 17:06:51

by Corey Ashford

[permalink] [raw]
Subject: Re: [PATCH] perf_event: fix error handling path

On 12/07/2010 05:51 PM, jovi zhang wrote:
> On Mon, Dec 6, 2010 at 9:59 AM, jovi zhang<[email protected]> wrote:
>> On Sun, Dec 5, 2010 at 8:29 PM, Thiago Farina<[email protected]> wrote:
>>>
>>> On Sat, Dec 4, 2010 at 1:19 AM,<[email protected]> wrote:
>>>> fix error handling path
>>>>
>>>> Signed-off-by: Jovi Zhang<[email protected]>
>>>> kernel/perf_event.c | 2 --
>>>> 1 files changed, 0 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/kernel/perf_event.c b/kernel/perf_event.c
>>>> index cb6c0d2..62f9e9d 100644
>>>> --- a/kernel/perf_event.c
>>>> +++ b/kernel/perf_event.c
>>>> @@ -1918,8 +1918,6 @@ static int get_callchain_buffers(void)
>>>> }
>>>>
>>>> err = alloc_callchain_buffers();
>>>> - if (err)
>>>> - release_callchain_buffers();
>>>
>>> Care to explain in the change log message? As I reader, is not clear
>>> to me what is wrong with this.
>>
>> Sorry, the description should be as:
>> fix error handling path. alloc_callchain_buffers() can return -ENOMEM,
>> in this time callchain_cpus_entries maybe is NULL, It will oops if
>> invoke release_callchain_buffers() when callchain_cpus_entries is
>> NULL.
>>
> I hope my understanding is right, is it?

One possible problem here is what if it returns an error other than
-ENOMEM, and the buffers do need to be released? Maybe you could change
the code to

err = alloc_callchain_buffers();
if (err != -ENOMEM)
release_callchain_buffers();


Currently, alloc_callchain_buffers cannot return any error code other
than -ENOMEM, but that might change in the future.

- Corey

2010-12-09 17:18:12

by Peter Zijlstra

[permalink] [raw]
Subject: Re: [PATCH] perf_event: fix error handling path

On Thu, 2010-12-09 at 09:06 -0800, Corey Ashford wrote:
> On 12/07/2010 05:51 PM, jovi zhang wrote:
> > On Mon, Dec 6, 2010 at 9:59 AM, jovi zhang<[email protected]> wrote:
> >> On Sun, Dec 5, 2010 at 8:29 PM, Thiago Farina<[email protected]> wrote:
> >>>
> >>> On Sat, Dec 4, 2010 at 1:19 AM,<[email protected]> wrote:
> >>>> fix error handling path
> >>>>
> >>>> Signed-off-by: Jovi Zhang<[email protected]>
> >>>> kernel/perf_event.c | 2 --
> >>>> 1 files changed, 0 insertions(+), 2 deletions(-)
> >>>>
> >>>> diff --git a/kernel/perf_event.c b/kernel/perf_event.c
> >>>> index cb6c0d2..62f9e9d 100644
> >>>> --- a/kernel/perf_event.c
> >>>> +++ b/kernel/perf_event.c
> >>>> @@ -1918,8 +1918,6 @@ static int get_callchain_buffers(void)
> >>>> }
> >>>>
> >>>> err = alloc_callchain_buffers();
> >>>> - if (err)
> >>>> - release_callchain_buffers();
> >>>
> >>> Care to explain in the change log message? As I reader, is not clear
> >>> to me what is wrong with this.
> >>
> >> Sorry, the description should be as:
> >> fix error handling path. alloc_callchain_buffers() can return -ENOMEM,
> >> in this time callchain_cpus_entries maybe is NULL, It will oops if
> >> invoke release_callchain_buffers() when callchain_cpus_entries is
> >> NULL.
> >>
> > I hope my understanding is right, is it?
>
> One possible problem here is what if it returns an error other than
> -ENOMEM, and the buffers do need to be released? Maybe you could change
> the code to
>
> err = alloc_callchain_buffers();
> if (err != -ENOMEM)
> release_callchain_buffers();
>
>
> Currently, alloc_callchain_buffers cannot return any error code other
> than -ENOMEM, but that might change in the future.

The kernel convention is to fully clean up after yourself if you return
an error. So in that sense the patch seems right.

Anyway, anybody care to post a new patch with slightly extended
changelog?

2010-12-10 02:27:53

by Jovi Zhang

[permalink] [raw]
Subject: Re: [PATCH] perf_event: fix error handling path

On Fri, Dec 10, 2010 at 1:17 AM, Peter Zijlstra <[email protected]> wrote:
> On Thu, 2010-12-09 at 09:06 -0800, Corey Ashford wrote:
>> On 12/07/2010 05:51 PM, jovi zhang wrote:
>> > On Mon, Dec 6, 2010 at 9:59 AM, jovi zhang<[email protected]>  wrote:
>> >> On Sun, Dec 5, 2010 at 8:29 PM, Thiago Farina<[email protected]>  wrote:
>> >>>
>> >>> On Sat, Dec 4, 2010 at 1:19 AM,<[email protected]>  wrote:
>> >>>> fix error handling path
>> >>>>
>> >>>> Signed-off-by: Jovi Zhang<[email protected]>
>> >>>>   kernel/perf_event.c |    2 --
>> >>>>   1 files changed, 0 insertions(+), 2 deletions(-)
>> >>>>
>> >>>> diff --git a/kernel/perf_event.c b/kernel/perf_event.c
>> >>>> index cb6c0d2..62f9e9d 100644
>> >>>> --- a/kernel/perf_event.c
>> >>>> +++ b/kernel/perf_event.c
>> >>>> @@ -1918,8 +1918,6 @@ static int get_callchain_buffers(void)
>> >>>>         }
>> >>>>
>> >>>>         err = alloc_callchain_buffers();
>> >>>> -       if (err)
>> >>>> -               release_callchain_buffers();
>> >>>
>> >>> Care to explain in the change log message? As I reader, is not clear
>> >>> to me what is wrong with this.
>> >>
>> >> Sorry, the description should be as:
>> >> fix error handling path. alloc_callchain_buffers() can return -ENOMEM,
>> >> in this time callchain_cpus_entries maybe is NULL, It will oops if
>> >> invoke release_callchain_buffers() when callchain_cpus_entries is
>> >> NULL.
>> >>
>> > I hope my understanding is right, is it?
>>
>> One possible problem here is what if it returns an error other than
>> -ENOMEM, and the buffers do need to be released?  Maybe you could change
>> the code to
>>
>> err = alloc_callchain_buffers();
>> if (err != -ENOMEM)
>>       release_callchain_buffers();
>>

Thanks for you suggestion. I also thought it, but I think it should
release the buffers in alloc_callchain_buffers if there have some
error(like many other part of kernel code).

It's not a good way to trace return error code outside of
alloc_callchain_buffer, if there have many return error code in
future, maybe need to write code like this:
if(err && err != -ENOMEM && err != -EXXX ) ...

It's very ugly. is right?

>>
>> Currently, alloc_callchain_buffers cannot return any error code other
>> than -ENOMEM, but that might change in the future.
>
> The kernel convention is to fully clean up after yourself if you return
> an error. So in that sense the patch seems right.
>
> Anyway, anybody care to post a new patch with slightly extended
> changelog?
>