Re: threads-max observe limits

[Heinrich Schuchardt]

On 9/22/19 8:58 AM, Michal Hocko wrote:
> On Thu 19-09-19 14:33:24, Eric W. Biederman wrote:
>> Michal Hocko <mhocko@xxxxxxxxxx> writes:
>>
>>> On Tue 17-09-19 12:26:18, Eric W. Biederman wrote:
> [...]
>>>> Michal is it a very small effect your customers are seeing?
>>>> Is it another bug somewhere else?
>>>
>>> I am still trying to get more information. Reportedly they see a
>>> different auto tuned limit between two kernel versions which results in
>>> an applicaton complaining. As already mentioned this might be a side
>>> effect of something else and this is not yet fully analyzed. My main
>>> point for bringing up this discussion is ...
>>
>> Please this sounds like the kind of issue that will reveal something
>> deeper about what is going on.
>
> Yes, I have pushed for that information even before starting discussion
> here. I haven't heard from the customer yet unfortunatelly.
>
>>>> b) Not being able to bump threads_max to the physical limit of
>>>>    the machine is very clearly a regression.
>>>
>>> ... exactly this part. The changelog of the respective patch doesn't
>>> really exaplain why it is needed except of "it sounds like a good idea
>>> to be consistent".
>>
>> I suggest doing a partial revert to just:
>>
>> diff --git a/kernel/fork.c b/kernel/fork.c
>> index 7a74ade4e7d6..de8264ea34a7 100644
>> --- a/kernel/fork.c
>> +++ b/kernel/fork.c
>> @@ -2943,7 +2943,7 @@ int sysctl_max_threads(struct ctl_table *table, int write,
>>  	if (ret || !write)
>>  		return ret;
>>
>> -	set_max_threads(threads);
>> +	max_threads = threads;
>>
>>  	return 0;
>>  }
>>
>> proc_dointvec_minmax limiting the values to MIN_THREADS and MAX_THREADS
>> is justifiable.  Those are the minimum and maximum values the kernel can
>> function with.
>
> MAX_THREADS limit makes perfect sense because crossing it reflects a
> real constrain for correctness. MIN_THREADS, on the other hand, is only
> the low gate for auto tuning to not come with an unbootable system. I do
> not think we should jump into the admin call on the lower bound. There
> might be a good reason to restrict the number of threads to 1. So here
> is what I would like to go with.

Did this patch when applied to the customer's kernel solve any problem?

WebSphere MQ is a messaging application. If it hits the current limits
of threads-max, there is a bug in the software or in the way that it has
been set up at the customer. Instead of messing around with the kernel
the application should be fixed.

With this patch you allow administrators to set values that will crash
their system. And they will not even have a way to find out the limits
which he should adhere to. So expect a lot of systems to be downed this way.

Best regards

Heinrich

>
>>From 711000fdc243b6bc68a92f9ef0017ae495086d39 Mon Sep 17 00:00:00 2001
> From: Michal Hocko <mhocko@xxxxxxxx>
> Date: Sun, 22 Sep 2019 08:45:28 +0200
> Subject: [PATCH] kernel/sysctl.c: do not override max_threads provided by
>  userspace
>
> Partially revert 16db3d3f1170 ("kernel/sysctl.c: threads-max observe limits")
> because the patch is causing a regression to any workload which needs to
> override the auto-tuning of the limit provided by kernel.
>
> set_max_threads is implementing a boot time guesstimate to provide a
> sensible limit of the concurrently running threads so that runaways will
> not deplete all the memory. This is a good thing in general but there
> are workloads which might need to increase this limit for an application
> to run (reportedly WebSpher MQ is affected) and that is simply not
> possible after the mentioned change. It is also very dubious to override
> an admin decision by an estimation that doesn't have any direct relation
> to correctness of the kernel operation.
>
> Fix this by dropping set_max_threads from sysctl_max_threads so any
> value is accepted as long as it fits into MAX_THREADS which is important
> to check because allowing more threads could break internal robust futex
> restriction. While at it, do not use MIN_THREADS as the lower boundary
> because it is also only a heuristic for automatic estimation and admin
> might have a good reason to stop new threads to be created even when
> below this limit.
>
> Fixes: 16db3d3f1170 ("kernel/sysctl.c: threads-max observe limits")
> Cc: stable
> Signed-off-by: Michal Hocko <mhocko@xxxxxxxx>
> ---
>  kernel/fork.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/kernel/fork.c b/kernel/fork.c
> index 2852d0e76ea3..ef865be37e98 100644
> --- a/kernel/fork.c
> +++ b/kernel/fork.c
> @@ -2929,7 +2929,7 @@ int sysctl_max_threads(struct ctl_table *table, int write,
>  	struct ctl_table t;
>  	int ret;
>  	int threads = max_threads;
> -	int min = MIN_THREADS;
> +	int min = 1;
>  	int max = MAX_THREADS;
>
>  	t = *table;
> @@ -2941,7 +2941,7 @@ int sysctl_max_threads(struct ctl_table *table, int write,
>  	if (ret || !write)
>  		return ret;
>
> -	set_max_threads(threads);
> +	max_threads = threads;
>
>  	return 0;
>  }
>