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

[Wang, Haiyue]

On 2018-04-13 21:50, Corey Minyard wrote:

On 04/07/2018 02:54 AM, Wang, Haiyue wrote:

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



Yes, though there are practical maximums that are much smaller than 
1000 bytes.






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. :)


I would argue that the way it is now expresses the intent of the code 
better
than one allocation split into three parts.  Expressing your intent is 
more

important than the number of checks and a minuscule performance
improvement.  For me it makes the code easier to understand.  If you had
a tool that checked for out-of-bounds memory access, then a single 
allocation

might not find an overrun between the parts.  Smaller allocations tend
to result in less memory fragmentation.


When I wrote the commit, I felt that the message was not so professional,
and the reason sounded weak. The driver development is a complex work,
needs considering more things, not just one. Thanks for your patience.

My preference is to leave it as it is.  However, it's not that 
important, and

if you really want this patch, I can include it.


So leave it as it is, abandon this patch. :-)

BTW, another patch about KCS BMC chip support:
https://lkml.org/lkml/2018/3/22/284
Look forward your reviewing, I've tried my best to make it better.


Thanks,

-corey



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 
---
  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(_bmc->mutex);
  init_waitqueue_head(_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);

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

[Wang, Haiyue]

On 2018-04-13 21:50, Corey Minyard wrote:

On 04/07/2018 02:54 AM, Wang, Haiyue wrote:

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



Yes, though there are practical maximums that are much smaller than 
1000 bytes.






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. :)


I would argue that the way it is now expresses the intent of the code 
better
than one allocation split into three parts.  Expressing your intent is 
more

important than the number of checks and a minuscule performance
improvement.  For me it makes the code easier to understand.  If you had
a tool that checked for out-of-bounds memory access, then a single 
allocation

might not find an overrun between the parts.  Smaller allocations tend
to result in less memory fragmentation.


When I wrote the commit, I felt that the message was not so professional,
and the reason sounded weak. The driver development is a complex work,
needs considering more things, not just one. Thanks for your patience.

My preference is to leave it as it is.  However, it's not that 
important, and

if you really want this patch, I can include it.


So leave it as it is, abandon this patch. :-)

BTW, another patch about KCS BMC chip support:
https://lkml.org/lkml/2018/3/22/284
Look forward your reviewing, I've tried my best to make it better.


Thanks,

-corey



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 
---
  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(_bmc->mutex);
  init_waitqueue_head(_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);

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

[Corey Minyard]

On 04/07/2018 02:54 AM, Wang, Haiyue wrote:

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



Yes, though there are practical maximums that are much smaller than 1000 
bytes.






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. :)


I would argue that the way it is now expresses the intent of the code better
than one allocation split into three parts.  Expressing your intent is more
important than the number of checks and a minuscule performance
improvement.  For me it makes the code easier to understand.  If you had
a tool that checked for out-of-bounds memory access, then a single 
allocation

might not find an overrun between the parts.  Smaller allocations tend
to result in less memory fragmentation.

My preference is to leave it as it is.  However, it's not that 
important, and

if you really want this patch, I can include it.

Thanks,

-corey



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 
---
  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(_bmc->mutex);
  init_waitqueue_head(_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);

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

[Corey Minyard]

On 04/07/2018 02:54 AM, Wang, Haiyue wrote:

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



Yes, though there are practical maximums that are much smaller than 1000 
bytes.






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. :)


I would argue that the way it is now expresses the intent of the code better
than one allocation split into three parts.  Expressing your intent is more
important than the number of checks and a minuscule performance
improvement.  For me it makes the code easier to understand.  If you had
a tool that checked for out-of-bounds memory access, then a single 
allocation

might not find an overrun between the parts.  Smaller allocations tend
to result in less memory fragmentation.

My preference is to leave it as it is.  However, it's not that 
important, and

if you really want this patch, I can include it.

Thanks,

-corey



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 
---
  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(_bmc->mutex);
  init_waitqueue_head(_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);

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

[Wang, Haiyue]

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 
---
  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(_bmc->mutex);
  init_waitqueue_head(_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);

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

[Wang, Haiyue]

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 
---
  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(_bmc->mutex);
  init_waitqueue_head(_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);

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

[Wang, Haiyue]

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 
---
  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(_bmc->mutex);
  init_waitqueue_head(_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);

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

[Wang, Haiyue]

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 
---
  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(_bmc->mutex);
  init_waitqueue_head(_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);

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

[Corey Minyard]

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 
---
  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(_bmc->mutex);
init_waitqueue_head(_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);

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

[Corey Minyard]

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 
---
  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(_bmc->mutex);
init_waitqueue_head(_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);

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

[Wang, Haiyue]

Just a small piece of cake, not so urgent. I just try to understand

the code commitment process, such as time etc. :)

