Re: [PATCH] nvdimm: Avoid wasting some memory.

[Christophe JAILLET]

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@xxxxxxxxxx>
> > ---
> >   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@xxxxxxxxx>.

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@xxxxxxxxxx/