Messages in this thread Patch in this message |  | | Subject | Re: [PATCH ipmi/kcs_bmc v1] ipmi: kcs_bmc: optimize the data buffers allocation | From | "Wang, Haiyue" <> | Date | Sat, 7 Apr 2018 15:54:53 +0800 |
| |
Hi Corey,
Since IPMI 2.0 just defined minimum, no maximum:
----
KCS/SMIC Input : Required: 40 bytes IPMI Message, minimum
KCS/SMIC Output : Required: 38 bytes IPMI Message, minimum
----
We can enlarge the block size for avoiding waste, and make our driver
support most worst message size case. And I think this patch make checking
simple (from 3 to 1), and the code clean, this is the biggest reason I want to
change. The TLB is just memory management study from book, no data to
support access improvement. :)
BR,
Haiyue
On 2018-04-07 10:37, Wang, Haiyue wrote: > > > On 2018-04-07 05:47, Corey Minyard wrote: >> 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. >> > I got this idea from another code review, but that patch allocates 30 > more > 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 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. >> > As the kcs has memory copy between in/out/kbuffer, put them in the same > 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@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); >> >> >
|  |