[PATCH] kmalloc_index optimization(add kmalloc max size check)

[PATCH] kmalloc_index optimization(add kmalloc max size check)

[Bernard Zhao]

kmalloc size should never exceed KMALLOC_MAX_SIZE.
kmalloc_index realise if size is exceed KMALLOC_MAX_SIZE, e.g 64M,
kmalloc_index just return index 26, but never check with OS`s max
kmalloc config KMALLOC_MAX_SIZE. This index`s kmalloc caches maybe
not create in function create_kmalloc_caches.
We can throw an warninginfo in kmalloc at the beginning, instead of
being guaranteed by the buddy alloc behind.

Signed-off-by: Bernard Zhao <bernard@vivo.com>
---
 include/linux/slab.h | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/include/linux/slab.h b/include/linux/slab.h
index 6d45488..59b60d2 100644
--- a/include/linux/slab.h
+++ b/include/linux/slab.h
@@ -351,6 +351,10 @@ static __always_inline unsigned int kmalloc_index(size_t size)
 	if (!size)
 		return 0;
 
+	/* size should never exceed KMALLOC_MAX_SIZE. */
+	if (size > KMALLOC_MAX_SIZE)
+		WARN(1, "size exceed max kmalloc size!\n");
+
 	if (size <= KMALLOC_MIN_SIZE)
 		return KMALLOC_SHIFT_LOW;
 
-- 
2.7.4

Re: [PATCH] kmalloc_index optimization(add kmalloc max size check)

[Michal Hocko]

On Fri 17-04-20 00:09:35, Bernard Zhao wrote:
> kmalloc size should never exceed KMALLOC_MAX_SIZE.
> kmalloc_index realise if size is exceed KMALLOC_MAX_SIZE, e.g 64M,
> kmalloc_index just return index 26, but never check with OS`s max
> kmalloc config KMALLOC_MAX_SIZE. This index`s kmalloc caches maybe
> not create in function create_kmalloc_caches.
> We can throw an warninginfo in kmalloc at the beginning, instead of
> being guaranteed by the buddy alloc behind.

I am sorry but I do not follow. What does this patch optimizes? AFAICS,
it adds a branch for everybody for something that is highly unlikely
usage. Btw. we already do handle those impossible cases. We could argue
that BUG() is a bit harsh reaction but a lack of reports suggests this
is not a real problem in fact.

So what exactly do you want to achieve here?

> Signed-off-by: Bernard Zhao <bernard@vivo.com>
> ---
>  include/linux/slab.h | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/include/linux/slab.h b/include/linux/slab.h
> index 6d45488..59b60d2 100644
> --- a/include/linux/slab.h
> +++ b/include/linux/slab.h
> @@ -351,6 +351,10 @@ static __always_inline unsigned int kmalloc_index(size_t size)
>  	if (!size)
>  		return 0;
>  
> +	/* size should never exceed KMALLOC_MAX_SIZE. */
> +	if (size > KMALLOC_MAX_SIZE)
> +		WARN(1, "size exceed max kmalloc size!\n");
> +
>  	if (size <= KMALLOC_MIN_SIZE)
>  		return KMALLOC_SHIFT_LOW;
>  
> -- 
> 2.7.4
> 

-- 
Michal Hocko
SUSE Labs

Re:Re: [PATCH] kmalloc_index optimization(add kmalloc max size check)

[赵军奎]

