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

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

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/

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/

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/

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

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

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

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

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/

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/

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/

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

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/

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

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

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/

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/

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

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/