Re: [PATCH V2 3/4] oprofile: Abstract the perf-events backend

Matt Fleming
2010-08-27T12:44:09Z

On Fri, Aug 27, 2010 at 11:41:31AM +0100, Will Deacon wrote:
> Hi Matt,
>
> I'm still not happy with the init/exit alloc/free code:
>
> On Thu, 2010-08-26 at 20:09 +0100, Matt Fleming wrote:
>
> [...]
>
> > +int oprofile_perf_init(void)
> > +{
> > + u32 counter_size = sizeof(struct op_counter_config);
> > + int cpu;
> > +
> > + counter_config = kcalloc(perf_num_counters, counter_size, GFP_KERNEL);
> > +
> > + if (!counter_config) {
> > + pr_info("oprofile: failed to allocate %d "
> > + "counters\n", perf_num_counters);
> > + return -ENOMEM;
> > + }
> > +
> > + for_each_possible_cpu(cpu) {
> > + perf_events[cpu] = kcalloc(perf_num_counters,
> > + sizeof(struct perf_event *), GFP_KERNEL);
> > + if (!perf_events[cpu]) {
> > + pr_info("oprofile: failed to allocate %d perf events "
> > + "for cpu %d\n", perf_num_counters, cpu);
> > + while (--cpu >= 0)
> > + kfree(perf_events[cpu]);
> > + kfree(counter_config);
> > + return -ENOMEM;
> > + }
> > + }
> > +
> > + return 0;
> > +}
>
> So here, if the perf_events allocation fails for a cpu, we free the
> stuff we've already allocated [including counter_config] and return
> -ENOMEM. Looking at drivers/oprofile/oprof.c:
>
> static int __init oprofile_init(void)
> {
> int err;
>
> err = oprofile_arch_init(&oprofile_ops);
> if (err < 0 || timer) {
> printk(KERN_INFO "oprofile: using timer interrupt.\n");
> err = oprofile_timer_init(&oprofile_ops);
> if (err)
> goto out_arch;
> }
> err = oprofilefs_register();
> if (err)
> goto out_arch;
> return 0;
>
> out_arch:
> oprofile_arch_exit();
> return err;
> }
>
> So now, if timer_init fails or oprofilefs_register fails, we will
> call oprofile_arch_exit, which calls oprofile_perf_exit:
>
> > +void oprofile_perf_exit(void)
> > +{
> > + int id, cpu;
> > +
> > + for_each_possible_cpu(cpu) {
> > + for (id = 0; id < perf_num_counters; ++id)
> > + oprofile_destroy_counter(cpu, id);
> > +
> > + kfree(perf_events[cpu]);
> > + }
> > +
> > + kfree(counter_config);
> > +}
>
> meaning that we will free everything again! This is what I
> was trying to avoid in patch 1/4, by moving the counter_config
> freeing into the *_exit function. Looking at it again, I think
> all the freeing should be moved to the *_exit function and the init
> function should just return error when allocation fails. What do you
> think?

*facepalm*

I dunno how I forgot to fix up that patch. Sorry. You're completely
right, I forgot to role your changes from patch 1/4 into 3/4 when I
shuffled the code around. Thank you for being diligent.

> > +/*
> > + * Create active perf events based on the perviously configured
> > + * attributes.
> > + */
>
> typo :)

Thanks.

> For what it's worth, I tested the series on my Cortex-A9 board and
> everything seemed to work fine. I'll give the patches another spin when
> we've sorted out these memory issues.

Excellent news. Thanks for testing! I'll get the next version of patch
3/4 out tonight.