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 <[email protected]>
---
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
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 <[email protected]>
> ---
> 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
From: Michal Hocko <[email protected]>
Date: 2020-04-17 19:39:28
To: Bernard Zhao <[email protected]>
Cc: Christoph Lameter <[email protected]>,Pekka Enberg <[email protected]>,David Rientjes <[email protected]>,Joonsoo Kim <[email protected]>,Andrew Morton <[email protected]>,[email protected],[email protected],[email protected]
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 <[email protected]>
>> ---
>> 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
On Fri 17-04-20 20:17:19, 赵军奎 wrote:
>
>
> From: Michal Hocko <[email protected]>
> Date: 2020-04-17 19:39:28
> To: Bernard Zhao <[email protected]>
> Cc: Christoph Lameter <[email protected]>,Pekka Enberg <[email protected]>,David Rientjes <[email protected]>,Joonsoo Kim <[email protected]>,Andrew Morton <[email protected]>,[email protected],[email protected],[email protected]
> 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
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.
From: Christopher Lameter <[email protected]>
Date: 2020-04-17 23:59:00
To: Bernard Zhao <[email protected]>
Cc: Pekka Enberg <[email protected]>,David Rientjes <[email protected]>,Joonsoo Kim <[email protected]>,Andrew Morton <[email protected]>,[email protected],[email protected],[email protected]
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.
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.
发件人：Christopher Lameter <[email protected]>
发送日期：2020-04-20 00:42:55
收件人："赵军奎" <[email protected]>
抄送人：Pekka Enberg <[email protected]>,David Rientjes <[email protected]>,Joonsoo Kim <[email protected]>,Andrew Morton <[email protected]>,[email protected],[email protected],[email protected]
主题：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
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 <[email protected]>
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;
>
>
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.