should cpus_in_xmon be volatile?

[Linas Vepstas]

On Thu, Nov 18, 2004 at 02:52:33PM -0800, Dave Hansen was heard to remark:
> I'm getting a warning (one of many) during a build of 2.6.10-rc2-mm2:
> 
> memhotplug/arch/ppc64/xmon/xmon.c: In function `xmon_core':
> memhotplug/arch/ppc64/xmon/xmon.c:401: warning: passing arg 1 of
> `__cpus_weight' discards qualifiers from pointer target type
> 
> It's this chunk of code:
>         
>         for (timeout = 100000000; timeout != 0; --timeout)
>         	if (cpus_weight(cpus_in_xmon) >= ncpus)
>                 	break;
> 
> Is that warning because cpus_in_xmon is volatile and __cpus_weight()
> doesn't take a pointer to a volatile type?  
> 
> I do notice that none of the test/set_bit() functions have volatile
> types, and I have the feeling this is because they take pointers to
> being with.  Does the fact that since __cpus_weight() takes a pointer
> that cpus_in_xmon doesn't really need to be declared volatile?


Well, to make >>that particular<< loop work correctly, the volatile is not
needed. Why? Because cpus_weight() is extern __bitmap_weight() and since
its extern, the compiler must be definition invoke it each time in the
loop, since the compiler must assume that the called routine is changing 
the value of the thing being pointed at. i.e. the call has a side-effect. 
So even with -O6 and without the volatile, that loop will compile correctly. 

However, if someone changed the extern __bitmap_weight() to be 
inline __bitmap_weight(), then the compiler could potentially see that 
it had no side effects, and decide to optimize away the entire loop.
If __bitmap_weight() was changed to be inline, then it would also
need to be changed to have a volatile argument. 

What the compiler is complaing about here is that it knows that 
__bitmap_weight() is not treating the arg as if it were volatile, and
thus it might work incorrectly (e.g. if it went to sleep, waiting 
for a bit to change; then it would sleep forever).

Since we know what __bitmap_weight() does, we know it doesn't need to 
be volatile, so that's OK.  Since we know that in other places we 
probably need to have cpus_in_xmon be volatile, we should leave it
volatile.  (for example, its used in othr places that I think might
break if it weren't declared volatile; not sure, didn't look that
carefully)  The proper solution is then to cast away volatile
before calling cpus_weight. 

That, at least, is how I see it.

--linas