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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <1282905691.16770.20.camel@e102144-lin.cambridge.arm.com>
Date:	Fri, 27 Aug 2010 11:41:31 +0100
From:	Will Deacon <will.deacon@....com>
To:	Matt Fleming <matt@...sole-pimps.org>
Cc:	linux-kernel@...r.kernel.org,
	Robert Richter <robert.richter@....com>,
	Paul Mundt <lethal@...ux-sh.org>,
	Russell King <linux@....linux.org.uk>,
	linux-arm-kernel@...ts.infradead.org, linux-sh@...r.kernel.org,
	Peter Zijlstra <peterz@...radead.org>,
	Ingo Molnar <mingo@...e.hu>,
	Frederic Weisbecker <fweisbec@...il.com>,
	Arnaldo Carvalho de Melo <acme@...hat.com>,
	linux-arch@...r.kernel.org
Subject: Re: [PATCH V2 3/4] oprofile: Abstract the perf-events backend

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?

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

typo :)

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.

Cheers,

Will

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/