| Message ID | 0db94f84920663f3bd45a73e2ae73950627a377f.1663506472.git.christophe.jaillet@wanadoo.fr (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Input: applespi - avoid wasting some memory | expand |
On Sun, Sep 18, 2022 at 03:08:17PM +0200, Christophe JAILLET wrote: > When the 'struct applespi_data' structure is allocated at the beginning of > applespi_probe(), 2504 bytes are allocated. > > Because of the way memory is allocated, it ends to a 4096 bytes allocation. > So, about 1500 bytes are wasted. > > Later in this function, when 'tx_buffer', 'tx_status', 'rx_buffer' and > 'msg_buf' are allocated, 256, 4, 256 and 512 bytes are requested (~1 ko). > A devm_ memory allocation has a small overhead of 40 bytes. So, for the > same reason as above, it ends to allocate 512, 64, 512 and 1024 (~2 ko). > > All that said, defining these 4 arrays as part of 'struct applespi_data' > saves 2 ko of runtime memory. > > 3504 bytes are now requested, and 4096 really allocated. All these 4 > arrays fit in the 'wasted' memory of the first allocation. > > Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr> > --- > Compile tested only. > --- > drivers/input/keyboard/applespi.c | 23 ++++------------------- > 1 file changed, 4 insertions(+), 19 deletions(-) > > diff --git a/drivers/input/keyboard/applespi.c b/drivers/input/keyboard/applespi.c > index fab5473ae5da..bee4ccfa2b05 100644 > --- a/drivers/input/keyboard/applespi.c > +++ b/drivers/input/keyboard/applespi.c > @@ -373,11 +373,11 @@ struct applespi_data { > struct input_dev *keyboard_input_dev; > struct input_dev *touchpad_input_dev; > > - u8 *tx_buffer; > - u8 *tx_status; > - u8 *rx_buffer; > + u8 tx_buffer[APPLESPI_PACKET_SIZE]; > + u8 tx_status[APPLESPI_STATUS_SIZE]; > + u8 rx_buffer[APPLESPI_PACKET_SIZE]; > > - u8 *msg_buf; > + u8 msg_buf[MAX_PKTS_PER_MSG * APPLESPI_PACKET_SIZE]; > unsigned int saved_msg_len; > > struct applespi_tp_info tp_info; This kind of change is generally broken in case DMA can be involved. Allocating the transfer buffers separately makes sure that alignment requirements are met and avoids hard-to-debug memory corruption issues. Johan
Le 19/09/2022 à 09:02, Johan Hovold a écrit : > On Sun, Sep 18, 2022 at 03:08:17PM +0200, Christophe JAILLET wrote: >> When the 'struct applespi_data' structure is allocated at the beginning of >> applespi_probe(), 2504 bytes are allocated. >> >> Because of the way memory is allocated, it ends to a 4096 bytes allocation. >> So, about 1500 bytes are wasted. >> >> Later in this function, when 'tx_buffer', 'tx_status', 'rx_buffer' and >> 'msg_buf' are allocated, 256, 4, 256 and 512 bytes are requested (~1 ko). >> A devm_ memory allocation has a small overhead of 40 bytes. So, for the >> same reason as above, it ends to allocate 512, 64, 512 and 1024 (~2 ko). >> >> All that said, defining these 4 arrays as part of 'struct applespi_data' >> saves 2 ko of runtime memory. >> >> 3504 bytes are now requested, and 4096 really allocated. All these 4 >> arrays fit in the 'wasted' memory of the first allocation. >> >> Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr> >> --- >> Compile tested only. >> --- >> drivers/input/keyboard/applespi.c | 23 ++++------------------- >> 1 file changed, 4 insertions(+), 19 deletions(-) >> >> diff --git a/drivers/input/keyboard/applespi.c b/drivers/input/keyboard/applespi.c >> index fab5473ae5da..bee4ccfa2b05 100644 >> --- a/drivers/input/keyboard/applespi.c >> +++ b/drivers/input/keyboard/applespi.c >> @@ -373,11 +373,11 @@ struct applespi_data { >> struct input_dev *keyboard_input_dev; >> struct input_dev *touchpad_input_dev; >> >> - u8 *tx_buffer; >> - u8 *tx_status; >> - u8 *rx_buffer; >> + u8 tx_buffer[APPLESPI_PACKET_SIZE]; >> + u8 tx_status[APPLESPI_STATUS_SIZE]; >> + u8 rx_buffer[APPLESPI_PACKET_SIZE]; >> >> - u8 *msg_buf; >> + u8 msg_buf[MAX_PKTS_PER_MSG * APPLESPI_PACKET_SIZE]; >> unsigned int saved_msg_len; >> >> struct applespi_tp_info tp_info; > > This kind of change is generally broken in case DMA can be involved. > > Allocating the transfer buffers separately makes sure that alignment > requirements are met and avoids hard-to-debug memory corruption issues. > > Johan > Got it. I'll keep away from it. Thanks for the feed-back and explanation. CJ
diff --git a/drivers/input/keyboard/applespi.c b/drivers/input/keyboard/applespi.c index fab5473ae5da..bee4ccfa2b05 100644 --- a/drivers/input/keyboard/applespi.c +++ b/drivers/input/keyboard/applespi.c @@ -373,11 +373,11 @@ struct applespi_data { struct input_dev *keyboard_input_dev; struct input_dev *touchpad_input_dev; - u8 *tx_buffer; - u8 *tx_status; - u8 *rx_buffer; + u8 tx_buffer[APPLESPI_PACKET_SIZE]; + u8 tx_status[APPLESPI_STATUS_SIZE]; + u8 rx_buffer[APPLESPI_PACKET_SIZE]; - u8 *msg_buf; + u8 msg_buf[MAX_PKTS_PER_MSG * APPLESPI_PACKET_SIZE]; unsigned int saved_msg_len; struct applespi_tp_info tp_info; @@ -1659,21 +1659,6 @@ static int applespi_probe(struct spi_device *spi) /* store the driver data */ spi_set_drvdata(spi, applespi); - /* create our buffers */ - applespi->tx_buffer = devm_kmalloc(&spi->dev, APPLESPI_PACKET_SIZE, - GFP_KERNEL); - applespi->tx_status = devm_kmalloc(&spi->dev, APPLESPI_STATUS_SIZE, - GFP_KERNEL); - applespi->rx_buffer = devm_kmalloc(&spi->dev, APPLESPI_PACKET_SIZE, - GFP_KERNEL); - applespi->msg_buf = devm_kmalloc_array(&spi->dev, MAX_PKTS_PER_MSG, - APPLESPI_PACKET_SIZE, - GFP_KERNEL); - - if (!applespi->tx_buffer || !applespi->tx_status || - !applespi->rx_buffer || !applespi->msg_buf) - return -ENOMEM; - /* set up our spi messages */ applespi_setup_read_txfrs(applespi); applespi_setup_write_txfrs(applespi);
When the 'struct applespi_data' structure is allocated at the beginning of applespi_probe(), 2504 bytes are allocated. Because of the way memory is allocated, it ends to a 4096 bytes allocation. So, about 1500 bytes are wasted. Later in this function, when 'tx_buffer', 'tx_status', 'rx_buffer' and 'msg_buf' are allocated, 256, 4, 256 and 512 bytes are requested (~1 ko). A devm_ memory allocation has a small overhead of 40 bytes. So, for the same reason as above, it ends to allocate 512, 64, 512 and 1024 (~2 ko). All that said, defining these 4 arrays as part of 'struct applespi_data' saves 2 ko of runtime memory. 3504 bytes are now requested, and 4096 really allocated. All these 4 arrays fit in the 'wasted' memory of the first allocation. Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr> --- Compile tested only. --- drivers/input/keyboard/applespi.c | 23 ++++------------------- 1 file changed, 4 insertions(+), 19 deletions(-)