[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <e82700e0-c27e-926f-cf07-11620afffea4@wanadoo.fr>
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/
Powered by blists - more mailing lists