From: Michal Hocko <mhocko@kernel.org>
Date: 2020-04-17 19:39:28
To:  Bernard Zhao <bernard@vivo.com>
Cc:  Christoph Lameter <cl@linux.com>,Pekka Enberg <penberg@kernel.org>,David Rientjes <rientjes@google.com>,Joonsoo Kim <iamjoonsoo.kim@lge.com>,Andrew Morton <akpm@linux-foundation.org>,linux-mm@kvack.org,linux-kernel@vger.kernel.org,kernel@vivo.com
Subject: Re: [PATCH] kmalloc_index optimization(add kmalloc max size check)>On Fri 17-04-20 00:09:35, Bernard Zhao wrote:
>> kmalloc size should never exceed KMALLOC_MAX_SIZE.
>> kmalloc_index realise if size is exceed KMALLOC_MAX_SIZE, e.g 64M,
>> kmalloc_index just return index 26, but never check with OS`s max
>> kmalloc config KMALLOC_MAX_SIZE. This index`s kmalloc caches maybe
>> not create in function create_kmalloc_caches.
>> We can throw an warninginfo in kmalloc at the beginning, instead of
>> being guaranteed by the buddy alloc behind.
>
>I am sorry but I do not follow. What does this patch optimizes? AFAICS,
>it adds a branch for everybody for something that is highly unlikely
>usage. Btw. we already do handle those impossible cases. We could argue
>that BUG() is a bit harsh reaction but a lack of reports suggests this
>is not a real problem in fact.
>
>So what exactly do you want to achieve here?
>

I'm not sure if my understanding has a gap. I think this should never happen. 
And maybe we could do some guarantees. This may be very effective when debugging.
The current code processing can only be found if the buddy application fails and returns 
afterwards. 
The first version I used when this happened was  BUG, but when I submitted the patch, 
an alert happened, prompting me to change to warn, so I posted a revision of the warn.
If this happends, kernel will throw an warning info.

>> Signed-off-by: Bernard Zhao <bernard@vivo.com>
>> ---
>>  include/linux/slab.h | 4 ++++
>>  1 file changed, 4 insertions(+)
>> 
>> diff --git a/include/linux/slab.h b/include/linux/slab.h
>> index 6d45488..59b60d2 100644
>> --- a/include/linux/slab.h
>> +++ b/include/linux/slab.h
>> @@ -351,6 +351,10 @@ static __always_inline unsigned int kmalloc_index(size_t size)
>>  	if (!size)
>>  		return 0;
>>  
>> +	/* size should never exceed KMALLOC_MAX_SIZE. */
>> +	if (size > KMALLOC_MAX_SIZE)
>> +		WARN(1, "size exceed max kmalloc size!\n");
>> +
>>  	if (size <= KMALLOC_MIN_SIZE)
>>  		return KMALLOC_SHIFT_LOW;
>>  
>> -- 
>> 2.7.4
>> 
>
>-- 
>Michal Hocko
>SUSE Labs

Re: Re: [PATCH] kmalloc_index optimization(add kmalloc max size check)

[Michal Hocko]

On Fri 17-04-20 20:17:19, 赵军奎 wrote:
> 
> 
> From: Michal Hocko <mhocko@kernel.org>
> Date: 2020-04-17 19:39:28
> To:  Bernard Zhao <bernard@vivo.com>
> Cc:  Christoph Lameter <cl@linux.com>,Pekka Enberg <penberg@kernel.org>,David Rientjes <rientjes@google.com>,Joonsoo Kim <iamjoonsoo.kim@lge.com>,Andrew Morton <akpm@linux-foundation.org>,linux-mm@kvack.org,linux-kernel@vger.kernel.org,kernel@vivo.com
> Subject: Re: [PATCH] kmalloc_index optimization(add kmalloc max size check)>On Fri 17-04-20 00:09:35, Bernard Zhao wrote:
> >> kmalloc size should never exceed KMALLOC_MAX_SIZE.
> >> kmalloc_index realise if size is exceed KMALLOC_MAX_SIZE, e.g 64M,
> >> kmalloc_index just return index 26, but never check with OS`s max
> >> kmalloc config KMALLOC_MAX_SIZE. This index`s kmalloc caches maybe
> >> not create in function create_kmalloc_caches.
> >> We can throw an warninginfo in kmalloc at the beginning, instead of
> >> being guaranteed by the buddy alloc behind.
> >
> >I am sorry but I do not follow. What does this patch optimizes? AFAICS,
> >it adds a branch for everybody for something that is highly unlikely
> >usage. Btw. we already do handle those impossible cases. We could argue
> >that BUG() is a bit harsh reaction but a lack of reports suggests this
> >is not a real problem in fact.
> >
> >So what exactly do you want to achieve here?
> >
> 
> I'm not sure if my understanding has a gap. I think this should never happen. 

