Re: [PATCH] parisc,metag: Do not hardcode maximum userspace stack size

[James Hogan]

Hi Helge,

On 04/05/14 08:28, Helge Deller wrote:
> On 05/02/2014 04:48 PM, James Bottomley wrote:
>> On Fri, 2014-05-02 at 12:54 +0100, James Hogan wrote:
>>> On 01/05/14 18:50, James Bottomley wrote:
>>>>
>>>>> +
>>>>> +config MAX_STACK_SIZE_MB
>>>>> +	int "Maximum user stack size (MB)"
>>>>> +	default 80
>>>>> +	range 8 256 if METAG
>>>>> +	range 8 2048
>>>>> +	depends on STACK_GROWSUP
>>>>> +	help
>>>>> +	  This is the maximum stack size in Megabytes in the VM layout of user
>>>>> +	  processes when the stack grows upwards (currently only on parisc and
>>>>> +	  metag arch). The stack will be located at the highest memory address
>>>>> +	  minus the given value, unless the RLIMIT_STACK hard limit is changed
>>>>> +	  to a smaller value in which case that is used.
>>>>> +
>>>>> +	  A sane initial value is 80 MB.
>>>>
>>>> There's one final issue with this: placement of the stack only really
>>>> matters on 32 bits.  We have three expanding memory areas: stack, heap
>>>> and maps.  On 64 bits these are placed well separated from each other on
>>>> 64 bits, so an artificial limit like this doesn't matter.
>>>
>>> Does the following fixup diff look reasonable? It forces
>>> MAX_STACK_SIZE_MB to 1024 and hides the Kconfig option for 64BIT,
>>> effectively leaving the behaviour unchanged in that case.
>>>
>>> diff --git a/mm/Kconfig b/mm/Kconfig
>>> index e80075979530..b0307f737bd7 100644
>>> --- a/mm/Kconfig
>>> +++ b/mm/Kconfig
>>> @@ -583,7 +583,8 @@ config GENERIC_EARLY_IOREMAP
>>>  	bool
>>>
>>>  config MAX_STACK_SIZE_MB
>>> -	int "Maximum user stack size (MB)"
>>> +	int "Maximum user stack size (MB)" if !64BIT
>>> +	default 1024 if 64BIT
>>>  	default 80
>>>  	range 8 256 if METAG
>>>  	range 8 2048
>>
>> Yes, I think that's probably correct ... 
> 
> No, it's not correct.
> It will then choose then a 1GB stack for compat tasks on 64bit kernel.

Sorry for the delay (I had most of last week off sick and still catching
up).

That's a good point. It makes me think the best way to handle it is in a
new definition in asm/processor.h, maybe STACK_SIZE_MAX. Does something
like this look better? This patch isn't getting any cleaner
unfortunately.