On 2018-04-07 05:47, Corey Minyard wrote:
On 03/15/2018 07:20 AM, Haiyue Wang wrote:I got this idea from another code review, but that patch allocates 30 more
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.
the same size memory block, reducing the devm_kmalloc call will be better.
For KCS only have 3, may be the key point is memory waste.
In the original case, you allocate three 1000 byte buffers, resulting in 3As the kcs has memory copy between in/out/kbuffer, put them in the same
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.
page will be better ? Such as the same TLB ? (Well, I just got this from book,
no real experience of memory accessing performance. And also, I was told
that using space to save the time. :-)).
Just my stupid thinking. I'm OK to drop this patch if it doesn't help with
performance, or something else.
BR.
Haiyue
-corey
Signed-off-by: Haiyue Wang <haiyue.wang@xxxxxxxxxxxxxxx>
---
 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);