lkml.org 
[lkml]   [2008]   [Oct]   [1]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [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@cam.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


\
 
 \ /
  Last update: 2008-10-01 17:39    [from the cache]
©2003-2011 Jasper Spaans. Advertise on this site