Re: [PATCH ipmi/kcs_bmc v1] ipmi: kcs_bmc: optimize the data buffers allocation

[Corey Minyard <tcminyard@gmail.com>]

On 03/15/2018 07:20 AM, Haiyue Wang wrote:
> Allocate a continuous memory block for the three KCS data buffers with
> related index assignment.

I'm finally getting to this.

Is there a reason you want to do this?  In general, it's better to not 
try to
outsmart your base system.  Depending on the memory allocator, in this
case, you might actually use more memory.  You probably won't use any
less.

In the original case, you allocate three 1000 byte buffers, resulting in 3
1024 byte slab allocated.

In the changed case, you will allocate a 3000 byte buffer, resulting in
a single 4096 byte slab allocation, wasting 1024 more bytes of memory.

-corey

> Signed-off-by: Haiyue Wang <haiyue.wang@linux.intel.com>
> ---
>   drivers/char/ipmi/kcs_bmc.c | 10 ++++++----
>   1 file changed, 6 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/char/ipmi/kcs_bmc.c b/drivers/char/ipmi/kcs_bmc.c
> index fbfc05e..dc19c0d 100644
> --- a/drivers/char/ipmi/kcs_bmc.c
> +++ b/drivers/char/ipmi/kcs_bmc.c
> @@ -435,6 +435,7 @@ static const struct file_operations kcs_bmc_fops = {
>   struct kcs_bmc *kcs_bmc_alloc(struct device *dev, int sizeof_priv, u32 channel)
>   {
>   	struct kcs_bmc *kcs_bmc;
> +	void *buf;
>   
>   	kcs_bmc = devm_kzalloc(dev, sizeof(*kcs_bmc) + sizeof_priv, GFP_KERNEL);
>   	if (!kcs_bmc)
> @@ -448,11 +449,12 @@ struct kcs_bmc *kcs_bmc_alloc(struct device *dev, int sizeof_priv, u32 channel)
>   	mutex_init(&kcs_bmc->mutex);
>   	init_waitqueue_head(&kcs_bmc->queue);
>   
> -	kcs_bmc->data_in = devm_kmalloc(dev, KCS_MSG_BUFSIZ, GFP_KERNEL);
> -	kcs_bmc->data_out = devm_kmalloc(dev, KCS_MSG_BUFSIZ, GFP_KERNEL);
> -	kcs_bmc->kbuffer = devm_kmalloc(dev, KCS_MSG_BUFSIZ, GFP_KERNEL);
> -	if (!kcs_bmc->data_in || !kcs_bmc->data_out || !kcs_bmc->kbuffer)
> +	buf = devm_kmalloc_array(dev, 3, KCS_MSG_BUFSIZ, GFP_KERNEL);
> +	if (!buf)
>   		return NULL;
> +	kcs_bmc->data_in  = buf;
> +	kcs_bmc->data_out = buf + KCS_MSG_BUFSIZ;
> +	kcs_bmc->kbuffer  = buf + KCS_MSG_BUFSIZ * 2;
>   
>   	kcs_bmc->miscdev.minor = MISC_DYNAMIC_MINOR;
>   	kcs_bmc->miscdev.name = dev_name(dev);