Re: [PATCH UPDATED] percpu: use dynamic percpu allocator as the default percpu allocator

Ingo Molnar
2009-04-02T03:45:00Z

* Christoph Lameter <cl@linux.com> wrote:

> On Thu, 2 Apr 2009, Ingo Molnar wrote:
>
> > So ... we regressed the performance of percpu_free(NULL) with a
> > potential cross-CPU cacheline bounce. Without the patch,
> > percpu_free(NULL) would never do such a bounce. So i dont think
> > the patch is a good idea.
>
> But percpu_free is not an operation typical for hot code paths.
> Per cpu variables are allocated and freed in rarely used code
> paths. Typically the use of the per cpu variables occurs in hot
> code paths, not their allocations.

to quote an earlier part of my mail:

> > We encourage kfree(NULL) and it triggers commonly in the kernel
> > today [on distro kernels we checked it can trigger once per
> > syscall!] - so i think we should consider free_percpu(NULL) a
> > possibly common pattern too. (even though today it's likely
> > _not_ common at all.)

I specifically mentioned that it is not at all common now.

But there's no reason why an object shutdown fastpath with an
optional percpu buffer (say for debug statistics, not enabled by
default) couldnt look like this:

percpu_free(NULL);

We actually have such patterns of kfree(ptr) use, where the _common_
case in a fastpath is kfree(NULL).

So, had your patch been applied as-is, we might have created this
situation.

Yes, it can be fixed, and yes, it is probably not worth even
bothering (percpu alloc/free goes over all online CPUs) - but i
found it kind of interesting that this random specific example we
ran into (i totally didnt go out and try to find some other, better
example) showed so many variable placement ambiguities and false
cacheline sharing problems, and showed visible inefficiencies in the
layout (i showed a whole line of variables wastefully spaces -
unrelated to the percpu allocator).

So these examples almost seemed to support my earlier points in the
thread where i suggested that variable placement is often random and
ambigious, and that we could fix inefficiencies in the layout ;-)

Ingo