* [PATCH] perf_event: fix error handling path
@ 2010-12-04 3:19 bookjovi
2010-12-05 12:29 ` Thiago Farina
0 siblings, 1 reply; 7+ messages in thread
From: bookjovi @ 2010-12-04 3:19 UTC (permalink / raw)
To: a.p.zijlstra, paulus, mingo, acme, linux-kernel; +Cc: Jovi Zhang
fix error handling path
Signed-off-by: Jovi Zhang <bookjovi@gmail.com>
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);
^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH] perf_event: fix error handling path
2010-12-04 3:19 [PATCH] perf_event: fix error handling path bookjovi
@ 2010-12-05 12:29 ` Thiago Farina
2010-12-06 1:59 ` jovi zhang
0 siblings, 1 reply; 7+ messages in thread
From: Thiago Farina @ 2010-12-05 12:29 UTC (permalink / raw)
To: bookjovi; +Cc: a.p.zijlstra, paulus, mingo, acme, linux-kernel
On Sat, Dec 4, 2010 at 1:19 AM, <bookjovi@gmail.com> wrote:
> fix error handling path
>
> Signed-off-by: Jovi Zhang <bookjovi@gmail.com>
> 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.
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] perf_event: fix error handling path
2010-12-05 12:29 ` Thiago Farina
@ 2010-12-06 1:59 ` jovi zhang
2010-12-08 1:51 ` jovi zhang
0 siblings, 1 reply; 7+ messages in thread
From: jovi zhang @ 2010-12-06 1:59 UTC (permalink / raw)
To: Thiago Farina; +Cc: a.p.zijlstra, paulus, mingo, acme, linux-kernel
On Sun, Dec 5, 2010 at 8:29 PM, Thiago Farina <tfransosi@gmail.com> wrote:
>
> On Sat, Dec 4, 2010 at 1:19 AM, <bookjovi@gmail.com> wrote:
> > fix error handling path
> >
> > Signed-off-by: Jovi Zhang <bookjovi@gmail.com>
> > 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.
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] perf_event: fix error handling path
2010-12-06 1:59 ` jovi zhang
@ 2010-12-08 1:51 ` jovi zhang
2010-12-09 17:06 ` Corey Ashford
0 siblings, 1 reply; 7+ messages in thread
From: jovi zhang @ 2010-12-08 1:51 UTC (permalink / raw)
To: Thiago Farina; +Cc: a.p.zijlstra, paulus, mingo, acme, linux-kernel
On Mon, Dec 6, 2010 at 9:59 AM, jovi zhang <bookjovi@gmail.com> wrote:
> On Sun, Dec 5, 2010 at 8:29 PM, Thiago Farina <tfransosi@gmail.com> wrote:
>>
>> On Sat, Dec 4, 2010 at 1:19 AM, <bookjovi@gmail.com> wrote:
>> > fix error handling path
>> >
>> > Signed-off-by: Jovi Zhang <bookjovi@gmail.com>
>> > 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?
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] perf_event: fix error handling path
2010-12-08 1:51 ` jovi zhang
@ 2010-12-09 17:06 ` Corey Ashford
2010-12-09 17:17 ` Peter Zijlstra
0 siblings, 1 reply; 7+ messages in thread
From: Corey Ashford @ 2010-12-09 17:06 UTC (permalink / raw)
To: jovi zhang; +Cc: Thiago Farina, a.p.zijlstra, paulus, mingo, acme, linux-kernel
On 12/07/2010 05:51 PM, jovi zhang wrote:
> On Mon, Dec 6, 2010 at 9:59 AM, jovi zhang<bookjovi@gmail.com> wrote:
>> On Sun, Dec 5, 2010 at 8:29 PM, Thiago Farina<tfransosi@gmail.com> wrote:
>>>
>>> On Sat, Dec 4, 2010 at 1:19 AM,<bookjovi@gmail.com> wrote:
>>>> fix error handling path
>>>>
>>>> Signed-off-by: Jovi Zhang<bookjovi@gmail.com>
>>>> 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
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] perf_event: fix error handling path
2010-12-09 17:06 ` Corey Ashford
@ 2010-12-09 17:17 ` Peter Zijlstra
2010-12-10 2:27 ` jovi zhang
0 siblings, 1 reply; 7+ messages in thread
From: Peter Zijlstra @ 2010-12-09 17:17 UTC (permalink / raw)
To: Corey Ashford
Cc: jovi zhang, Thiago Farina, paulus, mingo, acme, linux-kernel
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<bookjovi@gmail.com> wrote:
> >> On Sun, Dec 5, 2010 at 8:29 PM, Thiago Farina<tfransosi@gmail.com> wrote:
> >>>
> >>> On Sat, Dec 4, 2010 at 1:19 AM,<bookjovi@gmail.com> wrote:
> >>>> fix error handling path
> >>>>
> >>>> Signed-off-by: Jovi Zhang<bookjovi@gmail.com>
> >>>> 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?
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] perf_event: fix error handling path
2010-12-09 17:17 ` Peter Zijlstra
@ 2010-12-10 2:27 ` jovi zhang
0 siblings, 0 replies; 7+ messages in thread
From: jovi zhang @ 2010-12-10 2:27 UTC (permalink / raw)
To: Peter Zijlstra
Cc: Corey Ashford, Thiago Farina, paulus, mingo, acme, linux-kernel
On Fri, Dec 10, 2010 at 1:17 AM, Peter Zijlstra <a.p.zijlstra@chello.nl> 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<bookjovi@gmail.com> wrote:
>> >> On Sun, Dec 5, 2010 at 8:29 PM, Thiago Farina<tfransosi@gmail.com> wrote:
>> >>>
>> >>> On Sat, Dec 4, 2010 at 1:19 AM,<bookjovi@gmail.com> wrote:
>> >>>> fix error handling path
>> >>>>
>> >>>> Signed-off-by: Jovi Zhang<bookjovi@gmail.com>
>> >>>> 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?
>
^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2010-12-10 2:27 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2010-12-04 3:19 [PATCH] perf_event: fix error handling path bookjovi
2010-12-05 12:29 ` Thiago Farina
2010-12-06 1:59 ` jovi zhang
2010-12-08 1:51 ` jovi zhang
2010-12-09 17:06 ` Corey Ashford
2010-12-09 17:17 ` Peter Zijlstra
2010-12-10 2:27 ` jovi zhang
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.