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

[Mathieu Desnoyers]

* Nicolas Pitre (nico@xxxxxxx) wrote:
> 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

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@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/