2009-03-04 12:22:27

by Frank Seidel

Subject: [PATCH] ipmi: reduce stack size of msghandler

From: Frank Seidel <[email protected]>

Reduce stack memory footprint in ipmi_msghandler
for send_panic_events. (From 992 bytes on i386
down to below 100)

Signed-off-by: Frank Seidel <[email protected]>
---
drivers/char/ipmi/ipmi_msghandler.c | 39 ++++++++++++++++++++++++------------
1 file changed, 27 insertions(+), 12 deletions(-)

--- a/drivers/char/ipmi/ipmi_msghandler.c
+++ b/drivers/char/ipmi/ipmi_msghandler.c
@@ -4001,8 +4001,20 @@ static void send_panic_events(char *str)
unsigned char data[16];
struct ipmi_system_interface_addr *si;
struct ipmi_addr addr;
- struct ipmi_smi_msg smi_msg;
- struct ipmi_recv_msg recv_msg;
+ struct ipmi_smi_msg *smi_msg;
+ struct ipmi_recv_msg *recv_msg;
+
+ smi_msg = kmalloc(sizeof(*smi_msg), GFP_KERNEL);
+ recv_msg = kmalloc(sizeof(*recv_msg), GFP_KERNEL);
+
+ if (!smi_msg || !recv_msg) {
+ printk(KERN_ERR PFX "Could not allocate memory\n");
+ if (smi_msg)
+ kfree(smi_msg);
+ else if (recv_msg)
+ kfree(recv_msg);
+ return;
+ }

si = (struct ipmi_system_interface_addr *) &addr;
si->addr_type = IPMI_SYSTEM_INTERFACE_ADDR_TYPE;
@@ -4030,8 +4042,8 @@ static void send_panic_events(char *str)
data[7] = str[2];
}

- smi_msg.done = dummy_smi_done_handler;
- recv_msg.done = dummy_recv_done_handler;
+ smi_msg->done = dummy_smi_done_handler;
+ recv_msg->done = dummy_recv_done_handler;

/* For every registered interface, send the event. */
list_for_each_entry_rcu(intf, &ipmi_interfaces, link) {
@@ -4048,8 +4060,8 @@ static void send_panic_events(char *str)
0,
&msg,
intf,
- &smi_msg,
- &recv_msg,
+ smi_msg,
+ recv_msg,
0,
intf->channels[0].address,
intf->channels[0].lun,
@@ -4107,8 +4119,8 @@ static void send_panic_events(char *str)
0,
&msg,
intf,
- &smi_msg,
- &recv_msg,
+ smi_msg,
+ recv_msg,
0,
intf->channels[0].address,
intf->channels[0].lun,
@@ -4127,8 +4139,8 @@ static void send_panic_events(char *str)
0,
&msg,
intf,
- &smi_msg,
- &recv_msg,
+ smi_msg,
+ recv_msg,
0,
intf->channels[0].address,
intf->channels[0].lun,
@@ -4195,8 +4207,8 @@ static void send_panic_events(char *str)
0,
&msg,
intf,
- &smi_msg,
- &recv_msg,
+ smi_msg,
+ recv_msg,
0,
intf->channels[0].address,
intf->channels[0].lun,
@@ -4204,6 +4216,9 @@ static void send_panic_events(char *str)
}
}
#endif /* CONFIG_IPMI_PANIC_STRING */
+
+ kfree(smi_msg);
+ kfree(recv_msg);
}
#endif /* CONFIG_IPMI_PANIC_EVENT */

2009-03-04 14:59:35

by Corey Minyard

Subject: Re: [PATCH] ipmi: reduce stack size of msghandler

Frank Seidel wrote:
> From: Frank Seidel <[email protected]>
>
> Reduce stack memory footprint in ipmi_msghandler
> for send_panic_events. (From 992 bytes on i386
> down to below 100)
>
I'm not sure this is a good idea. This only occurs in a panic
situation, so it may be best to not dynamically allocate memory.

However, 992 bytes is a big chunk of data to swallow on the stack, even
on a panic. What may be best here is to use static data for this
operation, then use an atomic to control access to the static data. I
believe an atomic is better than a lock because it avoids problems with
possible re-entrancy with another panic on the same CPU.

Thanks,

-corey

> Signed-off-by: Frank Seidel <[email protected]>
> ---
> drivers/char/ipmi/ipmi_msghandler.c | 39 ++++++++++++++++++++++++------------
> 1 file changed, 27 insertions(+), 12 deletions(-)
>
> --- a/drivers/char/ipmi/ipmi_msghandler.c
> +++ b/drivers/char/ipmi/ipmi_msghandler.c
> @@ -4001,8 +4001,20 @@ static void send_panic_events(char *str)
> unsigned char data[16];
> struct ipmi_system_interface_addr *si;
> struct ipmi_addr addr;
> - struct ipmi_smi_msg smi_msg;
> - struct ipmi_recv_msg recv_msg;
> + struct ipmi_smi_msg *smi_msg;
> + struct ipmi_recv_msg *recv_msg;
> +
> + smi_msg = kmalloc(sizeof(*smi_msg), GFP_KERNEL);
> + recv_msg = kmalloc(sizeof(*recv_msg), GFP_KERNEL);
> +
> + if (!smi_msg || !recv_msg) {
> + printk(KERN_ERR PFX "Could not allocate memory\n");
> + if (smi_msg)
> + kfree(smi_msg);
> + else if (recv_msg)
> + kfree(recv_msg);
> + return;
> + }
>
> si = (struct ipmi_system_interface_addr *) &addr;
> si->addr_type = IPMI_SYSTEM_INTERFACE_ADDR_TYPE;
> @@ -4030,8 +4042,8 @@ static void send_panic_events(char *str)
> data[7] = str[2];
> }
>
> - smi_msg.done = dummy_smi_done_handler;
> - recv_msg.done = dummy_recv_done_handler;
> + smi_msg->done = dummy_smi_done_handler;
> + recv_msg->done = dummy_recv_done_handler;
>
> /* For every registered interface, send the event. */
> list_for_each_entry_rcu(intf, &ipmi_interfaces, link) {
> @@ -4048,8 +4060,8 @@ static void send_panic_events(char *str)
> 0,
> &msg,
> intf,
> - &smi_msg,
> - &recv_msg,
> + smi_msg,
> + recv_msg,
> 0,
> intf->channels[0].address,
> intf->channels[0].lun,
> @@ -4107,8 +4119,8 @@ static void send_panic_events(char *str)
> 0,
> &msg,
> intf,
> - &smi_msg,
> - &recv_msg,
> + smi_msg,
> + recv_msg,
> 0,
> intf->channels[0].address,
> intf->channels[0].lun,
> @@ -4127,8 +4139,8 @@ static void send_panic_events(char *str)
> 0,
> &msg,
> intf,
> - &smi_msg,
> - &recv_msg,
> + smi_msg,
> + recv_msg,
> 0,
> intf->channels[0].address,
> intf->channels[0].lun,
> @@ -4195,8 +4207,8 @@ static void send_panic_events(char *str)
> 0,
> &msg,
> intf,
> - &smi_msg,
> - &recv_msg,
> + smi_msg,
> + recv_msg,
> 0,
> intf->channels[0].address,
> intf->channels[0].lun,
> @@ -4204,6 +4216,9 @@ static void send_panic_events(char *str)
> }
> }
> #endif /* CONFIG_IPMI_PANIC_STRING */
> +
> + kfree(smi_msg);
> + kfree(recv_msg);
> }
> #endif /* CONFIG_IPMI_PANIC_EVENT */
>
>
>