AA's 00_backout_gcc_3-0-patch-1

AA's 00_backout_gcc_3-0-patch-1

[Marcelo Tosatti]

Andrea,

For what reason are you doing this?

diff -urN 2.4.6pre3/kernel/timer.c backoutgcc/kernel/timer.c
--- 2.4.6pre3/kernel/timer.c	Wed Jun 13 04:02:52 2001
+++ backoutgcc/kernel/timer.c	Wed Jun 13 15:49:13 2001
@@ -32,7 +32,7 @@
 long tick = (1000000 + HZ/2) / HZ;	/* timer interrupt period */

 /* The current time */
-struct timeval xtime __attribute__ ((aligned (16)));
+volatile struct timeval xtime __attribute__ ((aligned (16)));

 /* Don't completely fail for HZ > 500.  */
 int tickadj = 500/HZ ? : 1;		/* microsecs */

Re: AA's 00_backout_gcc_3-0-patch-1

[Andrea Arcangeli]

On Mon, May 26, 2003 at 07:30:44PM -0300, Marcelo Tosatti wrote:
> 
> Andrea,
> 
> For what reason are you doing this?

yes, if you ask the gcc developers any piece of memory that will change
under gcc, despite gcc has no clue that it can change under it, MUST (in
RFC sense) be marked volatile.

this avoids possible crashes for example with a switch(xtime.tv_sec)
case 0,1,2,3,4 etc.. gcc could generate an hash to get the cases fast and
verify the xtime.tv_sec is <= 4 before derferencing tv_sec again. But
tv_sec could change under gcc and it would jump to random.

However with xtime itself, it would be very rare to get failures like
the above, we're not going to compare xtime that often in a gcc-crashing
way.

Still xtime is one obvious target for being marked volatile, and for
correctness I like it. Performance shouldn't matter.

Overall in kernel we disagreed to follow the MUST requrested by the gcc
developers, we often want to do comparisons of variables out of locks to
know if we need to take the lock and work on a garbage collection or
stuff like that and we for sure don't want to mark those variables
volatile since they must be cached and not spilled all the time, under
the locks. Linus as well was against using volatile for every piece of
memory that can change under gcc. The decision is been basically to
outsmart gcc in choosing if gcc has rights to generate kernel crashing
code or not. This makes kernel developement even more difficult since
you've to imagine whatever smart thing gcc can do with your not
serialized code to know if you're forced to mark the stuff volatile, but
it'll generate the very best performance.

As for xtime since it won't hurt performance, and since it's an obvious
volatile candidate I preferred to take the obviously safe approch. I
prefer to take the outsmart-gcc-optimizations way, only when it is
worthwhile.

> 
> diff -urN 2.4.6pre3/kernel/timer.c backoutgcc/kernel/timer.c
> --- 2.4.6pre3/kernel/timer.c	Wed Jun 13 04:02:52 2001
> +++ backoutgcc/kernel/timer.c	Wed Jun 13 15:49:13 2001
> @@ -32,7 +32,7 @@
>  long tick = (1000000 + HZ/2) / HZ;	/* timer interrupt period */
> 
>  /* The current time */
> -struct timeval xtime __attribute__ ((aligned (16)));
> +volatile struct timeval xtime __attribute__ ((aligned (16)));
> 
>  /* Don't completely fail for HZ > 500.  */
>  int tickadj = 500/HZ ? : 1;		/* microsecs */
> 


Andrea

Re: AA's 00_backout_gcc_3-0-patch-1

[J.A. Magallon]

On 05.27, Andrea Arcangeli wrote:
> On Mon, May 26, 2003 at 07:30:44PM -0300, Marcelo Tosatti wrote:
[...]
> 
> Overall in kernel we disagreed to follow the MUST requrested by the gcc
> developers, we often want to do comparisons of variables out of locks to
> know if we need to take the lock and work on a garbage collection or
> stuff like that and we for sure don't want to mark those variables
> volatile since they must be cached and not spilled all the time, under
> the locks. Linus as well was against using volatile for every piece of
> memory that can change under gcc. The decision is been basically to
> outsmart gcc in choosing if gcc has rights to generate kernel crashing
> code or not. This makes kernel developement even more difficult since
> you've to imagine whatever smart thing gcc can do with your not
> serialized code to know if you're forced to mark the stuff volatile, but
> it'll generate the very best performance.
> 

So you are telling that you are allowed to do something like:

int* a = 0x320;

for (i=1000 samples)
	v[i] = *a;

in kernel code and you trust gcc to not optimize the loop away ??
What is volatile is volatile...

-- 
J.A. Magallon <jamagallon@able.es>      \                 Software is like sex:
werewolf.able.es                         \           It's better when it's free
Mandrake Linux release 9.2 (Cooker) for i586
Linux 2.4.21-rc3-jam1 (gcc 3.2.3 (Mandrake Linux 9.2 3.2.3-1mdk))

Re: AA's 00_backout_gcc_3-0-patch-1

[Andrea Arcangeli]

On Tue, May 27, 2003 at 01:15:48AM +0200, J.A. Magallon wrote:
> 
> On 05.27, Andrea Arcangeli wrote:
> > On Mon, May 26, 2003 at 07:30:44PM -0300, Marcelo Tosatti wrote:
> [...]
> > 
> > Overall in kernel we disagreed to follow the MUST requrested by the gcc
> > developers, we often want to do comparisons of variables out of locks to
> > know if we need to take the lock and work on a garbage collection or
> > stuff like that and we for sure don't want to mark those variables
> > volatile since they must be cached and not spilled all the time, under
> > the locks. Linus as well was against using volatile for every piece of
> > memory that can change under gcc. The decision is been basically to
> > outsmart gcc in choosing if gcc has rights to generate kernel crashing
> > code or not. This makes kernel developement even more difficult since
> > you've to imagine whatever smart thing gcc can do with your not
> > serialized code to know if you're forced to mark the stuff volatile, but
> > it'll generate the very best performance.
> > 
> 
> So you are telling that you are allowed to do something like:
> 
> int* a = 0x320;
> 
> for (i=1000 samples)
> 	v[i] = *a;
> 
> in kernel code and you trust gcc to not optimize the loop away ??
> What is volatile is volatile...

probably I didn't explained correctly but it's not really a matter of
optimizations, it's a matter of "kernel crashing gcc produced asm". I
don't expect gcc to generate kernel crashing code from the above, no
matter if the ram at virtual address 0x320 changes or not. But of course
I expect gcc to optimize the above and to cache the result of *a and to
read from 0x320 only once. Regardless it won't kernel-crash, that's the
bit that matters to me.

to clarify further what is the stuff that we've to care about: if you
wrote this instead of the above:

	int* a = 0x320;

	switch(*a) {

	case 0:
		xx(a);
		break;
	case 1:
		yy(a);
		break;
	default:
		zz(a);
		break;

	}

then you should definitely put a volatile before "int * a" or the kernel
could crash randomly while excuting the above if the ram at address
0x320 can change under gcc (despite both xx,yy,zz would all be robust
internally to whatever value a could have, and xx,yy,zz could even take
a spinlock internally before re-reading a, still the kernel could crash).

Of course the suggestion made from the gcc developers would make it
impossible to run into the kernel crashing asm, because they would
always ask us to define stuff as volatile. We don't obey to that rule so
we must imagine how gcc could ever compile the gcc code _every_ time we
deal with non volatile marked memory that can change under gcc (i.e.
outside locks).

At the moment it seems the only potential kernel crashing optimization
in gcc is while dealing with switch, but it wasn't clear if there could
be more.

Andrea