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 <[email protected]>
---
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;
+
rc = __nd_btt_probe(to_nd_btt(btt_dev), ndns, btt_sb);
dev_dbg(dev, "btt: %s\n", rc == 0 ? dev_name(btt_dev) : "<none>");
if (rc < 0) {
--
2.34.1
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 <[email protected]>
> ---
> 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?
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 <[email protected]>
>> ---
>> 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 <[email protected]>.
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/
Hi Christophe,
Thank you for the patch! Perhaps something to improve:
[auto build test WARNING on v6.0-rc3]
[also build test WARNING on linus/master next-20220901]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]
url: https://github.com/intel-lab-lkp/linux/commits/Christophe-JAILLET/nvdimm-Avoid-wasting-some-memory/20220904-215140
base: b90cb1053190353cc30f0fef0ef1f378ccc063c5
config: x86_64-randconfig-a014 (https://download.01.org/0day-ci/archive/20220905/[email protected]/config)
compiler: clang version 14.0.6 (https://github.com/llvm/llvm-project f28c006a5895fc0e329fe15fead81e37457cb1d1)
reproduce (this is a W=1 build):
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# https://github.com/intel-lab-lkp/linux/commit/af94e709929390501b3d2f6e933fa0c1244a2029
git remote add linux-review https://github.com/intel-lab-lkp/linux
git fetch --no-tags linux-review Christophe-JAILLET/nvdimm-Avoid-wasting-some-memory/20220904-215140
git checkout af94e709929390501b3d2f6e933fa0c1244a2029
# save the config file
mkdir build_dir && cp config build_dir/.config
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=x86_64 SHELL=/bin/bash drivers/nvdimm/
If you fix the issue, kindly add following tag where applicable
Reported-by: kernel test robot <[email protected]>
All warnings (new ones prefixed by >>):
>> drivers/nvdimm/btt_devs.c:335:6: warning: no previous prototype for function 'nd_btt_free' [-Wmissing-prototypes]
void nd_btt_free(void *data)
^
drivers/nvdimm/btt_devs.c:335:1: note: declare 'static' if the function is not intended to be used outside of this translation unit
void nd_btt_free(void *data)
^
static
1 warning generated.
vim +/nd_btt_free +335 drivers/nvdimm/btt_devs.c
334
> 335 void nd_btt_free(void *data)
336 {
337 kfree(data);
338 }
339
--
0-DAY CI Kernel Test Service
https://01.org/lkp