Date:	Wed, 01 Oct 2008 11:36:31 -0400 (EDT)
From:	Nicolas Pitre <nico@....org>
To:	David Howells <dhowells@...hat.com>
Cc:	Peter Zijlstra <a.p.zijlstra@...llo.nl>, torvalds@...l.org,
	akpm@...ux-foundation.org, linux-am33-list@...hat.com,
	lkml <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH 1/2] MN10300: Move asm-arm/cnt32_to_63.h to include/linux/

On Wed, 1 Oct 2008, David Howells wrote:

> Nicolas Pitre <nico@....org> wrote:
> 
> > Disabling preemption is unneeded.
> 
> I think you may be wrong on that.  MEI came up with the following point:
> 
> 	I think either disable preemption or disable interrupt is really
> 	necessary for cnt32_to_63 macro, because there seems to be assumption
> 	that the series of the code (1)-(4) must be executed within a half
> 	period of the 32-bit counter.

This is still wrong.  There is no need for any kind of locking what so 
ever, period.  That's the _point_ of the whole algorithm.  The only 
constraint is that the code has to be executed at least once per half 
period of the 32 bit counter, which is typically once every couple 
minutes or so.

> 	-------------------------------------------------
> 	#define cnt32_to_63(cnt_lo) 
> 	({ 
> 	       static volatile u32 __m_cnt_hi = 0; 
> 	       cnt32_to_63_t __x; 
> 	(1)    __x.hi = __m_cnt_hi; 
> 	(2)    __x.lo = (cnt_lo); 
> 	(3)    if (unlikely((s32)(__x.hi ^ __x.lo) < 0))
> 	(4)            __m_cnt_hi = __x.hi = (__x.hi ^ 0x80000000) + (__x.hi >> 31); 
> 	       __x.val; 
> 	})
> 	-------------------------------------------------
> 
> 	If a task is preempted while executing the series of the code and
> 	scheduled again after the half period of the 32-bit counter, the task
> 	may destroy __m_cnt_hi.

Oh, that's a possibility.  In that case __m_cnt_hi will be reverted to a 
previous state just like if cnt32_to_63() has not been called yet since 
the new half period.  And a subsequent call will fix that again.

> Their suggested remedy is:
> 
> 	So I think it's better to disable interrupt the cnt32_to_63 and to
> 	ensure that the series of the code are executed within a short period.
> 
> I think this is excessive...  If we're sat there with interrupts disabled for
> more than a half period (65s) then we've got other troubles.  I think
> disabling preemption for the duration ought to be enough.  What do you think?

I think this is excessive too.  If you're preempted away for that long 
you still have a problem.  And if that's a real concern because you run 
RT and have tasks frequently blocked for that long then don't use this 
interface for critical counters for which absolute correctness is 
essential, which is not the case for sched_clock() anyway.  A 
sched_clock() implementation is meant to be as lightweight as possible 
even if it lacks in absolute precision, and the worst that could happen 
if you actually manage to cause a glitch in the returned value from 
sched_clock() is possibly the scheduling of the wrong task in a non RT 
scheduler, and we determined that a RT scheduler is needed to cause this 
glitch already.

> Now, I'm happy to put these in sched_clock() rather then cnt32_to_63() for my
> purposes (see attached patch).

If you still feel paranoid about this then I can't stop you from adding 
extra locking in your own code.  On machines I've created cnt32_to_63() 
for, the critical half period delay can be like 9 minutes and worrying 
about races that could last that long is really about ignoring a much 
worse problem.


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

Powered by blists - more mailing lists