Re: [PATCH] oprofile, arm: proper release resources on failure

[Robert Richter <robert.richter@amd.com>]

On 29.09.10 11:39:44, Will Deacon wrote:

> I have a few minor clarifications:
> > 
> 
> > diff --git a/arch/arm/oprofile/common.c b/arch/arm/oprofile/common.c
> > index 0691176..72e09eb 100644
> > --- a/arch/arm/oprofile/common.c
> > +++ b/arch/arm/oprofile/common.c
> > @@ -102,6 +102,7 @@ static int op_create_counter(int cpu, int event)
> >         if (IS_ERR(pevent)) {
> >                 ret = PTR_ERR(pevent);
> >         } else if (pevent->state != PERF_EVENT_STATE_ACTIVE) {
> > +               perf_event_release_kernel(pevent);
> >                 pr_warning("oprofile: failed to enable event %d "
> >                                 "on CPU %d\n", event, cpu);
> >                 ret = -EBUSY;
> 
> 
> Yup, this is needed. Thanks! It would be nice to do away with the
> else statement altogether but I think adding a pinned event and
> then failing to activate it still succeeds in
> perf_event_create_kernel_counter.

Yes, I have cleanup code already for this. But as this is an -rc and
stable fix, I didn't want to change too much here. But I will send a
follow-on patch with the cleanup I made.

> 
> 
> > @@ -365,6 +366,7 @@ int __init oprofile_arch_init(struct oprofile_operations *ops)
> >         ret = init_driverfs();
> >         if (ret) {
> >                 kfree(counter_config);
> > +               counter_config = NULL;
> >                 return ret;
> >         }
> > 
> > @@ -402,7 +404,6 @@ void oprofile_arch_exit(void)
> >         struct perf_event *event;
> > 
> >         if (*perf_events) {
> > -               exit_driverfs();
> >                 for_each_possible_cpu(cpu) {
> >                         for (id = 0; id < perf_num_counters; ++id) {
> >                                 event = perf_events[cpu][id];
> > @@ -413,8 +414,10 @@ void oprofile_arch_exit(void)
> >                 }
> >         }
> > 
> > -       if (counter_config)
> > +       if (counter_config) {
> >                 kfree(counter_config);
> > +               exit_driverfs();
> > +       }
> >  }
> >  #else
> >  int __init oprofile_arch_init(struct oprofile_operations *ops)
> > --
> > 1.7.2.2
> > 
> 
> Hmm, these three hunks conflict with the patches I posted last
> month to fix the resource allocation and freeing. Can't we
> merge those patches instead? I have versions against -rc6 here:

Yes, these patches are also in my oprofile/core branch and its changes
conflict, but are scheduled for the next merge window. This fix is for
urgent and for 2.6.36. We will then merge back rc7 or 2.6.36 to
oprofile/core and solve the conflicts. But if you don't have something
else for -rc7/.36 that conflicts, this should be ok.

-Robert

-- 
Advanced Micro Devices, Inc.
Operating System Research Center