Re: [PATCH] ipmi: kcs_bmc: Avoid wasting some memory.

[Corey Minyard <minyard@acm.org>]

Adding Andrew, the author of this code.

On Sun, Sep 04, 2022 at 03:35:16PM +0200, Christophe JAILLET wrote:
> KCS_MSG_BUFSIZ is 1000.
> 
> When using devm_kmalloc(), there is a small memory overhead and, on most
> systems, this leads to 40 bytes of extra memory allocation.
> So 1040 bytes are expected to be allocated.
> 
> The memory allocator works with fixed size hunks of memory. In this case,
> it will require 2048 bytes of memory because more than 1024 bytes are
> required.
> 
> So, when requesting 3 x 1000 bytes, it ends up to 2048 x 3.
> 
> In order to avoid wasting 3ko of memory, allocate buffers all at once.
> 3000+40 bytes will be required and 4ko allocated. This still wastes 1ko,
> but it is already better.
> 
> Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr>
> ---
> Looking at this code, I wonder why priv->miscdev.name is not freed in
> kcs_bmc_ipmi_remove_device()?

If I understand correctly, none of these need to be freed.  devm
allocated memory is freed automatically when the device is removed.

> 
> If this make sense, this also mean that KCS_MSG_BUFSIZ can be increased at
> no cost.
> Or it could be slightly reduce to around 1024-40-1 bytes to keep the logic
> which is in place.
> 
> Another solution would be to use just kmalloc and add a
> devm_add_action_or_reset() call and a function that frees the memory.
> If it make sense, KCS_MSG_BUFSIZ could be increased to 1024 and we would
> allocate just a little above 3x1024 bytes.
> ---
>  drivers/char/ipmi/kcs_bmc_cdev_ipmi.c | 11 +++++------
>  1 file changed, 5 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/char/ipmi/kcs_bmc_cdev_ipmi.c b/drivers/char/ipmi/kcs_bmc_cdev_ipmi.c
> index 486834a962c3..15a4a39a6478 100644
> --- a/drivers/char/ipmi/kcs_bmc_cdev_ipmi.c
> +++ b/drivers/char/ipmi/kcs_bmc_cdev_ipmi.c
> @@ -485,14 +485,15 @@ static int kcs_bmc_ipmi_add_device(struct kcs_bmc_device *kcs_bmc)
>  
>  	priv->client.dev = kcs_bmc;
>  	priv->client.ops = &kcs_bmc_ipmi_client_ops;
> -	priv->data_in = devm_kmalloc(kcs_bmc->dev, KCS_MSG_BUFSIZ, GFP_KERNEL);
> -	priv->data_out = devm_kmalloc(kcs_bmc->dev, KCS_MSG_BUFSIZ, GFP_KERNEL);
> -	priv->kbuffer = devm_kmalloc(kcs_bmc->dev, KCS_MSG_BUFSIZ, GFP_KERNEL);
> +	/* Allocate buffers all at once */
> +	priv->data_in = devm_kmalloc(kcs_bmc->dev, KCS_MSG_BUFSIZ * 3, GFP_KERNEL);
> +	priv->data_out = priv->data_in + KCS_MSG_BUFSIZ;
> +	priv->kbuffer  = priv->data_in + KCS_MSG_BUFSIZ * 2;

You are doing arithmetic on a possibly NULL pointer.  It's generally ok,
but kind of frowned upon.

Andew, what do you think?  I guess it saves a little memory.

-Corey

>  
>  	priv->miscdev.minor = MISC_DYNAMIC_MINOR;
>  	priv->miscdev.name = devm_kasprintf(kcs_bmc->dev, GFP_KERNEL, "%s%u", DEVICE_NAME,
>  					   kcs_bmc->channel);
> -	if (!priv->data_in || !priv->data_out || !priv->kbuffer || !priv->miscdev.name)
> +	if (!priv->data_in || !priv->miscdev.name)
>  		return -EINVAL;
>  
>  	priv->miscdev.fops = &kcs_bmc_ipmi_fops;
> @@ -531,8 +532,6 @@ static int kcs_bmc_ipmi_remove_device(struct kcs_bmc_device *kcs_bmc)
>  
>  	misc_deregister(&priv->miscdev);
>  	kcs_bmc_disable_device(priv->client.dev, &priv->client);
> -	devm_kfree(kcs_bmc->dev, priv->kbuffer);
> -	devm_kfree(kcs_bmc->dev, priv->data_out);
>  	devm_kfree(kcs_bmc->dev, priv->data_in);
>  	devm_kfree(kcs_bmc->dev, priv);
>  
> -- 
> 2.34.1
>