fork pagesize patch

fork pagesize patch

[David Howells]

Hi Linus,

You seem to have turned:

	+#if THREAD_SIZE >= PAGE_SIZE
		max_threads = mempages / (THREAD_SIZE/PAGE_SIZE) / 8;
	+#else
	+	max_threads = mempages / 8;
	+#endif
	+

Into:

	if (THREAD_SIZE >= PAGE_SIZE)
		max_threads = mempages / (THREAD_SIZE/PAGE_SIZE) / 8;
	else
		max_threads = mempages / 8;

Please don't do that. What you've done causes a divide-by-zero error to be
emitted by the compiler if PAGE_SIZE > THREAD_SIZE. That's why I used the
preprocessor in the first place.

On FRV arch the minimum page size the MMU permits (when there is an MMU) is
16KB; however, that's rather a lot of stack space for the kernel, so I only
allocate half that.

David

Re: fork pagesize patch

[Linus Torvalds]

On Tue, 16 Nov 2004, David Howells wrote:
> 
> You seem to have turned:

No, Andrew probably did.

> 
> 	+#if THREAD_SIZE >= PAGE_SIZE
> 		max_threads = mempages / (THREAD_SIZE/PAGE_SIZE) / 8;
> 	+#else
> 	+	max_threads = mempages / 8;
> 	+#endif
> 	+
> 
> Into:
> 
> 	if (THREAD_SIZE >= PAGE_SIZE)
> 		max_threads = mempages / (THREAD_SIZE/PAGE_SIZE) / 8;
> 	else
> 		max_threads = mempages / 8;
> 
> Please don't do that. What you've done causes a divide-by-zero error to be
> emitted by the compiler if PAGE_SIZE > THREAD_SIZE. That's why I used the
> preprocessor in the first place.

What kind of broken compiler are _you_ using? Fix your compiler.

If THREAD_SIZE is smaller than PAGE_SIZE, then the compiler IS NOT ALLOWED 
to do the divide-by-zero. Your compiler is so incredibly broken that this 
is not even worth fixing. Complain to the gcc guys.

		Linus

Re: fork pagesize patch

[David Howells]

> > Please don't do that. What you've done causes a divide-by-zero error to be
> > emitted by the compiler if PAGE_SIZE > THREAD_SIZE. That's why I used the
> > preprocessor in the first place.
> 
> What kind of broken compiler are _you_ using? Fix your compiler.

Sorry... I meant warning not error. It doesn't actually stop it building a
working kernel, but gcc _does_ complain, and not unreasonably, I think.

David

Re: fork pagesize patch

[Linus Torvalds]

On Tue, 16 Nov 2004, David Howells wrote:
> 
> > > Please don't do that. What you've done causes a divide-by-zero error to be
> > > emitted by the compiler if PAGE_SIZE > THREAD_SIZE. That's why I used the
> > > preprocessor in the first place.
> > 
> > What kind of broken compiler are _you_ using? Fix your compiler.
> 
> Sorry... I meant warning not error. It doesn't actually stop it building a
> working kernel, but gcc _does_ complain, and not unreasonably, I think.

I think it _is_ unreasonable. It's like doing

	if (a)
		x /= a;

and the compiler complaining that "a" can be zero. The above is perfectly
reasonable code and may well have a constant zero as part of inlining or
macro expansion. A compiler that does that is being silly. It's being
_especially_ silly since it evaluates to

	if (0)
		..

at compile time in this case. Yeah, yeah, sparse gets it wrong too, but 
only if the left-hand side is zero as well.

But yes, at least the compiler isn't totally buggy if it's just a warning. 
After all, you can warn about anything you like ;)

Anyway, to make it not warn, why not change it to

	max_threads = mempages / (8*THREAD_SIZE/PAGE_SIZE);

instead, and be done with it? If the thread size is _that_ small that we 
still divide by zero, color me impressed.

		Linus

Re: fork pagesize patch

[David Howells]

> > Sorry... I meant warning not error. It doesn't actually stop it building a
> > working kernel, but gcc _does_ complain, and not unreasonably, I think.
>
> I think it _is_ unreasonable. It's like doing
>
> 	if (a)
> 		x /= a;

Doing it with variables is not exactly the same. The compiler has been told to
optimise arithmetic on constants, and as such it has to represent a div-by-0
result, which obviously it can't.

Now, I agree that given the if-condition automatically precludes the div-by-0,
but maybe that's asking too much. Perhaps I should go and raise hell over it
with the gcc posse.

> Anyway, to make it not warn, why not change it to
>
> 	max_threads = mempages / (8*THREAD_SIZE/PAGE_SIZE);
>
> instead, and be done with it?

And drop the conditional entirely? I can go along with that.

> If the thread size is _that_ small that we still divide by zero, color me
> impressed.

Not unless you spell colour correctly:-)

David

Re: fork pagesize patch

[Linus Torvalds]

On Tue, 16 Nov 2004, David Howells wrote:
> >
> > I think it _is_ unreasonable. It's like doing
> >
> > 	if (a)
> > 		x /= a;
> 
> Doing it with variables is not exactly the same. The compiler has been told to
> optimise arithmetic on constants, and as such it has to represent a div-by-0
> result, which obviously it can't.

But you snipped the part where the above source code _does_ end up being 
done on constants - in macro expansion and in inline functions. So the 
compiler really _can_ have a constant zero in the divide, and it really 
_can_ come from perfectly normal code. 

In fact, maybe code like the kernel had.

> > Anyway, to make it not warn, why not change it to
> >
> > 	max_threads = mempages / (8*THREAD_SIZE/PAGE_SIZE);
> >
> > instead, and be done with it?
> 
> And drop the conditional entirely? I can go along with that.

Right. It looks like the obvious thing to do, and is really what the code 
_tried_ to do in the first place.

		Linus