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

From: 赵军奎 <bernard@vivo.com>
To: Michal Hocko <mhocko@kernel.org>
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:Re: [PATCH] kmalloc_index optimization(add kmalloc max size check)
Date: Fri, 17 Apr 2020 20:17:19 +0800 (GMT+08:00)	[thread overview]
Message-ID: <AOsA0QC2CNKuIHXhFI2eG4oJ.3.1587125839812.Hmail.bernard@vivo.com> (raw)
In-Reply-To: <20200417113928.GL26707@dhcp22.suse.cz>

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