Re: [RFC][PATCH 0/5] Introduce checks for preemptable code for this_cpu_read/write()

[Thomas Gleixner <tglx@linutronix.de>]

On Wed, 21 Sep 2011, Christoph Lameter wrote:
> On Tue, 20 Sep 2011, Steven Rostedt wrote:
> 
> > BTW, Can you explain to me where the this_cpu_*() ops were designed to
> > be used? The only places where "this_cpu_*()" is used in slub.c and
> > page_alloc.c have irqs disabled on their use. I thought this was for
> > slub and page_alloc?
> 
> Initially these were used for statistics that used per cpu counters. The
> slub thing was an outgrow of this.
> 
> > Is this_cpu() made just for statistics? I see it used in the inode code
> > for that, and some accounting in the namespace.c code.
> 
> That is the main use case yes.
> 
> > Note and there's places all over the kernel that uses this_cpu_read()
> > and thinks preemption should be disabled. Just look at
> > arch/x86/mm/tlb.c:
> >
> > 	/* Caller has disabled preemption */
> > 	sender = this_cpu_read(tlb_vector_offset);
> >
> > Why the comment?
> >
> > My argument is that this_cpu_* is just confusing. Rename your use case
> > and keep this_cpu_*() as what you want __this_cpu_*() to be.
> 
> Thought about this a bit last night. I think the main issue are these
> this_cpu_read() and this_cpu_write() operations since people use those
> irresponsibly. It usually does not make sense to read a value from a
> random cpu nor does writing make sense. The situation is different for
> per cpu counter increments where it does not matter which cpus counter is
> incremented since we sum them up later anyways.
> 
> How about getting rid of this_cpu_read() and this_cpu_write() entirely?
> 
> Only allow __this_cpu_read and __this_cpu_write. There we check that the
> caller has disabled preemption.
> 
> For the rare special cases (are there any?) that are legitimate use cases
> for this_cpu_read/write we can use manual determination of per cpu
> pointers and then just do a load via the pointer?
> 
> Or alternatively give this_cpu_read and write special names that make
> their dangerousness clear.
> 
> In the case of slub there are only some this_cpu_write() things that can
> be __this_cpu_write without a problem.
> 
> The __this_cpu_ptr() can become this_cpu_ptr as far as I can tell. This
> should make it consistent so that we can check for disabled preemption for
> all __this_cpu thingies.
 
The problem I have with that approach is that this_cpu_inc/dec still
look too close to __this_cpu_*.

It would really be nice to rename this_cpu_inc/dec to something which
makes it clear that this is statistics and does not care a whit about
the actual cpu on which this happens.

Thanks,

	tglx