Yes. Have a look at the code and how all existing sizes map to an index
with a BUG() fallback so this is already handled. As I've said the
existing BUG() is far from optimal but a complete lack of bug reports
hitting this mark suggests this path is not really triggered.

And I do have objection to your patch. Because a) the description
doesn't state the problem which it is fixing and b) the patch adds a
test which everybody going this path has to evaluate and which should
never trigger. So despite your subject line, there is no actual
optimization but quite contrary.

-- 
Michal Hocko
SUSE Labs

Re: [PATCH] kmalloc_index optimization(add kmalloc max size check)

[Christopher Lameter]

On Fri, 17 Apr 2020, Bernard Zhao wrote:

> kmalloc size should never exceed KMALLOC_MAX_SIZE.
> kmalloc_index realise if size is exceed KMALLOC_MAX_SIZE, e.g 64M,
> kmalloc_index just return index 26, but never check with OS`s max
> kmalloc config KMALLOC_MAX_SIZE. This index`s kmalloc caches maybe
> not create in function create_kmalloc_caches.
> We can throw an warninginfo in kmalloc at the beginning, instead of
> being guaranteed by the buddy alloc behind.

kmalloc_index(0 already bugs if the allocation is more than 64M


...

   if (size <=  64 * 1024 * 1024) return 26;
        BUG();


You could modify that to check for KMALLOC_MAX_SIZE with some more
conditionals but then kmalloc_index) is written so that the compiler gets
constant folding right.

If you have a patch like that then please verify that all c compilers in
use perform correct constant folding and do not add unnecessary code.

Re:Re: [PATCH] kmalloc_index optimization(add kmalloc max size check)

[赵军奎]

From: Christopher Lameter <cl@linux.com>
Date: 2020-04-17 23:59:00
To:  Bernard Zhao <bernard@vivo.com>
Cc:  Pekka Enberg <penberg@kernel.org>,David Rientjes <rientjes@google.com>,Joonsoo Kim <iamjoonsoo.kim@lge.com>,Andrew Morton <akpm@linux-foundation.org>,linux-mm@kvack.org,linux-kernel@vger.kernel.org,kernel@vivo.com
Subject: Re: [PATCH] kmalloc_index optimization(add kmalloc max size check)>On Fri, 17 Apr 2020, Bernard Zhao wrote:
>
>> kmalloc size should never exceed KMALLOC_MAX_SIZE.
>> kmalloc_index realise if size is exceed KMALLOC_MAX_SIZE, e.g 64M,
>> kmalloc_index just return index 26, but never check with OS`s max
>> kmalloc config KMALLOC_MAX_SIZE. This index`s kmalloc caches maybe
>> not create in function create_kmalloc_caches.
>> We can throw an warninginfo in kmalloc at the beginning, instead of
>> being guaranteed by the buddy alloc behind.
>
>kmalloc_index(0 already bugs if the allocation is more than 64M
>
>
>...
>
>   if (size <=  64 * 1024 * 1024) return 26;
>        BUG();
>
>
>You could modify that to check for KMALLOC_MAX_SIZE with some more
>conditionals but then kmalloc_index) is written so that the compiler gets
>constant folding right.
>
>If you have a patch like that then please verify that all c compilers in
>use perform correct constant folding and do not add unnecessary code.
>
>

Sorry for the misunderstanding.
What I meant was that the “if”in kmalloc_index should be consistent 
with KMALLOC_MAX_SIZE. For example, if the MAX_ZONEORDER configured 
by the kernel is 11, which is 4M, then size >4M should trigger BUG(). If the 
configuration is smaller, e.g MAX_ZONEORDER is 9, 1M, then the size > 1M 
should be BUG. 
But the current code is not, kmalloc_index will only be BUG() when it exceeds 64M.

Re:Re: [PATCH] kmalloc_index optimization(add kmalloc max size check)

[Christopher Lameter]

