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