Re: [RFC patch 08/18] cnt32_to_63 should use smp_rmb()

[Mathieu Desnoyers]

* Russell King (rmk+lkml@xxxxxxxxxxxxxxxx) wrote:
> On Fri, Nov 07, 2008 at 04:36:10PM -0500, Mathieu Desnoyers wrote:
> > * Russell King (rmk+lkml@xxxxxxxxxxxxxxxx) wrote:
> > > On Fri, Nov 07, 2008 at 11:47:58AM -0500, Mathieu Desnoyers wrote:
> > > > But any get_cycles() user of cnt32_to_63() should be shot down. The
> > > > bright side is  : there is no way get_cycles() can be used with this
> > > > new code. :)
> > > > 
> > > > e.g. of incorrect users for arm (unless they are UP only, but that seems
> > > > like a weird design argument) :
> > > > 
> > > > mach-sa1100/include/mach/SA-1100.h:#define OSCR     __REG(0x90000010)
> > > > /* OS timer Counter Reg. */
> > > > mach-sa1100/generic.c:  unsigned long long v = cnt32_to_63(OSCR);
> > > > mach-pxa/include/mach/pxa-regs.h:#define OSCR   __REG(0x40A00010)  /* OS
> > > > Timer Counter Register */
> > > > mach-pxa/time.c:  unsigned long long v = cnt32_to_63(OSCR);
> > > 
> > > It's strange for you to make that assertion when PXA was the exact
> > > platform that Nicolas created this code for - and that's a platform
> > > where preempt has been widely used.
> > > 
> > > The two you mention are both ARMv5 or older architectures, and the
> > > first real SMP ARM architecture is ARMv6.  So architecturally they
> > > are UP only.
> > 
> > Ok. And hopefully they do not execute instructions speculatively ?
> 
> Again, that's ARMv6 and later.
> 
> > Because then a instruction sync would be required between the __m_hi_cnt
> > read and get_cycles.
> 
> What get_cycles?  This is the ARM implementation of get_cycles():
> 
> static inline cycles_t get_cycles (void)
> {
>         return 0;
> }
> 
> Maybe you're using a name for one thing which means something else to
> other people?  Please don't use confusing vocabulary.
> 

get_cycles() is expected to be a cpu register read which reads a cycle
counter. As far as I can tell,

#define OSCR     __REG(0x90000010)    /* OS timer Counter Reg. */

seems to fit this definition pretty closely. Or maybe not ?

I am personnally not trying to find someone to blame. Just trying to
figure out how to fix the existing code or, at the very least, to make
sure nobody will ask me to use a piece of code not suitable for
tracing clock source purposes.

Mathieu

> > If you design such stuff with portability in mind, you'd use per-cpu
> > variables, which ends up being a single variable in the single-cpu
> > special-case.
> 
> Explain how and why sched_clock(), which is a global time source, should
> use per-cpu variables.
> 
> > > So, tell me why you say "unless they are UP only, but that seems like
> > > a weird design argument"?  If the platforms can only ever be UP only,
> > > what's wrong with UP only code being used with them?  (Not that I'm
> > > saying anything there about cnt32_to_63.)
> > 
> > That's fine, as long as the code does not end up in include/linux and
> > stays in arch/arm/up-only-subarch/.
> 
> Well, that's where it was - private to ARM.  Then David Howells came
> along and unilaterally - and without reference to anyone as far as I
> can see - moved it to include/linux.
> 
> Neither Nicolas, nor me had any idea that it was going to move into
> include/linux - the first we knew of it was when pulling the change
> from Linus' tree.
> 
> Look, if people in the kernel community can't or won't communicate
> with others (either through malice, purpose or accident), you can
> expect this kind of crap to happen.
> 
> > When one try to create architecture agnostic code (which is what is
> > likely to be palatable to arch agnostic headers), designing with UP in
> > mind does not make much sense.
> 
> It wasn't architecture agnostic code.  It was ARM specific.  We have
> a "version control system" which stores "comments" for changes to the
> kernel tree.  Please use it to find out the true story.  I'll save
> you the trouble, here's the commits with full comments:
> 
> $ git log include/linux/cnt32_to_63.h
> commit b4f151ff899362fec952c45d166252c9912c041f
> Author: David Howells <dhowells@xxxxxxxxxx>
> Date:   Wed Sep 24 17:48:26 2008 +0100
> 
>     MN10300: Move asm-arm/cnt32_to_63.h to include/linux/
> 
>     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@xxxxxxxxxx>
>     Signed-off-by: Linus Torvalds <torvalds@xxxxxxxxxxxxxxxxxxxx>
> 
> $ git log -- arch/arm/include/asm/cnt32_to_63.h include/asm-arm/cnt32_to_63.h
> commit bc173c5789e1fc6065fd378edc815914b40ee86b
> Author: David Howells <dhowells@xxxxxxxxxx>
> Date:   Fri Sep 26 16:22:58 2008 +0100
> 
>     ARM: Delete ARM's own cnt32_to_63.h
> 
>     Delete ARM's own cnt32_to_63.h as the copy in include/linux/ should now be
>     used instead.
> 
>     Signed-off-by: David Howells <dhowells@xxxxxxxxxx>
>     Signed-off-by: Linus Torvalds <torvalds@xxxxxxxxxxxxxxxxxxxx>
> 
> commit 4baa9922430662431231ac637adedddbb0cfb2d7
> Author: Russell King <rmk@xxxxxxxxxxxxxxxxxxxxxxx>
> Date:   Sat Aug 2 10:55:55 2008 +0100
> 
>     [ARM] move include/asm-arm to arch/arm/include/asm
> 
>     Move platform independent header files to arch/arm/include/asm, leaving
>     those in asm/arch* and asm/plat* alone.
> 
>     Signed-off-by: Russell King <rmk+kernel@xxxxxxxxxxxxxxxx>
> 
> commit 838ccbc35eae5b44d47724e5f694dbec4a26d269
> Author: Nicolas Pitre <nico@xxxxxxx>
> Date:   Mon Dec 4 20:19:31 2006 +0100
> 
>     [ARM] 3978/1: macro to provide a 63-bit value from a 32-bit hardware counter
> 
>     This is done in a completely lockless fashion. Bits 0 to 31 of the count
>     are provided by the hardware while bits 32 to 62 are stored in memory.
>     The top bit in memory is used to synchronize with the hardware count
>     half-period.  When the top bit of both counters (hardware and in memory)
>     differ then the memory is updated with a new value, incrementing it when
>     the hardware counter wraps around.  Because a word store in memory is
>     atomic then the incremented value will always be in synch with the top
>     bit indicating to any potential concurrent reader if the value in memory
>     is up to date or not wrt the needed increment.  And any race in updating
>     the value in memory is harmless as the same value would be stored more
>     than once.
> 
>     Signed-off-by: Nicolas Pitre <nico@xxxxxxx>
>     Signed-off-by: Russell King <rmk+kernel@xxxxxxxxxxxxxxxx>
> 
> So, stop slinging mud onto Nicolas and me over this.  The resulting
> mess is clearly not our creation.
> 
> -- 
> Russell King
>  Linux kernel    2.6 ARM Linux   - http://www.arm.linux.org.uk/
>  maintainer of:

-- 
Mathieu Desnoyers
OpenPGP key fingerprint: 8CD5 52C3 8E3C 4140 715F  BA06 3F25 A8FE 3BAE 9A68
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/