svn commit: r232456 - projects/armv6/sys/arm/include

[Olivier Houchard]

On Tue, Mar 06, 2012 at 03:58:36PM +1100, Bruce Evans wrote:
> On Sat, 3 Mar 2012, Olivier Houchard wrote:
> 
> >On Sun, Mar 04, 2012 at 02:14:46AM +1100, Bruce Evans wrote:
> >>On Sat, 3 Mar 2012, Olivier Houchard wrote:
> 
> Sorry this reply took so long.
> 

Hi Bruce,

Now I'm the one taking a long time to answer, sorry.

> >
> >I understand your concerns. These casts are indeed bogus, and will become 
> >even
> >more when we'll support 64bits arm, which should come with 64bits long.
> 
> Won't they just fail then, and need separate functions which mostly make
> the problem go away, as on amd64?  The patch seems to add a lot of 64-bit
> support, which confused me at first.
> 

Well, 64bits support is still far away, the architecture manual isn't even
available yet, I just wrote that thinking it would be useful later, but of
course more work will be required.

> >I can't do much for long which should be 64bits even on 32bits machines, 
> >that
> >is set in stone now, however I can certainly remove the bogus casts.
> >
> >Would the attached patch be OK for you ?
> >It duplicates the various atomic functions to add a _long variant (for 
> >armv6
> >at least, for armv5 it just introduces _long variants which calls the _32
> >version, but at least it should catch any signedness/type error), and it
> >removes the bogus casts for the ptr version, and just #defines it to the 
> >__32
> >version, since that's what uintptr_t is.
> 
> This seems to be essentially correct -- don't use blind casts in macros,
> and don't duplicate the functions (at least large ones), but provide
> wrappers implemented as inline functions.  The wrappers do the same thing
> as the macros, but in a type-safe way.  Perhaps they can be generated
> a bit more automatically or otherwise reduced to 1 line each, but I don't
> want to use complicated macros for this.
> 

Yeah, I'd rather have them understandable.

> % Index: atomic.h
> % ===================================================================
> % --- atomic.h	(revision 232462)
> % +++ atomic.h	(working copy)
> % @@ -74,6 +74,21 @@
> %  #endif
> %  }
> % 
> % +#define ATOMIC_ACQ_REL_LONG(NAME)					\
> % +static __inline void						 \
> % +atomic_##NAME##_acq_long(__volatile u_long *p, u_long v)		\
> % +{									\
> % +	atomic_##NAME##_long(p, v);					\
> % +	__do_dmb();							\
> % +}									\
> % +									\
> % +static __inline  void						 \
> % +atomic_##NAME##_rel_long(__volatile u_long *p, u_long v)		\
> % +{									\
> % +	__do_dmb();							\
> % +	atomic_##NAME##_long(p, v);					\
> % +}
> % +
> %  #define	ATOMIC_ACQ_REL(NAME, WIDTH)				 \
> %  static __inline  void						 \
> %  atomic_##NAME##_acq_##WIDTH(__volatile uint##WIDTH##_t *p, 
> uint##WIDTH##_t v)\
> 
> Small functions can be done like this, by duplicating the code for
> longs, and putting it in macros, but I think the above already has too
> much macro-ization.  Code like this is hard to read, and probably not
> even much shorter and easier to write, if you organize the wrappers
> in the best way.


Well that's a matter of point of view, that would be a lot of wrappers to 
write, wrong ?

> 
> % ...
> % @@ -489,9 +649,64 @@
> %  #define atomic_subtract_rel_32	atomic_subtract_32
> %  #define atomic_subtract_acq_32	atomic_subtract_32
> %  #define atomic_store_rel_32	atomic_store_32
> % +#define atomic_store_rel_long	atomic_store_long
> %  #define atomic_load_acq_32	atomic_load_32
> % +#define atomic_load_acq_long	atomic_load_long
> %  #undef __with_interrupts_disabled
> % 
> % +static __inline void
> % +atomic_add_long(volatile u_long *p, u_long v)
> % +{
> % +
> % +	atomic_add_32((volatile uint32_t *)p, (volatile uint32_t)v);
> % +}
> 
> Most of the previous changes seem to be for adding 64-bit longs.  Now I
> think we're back in the 32-bit long case, with wrappers like I want
> (more verbose than I want).
> 

Actually, there're 2 sets of arm atomic functions, because the earlier
revisions of the arm architectures lacked any decent way to do atomic
operations, so we have to do it another way. Those earlier arm revisions will
never support 64bits, so we can just use the 32bits versions.

> Casting v seems bogus.  How can a function parameter that is passed by
> value be volatile?  Old arm code has just 2 simimlar volatiles,
> both for the non-pointer parameter in 2 cmpset_32's.  Maybe it needs
> to be volatile in the asm that accesses it, but I doubt this, and
> casting it here doesn't make it volatile there.  amd64 only uses
> volatiles for pointed-to variables.

Indeed those casts are just the result of me not thinking before copy/pasting
code over and over, and they won't be committed.
> 
> % ...
> %  #endif /* _LOCORE */
> 
> LOCORE ifdefs in atomic.h are bogus, and were removed on amd64.  The
> comment on this one is backwards, at least in the main arm tree (the
> code says #ifndef, not #ifdef).  Adding C functions unused LOCORE part
> of the code ensure that it is unusable as well as unused in asm files.

Right, it'll be gone when I'll commit it.

Thanks a lot !

Olivier