[-- Attachment #1: Type: text/plain, Size: 241 bytes --]

On Sat, 18 Apr 2020, 赵军奎 wrote:

> Sorry for the misunderstanding.

What misunderstanding?

> But the current code is not, kmalloc_index will only be BUG() when it exceeds 64M.

Yes that is what you may want to fix as I said.

Re:Re:Re: [PATCH] kmalloc_index optimization(add kmalloc max size check)

[赵军奎]

发件人：Christopher Lameter <cl@linux.com>
发送日期：2020-04-20 00:42:55
收件人："赵军奎" <bernard@vivo.com>
抄送人：Pekka Enberg <penberg@kernel.org>,David Rientjes <rientjes@google.com>,Joonsoo Kim <iamjoonsoo.kim@lge.com>,Andrew Morton <akpm@linux-foundation.org>,linux-mm@kvack.org,linux-kernel@vger.kernel.org,kernel@vivo.com
主题：Re:Re: [PATCH] kmalloc_index optimization(add kmalloc max size check)>On Sat, 18 Apr 2020, 赵军奎 wrote:
>
>> Sorry for the misunderstanding.
>
>What misunderstanding?
There is no gap now.
>
>> But the current code is not, kmalloc_index will only be BUG() when it exceeds 64M.
>
>Yes that is what you may want to fix as I said.

>You could modify that to check for KMALLOC_MAX_SIZE with some more
>conditionals but then kmalloc_index) is written so that the compiler gets
>constant folding right.
For this point, I am afraid i didn`t catch your idea.
I am not sure how to modify it....
Is there some similar code implementation in the kernel？

BR//bernard

Re: [PATCH] kmalloc_index optimization(add kmalloc max size check)

[Vlastimil Babka]

On 4/17/20 9:09 AM, Bernard Zhao wrote:
> kmalloc size should never exceed KMALLOC_MAX_SIZE.
> kmalloc_index realise if size is exceed KMALLOC_MAX_SIZE, e.g 64M,
> kmalloc_index just return index 26, but never check with OS`s max
> kmalloc config KMALLOC_MAX_SIZE. This index`s kmalloc caches maybe
> not create in function create_kmalloc_caches.
> We can throw an warninginfo in kmalloc at the beginning, instead of
> being guaranteed by the buddy alloc behind.
> 
> Signed-off-by: Bernard Zhao <bernard@vivo.com>

kmalloc_index() is only called from kmalloc() and kmalloc_node() for
compile-time constant sizes up to KMALLOC_MAX_CACHE_SIZE, which is smaller
(SLUB, SLOB) or equal (SLAB) than KMALLOC_MAX_SIZE. So your patch is effectively
a no-op and we better shouldn't tease the compiler too much, so that
kmalloc_index() stays fully compile-time evaluated.

> ---
>  include/linux/slab.h | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/include/linux/slab.h b/include/linux/slab.h
> index 6d45488..59b60d2 100644
> --- a/include/linux/slab.h
> +++ b/include/linux/slab.h
> @@ -351,6 +351,10 @@ static __always_inline unsigned int kmalloc_index(size_t size)
>  	if (!size)
>  		return 0;
>  
> +	/* size should never exceed KMALLOC_MAX_SIZE. */
> +	if (size > KMALLOC_MAX_SIZE)
> +		WARN(1, "size exceed max kmalloc size!\n");
> +
>  	if (size <= KMALLOC_MIN_SIZE)
>  		return KMALLOC_SHIFT_LOW;
>  
> 

Re:Re:Re: [PATCH] kmalloc_index optimization(add kmalloc max size check)

[Christopher Lameter]

[-- Attachment #1: Type: text/plain, Size: 473 bytes --]

On Tue, 21 Apr 2020, 赵军奎 wrote:

> >You could modify that to check for KMALLOC_MAX_SIZE with some more
> >conditionals but then kmalloc_index) is written so that the compiler gets
> >constant folding right.
> For this point, I am afraid i didn`t catch your idea.
> I am not sure how to modify it....
> Is there some similar code implementation in the kernel？

No. That is where you creativity and ingenuity come in. Otherwise you need
to leave it alone.