linux-kernel - Re: [PATCH] nvdimm: Avoid wasting some memory.

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Sun, 4 Sep 2022 18:05:02 +0200
From:   Christophe JAILLET <christophe.jaillet@...adoo.fr>
To:     Dan Williams <dan.j.williams@...el.com>,
        Vishal Verma <vishal.l.verma@...el.com>,
        Dave Jiang <dave.jiang@...el.com>,
        Ira Weiny <ira.weiny@...el.com>
Cc:     linux-kernel@...r.kernel.org, kernel-janitors@...r.kernel.org,
        nvdimm@...ts.linux.dev
Subject: Re: [PATCH] nvdimm: Avoid wasting some memory.

Le 04/09/2022 à 16:38, Dan Williams a écrit :
> Christophe JAILLET wrote:
>> sizeof(struct btt_sb) is 4096.
>>
>> When using devm_kzalloc(), there is a small memory overhead and, on most
>> systems, this leads to 40 bytes of extra memory allocation.
>> So 5036 bytes are expected to be allocated.
>>
>> The memory allocator works with fixed size hunks of memory. In this case,
>> it will require 8192 bytes of memory because more than 4096 bytes are
>> required.
>>
>> In order to avoid wasting 4ko of memory, just use kzalloc() and add a
>> devm action to free it when needed.
>>
>> Signed-off-by: Christophe JAILLET <christophe.jaillet@...adoo.fr>
>> ---
>>   drivers/nvdimm/btt_devs.c | 17 ++++++++++++++++-
>>   1 file changed, 16 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/nvdimm/btt_devs.c b/drivers/nvdimm/btt_devs.c
>> index fabbb31f2c35..7b79fb0b0338 100644
>> --- a/drivers/nvdimm/btt_devs.c
>> +++ b/drivers/nvdimm/btt_devs.c
>> @@ -332,6 +332,11 @@ static int __nd_btt_probe(struct nd_btt *nd_btt,
>>   	return 0;
>>   }
>>   
>> +void nd_btt_free(void *data)
>> +{
>> +	kfree(data);
>> +}
>> +
>>   int nd_btt_probe(struct device *dev, struct nd_namespace_common *ndns)
>>   {
>>   	int rc;
>> @@ -356,7 +361,17 @@ int nd_btt_probe(struct device *dev, struct nd_namespace_common *ndns)
>>   	nvdimm_bus_unlock(&ndns->dev);
>>   	if (!btt_dev)
>>   		return -ENOMEM;
>> -	btt_sb = devm_kzalloc(dev, sizeof(*btt_sb), GFP_KERNEL);
>> +
>> +	/*
>> +	 * 'struct btt_sb' is 4096. Using devm_kzalloc() would waste 4 ko of
>> +	 * memory because, because of a small memory over head, 8192 bytes
>> +	 * would be allocated. So keep this kzalloc()+devm_add_action_or_reset()
>> +	 */
>> +	btt_sb = kzalloc(sizeof(*btt_sb), GFP_KERNEL);
>> +	rc = devm_add_action_or_reset(dev, nd_btt_free, btt_sb);
>> +	if (rc)
>> +		return rc;
> 
> Thanks for the analysis and the patch. However, shouldn't this be
> something that is addressed internal to devm_kzalloc() rather than
> open-coded at every potential call site?
> 

Hi,
it would be fine, but it is not that easy.
(read: any idea to implement it is welcomed :) )

I made a try a few weeks ago. See [1].
It triggered obvious issues spotted by 0day robot <lkp@...el.com>.

In fact, "making clever things" in devm_kmalloc() prevent using 
devm_kfree() (or would require some over-engineering).


Greg also argued that it is likely that devm_  allocated memory does not 
happen that much.


I posted today a few similar patches as the one above in different 
subsystem to get some feed-backs whether open-coding it in "interesting" 
places make sense or not.

Spotted such places is not that hard with a home made additional check 
in smatch.

CJ

[1]: 
https://lore.kernel.org/all/92ec2f78e8d38f68da95d9250cf3f86b2fbe78ad.1658570017.git.christophe.jaillet@wanadoo.fr/