Re: [PATCH 1/2] MN10300: Move asm-arm/cnt32_to_63.h to include/linux/

[Nicolas Pitre <nico@cam.org>]

On Fri, 26 Sep 2008, Peter Zijlstra wrote:

> On Fri, 2008-09-26 at 08:03 -0400, Nicolas Pitre wrote:
> > On Fri, 26 Sep 2008, Peter Zijlstra wrote:
> > 
> > > On Wed, 2008-09-24 at 17:48 +0100, David Howells wrote:
> > > > Move asm-arm/cnt32_to_63.h to include/linux/ so that MN10300 can make use of it
> > > > too.
> > > > 
> > > > Signed-off-by: David Howells <dhowells@redhat.com>
> > > > ---
> > > > 
> > > >  arch/arm/mach-pxa/time.c       |    2 +
> > > >  arch/arm/mach-sa1100/generic.c |    2 +
> > > >  arch/arm/mach-versatile/core.c |    2 +
> > > >  include/linux/cnt32_to_63.h    |   80 ++++++++++++++++++++++++++++++++++++++++
> > > >  4 files changed, 83 insertions(+), 3 deletions(-)
> > > >  create mode 100644 include/linux/cnt32_to_63.h
> > > 
> > > Didn't you forget to remove the old one?
> > > 
> > > 
> > > > +#define cnt32_to_63(cnt_lo) \
> > > > +({ \
> > > > +	static volatile u32 __m_cnt_hi; \
> > > > +	union cnt32_to_63 __x; \
> > > > +	__x.hi = __m_cnt_hi; \
> > > > +	__x.lo = (cnt_lo); \
> > > > +	if (unlikely((s32)(__x.hi ^ __x.lo) < 0)) \
> > > > +		__m_cnt_hi = __x.hi = (__x.hi ^ 0x80000000) + (__x.hi >> 31); \
> > > > +	__x.val; \
> > > > +})
> > > > +
> > > > +#endif
> > > 
> > > That code is way to smart :-)
> > > 
> > > Better make sure that non of its users are SMP capable though.
> > 
> > Why would that matter?
> 
> #define cnt32_to_63(cnt_lo)
> ({
> 	static DEFINE_PER_CPU(u32, __m_cnt_hi);
> 
>        union cnt32_to_63 __x;
> 	u32 *__m_cnt_hi_ptr = &get_cpu_var(__m_cnt_hi);

Nope.  That disables preemption which is against the purpose of this 
code.  Disabling preemption is unneeded.

> 	__x.hi = *__m_cnt_hi_ptr;
> 	__x.lo = (cnt_lo);
> 
> 	if (unlikely((s32)(__x.hi ^ __x.lo) < 0))
> 		*__m_cnt_hi_ptr = __x.hi = (__x.hi ^ 0x80000000) + (__x.hi >> 31);
> 
> 	put_cpu_var(__m_cnt_hi);
> 
> 	__x.val;
> })
> 
> And I'm not quite sure why you had volatile in there.. but that should
> be replaced by a barrier().

That's to ensure that __x.hi is always (re)read before __x.lo, and not 
factorized out of a possible outer loop if gcc felt like doing such an 
optimization.  However I didn't want other unrelated variables to be 
affected by an undiscriminating barrier().  But that's a weak argument 
as I suspect in most case inserting a barrier() between the 2 reads will 
generate the same code.

> Also, we could probably use __get_cpu_var() and put BUG_ON(!in_atomic())
> in there since if this stuff is per cpu, it'd better be atomic anyway -
> you don't want to read a different cpu's counter than the one you're
> using to upgrade.

But again, the original code had no such restriction.  It works with 
mixed contexts.

If the base counter is per CPU, then the static variable has to be per 
CPU of course.  But if the base counter is global then the static 
variable should be shared by all CPUs unless you can guarantee that all 
CPUs will always call this at least once per half period of the base 
counter.


Nicolas