Re: [patch 5/8] x86: use this_cpu_xxx to replace percpu_xxx funcs

Re: [patch 5/8] x86: use this_cpu_xxx to replace percpu_xxx funcs

[Ingo Molnar]

* akpm@linux-foundation.org <akpm@linux-foundation.org> wrote:

> From: Alex Shi <alex.shi@intel.com>
> Subject: x86: use this_cpu_xxx to replace percpu_xxx funcs
> 
> Since percpu_xxx() serial functions are duplicate with this_cpu_xxx(). 
> Removing percpu_xxx() definition and replacing them by this_cpu_xxx() in
> code.
> 
> And further more, as Christoph Lameter's requirement, I try to use
> __this_cpu_xx to replace this_cpu_xxx if it is in preempt safe scenario.
> The preempt safe scenarios include:
> 1, in irq/softirq/nmi handler
> 2, protected by preempt_disable
> 3, protected by spin_lock
> 4, if the code context imply that it is preempt safe, like the code is
> follows or be followed a preempt safe code.
> 
> BTW, In fact, this_cpu_xxx are same as __this_cpu_xxx since all funcs
> implement in a single instruction for x86 machine.  But it maybe other
> platforms' performance.

but this is an x86 only patch.

> -	percpu_write(irq_regs, new_regs);
> +	__this_cpu_write(irq_regs, new_regs);

So what's the point of all this ugly churn?

Will percpu_write() be removed altogether? If so then the 
changelog should say *that*, and explain that on x86 this is a 
simple renaming of the API, not a long explanation about 
scenarios that don't actually matter.

If percpu_write() is not being removed then I don't see how this 
patch is an improvement: it's supposed to result in the same 
instructions being emitted, and __this_cpu_write() et al are 
distinctly longer to write ...

So what's the plan here?

Thanks,

	Ingo

Re: [patch 5/8] x86: use this_cpu_xxx to replace percpu_xxx funcs

[Andrew Morton]

On Tue, 6 Mar 2012 08:46:01 +0100 Ingo Molnar <mingo@elte.hu> wrote:

> Will percpu_write() be removed altogether? If so then the 
> changelog should say *that*, and explain that on x86 this is a 
> simple renaming of the API, not a long explanation about 
> scenarios that don't actually matter.
> 
> If percpu_write() is not being removed then I don't see how this 
> patch is an improvement: it's supposed to result in the same 
> instructions being emitted, and __this_cpu_write() et al are 
> distinctly longer to write ...
> 
> So what's the plan here?

All percpu_xxx() functions get removed.

That was explained in the [patch 0/n] changelog (available on lkml ;))
but a) I chose to feed the prereq patches through maintainers and b) I
didn't edit all the changelogs.  I should have.

Re: [patch 5/8] x86: use this_cpu_xxx to replace percpu_xxx funcs

[Ingo Molnar]

* Andrew Morton <akpm@linux-foundation.org> wrote:

> On Tue, 6 Mar 2012 08:46:01 +0100 Ingo Molnar <mingo@elte.hu> wrote:
> 
> > Will percpu_write() be removed altogether? If so then the 
> > changelog should say *that*, and explain that on x86 this is a 
> > simple renaming of the API, not a long explanation about 
> > scenarios that don't actually matter.
> > 
> > If percpu_write() is not being removed then I don't see how this 
> > patch is an improvement: it's supposed to result in the same 
> > instructions being emitted, and __this_cpu_write() et al are 
> > distinctly longer to write ...
> > 
> > So what's the plan here?
> 
> All percpu_xxx() functions get removed.

Fair enough - then please see my namespace comments for the 
other patch, before we start spreading these APIs to hundreds of 
places ...

Also, IMHO the lack of debugging API is troubling as well. 
Thomas found a couple of really, really hairy this_cpu related 
bugs.

Thanks,

	Ingo

Re: [patch 5/8] x86: use this_cpu_xxx to replace percpu_xxx funcs

[Christoph Lameter]

On Tue, 6 Mar 2012, Ingo Molnar wrote:

> > All percpu_xxx() functions get removed.
>
> Fair enough - then please see my namespace comments for the
> other patch, before we start spreading these APIs to hundreds of
> places ...

Its already almost everywhere... This removes some of the last holdouts.

> Also, IMHO the lack of debugging API is troubling as well.

For that there would need to be some basic understanding as to what is
going on here. These are fundamentally atomic style operations and I keep
hearing that people want to check if they are in a preempt section etc.
I do not see how that is possible for the this_cpu_ops.

However, checks could be done this for the __this_cpu_ops (which rely on
the processor not changing during their execution for their fallback
implementation, therefore preempt should be off) by simply adding a check
to the basic macros (add to __this_cpu_ptr for example). The problem is
then that there needs to be an audit first to see if there are cases where
we run into trouble with such an approach. There are situations in which
the use of __this_cpu ops is legitimate for other reasons if no concurrent
per cpu access can occur or we cannot be moved to a different processor
(because the process pinned to a processor f.e.)

> Thomas found a couple of really, really hairy this_cpu related
> bugs.

Well yeah if one uses per cpu operations and then allows execution to
occur on a different processor then one will not operate on the same
entity. The same issue would have occurred if they had used the old
percpu operations.