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 <[email protected]>
---
Looking at this code, I wonder why priv->miscdev.name is not freed in
kcs_bmc_ipmi_remove_device()?
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;
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
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 <[email protected]>
> ---
> 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
>