[RFC patch 00/18] Trace Clock v2 - Kernel

This is a discussion on [RFC patch 00/18] Trace Clock v2 - Kernel ; Andrew Morton wrote: > I'd expect it to behave in the same way as it would if the function was > implemented out-of-line. > > But it occurs to me that the modrobe-doesnt-work thing would happen if > the function ...

+ Reply to Thread
Page 3 of 6 FirstFirst 1 2 3 4 5 ... LastLast
Results 41 to 60 of 112

Thread: [RFC patch 00/18] Trace Clock v2

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


    Andrew Morton wrote:

    > I'd expect it to behave in the same way as it would if the function was
    > implemented out-of-line.
    >
    > But it occurs to me that the modrobe-doesnt-work thing would happen if
    > the function _is_ inlined anyway, so we won't be doing that.
    >
    > Whatever. Killing this many puppies because gcc may do something so
    > bizarrely wrong isn't justifiable.


    With gcc, you get one instance of the static variable from inside a static
    (inline or outofline) function per .o file that invokes it, and these do not
    merge even though they're common symbols. I asked around and the opinion
    seems to be that this is correct C. I suppose it's the equivalent of cutting
    and pasting a function between several files - why should the compiler assume
    it's the same function in each?

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

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

    * David Howells (dhowells@redhat.com) wrote:
    > Mathieu Desnoyers wrote:
    >
    > > Assume the time source is a global clock which insures that time will never
    > > *ever* go backward. Use a smp_rmb() to make sure the cnt_lo value is read before
    > > __m_cnt_hi.

    >
    > If you have an smp_rmb(), then don't you need an smp_wmb()/smp_mb() to match
    > it to make it work? And is your assumption valid that smp_rmb() will affect
    > memory vs the I/O access to read the clock? There's no requirement that
    > cnt_lo will have been read from an MMIO location at all.
    >
    > David


    I want to make sure

    __m_cnt_hi
    is read before
    mmio cnt_lo read

    for the detailed reasons explained in my previous discussion with
    Nicolas here :
    http://lkml.org/lkml/2008/10/21/1

    I use smp_rmb() to do this on SMP systems (hrm, actually, a rmb() could
    be required so it works also on UP systems safely wrt interrupts).

    The write side is between the hardware counter, which is assumed to
    increment monotonically between each read, and the value __m_cnt_hi
    updated by the CPU. I don't see where we could put a wmb() there.

    Without barrier, the smp race looks as follow :


    CPU A B
    read hw cnt low (0xFFFFFFFA)
    read __m_cnt_hi (0x80000000)
    read hw cnt low (0x00000001)
    (wrap detected :
    (s32)(0x80000000 ^ 0x1) < 0)
    write __m_cnt_hi = 0x00000001
    read __m_cnt_hi (0x00000001)
    return 0x0000000100000001
    (wrap detected :
    (s32)(0x00000001 ^ 0xFFFFFFFA) < 0)
    write __m_cnt_hi = 0x80000001
    return 0x80000001FFFFFFFA
    (time jumps)

    And UP interrupt race :

    Thread context Interrupts
    read hw cnt low (0xFFFFFFFA)
    read __m_cnt_hi (0x80000000)
    read hw cnt low (0x00000001)
    (wrap detected :
    (s32)(0x80000000 ^ 0x1) < 0)
    write __m_cnt_hi = 0x00000001
    read __m_cnt_hi (0x00000001)
    return 0x0000000100000001
    (wrap detected :
    (s32)(0x00000001 ^ 0xFFFFFFFA) < 0)
    write __m_cnt_hi = 0x80000001
    return 0x80000001FFFFFFFA
    (time jumps)

    New code to fix it here with full rmb() :

    static inline u64 cnt32_to_63(u32 io_addr, u32 *__m_cnt_hi)
    {
    union cnt32_to_63 __x;
    __x.hi = *__m_cnt_hi; /* memory read for high bits internal state */
    rmb(); /*
    * read high bits before low bits insures time
    * does not go backward. Sync across
    * CPUs and for interrupts.
    */
    __x.lo = readl(cnt_lo); /* mmio read */
    if (unlikely((s32)(__x.hi ^ __x.lo) < 0))
    *__m_cnt_hi =
    __x.hi = (__x.hi ^ 0x80000000) + (__x.hi >> 31);
    return __x.val;
    }

    Mathieu

    --
    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@vger.kernel.org
    More majordomo info at http://vger.kernel.org/majordomo-info.html
    Please read the FAQ at http://www.tux.org/lkml/

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

    On Fri, 07 Nov 2008 11:47:47 -0500 (EST) Nicolas Pitre wrote:

    > > btw, do you know how damned irritating and frustrating it is for a code
    > > reviewer to have his comments deliberately ignored and deleted in
    > > replies?

    >
    > Do you know how irritating and frustrating it is when reviewers don't
    > care reading the damn comments along with the code?


    As you still seek to ignore it, I shall repeat my earlier question.
    Please do not delete it again.

    It apparently tries to avoid races via ordering tricks, as long
    as it is called with sufficient frequency. But nothing guarantees
    that it _is_ called sufficiently frequently?

    Things like tickless kernels and SCHED_RR can surely cause
    sched_clock() to not be called for arbitrary periods.

    Userspace cli() will definitely do this, but it is expected to break
    stuff and is not as legitiate a thing to do.


    I'm just giving up on the tastefulness issue.
    --
    To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
    the body of a message to majordomo@vger.kernel.org
    More majordomo info at http://vger.kernel.org/majordomo-info.html
    Please read the FAQ at http://www.tux.org/lkml/

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

    On Fri, 07 Nov 2008 17:10:00 +0000 David Howells wrote:

    >
    > Andrew Morton wrote:
    >
    > > I'd expect it to behave in the same way as it would if the function was
    > > implemented out-of-line.
    > >
    > > But it occurs to me that the modrobe-doesnt-work thing would happen if
    > > the function _is_ inlined anyway, so we won't be doing that.
    > >
    > > Whatever. Killing this many puppies because gcc may do something so
    > > bizarrely wrong isn't justifiable.

    >
    > With gcc, you get one instance of the static variable from inside a static
    > (inline or outofline) function per .o file that invokes it, and these do not
    > merge even though they're common symbols. I asked around and the opinion
    > seems to be that this is correct C. I suppose it's the equivalent of cutting
    > and pasting a function between several files - why should the compiler assume
    > it's the same function in each?
    >


    OK, thanks, I guess that makes sense. For static inline. I wonder if
    `extern inline' or plain old `inline' should change it.

    It's one of those things I hope I never need to know about, but perhaps
    we do somewhere have static storage in an inline. Wouldn't surprise
    me, and I bet that if we do, it's a bug.
    --
    To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
    the body of a message to majordomo@vger.kernel.org
    More majordomo info at http://vger.kernel.org/majordomo-info.html
    Please read the FAQ at http://www.tux.org/lkml/

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


    On Fri, 7 Nov 2008, Mathieu Desnoyers wrote:
    >
    > __m_cnt_hi
    > is read before
    > mmio cnt_lo read
    >
    > for the detailed reasons explained in my previous discussion with
    > Nicolas here :
    > http://lkml.org/lkml/2008/10/21/1
    >
    > I use smp_rmb() to do this on SMP systems (hrm, actually, a rmb() could
    > be required so it works also on UP systems safely wrt interrupts).


    smp_rmb turns into a compiler barrier on UP and should prevent the below
    description.

    -- Steve

    >
    > The write side is between the hardware counter, which is assumed to
    > increment monotonically between each read, and the value __m_cnt_hi
    > updated by the CPU. I don't see where we could put a wmb() there.
    >
    > Without barrier, the smp race looks as follow :
    >
    >
    > CPU A B
    > read hw cnt low (0xFFFFFFFA)
    > read __m_cnt_hi (0x80000000)
    > read hw cnt low (0x00000001)
    > (wrap detected :
    > (s32)(0x80000000 ^ 0x1) < 0)
    > write __m_cnt_hi = 0x00000001
    > read __m_cnt_hi (0x00000001)
    > return 0x0000000100000001
    > (wrap detected :
    > (s32)(0x00000001 ^ 0xFFFFFFFA) < 0)
    > write __m_cnt_hi = 0x80000001
    > return 0x80000001FFFFFFFA
    > (time jumps)
    >
    > And UP interrupt race :
    >
    > Thread context Interrupts
    > read hw cnt low (0xFFFFFFFA)
    > read __m_cnt_hi (0x80000000)
    > read hw cnt low (0x00000001)
    > (wrap detected :
    > (s32)(0x80000000 ^ 0x1) < 0)
    > write __m_cnt_hi = 0x00000001
    > read __m_cnt_hi (0x00000001)
    > return 0x0000000100000001
    > (wrap detected :
    > (s32)(0x00000001 ^ 0xFFFFFFFA) < 0)
    > write __m_cnt_hi = 0x80000001
    > return 0x80000001FFFFFFFA
    > (time jumps)
    >

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

  6. Re: [RFC patch 17/18] MIPS : Trace clock

    * Peter Zijlstra (a.p.zijlstra@chello.nl) wrote:
    > On Fri, 2008-11-07 at 00:23 -0500, Mathieu Desnoyers wrote:
    > > Note for Peter Zijlstra :
    > > You should probably have a look at lockdep.c raw_spinlock_t lockdep_lock usage.
    > > I suspect it may be used with preemption enabled in graph_lock(). (not sure
    > > though, but it's worth double-checking.

    >
    > Are you worried about the graph_lock() instance in
    > lookup_chain_cache() ?
    >
    > That is the locking for validate_chain,
    >
    > __lock_acquire()
    > validate_chain()
    > look_up_chain_cache()
    > graph_lock()
    > check_prevs_add()
    > check_prev_add()
    > graph_unlock()
    > graph_lock()
    > graph_unlock()
    >
    > which is all done without modifying IRQ state.
    >
    > However, __lock_acquire() is only called with IRQs disabled:
    >
    > lock_acquire()
    > raw_local_irq_save()
    > __lock_acquire()
    >
    > lock_release()
    > raw_local_irq_save()
    > __lock_release()
    > lock_release_nested()
    > __lock_acquire()
    > lock_release_non_nested()
    > __lock_acquire()
    >
    > lock_set_subclass()
    > raw_local_irq_save()
    > __lock_set_subclass()
    > __lock_acquire()
    >
    > So I think we're good.
    >


    Yep, looks good.

    Mathieu


    --
    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@vger.kernel.org
    More majordomo info at http://vger.kernel.org/majordomo-info.html
    Please read the FAQ at http://www.tux.org/lkml/

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

    * Andrew Morton (akpm@linux-foundation.org) wrote:
    > On Fri, 07 Nov 2008 17:10:00 +0000 David Howells wrote:
    >
    > >
    > > Andrew Morton wrote:
    > >
    > > > I'd expect it to behave in the same way as it would if the function was
    > > > implemented out-of-line.
    > > >
    > > > But it occurs to me that the modrobe-doesnt-work thing would happen if
    > > > the function _is_ inlined anyway, so we won't be doing that.
    > > >
    > > > Whatever. Killing this many puppies because gcc may do something so
    > > > bizarrely wrong isn't justifiable.

    > >
    > > With gcc, you get one instance of the static variable from inside a static
    > > (inline or outofline) function per .o file that invokes it, and these do not
    > > merge even though they're common symbols. I asked around and the opinion
    > > seems to be that this is correct C. I suppose it's the equivalent of cutting
    > > and pasting a function between several files - why should the compiler assume
    > > it's the same function in each?
    > >

    >
    > OK, thanks, I guess that makes sense. For static inline. I wonder if
    > `extern inline' or plain old `inline' should change it.
    >
    > It's one of those things I hope I never need to know about, but perhaps
    > we do somewhere have static storage in an inline. Wouldn't surprise
    > me, and I bet that if we do, it's a bug.


    Tracepoints actually use that. It could be changed so they use :

    DECLARE_TRACE() (in include/trace/group.h)
    DEFINE_TRACE() (in the appropriate kernel c file)
    trace_somename(); (in the code)

    instead. That would actually make more sense and remove the need for
    multiple declarations when the same tracepoint name is used in many
    spots (this is a problem kmemtrace has, it generates a lot of tracepoint
    declarations).

    Mathieu

    --
    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@vger.kernel.org
    More majordomo info at http://vger.kernel.org/majordomo-info.html
    Please read the FAQ at http://www.tux.org/lkml/

  8. Re: [RFC patch 07/18] Trace clock core

    * Andrew Morton (akpm@linux-foundation.org) wrote:
    > On Fri, 7 Nov 2008 11:12:38 -0500 Mathieu Desnoyers wrote:
    >
    > > * Andrew Morton (akpm@linux-foundation.org) wrote:
    > > > On Fri, 7 Nov 2008 01:16:43 -0500 Mathieu Desnoyers wrote:
    > > >
    > > > > > Is there something we should be fixing in m68k?
    > > > > >
    > > > >
    > > > > Yes, but I fear it's going to go deep into include hell :-(
    > > >
    > > > Oh, OK. I thought that the comment meant that m68k's on_each_cpu()
    > > > behaves differently at runtime from other architectures (and wrongly).
    > > >
    > > > If it's just some compile-time #include snafu then that's far less
    > > > of a concern.
    > > >

    > >
    > > Should I simply remove this comment then ?
    > >

    >
    > umm, it could perhaps be clarified - mention that it's needed for an
    > include order problem.
    >
    > It's a bit odd. Surely by the time we've included these:
    >
    > +#include
    > +#include
    > +#include
    > +#include
    > +#include
    > +#include
    > +#include
    > +#include
    > +#include
    > +#include
    >
    > someone has already included sched.h, and the definition of
    > _LINUX_SCHED_H will cause the later inclusion to not change anything?
    >


    Maybe now it's ok, but in the past, sched.h was not included..
    surprisingly.

    I'll just write a clearer comment.

    Thanks,

    Mathieu

    --
    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@vger.kernel.org
    More majordomo info at http://vger.kernel.org/majordomo-info.html
    Please read the FAQ at http://www.tux.org/lkml/

  9. Re: [RFC patch 04/18] get_cycles() : powerpc64 HAVE_GET_CYCLES

    * Josh Boyer (jwboyer@linux.vnet.ibm.com) wrote:
    > On Fri, Nov 07, 2008 at 12:23:40AM -0500, Mathieu Desnoyers wrote:
    > >This patch selects HAVE_GET_CYCLES and makes sure get_cycles_barrier() and
    > >get_cycles_rate() are implemented.
    > >
    > >Signed-off-by: Mathieu Desnoyers
    > >CC: benh@kernel.crashing.org
    > >CC: paulus@samba.org
    > >CC: David Miller
    > >CC: Linus Torvalds
    > >CC: Andrew Morton
    > >CC: Ingo Molnar
    > >CC: Peter Zijlstra
    > >CC: Thomas Gleixner
    > >CC: Steven Rostedt
    > >CC: linux-arch@vger.kernel.org
    > >---
    > > arch/powerpc/Kconfig | 1 +
    > > arch/powerpc/include/asm/timex.h | 11 +++++++++++
    > > 2 files changed, 12 insertions(+)
    > >
    > >Index: linux.trees.git/arch/powerpc/Kconfig
    > >================================================== =================
    > >--- linux.trees.git.orig/arch/powerpc/Kconfig 2008-11-07 00:09:44.000000000 -0500
    > >+++ linux.trees.git/arch/powerpc/Kconfig 2008-11-07 00:09:46.000000000 -0500
    > >@@ -121,6 +121,7 @@ config PPC
    > > select HAVE_DMA_ATTRS if PPC64
    > > select USE_GENERIC_SMP_HELPERS if SMP
    > > select HAVE_OPROFILE
    > >+ select HAVE_GET_CYCLES if PPC64

    >
    > So maybe it's just me because it's Friday and I'm on vacation, but I don't
    > see anything overly specific to ppc64 here. In fact, you use get_cycles_rate
    > for all of powerpc in a later patch in the series.
    >
    > Is there something special about HAVE_GET_CYCLES that I'm missing that would
    > make it only apply to ppc64 and not ppc32?
    >
    > josh


    Hi Josh,

    powerpc32 only uses the 32 LSBs for the TSC in the current get_cycles()
    implementation. We could either define HAVE_GET_CYCLES_32 like I did on
    mips32, or change get_cycles so it also reads the 32 MSBs in a loop like
    this (it does not take care of the CPU_FTR_CELL_TB_BUG though) :

    typedef unsigned long long cycles_t;

    cycles_t get_cycles_ppc32(void)
    {
    union {
    cycles_t v;
    struct {
    u32 ms32, ls32; /* powerpc is big endian */
    } s;
    } cycles;

    do {
    cycles.ls32 = mftbu();
    cycles.ms32 = mftbl();
    } while (cycles.ls32 != mftbu());
    return cycles.v;
    }

    I'd prefer this second solution. If one needs a specific get_cycles() to
    be only 32-bits (but really really fast) for the scheduler, then this
    could be a get_cycles_sched() or something like this which does not
    guarantee that it returns full 64-bits...

    Mathieu

    --
    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@vger.kernel.org
    More majordomo info at http://vger.kernel.org/majordomo-info.html
    Please read the FAQ at http://www.tux.org/lkml/

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

    On Fri, 7 Nov 2008 13:00:41 -0500
    Mathieu Desnoyers wrote:

    > * Andrew Morton (akpm@linux-foundation.org) wrote:
    > > On Fri, 07 Nov 2008 17:10:00 +0000 David Howells wrote:
    > >
    > > >
    > > > Andrew Morton wrote:
    > > >
    > > > > I'd expect it to behave in the same way as it would if the function was
    > > > > implemented out-of-line.
    > > > >
    > > > > But it occurs to me that the modrobe-doesnt-work thing would happen if
    > > > > the function _is_ inlined anyway, so we won't be doing that.
    > > > >
    > > > > Whatever. Killing this many puppies because gcc may do something so
    > > > > bizarrely wrong isn't justifiable.
    > > >
    > > > With gcc, you get one instance of the static variable from inside a static
    > > > (inline or outofline) function per .o file that invokes it, and these do not
    > > > merge even though they're common symbols. I asked around and the opinion
    > > > seems to be that this is correct C. I suppose it's the equivalent of cutting
    > > > and pasting a function between several files - why should the compiler assume
    > > > it's the same function in each?
    > > >

    > >
    > > OK, thanks, I guess that makes sense. For static inline. I wonder if
    > > `extern inline' or plain old `inline' should change it.
    > >
    > > It's one of those things I hope I never need to know about, but perhaps
    > > we do somewhere have static storage in an inline. Wouldn't surprise
    > > me, and I bet that if we do, it's a bug.

    >
    > Tracepoints actually use that.


    Referring to include/linux/tracepoint.hEFINE_TRACE()?

    It does look a bit fragile. Does every .c file which included
    include/trace/block.h get a copy of __tracepoint_block_rq_issue,
    whether or not it used that tracepoint? Hopefully not.

    > It could be changed so they use :
    >
    > DECLARE_TRACE() (in include/trace/group.h)
    > DEFINE_TRACE() (in the appropriate kernel c file)
    > trace_somename(); (in the code)
    >
    > instead. That would actually make more sense and remove the need for
    > multiple declarations when the same tracepoint name is used in many
    > spots (this is a problem kmemtrace has, it generates a lot of tracepoint
    > declarations).


    I'm unsure of the requirements here. Do you _want_ each call to
    trace_block_rq_issue() to share some in-memory state? If so then yes,
    there's a problem with calls to trace_block_rq_issue() from within
    separate compilation units.

    otoh, if all calls to trace_block_rq_issue() are supposed to have
    independent state (which seems to be the case) then that could be
    addressed by making trace_block_rq_issue() a macro which defines
    static storage, as cnt32_to_63() shouldn't have done


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

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



    On Fri, 7 Nov 2008, Andrew Morton wrote:
    >
    > Referring to include/linux/tracepoint.hEFINE_TRACE()?
    >
    > It does look a bit fragile. Does every .c file which included
    > include/trace/block.h get a copy of __tracepoint_block_rq_issue,
    > whether or not it used that tracepoint? Hopefully not.


    Look at "ratelimit()" too. Broken, broken. Of course, I don't think it's
    actually _used_ anywhere, so..

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

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

    On Fri, 2008-11-07 at 10:21 -0800, Andrew Morton wrote:
    > On Fri, 7 Nov 2008 13:00:41 -0500
    > Mathieu Desnoyers wrote:
    > > > It's one of those things I hope I never need to know about, but perhaps
    > > > we do somewhere have static storage in an inline. Wouldn't surprise
    > > > me, and I bet that if we do, it's a bug.

    > >
    > > Tracepoints actually use that.

    >
    > Referring to include/linux/tracepoint.hEFINE_TRACE()?
    >
    > It does look a bit fragile. Does every .c file which included
    > include/trace/block.h get a copy of __tracepoint_block_rq_issue,
    > whether or not it used that tracepoint? Hopefully not.
    >
    > > It could be changed so they use :
    > >
    > > DECLARE_TRACE() (in include/trace/group.h)
    > > DEFINE_TRACE() (in the appropriate kernel c file)
    > > trace_somename(); (in the code)
    > >
    > > instead. That would actually make more sense and remove the need for
    > > multiple declarations when the same tracepoint name is used in many
    > > spots (this is a problem kmemtrace has, it generates a lot of tracepoint
    > > declarations).


    Could this scheme also help with the thousands of sparse warnings that
    kmemtrace produces because of the current arrangement, all of the form:

    include/linux/kmemtrace.h:33:2: warning: Initializer entry defined twice
    include/linux/kmemtrace.h:33:2: also defined here

    As you could have unique names for the tracepoints now, rather than the
    'unique' static storage? Or am I off-base here?

    Harvey



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

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

    * Andrew Morton (akpm@linux-foundation.org) wrote:
    > On Fri, 7 Nov 2008 13:00:41 -0500
    > Mathieu Desnoyers wrote:
    >
    > > * Andrew Morton (akpm@linux-foundation.org) wrote:
    > > > On Fri, 07 Nov 2008 17:10:00 +0000 David Howells wrote:
    > > >
    > > > >
    > > > > Andrew Morton wrote:
    > > > >
    > > > > > I'd expect it to behave in the same way as it would if the function was
    > > > > > implemented out-of-line.
    > > > > >
    > > > > > But it occurs to me that the modrobe-doesnt-work thing would happen if
    > > > > > the function _is_ inlined anyway, so we won't be doing that.
    > > > > >
    > > > > > Whatever. Killing this many puppies because gcc may do something so
    > > > > > bizarrely wrong isn't justifiable.
    > > > >
    > > > > With gcc, you get one instance of the static variable from inside a static
    > > > > (inline or outofline) function per .o file that invokes it, and these do not
    > > > > merge even though they're common symbols. I asked around and the opinion
    > > > > seems to be that this is correct C. I suppose it's the equivalent of cutting
    > > > > and pasting a function between several files - why should the compiler assume
    > > > > it's the same function in each?
    > > > >
    > > >
    > > > OK, thanks, I guess that makes sense. For static inline. I wonder if
    > > > `extern inline' or plain old `inline' should change it.
    > > >
    > > > It's one of those things I hope I never need to know about, but perhaps
    > > > we do somewhere have static storage in an inline. Wouldn't surprise
    > > > me, and I bet that if we do, it's a bug.

    > >
    > > Tracepoints actually use that.

    >
    > Referring to include/linux/tracepoint.hEFINE_TRACE()?
    >
    > It does look a bit fragile. Does every .c file which included
    > include/trace/block.h get a copy of __tracepoint_block_rq_issue,
    > whether or not it used that tracepoint? Hopefully not.
    >


    No, __tracepoint_block_rq_issue is only instanciated if the static
    inline function is used. One instance per use.

    > > It could be changed so they use :
    > >
    > > DECLARE_TRACE() (in include/trace/group.h)
    > > DEFINE_TRACE() (in the appropriate kernel c file)
    > > trace_somename(); (in the code)
    > >
    > > instead. That would actually make more sense and remove the need for
    > > multiple declarations when the same tracepoint name is used in many
    > > spots (this is a problem kmemtrace has, it generates a lot of tracepoint
    > > declarations).

    >
    > I'm unsure of the requirements here. Do you _want_ each call to
    > trace_block_rq_issue() to share some in-memory state? If so then yes,
    > there's a problem with calls to trace_block_rq_issue() from within
    > separate compilation units.
    >
    > otoh, if all calls to trace_block_rq_issue() are supposed to have
    > independent state (which seems to be the case) then that could be
    > addressed by making trace_block_rq_issue() a macro which defines
    > static storage, as cnt32_to_63() shouldn't have done
    >


    They could share the same data, given it *has* to be the same. I'll
    try to fix this.

    Mathieu

    >


    --
    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@vger.kernel.org
    More majordomo info at http://vger.kernel.org/majordomo-info.html
    Please read the FAQ at http://www.tux.org/lkml/

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

    On Fri, 7 Nov 2008 10:36:27 -0800 (PST)
    Linus Torvalds wrote:

    >
    >
    > On Fri, 7 Nov 2008, Andrew Morton wrote:
    > >
    > > Referring to include/linux/tracepoint.hEFINE_TRACE()?
    > >
    > > It does look a bit fragile. Does every .c file which included
    > > include/trace/block.h get a copy of __tracepoint_block_rq_issue,
    > > whether or not it used that tracepoint? Hopefully not.

    >
    > Look at "ratelimit()" too. Broken, broken.


    Yup. Easy enough to fix, but...

    > Of course, I don't think it's
    > actually _used_ anywhere, so..
    >


    removing it altogether would be best, I think. It's a bit of an odd
    thing.

    I'll see what this

    --- a/include/linux/ratelimit.h~a
    +++ a/include/linux/ratelimit.h
    @@ -17,11 +17,4 @@ struct ratelimit_state {
    struct ratelimit_state name = {interval, burst,}

    extern int __ratelimit(struct ratelimit_state *rs);
    -
    -static inline int ratelimit(void)
    -{
    - static DEFINE_RATELIMIT_STATE(rs, DEFAULT_RATELIMIT_INTERVAL,
    - DEFAULT_RATELIMIT_BURST);
    - return __ratelimit(&rs);
    -}
    #endif

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

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

    * Harvey Harrison (harvey.harrison@gmail.com) wrote:
    > On Fri, 2008-11-07 at 10:21 -0800, Andrew Morton wrote:
    > > On Fri, 7 Nov 2008 13:00:41 -0500
    > > Mathieu Desnoyers wrote:
    > > > > It's one of those things I hope I never need to know about, but perhaps
    > > > > we do somewhere have static storage in an inline. Wouldn't surprise
    > > > > me, and I bet that if we do, it's a bug.
    > > >
    > > > Tracepoints actually use that.

    > >
    > > Referring to include/linux/tracepoint.hEFINE_TRACE()?
    > >
    > > It does look a bit fragile. Does every .c file which included
    > > include/trace/block.h get a copy of __tracepoint_block_rq_issue,
    > > whether or not it used that tracepoint? Hopefully not.
    > >
    > > > It could be changed so they use :
    > > >
    > > > DECLARE_TRACE() (in include/trace/group.h)
    > > > DEFINE_TRACE() (in the appropriate kernel c file)
    > > > trace_somename(); (in the code)
    > > >
    > > > instead. That would actually make more sense and remove the need for
    > > > multiple declarations when the same tracepoint name is used in many
    > > > spots (this is a problem kmemtrace has, it generates a lot of tracepoint
    > > > declarations).

    >
    > Could this scheme also help with the thousands of sparse warnings that
    > kmemtrace produces because of the current arrangement, all of the form:
    >
    > include/linux/kmemtrace.h:33:2: warning: Initializer entry defined twice
    > include/linux/kmemtrace.h:33:2: also defined here
    >
    > As you could have unique names for the tracepoints now, rather than the
    > 'unique' static storage? Or am I off-base here?
    >


    Exactly.

    Mathieu

    > Harvey
    >
    >
    >


    --
    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@vger.kernel.org
    More majordomo info at http://vger.kernel.org/majordomo-info.html
    Please read the FAQ at http://www.tux.org/lkml/

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

    * Steven Rostedt (rostedt@goodmis.org) wrote:
    >
    > On Fri, 7 Nov 2008, Mathieu Desnoyers wrote:
    > >
    > > __m_cnt_hi
    > > is read before
    > > mmio cnt_lo read
    > >
    > > for the detailed reasons explained in my previous discussion with
    > > Nicolas here :
    > > http://lkml.org/lkml/2008/10/21/1
    > >
    > > I use smp_rmb() to do this on SMP systems (hrm, actually, a rmb() could
    > > be required so it works also on UP systems safely wrt interrupts).

    >
    > smp_rmb turns into a compiler barrier on UP and should prevent the below
    > description.
    >


    Ah, right, preserving program order on UP should be enough. smp_rmb()
    then.

    Thanks,

    Mathieu

    > -- Steve
    >
    > >
    > > The write side is between the hardware counter, which is assumed to
    > > increment monotonically between each read, and the value __m_cnt_hi
    > > updated by the CPU. I don't see where we could put a wmb() there.
    > >
    > > Without barrier, the smp race looks as follow :
    > >
    > >
    > > CPU A B
    > > read hw cnt low (0xFFFFFFFA)
    > > read __m_cnt_hi (0x80000000)
    > > read hw cnt low (0x00000001)
    > > (wrap detected :
    > > (s32)(0x80000000 ^ 0x1) < 0)
    > > write __m_cnt_hi = 0x00000001
    > > read __m_cnt_hi (0x00000001)
    > > return 0x0000000100000001
    > > (wrap detected :
    > > (s32)(0x00000001 ^ 0xFFFFFFFA) < 0)
    > > write __m_cnt_hi = 0x80000001
    > > return 0x80000001FFFFFFFA
    > > (time jumps)
    > >
    > > And UP interrupt race :
    > >
    > > Thread context Interrupts
    > > read hw cnt low (0xFFFFFFFA)
    > > read __m_cnt_hi (0x80000000)
    > > read hw cnt low (0x00000001)
    > > (wrap detected :
    > > (s32)(0x80000000 ^ 0x1) < 0)
    > > write __m_cnt_hi = 0x00000001
    > > read __m_cnt_hi (0x00000001)
    > > return 0x0000000100000001
    > > (wrap detected :
    > > (s32)(0x00000001 ^ 0xFFFFFFFA) < 0)
    > > write __m_cnt_hi = 0x80000001
    > > return 0x80000001FFFFFFFA
    > > (time jumps)
    > >


    --
    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@vger.kernel.org
    More majordomo info at http://vger.kernel.org/majordomo-info.html
    Please read the FAQ at http://www.tux.org/lkml/

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

    On Fri, 2008-11-07 at 14:18 -0500, Mathieu Desnoyers wrote:
    > * Steven Rostedt (rostedt@goodmis.org) wrote:
    > >
    > > On Fri, 7 Nov 2008, Mathieu Desnoyers wrote:
    > > >
    > > > __m_cnt_hi
    > > > is read before
    > > > mmio cnt_lo read
    > > >
    > > > for the detailed reasons explained in my previous discussion with
    > > > Nicolas here :
    > > > http://lkml.org/lkml/2008/10/21/1
    > > >
    > > > I use smp_rmb() to do this on SMP systems (hrm, actually, a rmb() could
    > > > be required so it works also on UP systems safely wrt interrupts).

    > >
    > > smp_rmb turns into a compiler barrier on UP and should prevent the below
    > > description.
    > >

    >
    > Ah, right, preserving program order on UP should be enough. smp_rmb()
    > then.



    I'm not quite sure I'm following here. Is this a global hardware clock
    you're reading from multiple cpus, if so, are you sure smp_rmb() will
    indeed be enough to sync the read?

    (In which case the smp_wmb() is provided by the hardware increasing the
    clock?)

    If these are per-cpu clocks then even in the smp case we'd be good with
    a plain barrier() because you'd only ever want to read your own cpu's
    clock (and have a separate __m_cnt_hi per cpu).

    Or am I totally missing out on something?

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

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


    On Fri, 7 Nov 2008, Mathieu Desnoyers wrote:
    >
    > I want to make sure
    >
    > __m_cnt_hi
    > is read before
    > mmio cnt_lo read


    Hmm, let me make sure I understand why there is no wmb.

    Paul, can you verify this?

    Mathieu, you do the following:

    read a
    smp_rmb
    reab b
    if (test b)
    write a

    So the idea is that you must read b to test it. And since we must read a
    before reading b we can see that we write a before either?

    The question remains, can the write happen before either of the reads?

    But since the read b is reading the hw clock, perhaps that just implies a
    wmb on the hardware side?

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

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

    * Peter Zijlstra (a.p.zijlstra@chello.nl) wrote:
    > On Fri, 2008-11-07 at 14:18 -0500, Mathieu Desnoyers wrote:
    > > * Steven Rostedt (rostedt@goodmis.org) wrote:
    > > >
    > > > On Fri, 7 Nov 2008, Mathieu Desnoyers wrote:
    > > > >
    > > > > __m_cnt_hi
    > > > > is read before
    > > > > mmio cnt_lo read
    > > > >
    > > > > for the detailed reasons explained in my previous discussion with
    > > > > Nicolas here :
    > > > > http://lkml.org/lkml/2008/10/21/1
    > > > >
    > > > > I use smp_rmb() to do this on SMP systems (hrm, actually, a rmb() could
    > > > > be required so it works also on UP systems safely wrt interrupts).
    > > >
    > > > smp_rmb turns into a compiler barrier on UP and should prevent the below
    > > > description.
    > > >

    > >
    > > Ah, right, preserving program order on UP should be enough. smp_rmb()
    > > then.

    >
    >
    > I'm not quite sure I'm following here. Is this a global hardware clock
    > you're reading from multiple cpus, if so, are you sure smp_rmb() will
    > indeed be enough to sync the read?
    >
    > (In which case the smp_wmb() is provided by the hardware increasing the
    > clock?)
    >
    > If these are per-cpu clocks then even in the smp case we'd be good with
    > a plain barrier() because you'd only ever want to read your own cpu's
    > clock (and have a separate __m_cnt_hi per cpu).
    >
    > Or am I totally missing out on something?
    >


    This is the global hardware clock scenario.

    We have to order an uncached mmio read wrt a cached variable read/write.
    The uncached mmio read vs smp_rmb() barrier (e.g. lfence instruction)
    should be insured by program order because the read will skip the cache
    and go directly to the bus. Luckily we only do a mmio read and no mmio
    write, so mmiowb() is not required.

    You might be right in that it could require more barriers.

    Given adequate program order, we can assume the the mmio read will
    happen "on the spot", but that the cached read may be delayed.

    What we want is :

    readl(io_addr)
    read __m_cnt_hi
    write __m_cnt_hi

    With the two reads in the correct order. If we consider two consecutive
    executions on the same CPU :

    readl(io_addr)
    read __m_cnt_hi
    write __m_cnt_hi

    readl(io_addr)
    read __m_cnt_hi
    write __m_cnt_hi

    We might have to order the read/write pair wrt the following readl, such
    as :

    smp_rmb(); /* Waits for every cached memory reads to complete */
    readl(io_addr);
    barrier(); /* Make sure the compiler leaves mmio read before cached read */
    read __m_cnt_hi
    write __m_cnt_hi

    smp_rmb(); /* Waits for every cached memory reads to complete */
    readl(io_addr)
    barrier(); /* Make sure the compiler leaves mmio read before cached read */
    read __m_cnt_hi
    write __m_cnt_hi

    Would that make more sense ?

    Mathieu

    --
    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@vger.kernel.org
    More majordomo info at http://vger.kernel.org/majordomo-info.html
    Please read the FAQ at http://www.tux.org/lkml/

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

    On Fri, 7 Nov 2008, Andrew Morton wrote:

    > On Fri, 07 Nov 2008 11:47:47 -0500 (EST) Nicolas Pitre wrote:
    >
    > > > btw, do you know how damned irritating and frustrating it is for a code
    > > > reviewer to have his comments deliberately ignored and deleted in
    > > > replies?

    > >
    > > Do you know how irritating and frustrating it is when reviewers don't
    > > care reading the damn comments along with the code?

    >
    > As you still seek to ignore it, I shall repeat my earlier question.
    > Please do not delete it again.
    >
    > It apparently tries to avoid races via ordering tricks, as long
    > as it is called with sufficient frequency. But nothing guarantees
    > that it _is_ called sufficiently frequently?
    >
    > Things like tickless kernels and SCHED_RR can surely cause
    > sched_clock() to not be called for arbitrary periods.


    On the machines this was initially written for, the critical period is
    in the order of minutes. And if you're afraid you might lack enough
    scheduling activities for that long, you simply have to keep the
    algorithm "warm" with a simple kernel timer which only purpose is to
    ensure it is called often enough.

    > Userspace cli() will definitely do this, but it is expected to break
    > stuff and is not as legitiate a thing to do.


    Why do you bring it on then?

    > I'm just giving up on the tastefulness issue.


    Taste is a pretty subjective matter.


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

+ Reply to Thread
Page 3 of 6 FirstFirst 1 2 3 4 5 ... LastLast