Re: [PATCH v3] oprofile, x86: Convert memory allocation to staticarray

[Robert Richter]

On 01.08.11 17:41:30, Andi Kleen wrote:
> On Mon, Aug 01, 2011 at 11:31:42PM +0200, Robert Richter wrote:
> > On 01.08.11 11:08:59, Maarten Lankhorst wrote:
> > > On -rt, allocators don't work from atomic context any more,
> > > and the maximum size of the array is known at compile time.
> > > 
> > > Signed-off-by: Maarten Lankhorst <m.b.lankhorst@xxxxxxxxx>
> > 
> > Applied to oprofile/core. Thanks, Maarten.
> 
> And what happens when the CPU reports more than 32 counters?
> You have a silent buffer overflow then.

>From the layout of IA32_PERF_GLOBAL_CTRL MSR it seems to be limited to
32. Anyway, if there might be cpus out soon with more than 32 counters
we can either extend the array to 256 or limit the counters used to 32.

> Besides I bet there are other cases like this all over the tree.
> 
> And the whole thing is tasteless. 
> 
> Nacked-by: Andi Kleen <ak@xxxxxxxxxxxxxxx>

This dynamic allocation is causing trouble from the beginning. Using a
static array removes a lot of NULL pointer checks:

 arch/x86/oprofile/op_model_ppro.c |   24 +-----------------------

The previous implementation silently dropped counters during setup on
failure. Also we have had many bugs caused by this dynamic allocation
(I counted 3 fixes). This all isn't it worth and you did not yet
explain the concerns you have (besides the potentially buffer overflow
which is fixable and probably of academic nature).

-Robert

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

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