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

[Nicolas Pitre]

On Sun, 9 Nov 2008, Mathieu Desnoyers wrote:

> * Nicolas Pitre (nico@xxxxxxx) 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 <nico@xxxxxxxxxxx>
> > ---
> > 
> ...
> > @@ -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@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/