Thanks!

BR,
Haiyue

On 2018-04-04 02:45, Corey Minyard wrote:

On 04/03/2018 01:00 AM, Wang, Haiyue wrote:

Hi Corey,

The 4.17 merge window is opened now, this patch is not yet in 
linux-next tree,


so it will be merged into 4.18 ?



Yeah, this came in kind of late, and I had some other critical
things I was having to focus on, so I've been kind of out of the loop.

If it's urgent, I can work on getting it into 4.17 later, but I'd rather
wait on 4.18.

-corey


Thanks & Regards,

Haiyue

On 2018-03-15 20:20, Haiyue Wang wrote:

Allocate a continuous memory block for the three KCS data buffers with
related index assignment.

Signed-off-by: Haiyue Wang 
---
  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(_bmc->mutex);
  init_waitqueue_head(_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);

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

[Wang, Haiyue]

Just a small piece of cake, not so urgent. I just try to understand

the code commitment process, such as time etc. :)

Thanks!

BR,
Haiyue

On 2018-04-04 02:45, Corey Minyard wrote:

On 04/03/2018 01:00 AM, Wang, Haiyue wrote:

Hi Corey,

The 4.17 merge window is opened now, this patch is not yet in 
linux-next tree,


so it will be merged into 4.18 ?



Yeah, this came in kind of late, and I had some other critical
things I was having to focus on, so I've been kind of out of the loop.

If it's urgent, I can work on getting it into 4.17 later, but I'd rather
wait on 4.18.

-corey


Thanks & Regards,

Haiyue

On 2018-03-15 20:20, Haiyue Wang wrote:

Allocate a continuous memory block for the three KCS data buffers with
related index assignment.

Signed-off-by: Haiyue Wang 
---
  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(_bmc->mutex);
  init_waitqueue_head(_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);

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

[Corey Minyard]

On 04/03/2018 01:00 AM, Wang, Haiyue wrote:

Hi Corey,

The 4.17 merge window is opened now, this patch is not yet in 
linux-next tree,


so it will be merged into 4.18 ?



Yeah, this came in kind of late, and I had some other critical
things I was having to focus on, so I've been kind of out of the loop.

If it's urgent, I can work on getting it into 4.17 later, but I'd rather
wait on 4.18.

-corey


Thanks & Regards,

Haiyue

On 2018-03-15 20:20, Haiyue Wang wrote:

Allocate a continuous memory block for the three KCS data buffers with
related index assignment.

Signed-off-by: Haiyue Wang 
---
  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(_bmc->mutex);
  init_waitqueue_head(_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);

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

[Corey Minyard]

On 04/03/2018 01:00 AM, Wang, Haiyue wrote:

Hi Corey,

The 4.17 merge window is opened now, this patch is not yet in 
linux-next tree,


so it will be merged into 4.18 ?



Yeah, this came in kind of late, and I had some other critical
things I was having to focus on, so I've been kind of out of the loop.

If it's urgent, I can work on getting it into 4.17 later, but I'd rather
wait on 4.18.

-corey


Thanks & Regards,

Haiyue

On 2018-03-15 20:20, Haiyue Wang wrote:

Allocate a continuous memory block for the three KCS data buffers with
related index assignment.

Signed-off-by: Haiyue Wang 
---
  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(_bmc->mutex);
  init_waitqueue_head(_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);

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

[Wang, Haiyue]

Hi Corey,

The 4.17 merge window is opened now, this patch is not yet in linux-next 
tree,


so it will be merged into 4.18 ?

Thanks & Regards,

Haiyue

On 2018-03-15 20:20, Haiyue Wang wrote:

Allocate a continuous memory block for the three KCS data buffers with
related index assignment.

Signed-off-by: Haiyue Wang 
---
  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(_bmc->mutex);
init_waitqueue_head(_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);

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

[Wang, Haiyue]

Hi Corey,

The 4.17 merge window is opened now, this patch is not yet in linux-next 
tree,


so it will be merged into 4.18 ?

Thanks & Regards,

Haiyue

On 2018-03-15 20:20, Haiyue Wang wrote:

Allocate a continuous memory block for the three KCS data buffers with
related index assignment.

Signed-off-by: Haiyue Wang 
---
  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(_bmc->mutex);
init_waitqueue_head(_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);

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

[Haiyue Wang]

Allocate a continuous memory block for the three KCS data buffers with
related index assignment.

Signed-off-by: Haiyue Wang 
---
 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(_bmc->mutex);
init_waitqueue_head(_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);
-- 
2.7.4

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

[Haiyue Wang]

Allocate a continuous memory block for the three KCS data buffers with
related index assignment.

Signed-off-by: Haiyue Wang 
---
 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(_bmc->mutex);
init_waitqueue_head(_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);
-- 
2.7.4