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

This is a discussion on [RFC patch 00/18] Trace Clock v2 - Kernel ; Russell King wrote: > It sounds to me as if the right answer is for it to move back to being an > ARM private thing with a MN10300 private copy, rather than it pretending > to be something everyone ...

+ Reply to Thread
Page 5 of 6 FirstFirst ... 3 4 5 6 LastLast
Results 81 to 100 of 112

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

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

    Russell King wrote:

    > It sounds to me as if the right answer is for it to move back to being an
    > ARM private thing with a MN10300 private copy, rather than it pretending
    > to be something everyone can use.


    The only problem I have with that is that there are then two independent
    copies of it:-(

    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()


    On Fri, 7 Nov 2008, David Howells wrote:

    > Steven Rostedt wrote:
    >
    > > > 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.

    >
    > Note that that does not guarantee that the two reads will be done in the order
    > you want. The compiler barrier _only_ affects the compiler. It does not stop
    > the CPU from doing the reads in any order it wants. You need something
    > stronger than smp_rmb() if you need the reads to be so ordered.


    For reading hardware devices that can indeed be correct. But for normal
    memory access on a uniprocessor, if the CPU were to reorder the reads that
    would effect the actual algorithm then that CPU is broken.

    read a
    <--- interrupt - should see read a here before read b is done.
    read b

    Now the fact that one of the reads is a hardware clock, then this
    statement might not be too strong. But the fact that it is a clock, and
    not some memory mapped device register, I still think smp_rmb is
    sufficient.

    -- 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/

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

    On Sat, 8 Nov 2008, Russell King wrote:

    > On Fri, Nov 07, 2008 at 11:41:55PM +0000, David Howells wrote:
    > > Russell King wrote:
    > >
    > > > 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.

    > >
    > > Excuse me, Russell, but I sent Nicolas an email prior to doing so asking him
    > > if he had any objections:
    > >
    > > To: Nicolas Pitre
    > > cc: dhowells@redhat.com
    > > Subject: Moving asm-arm/cnt32_to_63.h to include/linux/
    > > Date: Thu, 31 Jul 2008 16:04:04 +0100
    > >
    > > Hi Nicolas,
    > >
    > > Mind if I move include/asm-arm/cnt32_to_63.h to include/linux/?
    > >
    > > I need to use it for MN10300.
    > >
    > > David
    > >
    > > He didn't respond. Not only that, but I copied Nicolas on the patch to make
    > > the move and the patch to make MN10300 use it when I submitted it to Linus on
    > > the 24th September, so it's not like he didn't have plenty of time. He
    > > certainly saw that because he joined in the discussion of the second patch.
    > > Furthermore, he could've asked Linus to refuse the patch, or to revert it if
    > > it had already gone in.


    I was OK with the patch moving that code and I think I told you so as
    well. But...

    > > I suppose I should've cc'd the ARM list too... but why should it adversely
    > > affect ARM?

    >
    > I take back the "Neither Nicolas" bit but the rest of my comment stands
    > and remains valid.
    >
    > In light of akpm's demands to know how this got into the kernel, I decided
    > I'd put the story forward, especially as people in this thread are confused
    > about what it was designed for, and making random unfounded claiming that
    > its existing ARM uses are buggy when they aren't.


    .... I must agree with Russell that this is apparently creating more
    confusion with people than anything else.

    > It sounds to me as if the right answer is for it to move back to being an
    > ARM private thing with a MN10300 private copy, rather than it pretending
    > to be something everyone can use.


    I think this is OK if not everyone can use this. The main purpose for
    this code was to provide much increased accuracy for shed_clock() on
    processors with only a 32-bit hardware counter.

    Given that sched_clock() is already used in contexts where preemption is
    disabled, I don't mind the addition of a precision to the associated
    comment mentioning that it must be called at least once per
    half period of the base counter ***and*** not be preempted
    away for longer than the half period of the counter minus the longest
    period between two calls. The comment already mention a kernel timer
    which can be used to control the longest period between two calls.
    Implicit disabling of preemption is _NOT_ the goal of this code.

    I also don't mind having a real barrier for this code to be useful on
    other platforms. On the platform this was written for, any kind of
    barrier is defined as a compiler barrier which is perfectly fine and
    achieve the same effect as the current usage of volatile.

    I also don't mind making the high part of the counter always be a per
    CPU variable. Again this won't change anything on the target this was
    intended for and this would make this code useful for more usages, and
    possibly help making the needed barrier on SMP more lightweight. The
    usage requirement therefore becomes per CPU even if the base counter is
    global. There are per CPU timers with add_timer_on() so this can be
    ensured pretty easily.

    And if after all this the code doesn't suit your needs then just don't
    use it. Its documentation should be clear enough so if people start
    using it in contexts where it isn't appropriate then it's not the code's
    fault.


    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/

  4. [PATCH] clarify usage expectations for cnt32_to_63()

    Currently, all existing users of cnt32_to_63() are fine since the CPU
    architectures where it is used don't do read access reordering, and user
    mode preemption is disabled already. It is nevertheless a good idea to
    better elaborate usage requirements wrt preemption, and use an explicit
    memory barrier for proper results on CPUs that may perform instruction
    reordering.

    Signed-off-by: Nicolas Pitre
    ---

    On Sat, 8 Nov 2008, Nicolas Pitre wrote:

    > I think this is OK if not everyone can use this. The main purpose for
    > this code was to provide much increased accuracy for shed_clock() on
    > processors with only a 32-bit hardware counter.
    >
    > Given that sched_clock() is already used in contexts where preemption is
    > disabled, I don't mind the addition of a precision to the associated
    > comment mentioning that it must be called at least once per
    > half period of the base counter ***and*** not be preempted
    > away for longer than the half period of the counter minus the longest
    > period between two calls. The comment already mention a kernel timer
    > which can be used to control the longest period between two calls.
    > Implicit disabling of preemption is _NOT_ the goal of this code.
    >
    > I also don't mind having a real barrier for this code to be useful on
    > other platforms. On the platform this was written for, any kind of
    > barrier is defined as a compiler barrier which is perfectly fine and
    > achieve the same effect as the current usage of volatile.


    So here it is.

    I used a rmb() so this is also safe for mixed usages in and out of
    interrupt context. On the architecture I care about this is turned into
    a simple compiler barrier and therefore doesn't make a difference, while
    smp_rmb() is a noop which isn't right.

    I won't make a per_cpu_cnt32_to_63() version myself as I have no need
    for that. But if someone wants to use this witha per CPU counter which
    is not coherent between CPUs then be my guest. The usage requirements
    would be the same but on each used CPU instead of globally.

    diff --git a/include/linux/cnt32_to_63.h b/include/linux/cnt32_to_63.h
    index 8c0f950..584289d 100644
    --- a/include/linux/cnt32_to_63.h
    +++ b/include/linux/cnt32_to_63.h
    @@ -53,11 +53,19 @@ union cnt32_to_63 {
    * needed increment. And any race in updating the value in memory is harmless
    * as the same value would simply be stored more than once.
    *
    - * The only restriction for the algorithm to work properly is that this
    - * code must be executed at least once per each half period of the 32-bit
    - * counter to properly update the state bit in memory. This is usually not a
    - * problem in practice, but if it is then a kernel timer could be scheduled
    - * to manage for this code to be executed often enough.
    + * The restrictions for the algorithm to work properly are:
    + *
    + * 1) this code must be called at least once per each half period of the
    + * 32-bit counter;
    + *
    + * 2) this code must not be preempted for a duration longer than the
    + * 32-bit counter half period minus the longest period between two
    + * calls to this code.
    + *
    + * Those requirements ensure proper update to the state bit in memory.
    + * This is usually not a problem in practice, but if it is then a kernel
    + * timer should be scheduled to manage for this code to be executed often
    + * enough.
    *
    * Note that the top bit (bit 63) in the returned value should be considered
    * as garbage. It is not cleared here because callers are likely to use a
    @@ -68,9 +76,10 @@ union cnt32_to_63 {
    */
    #define cnt32_to_63(cnt_lo) \
    ({ \
    - static volatile u32 __m_cnt_hi; \
    + static u32 __m_cnt_hi; \
    union cnt32_to_63 __x; \
    __x.hi = __m_cnt_hi; \
    + rmb(); \
    __x.lo = (cnt_lo); \
    if (unlikely((s32)(__x.hi ^ __x.lo) < 0)) \
    __m_cnt_hi = __x.hi = (__x.hi ^ 0x80000000) + (__x.hi >> 31); \
    --
    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: [PATCH] clarify usage expectations for cnt32_to_63()

    * Nicolas Pitre (nico@cam.org) wrote:
    > Currently, all existing users of cnt32_to_63() are fine since the CPU
    > architectures where it is used don't do read access reordering, and user
    > mode preemption is disabled already. It is nevertheless a good idea to
    > better elaborate usage requirements wrt preemption, and use an explicit
    > memory barrier for proper results on CPUs that may perform instruction
    > reordering.
    >
    > Signed-off-by: Nicolas Pitre
    > ---
    >
    > On Sat, 8 Nov 2008, Nicolas Pitre wrote:
    >
    > > I think this is OK if not everyone can use this. The main purpose for
    > > this code was to provide much increased accuracy for shed_clock() on
    > > processors with only a 32-bit hardware counter.
    > >
    > > Given that sched_clock() is already used in contexts where preemption is
    > > disabled, I don't mind the addition of a precision to the associated
    > > comment mentioning that it must be called at least once per
    > > half period of the base counter ***and*** not be preempted
    > > away for longer than the half period of the counter minus the longest
    > > period between two calls. The comment already mention a kernel timer
    > > which can be used to control the longest period between two calls.
    > > Implicit disabling of preemption is _NOT_ the goal of this code.
    > >
    > > I also don't mind having a real barrier for this code to be useful on
    > > other platforms. On the platform this was written for, any kind of
    > > barrier is defined as a compiler barrier which is perfectly fine and
    > > achieve the same effect as the current usage of volatile.

    >
    > So here it is.
    >
    > I used a rmb() so this is also safe for mixed usages in and out of
    > interrupt context. On the architecture I care about this is turned into
    > a simple compiler barrier and therefore doesn't make a difference, while
    > smp_rmb() is a noop which isn't right.
    >


    Hum ? smp_rmb() is turned into a compiler barrier on !SMP architectures.
    Turning it into a NOP would be broken. Actually, ARM defines it as a
    barrier().

    I *think* that smp_rmb() would be enough, supposing the access to memory
    is done in program order wrt local interrupts in UP. This is basically
    Steven's question, which has not received any clear answer yet. I'd like
    to know what others think about it.

    Mathieu

    > I won't make a per_cpu_cnt32_to_63() version myself as I have no need
    > for that. But if someone wants to use this witha per CPU counter which
    > is not coherent between CPUs then be my guest. The usage requirements
    > would be the same but on each used CPU instead of globally.
    >
    > diff --git a/include/linux/cnt32_to_63.h b/include/linux/cnt32_to_63.h
    > index 8c0f950..584289d 100644
    > --- a/include/linux/cnt32_to_63.h
    > +++ b/include/linux/cnt32_to_63.h
    > @@ -53,11 +53,19 @@ union cnt32_to_63 {
    > * needed increment. And any race in updating the value in memory is harmless
    > * as the same value would simply be stored more than once.
    > *
    > - * The only restriction for the algorithm to work properly is that this
    > - * code must be executed at least once per each half period of the 32-bit
    > - * counter to properly update the state bit in memory. This is usually not a
    > - * problem in practice, but if it is then a kernel timer could be scheduled
    > - * to manage for this code to be executed often enough.
    > + * The restrictions for the algorithm to work properly are:
    > + *
    > + * 1) this code must be called at least once per each half period of the
    > + * 32-bit counter;
    > + *
    > + * 2) this code must not be preempted for a duration longer than the
    > + * 32-bit counter half period minus the longest period between two
    > + * calls to this code.
    > + *
    > + * Those requirements ensure proper update to the state bit in memory.
    > + * This is usually not a problem in practice, but if it is then a kernel
    > + * timer should be scheduled to manage for this code to be executed often
    > + * enough.
    > *
    > * Note that the top bit (bit 63) in the returned value should be considered
    > * as garbage. It is not cleared here because callers are likely to use a
    > @@ -68,9 +76,10 @@ union cnt32_to_63 {
    > */
    > #define cnt32_to_63(cnt_lo) \
    > ({ \
    > - static volatile u32 __m_cnt_hi; \
    > + static u32 __m_cnt_hi; \
    > union cnt32_to_63 __x; \
    > __x.hi = __m_cnt_hi; \
    > + rmb(); \
    > __x.lo = (cnt_lo); \
    > if (unlikely((s32)(__x.hi ^ __x.lo) < 0)) \
    > __m_cnt_hi = __x.hi = (__x.hi ^ 0x80000000) + (__x.hi >> 31); \


    --
    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/

  6. Re: [PATCH] clarify usage expectations for cnt32_to_63()

    On Sat, 8 Nov 2008, Mathieu Desnoyers wrote:

    > > I used a rmb() so this is also safe for mixed usages in and out of
    > > interrupt context. On the architecture I care about this is turned into
    > > a simple compiler barrier and therefore doesn't make a difference, while
    > > smp_rmb() is a noop which isn't right.
    > >

    >
    > Hum ? smp_rmb() is turned into a compiler barrier on !SMP architectures.
    > Turning it into a NOP would be broken. Actually, ARM defines it as a
    > barrier().


    Oh, right. I got confused somehow with read_barrier_depends().

    > I *think* that smp_rmb() would be enough, supposing the access to memory
    > is done in program order wrt local interrupts in UP. This is basically
    > Steven's question, which has not received any clear answer yet. I'd like
    > to know what others think about it.


    In the mean time a pure rmb() is the safest thing to do now. Once we
    can convince ourselves that out-of-order reads are always rolled back
    upon the arrival of an interrupt then this could be relaxed.


    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/

  7. Re: [PATCH] clarify usage expectations for cnt32_to_63()

    On Sat, 8 Nov 2008, Nicolas Pitre wrote:

    > On Sat, 8 Nov 2008, Mathieu Desnoyers wrote:
    >
    > > I *think* that smp_rmb() would be enough, supposing the access to memory
    > > is done in program order wrt local interrupts in UP. This is basically
    > > Steven's question, which has not received any clear answer yet. I'd like
    > > to know what others think about it.

    >
    > In the mean time a pure rmb() is the safest thing to do now. Once we
    > can convince ourselves that out-of-order reads are always rolled back
    > upon the arrival of an interrupt then this could be relaxed.


    After thinking about it some more, then a smp_rmb() must be correct.

    On UP, that would be completely insane if an exception didn't resume
    the whole sequence since the CPU cannot presume anything when returning
    from it.

    If the instruction flows says:

    READ A
    READ B

    And speculative execution makes for B to be read before A, and you get
    an interrupt after B was read but before A was read, then the program
    counter may only point at READ A upon returning from the exception and B
    will be read again. Doing otherwise would require the CPU to remember
    any reordering that it might have performed upon every exceptions which
    is completely insane.

    So smp_rmb() it shall be.


    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/

  8. [PATCH v2] clarify usage expectations for cnt32_to_63()

    Currently, all existing users of cnt32_to_63() are fine since the CPU
    architectures where it is used don't do read access reordering, and user
    mode preemption is disabled already. It is nevertheless a good idea to
    better elaborate usage requirements wrt preemption, and use an explicit
    memory barrier on SMP to avoid different CPUs accessing the counter
    value in the wrong order. On UP a simple compiler barrier is
    sufficient.

    Signed-off-by: Nicolas Pitre
    ---

    On Sun, 9 Nov 2008, Nicolas Pitre wrote:

    > On Sat, 8 Nov 2008, Nicolas Pitre wrote:
    >
    > > On Sat, 8 Nov 2008, Mathieu Desnoyers wrote:
    > >
    > > > I *think* that smp_rmb() would be enough, supposing the access to memory
    > > > is done in program order wrt local interrupts in UP. This is basically
    > > > Steven's question, which has not received any clear answer yet. I'd like
    > > > to know what others think about it.

    > >
    > > In the mean time a pure rmb() is the safest thing to do now. Once we
    > > can convince ourselves that out-of-order reads are always rolled back
    > > upon the arrival of an interrupt then this could be relaxed.

    >
    > After thinking about it some more, a smp_rmb() must be correct.
    >
    > On UP, that would be completely insane if an exception didn't resume
    > the whole sequence since the CPU cannot presume anything when returning
    > from it.
    >
    > If the instruction flows says:
    >
    > READ A
    > READ B
    >
    > And speculative execution makes for B to be read before A, and you get
    > an interrupt after B was read but before A was read, then the program
    > counter may only point at READ A upon returning from the exception and B
    > will be read again. Doing otherwise would require the CPU to remember
    > any reordering that it might have performed upon every exceptions which
    > is completely insane.
    >
    > So smp_rmb() it shall be.


    diff --git a/include/linux/cnt32_to_63.h b/include/linux/cnt32_to_63.h
    index 8c0f950..7605fdd 100644
    --- a/include/linux/cnt32_to_63.h
    +++ b/include/linux/cnt32_to_63.h
    @@ -16,6 +16,7 @@
    #include
    #include
    #include
    +#include

    /* this is used only to give gcc a clue about good code generation */
    union cnt32_to_63 {
    @@ -53,11 +54,19 @@ union cnt32_to_63 {
    * needed increment. And any race in updating the value in memory is harmless
    * as the same value would simply be stored more than once.
    *
    - * The only restriction for the algorithm to work properly is that this
    - * code must be executed at least once per each half period of the 32-bit
    - * counter to properly update the state bit in memory. This is usually not a
    - * problem in practice, but if it is then a kernel timer could be scheduled
    - * to manage for this code to be executed often enough.
    + * The restrictions for the algorithm to work properly are:
    + *
    + * 1) this code must be called at least once per each half period of the
    + * 32-bit counter;
    + *
    + * 2) this code must not be preempted for a duration longer than the
    + * 32-bit counter half period minus the longest period between two
    + * calls to this code.
    + *
    + * Those requirements ensure proper update to the state bit in memory.
    + * This is usually not a problem in practice, but if it is then a kernel
    + * timer should be scheduled to manage for this code to be executed often
    + * enough.
    *
    * Note that the top bit (bit 63) in the returned value should be considered
    * as garbage. It is not cleared here because callers are likely to use a
    @@ -68,9 +77,10 @@ union cnt32_to_63 {
    */
    #define cnt32_to_63(cnt_lo) \
    ({ \
    - static volatile u32 __m_cnt_hi; \
    + static u32 __m_cnt_hi; \
    union cnt32_to_63 __x; \
    __x.hi = __m_cnt_hi; \
    + smp_rmb(); \
    __x.lo = (cnt_lo); \
    if (unlikely((s32)(__x.hi ^ __x.lo) < 0)) \
    __m_cnt_hi = __x.hi = (__x.hi ^ 0x80000000) + (__x.hi >> 31); \
    --
    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: [PATCH v2] clarify usage expectations for cnt32_to_63()

    * Nicolas Pitre (nico@cam.org) wrote:
    > Currently, all existing users of cnt32_to_63() are fine since the CPU
    > architectures where it is used don't do read access reordering, and user
    > mode preemption is disabled already. It is nevertheless a good idea to
    > better elaborate usage requirements wrt preemption, and use an explicit
    > memory barrier on SMP to avoid different CPUs accessing the counter
    > value in the wrong order. On UP a simple compiler barrier is
    > sufficient.
    >
    > Signed-off-by: Nicolas Pitre
    > ---
    >

    ....
    > @@ -68,9 +77,10 @@ union cnt32_to_63 {
    > */
    > #define cnt32_to_63(cnt_lo) \
    > ({ \
    > - static volatile u32 __m_cnt_hi; \
    > + static u32 __m_cnt_hi; \


    It's important to get the smp_rmb() here, which this patch provides, so
    consider this patch to be acked-by me. The added documentation is needed
    too.

    But I also think that declaring the static u32 __m_cnt_hi here is
    counter-intuitive for developers who wish to use it.

    I'd recommend letting the declaration be done outside of cnt32_to_63 so
    the same variable can be passed as parameter to more than one execution
    site.

    Mathieu


    > union cnt32_to_63 __x; \
    > __x.hi = __m_cnt_hi; \
    > + smp_rmb(); \
    > __x.lo = (cnt_lo); \
    > if (unlikely((s32)(__x.hi ^ __x.lo) < 0)) \
    > __m_cnt_hi = __x.hi = (__x.hi ^ 0x80000000) + (__x.hi >> 31); \


    --
    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()

    Steven Rostedt wrote:

    > > Note that that does not guarantee that the two reads will be done in the
    > > order you want. The compiler barrier _only_ affects the compiler. It
    > > does not stop the CPU from doing the reads in any order it wants. You
    > > need something stronger than smp_rmb() if you need the reads to be so
    > > ordered.

    >
    > For reading hardware devices that can indeed be correct. But for normal
    > memory access on a uniprocessor, if the CPU were to reorder the reads that
    > would effect the actual algorithm then that CPU is broken.
    >
    > read a
    > <--- interrupt - should see read a here before read b is done.
    > read b


    Life isn't that simple. Go and read the section labelled "The things cpus get
    up to" in Documentation/memory-barriers.txt.

    The two reads we're talking about are independent of each other. Independent
    reads and writes can be reordered and merged at will by the CPU, subject to
    restrictions imposed by barriers, cacheability attributes, MMIO attributes and
    suchlike.

    You can get read b happening before read a, but in such a case both
    instructions will be in the CPU's execution pipeline. When an interrupt
    occurs, the CPU will presumably finish clearing what's in its pipeline before
    going and servicing the interrupt handler.

    If a CPU is strictly ordered with respect to reads, do you actually need read
    barriers?

    The fact that a pair of reads might be part of an algorithm that is critically
    dependent on the ordering of those reads isn't something the CPU cares about.
    It doesn't know there's an algorithm there.

    > Now the fact that one of the reads is a hardware clock, then this
    > statement might not be too strong. But the fact that it is a clock, and
    > not some memory mapped device register, I still think smp_rmb is
    > sufficient.


    To quote again from memory-barriers.txt, section "CPU memory barriers":

    Mandatory barriers should not be used to control SMP effects, since
    mandatory barriers unnecessarily impose overhead on UP systems. They
    may, however, be used to control MMIO effects on accesses through
    relaxed memory I/O windows. These are required even on non-SMP
    systems as they affect the order in which memory operations appear to
    a device by prohibiting both the compiler and the CPU from reordering
    them.

    Section "Accessing devices":

    (2) If the accessor functions are used to refer to an I/O memory window with
    relaxed memory access properties, then _mandatory_ memory barriers are
    required to enforce ordering.

    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/

  11. Re: [PATCH v2] clarify usage expectations for cnt32_to_63()

    On Sun, 9 Nov 2008, Mathieu Desnoyers wrote:

    > * Nicolas Pitre (nico@cam.org) wrote:
    > > Currently, all existing users of cnt32_to_63() are fine since the CPU
    > > architectures where it is used don't do read access reordering, and user
    > > mode preemption is disabled already. It is nevertheless a good idea to
    > > better elaborate usage requirements wrt preemption, and use an explicit
    > > memory barrier on SMP to avoid different CPUs accessing the counter
    > > value in the wrong order. On UP a simple compiler barrier is
    > > sufficient.
    > >
    > > Signed-off-by: Nicolas Pitre
    > > ---
    > >

    > ...
    > > @@ -68,9 +77,10 @@ union cnt32_to_63 {
    > > */
    > > #define cnt32_to_63(cnt_lo) \
    > > ({ \
    > > - static volatile u32 __m_cnt_hi; \
    > > + static u32 __m_cnt_hi; \

    >
    > It's important to get the smp_rmb() here, which this patch provides, so
    > consider this patch to be acked-by me. The added documentation is needed
    > too.


    Thanks.

    > But I also think that declaring the static u32 __m_cnt_hi here is
    > counter-intuitive for developers who wish to use it.


    I'm rather not convinced of that. And this is a much bigger change
    affecting all callers so I'd defer such change even if I was convinced
    of it.

    > I'd recommend letting the declaration be done outside of cnt32_to_63 so
    > the same variable can be passed as parameter to more than one execution
    > site.


    Do you really have such instances where multiple call sites are needed?
    That sounds even more confusing to me than the current model. Better
    encapsulate the usage of this macro within some function which has a
    stronger meaning, such as sched_clock(), and call _that_ from multiple
    sites instead.


    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/

  12. Re: [PATCH v2] clarify usage expectations for cnt32_to_63()

    On Sun, Nov 09, 2008 at 08:34:23AM -0500, Nicolas Pitre wrote:
    > Do you really have such instances where multiple call sites are needed?
    > That sounds even more confusing to me than the current model. Better
    > encapsulate the usage of this macro within some function which has a
    > stronger meaning, such as sched_clock(), and call _that_ from multiple
    > sites instead.


    What if sched_clock() is inline and uses cnt32_to_63()? I think that's
    where the problem lies. Better add a comment that it shouldn't be used
    inside another inline function.

    --
    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@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()


    On Sun, 9 Nov 2008, David Howells wrote:

    > Steven Rostedt wrote:
    >
    > > > Note that that does not guarantee that the two reads will be done in the
    > > > order you want. The compiler barrier _only_ affects the compiler. It
    > > > does not stop the CPU from doing the reads in any order it wants. You
    > > > need something stronger than smp_rmb() if you need the reads to be so
    > > > ordered.

    > >
    > > For reading hardware devices that can indeed be correct. But for normal
    > > memory access on a uniprocessor, if the CPU were to reorder the reads that
    > > would effect the actual algorithm then that CPU is broken.


    Please read what I said above again.

    "For reading hardware devices that can indeed be correct."

    There I agree that accessing devices will require a rmb.

    "But for normal memory access on a uniprocessor, if the CPU were to
    reorder the reads that would effect the actual algorithm then that CPU is
    broken."

    Here I'm talking about accessing normal RAM. If the CPU decides to read b
    before reading a then that will break the code.

    > >
    > > read a
    > > <--- interrupt - should see read a here before read b is done.
    > > read b

    >
    > Life isn't that simple. Go and read the section labelled "The things cpus get
    > up to" in Documentation/memory-barriers.txt.


    I've read it. Several times ;-)

    >
    > The two reads we're talking about are independent of each other. Independent
    > reads and writes can be reordered and merged at will by the CPU, subject to
    > restrictions imposed by barriers, cacheability attributes, MMIO attributes and
    > suchlike.
    >
    > You can get read b happening before read a, but in such a case both
    > instructions will be in the CPU's execution pipeline. When an interrupt
    > occurs, the CPU will presumably finish clearing what's in its pipeline before
    > going and servicing the interrupt handler.


    This above sounds like you just answered my question, and a smp_rmb is
    enough. If an interrupt occurs, then the read a and read b will be
    completed. Really does not matter in which order, as long as the interrupt
    itself does not see the read b before the read a.


    >
    > If a CPU is strictly ordered with respect to reads, do you actually need read
    > barriers?
    >
    > The fact that a pair of reads might be part of an algorithm that is critically
    > dependent on the ordering of those reads isn't something the CPU cares about.
    > It doesn't know there's an algorithm there.
    >
    > > Now the fact that one of the reads is a hardware clock, then this
    > > statement might not be too strong. But the fact that it is a clock, and
    > > not some memory mapped device register, I still think smp_rmb is
    > > sufficient.

    >
    > To quote again from memory-barriers.txt, section "CPU memory barriers":
    >
    > Mandatory barriers should not be used to control SMP effects, since
    > mandatory barriers unnecessarily impose overhead on UP systems. They
    > may, however, be used to control MMIO effects on accesses through
    > relaxed memory I/O windows. These are required even on non-SMP
    > systems as they affect the order in which memory operations appear to
    > a device by prohibiting both the compiler and the CPU from reordering
    > them.
    >
    > Section "Accessing devices":
    >
    > (2) If the accessor functions are used to refer to an I/O memory window with
    > relaxed memory access properties, then _mandatory_ memory barriers are
    > required to enforce ordering.


    My confidence on reading a clock is not as strong that a smp_rmb is
    enough. And it may not be. I'll have to think about this a bit more.
    Again, the question arrises with:

    read a (memory)
    <---- interrupt
    read b (clock)

    Will the b be seen before the interrupt occurred, and before the a is
    read? That is what will break the algorithm on UP. If we can not
    guarantee this statement, then a rmb is needed.

    -- 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/

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

    * David Howells (dhowells@redhat.com) wrote:
    > Steven Rostedt wrote:
    >
    > > > Note that that does not guarantee that the two reads will be done in the
    > > > order you want. The compiler barrier _only_ affects the compiler. It
    > > > does not stop the CPU from doing the reads in any order it wants. You
    > > > need something stronger than smp_rmb() if you need the reads to be so
    > > > ordered.

    > >
    > > For reading hardware devices that can indeed be correct. But for normal
    > > memory access on a uniprocessor, if the CPU were to reorder the reads that
    > > would effect the actual algorithm then that CPU is broken.
    > >
    > > read a
    > > <--- interrupt - should see read a here before read b is done.
    > > read b

    >
    > Life isn't that simple. Go and read the section labelled "The things cpus get
    > up to" in Documentation/memory-barriers.txt.
    >
    > The two reads we're talking about are independent of each other. Independent
    > reads and writes can be reordered and merged at will by the CPU, subject to
    > restrictions imposed by barriers, cacheability attributes, MMIO attributes and
    > suchlike.
    >
    > You can get read b happening before read a, but in such a case both
    > instructions will be in the CPU's execution pipeline. When an interrupt
    > occurs, the CPU will presumably finish clearing what's in its pipeline before
    > going and servicing the interrupt handler.
    >
    > If a CPU is strictly ordered with respect to reads, do you actually need read
    > barriers?
    >
    > The fact that a pair of reads might be part of an algorithm that is critically
    > dependent on the ordering of those reads isn't something the CPU cares about.
    > It doesn't know there's an algorithm there.
    >
    > > Now the fact that one of the reads is a hardware clock, then this
    > > statement might not be too strong. But the fact that it is a clock, and
    > > not some memory mapped device register, I still think smp_rmb is
    > > sufficient.

    >
    > To quote again from memory-barriers.txt, section "CPU memory barriers":
    >
    > Mandatory barriers should not be used to control SMP effects, since
    > mandatory barriers unnecessarily impose overhead on UP systems. They
    > may, however, be used to control MMIO effects on accesses through
    > relaxed memory I/O windows. These are required even on non-SMP
    > systems



    > as they affect the order in which memory operations appear to a device



    In this particular case, we don't care about the order of memory
    operations as seen by the device, given we only read the mmio time
    source atomically. So considering what you said above about the fact
    that the CPU will flush all the pending operations in the pipeline
    before proceeding to service an interrupt, a simple barrier() should be
    enough to make the two operations appear in correct order wrt local
    interrupts. I therefore don't think a full rmb() is required to insure
    correct read order on UP, because, again, in this case we don't need to
    order accesses as seen by the device.

    Mathieu

    > by prohibiting both the compiler and the CPU from reordering
    > them.
    >
    > Section "Accessing devices":
    >
    > (2) If the accessor functions are used to refer to an I/O memory window with
    > relaxed memory access properties, then _mandatory_ memory barriers are
    > required to enforce ordering.
    >
    > David


    --
    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/

  15. Re: [PATCH v2] clarify usage expectations for cnt32_to_63()

    * Nicolas Pitre (nico@cam.org) wrote:
    > On Sun, 9 Nov 2008, Mathieu Desnoyers wrote:
    >
    > > * Nicolas Pitre (nico@cam.org) wrote:
    > > > Currently, all existing users of cnt32_to_63() are fine since the CPU
    > > > architectures where it is used don't do read access reordering, and user
    > > > mode preemption is disabled already. It is nevertheless a good idea to
    > > > better elaborate usage requirements wrt preemption, and use an explicit
    > > > memory barrier on SMP to avoid different CPUs accessing the counter
    > > > value in the wrong order. On UP a simple compiler barrier is
    > > > sufficient.
    > > >
    > > > Signed-off-by: Nicolas Pitre
    > > > ---
    > > >

    > > ...
    > > > @@ -68,9 +77,10 @@ union cnt32_to_63 {
    > > > */
    > > > #define cnt32_to_63(cnt_lo) \
    > > > ({ \
    > > > - static volatile u32 __m_cnt_hi; \
    > > > + static u32 __m_cnt_hi; \

    > >
    > > It's important to get the smp_rmb() here, which this patch provides, so
    > > consider this patch to be acked-by me. The added documentation is needed
    > > too.

    >
    > Thanks.
    >
    > > But I also think that declaring the static u32 __m_cnt_hi here is
    > > counter-intuitive for developers who wish to use it.

    >
    > I'm rather not convinced of that. And this is a much bigger change
    > affecting all callers so I'd defer such change even if I was convinced
    > of it.
    >
    > > I'd recommend letting the declaration be done outside of cnt32_to_63 so
    > > the same variable can be passed as parameter to more than one execution
    > > site.

    >
    > Do you really have such instances where multiple call sites are needed?
    > That sounds even more confusing to me than the current model. Better
    > encapsulate the usage of this macro within some function which has a
    > stronger meaning, such as sched_clock(), and call _that_ from multiple
    > sites instead.
    >
    >
    > Nicolas


    I see a few reasons for it :

    - If we want to inline the whole read function so we don't pay the extra
    runtime cost of a function call, this would become required.
    - If we want to create a per cpu timer which updates the value
    periodically without calling the function. We may want to add some
    WARN_ON or some sanity tests in this periodic update that would not be
    part of the standard read code. If we don't have access to this
    variable outside of the macro, this becomes impossible.

    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/

  16. Re: [PATCH v2] clarify usage expectations for cnt32_to_63()

    On Sun, 9 Nov 2008, Mathieu Desnoyers wrote:

    > * Nicolas Pitre (nico@cam.org) wrote:
    > > Do you really have such instances where multiple call sites are needed?
    > > That sounds even more confusing to me than the current model. Better
    > > encapsulate the usage of this macro within some function which has a
    > > stronger meaning, such as sched_clock(), and call _that_ from multiple
    > > sites instead.

    >
    > I see a few reasons for it :
    >
    > - If we want to inline the whole read function so we don't pay the extra
    > runtime cost of a function call, this would become required.


    You can inline it as you want as long as it remains in the same .c file.
    The static variable is still shared amongst all call sites in that case.

    > - If we want to create a per cpu timer which updates the value
    > periodically without calling the function. We may want to add some
    > WARN_ON or some sanity tests in this periodic update that would not be
    > part of the standard read code. If we don't have access to this
    > variable outside of the macro, this becomes impossible.


    I don't see how you could update the variable without calling the
    function somehow or duplicating it. As to the sanity check argument:
    you can perform such checks on the result rather than the internal
    variable which IMHO would be more logical.

    And if you want a per CPU version, then it's better to create a
    per_cpu_cnt32_to_63() variant which could use a relaxed barrier in that
    case.


    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/

  17. Re: [PATCH v2] clarify usage expectations for cnt32_to_63()

    On Sun, 09 Nov 2008 23:20:00 -0500 (EST) Nicolas Pitre wrote:

    > On Sun, 9 Nov 2008, Mathieu Desnoyers wrote:
    >
    > > * Nicolas Pitre (nico@cam.org) wrote:
    > > > Do you really have such instances where multiple call sites are needed?
    > > > That sounds even more confusing to me than the current model. Better
    > > > encapsulate the usage of this macro within some function which has a
    > > > stronger meaning, such as sched_clock(), and call _that_ from multiple
    > > > sites instead.

    > >
    > > I see a few reasons for it :
    > >
    > > - If we want to inline the whole read function so we don't pay the extra
    > > runtime cost of a function call, this would become required.

    >
    > You can inline it as you want as long as it remains in the same .c file.
    > The static variable is still shared amongst all call sites in that case.


    Please don't rely upon deep compiler behaviour like that. It is
    unobvious to the reader and it might break if someone uses it incorrectly,
    or if the compiler implementation changes, or if a non-gcc compiler is
    used, etc.

    It is far better to make the management of the state explicit and at
    the control of the caller. Get the caller to allocate the state and
    pass its address into this function. Simple, clear, explicit and
    robust.
    --
    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: [PATCH v2] clarify usage expectations for cnt32_to_63()

    On Sun, 9 Nov 2008, Andrew Morton wrote:

    > On Sun, 09 Nov 2008 23:20:00 -0500 (EST) Nicolas Pitre wrote:
    >
    > > On Sun, 9 Nov 2008, Mathieu Desnoyers wrote:
    > >
    > > > * Nicolas Pitre (nico@cam.org) wrote:
    > > > > Do you really have such instances where multiple call sites are needed?
    > > > > That sounds even more confusing to me than the current model. Better
    > > > > encapsulate the usage of this macro within some function which has a
    > > > > stronger meaning, such as sched_clock(), and call _that_ from multiple
    > > > > sites instead.
    > > >
    > > > I see a few reasons for it :
    > > >
    > > > - If we want to inline the whole read function so we don't pay the extra
    > > > runtime cost of a function call, this would become required.

    > >
    > > You can inline it as you want as long as it remains in the same .c file.
    > > The static variable is still shared amongst all call sites in that case.

    >
    > Please don't rely upon deep compiler behaviour like that. It is
    > unobvious to the reader and it might break if someone uses it incorrectly,
    > or if the compiler implementation changes, or if a non-gcc compiler is
    > used, etc.


    If a compiler doesn't reference the same storage for a static variable
    used by a function that gets inlined in the same compilation unit then
    it is utterly broken.

    > It is far better to make the management of the state explicit and at
    > the control of the caller. Get the caller to allocate the state and
    > pass its address into this function. Simple, clear, explicit and
    > robust.


    Sigh... What about this compromize then?

    diff --git a/include/linux/cnt32_to_63.h b/include/linux/cnt32_to_63.h
    index 7605fdd..74ce767 100644
    --- a/include/linux/cnt32_to_63.h
    +++ b/include/linux/cnt32_to_63.h
    @@ -32,8 +32,9 @@ union cnt32_to_63 {


    /**
    - * cnt32_to_63 - Expand a 32-bit counter to a 63-bit counter
    + * __cnt32_to_63 - Expand a 32-bit counter to a 63-bit counter
    * @cnt_lo: The low part of the counter
    + * @cnt_hi_p: Pointer to storage for the extended part of the counter
    *
    * Many hardware clock counters are only 32 bits wide and therefore have
    * a relatively short period making wrap-arounds rather frequent. This
    @@ -75,16 +76,31 @@ union cnt32_to_63 {
    * clear-bit instruction. Otherwise caller must remember to clear the top
    * bit explicitly.
    */
    -#define cnt32_to_63(cnt_lo) \
    +#define __cnt32_to_63(cnt_lo, cnt_hi_p) \
    ({ \
    - static u32 __m_cnt_hi; \
    union cnt32_to_63 __x; \
    - __x.hi = __m_cnt_hi; \
    + __x.hi = *(cnt_hi_p); \
    smp_rmb(); \
    __x.lo = (cnt_lo); \
    if (unlikely((s32)(__x.hi ^ __x.lo) < 0)) \
    - __m_cnt_hi = __x.hi = (__x.hi ^ 0x80000000) + (__x.hi >> 31); \
    + *(cnt_hi_p) = __x.hi = (__x.hi ^ 0x80000000) + (__x.hi >> 31); \
    __x.val; \
    })

    +/**
    + * cnt32_to_63 - Expand a 32-bit counter to a 63-bit counter
    + * @cnt_lo: The low part of the counter
    + *
    + * This is the same as __cnt32_to_63() except that the storage for the
    + * extended part of the counter is implicit. Because this uses a static
    + * variable, a user of this code must not be an inline function unless
    + * that function is contained in a single .c file for a given counter.
    + * All usage requirements for __cnt32_to_63() also apply here as well.
    + */
    +#define cnt32_to_63(cnt_lo) \
    +({ \
    + static u32 __m_cnt_hi; \
    + __cnt32_to_63(cnt_lo, &__m_cnt_hi); \
    +})
    +
    #endif

    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/

  19. Re: [PATCH v2] clarify usage expectations for cnt32_to_63()

    On Mon, 10 Nov 2008 16:34:54 -0500 (EST)
    Nicolas Pitre wrote:

    > > It is far better to make the management of the state explicit and at
    > > the control of the caller. Get the caller to allocate the state and
    > > pass its address into this function. Simple, clear, explicit and
    > > robust.

    >
    > Sigh... What about this compromize then?
    >
    > diff --git a/include/linux/cnt32_to_63.h b/include/linux/cnt32_to_63.h
    > index 7605fdd..74ce767 100644
    > --- a/include/linux/cnt32_to_63.h
    > +++ b/include/linux/cnt32_to_63.h
    > @@ -32,8 +32,9 @@ union cnt32_to_63 {
    >
    >
    > /**
    > - * cnt32_to_63 - Expand a 32-bit counter to a 63-bit counter
    > + * __cnt32_to_63 - Expand a 32-bit counter to a 63-bit counter
    > * @cnt_lo: The low part of the counter
    > + * @cnt_hi_p: Pointer to storage for the extended part of the counter
    > *
    > * Many hardware clock counters are only 32 bits wide and therefore have
    > * a relatively short period making wrap-arounds rather frequent. This
    > @@ -75,16 +76,31 @@ union cnt32_to_63 {
    > * clear-bit instruction. Otherwise caller must remember to clear the top
    > * bit explicitly.
    > */
    > -#define cnt32_to_63(cnt_lo) \
    > +#define __cnt32_to_63(cnt_lo, cnt_hi_p) \
    > ({ \
    > - static u32 __m_cnt_hi; \
    > union cnt32_to_63 __x; \
    > - __x.hi = __m_cnt_hi; \
    > + __x.hi = *(cnt_hi_p); \
    > smp_rmb(); \
    > __x.lo = (cnt_lo); \
    > if (unlikely((s32)(__x.hi ^ __x.lo) < 0)) \
    > - __m_cnt_hi = __x.hi = (__x.hi ^ 0x80000000) + (__x.hi >> 31); \
    > + *(cnt_hi_p) = __x.hi = (__x.hi ^ 0x80000000) + (__x.hi >> 31); \
    > __x.val; \
    > })


    This references its second argument twice, which can cause correctness
    or efficiency problems.

    There is no reason that this had to be implemented in cpp.
    Implementing it in C will fix the above problem.



    To the reader of the code, a call to cnt32_to_63() looks exactly like a
    plain old function call. Hiding the instantiation of the state storage
    inside this macro misleads the reader and hence is bad practice. This
    is one of the reasons why the management of that state should be
    performed by the caller and made explicit.

    I cannot think of any other cases in the kernel where this trick of
    instantiating static storage at a macro expansion site is performed.
    It is unusual. It will surprise readers. Surprising readers is
    undesirable.

    --
    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: [PATCH v2] clarify usage expectations for cnt32_to_63()

    On Mon, 10 Nov 2008, Andrew Morton wrote:

    > On Mon, 10 Nov 2008 16:34:54 -0500 (EST)
    > Nicolas Pitre wrote:
    >
    > > > It is far better to make the management of the state explicit and at
    > > > the control of the caller. Get the caller to allocate the state and
    > > > pass its address into this function. Simple, clear, explicit and
    > > > robust.

    > >
    > > Sigh... What about this compromize then?
    > >
    > > diff --git a/include/linux/cnt32_to_63.h b/include/linux/cnt32_to_63.h
    > > index 7605fdd..74ce767 100644
    > > --- a/include/linux/cnt32_to_63.h
    > > +++ b/include/linux/cnt32_to_63.h
    > > @@ -32,8 +32,9 @@ union cnt32_to_63 {
    > >
    > >
    > > /**
    > > - * cnt32_to_63 - Expand a 32-bit counter to a 63-bit counter
    > > + * __cnt32_to_63 - Expand a 32-bit counter to a 63-bit counter
    > > * @cnt_lo: The low part of the counter
    > > + * @cnt_hi_p: Pointer to storage for the extended part of the counter
    > > *
    > > * Many hardware clock counters are only 32 bits wide and therefore have
    > > * a relatively short period making wrap-arounds rather frequent. This
    > > @@ -75,16 +76,31 @@ union cnt32_to_63 {
    > > * clear-bit instruction. Otherwise caller must remember to clear the top
    > > * bit explicitly.
    > > */
    > > -#define cnt32_to_63(cnt_lo) \
    > > +#define __cnt32_to_63(cnt_lo, cnt_hi_p) \
    > > ({ \
    > > - static u32 __m_cnt_hi; \
    > > union cnt32_to_63 __x; \
    > > - __x.hi = __m_cnt_hi; \
    > > + __x.hi = *(cnt_hi_p); \
    > > smp_rmb(); \
    > > __x.lo = (cnt_lo); \
    > > if (unlikely((s32)(__x.hi ^ __x.lo) < 0)) \
    > > - __m_cnt_hi = __x.hi = (__x.hi ^ 0x80000000) + (__x.hi >> 31); \
    > > + *(cnt_hi_p) = __x.hi = (__x.hi ^ 0x80000000) + (__x.hi >> 31); \
    > > __x.val; \
    > > })

    >
    > This references its second argument twice, which can cause correctness
    > or efficiency problems.
    >
    > There is no reason that this had to be implemented in cpp.
    > Implementing it in C will fix the above problem.


    No, it won't, for correctness and efficiency reasons.

    And I've explained why already.

    No need to discuss this further if you can't get it.


    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 5 of 6 FirstFirst ... 3 4 5 6 LastLast