Date:	Fri, 7 Nov 2008 22:18:14 +0000
From:	Russell King <rmk+lkml@....linux.org.uk>
To:	Mathieu Desnoyers <mathieu.desnoyers@...ymtl.ca>
Cc:	David Howells <dhowells@...hat.com>,
	Andrew Morton <akpm@...ux-foundation.org>,
	Nicolas Pitre <nico@....org>,
	Linus Torvalds <torvalds@...ux-foundation.org>,
	Ingo Molnar <mingo@...e.hu>,
	Peter Zijlstra <a.p.zijlstra@...llo.nl>,
	linux-kernel@...r.kernel.org, Ralf Baechle <ralf@...ux-mips.org>,
	benh@...nel.crashing.org, paulus@...ba.org,
	David Miller <davem@...emloft.net>,
	Ingo Molnar <mingo@...hat.com>,
	Thomas Gleixner <tglx@...utronix.de>,
	Steven Rostedt <rostedt@...dmis.org>,
	linux-arch@...r.kernel.org
Subject: Re: [RFC patch 08/18] cnt32_to_63 should use smp_rmb()

On Fri, Nov 07, 2008 at 04:36:10PM -0500, Mathieu Desnoyers wrote:
> * Russell King (rmk+lkml@....linux.org.uk) 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.

> 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@...hat.com>
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@...hat.com>
    Signed-off-by: Linus Torvalds <torvalds@...ux-foundation.org>

$ git log -- arch/arm/include/asm/cnt32_to_63.h include/asm-arm/cnt32_to_63.h
commit bc173c5789e1fc6065fd378edc815914b40ee86b
Author: David Howells <dhowells@...hat.com>
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@...hat.com>
    Signed-off-by: Linus Torvalds <torvalds@...ux-foundation.org>

commit 4baa9922430662431231ac637adedddbb0cfb2d7
Author: Russell King <rmk@...-67.arm.linux.org.uk>
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@....linux.org.uk>

commit 838ccbc35eae5b44d47724e5f694dbec4a26d269
Author: Nicolas Pitre <nico@....org>
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@....org>
    Signed-off-by: Russell King <rmk+kernel@....linux.org.uk>

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:
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists