Re: [patch] tlb flush_data: replace per_cpu with an array

Ingo Molnar
2009-01-13T12:28:52Z

* Andi Kleen <andi@firstfloor.org> wrote:

> > No distro kernel will build with less than 8 CPUs anyway so this point
> > is moot.
>
> It has nothing to do with what the distro kernel builds with. As I
> stated clearly in my review the per cpu data is sized based on the
> possible map, which is discovered from the BIOS at runtime. So if your
> system has two cores only you will only have two copies of per cpu data.

You ignored my observation that by the time this hits distro kernels the
usual SMP hw size will be 8 or more cores.

> With this patch on the other hand you will always have 8 copies of this
> data; aka 1K no matter how many CPUs you have.

Firstly, it's 512 bytes (see below), secondly, with the percpu approach we
waste far more total memory as time goes on and the average core count
increases.

> So the description that it saves memory is flat out wrong on any system
> with less than 8 threads (which is by far the biggest majority of
> systems currently and in the forseeable future)
>
> > > You would need to cache line pad each entry then, otherwise you risk
> > > false sharing. [...]
> >
> > They are already cache line padded.
>
> Yes that's the problem here.

I fail to see a problem. It has to be padded and aligned no matter where
it is - and it is padded and aligned both before and after the patch. I
dont know why you keep insisting that there's a problem where there is
none.

> > > [...] That would make the array 1K on 128 bytes cache line system.
> >
> > 512 bytes.
>
> 8 * 128 = 1024

The default cacheline size in the x86 tree (for generic cpus) is 64 bytes,
not 128 bytes - so the default size of the array is 512 bytes, not 1024
bytes. (This is a change we made yesterday, so you can not have known
about it unless you follow the x86 tree closely.)

Ingo