Re: [PATCH 3/4] scsi: ipr: use kmalloc() to allocate IPR dump buffer memory
From: Hannes Reinecke
Date: Fri Jul 03 2026 - 04:36:10 EST
On 7/3/26 9:49 AM, Mike Rapoport wrote:
On Thu, Jul 02, 2026 at 09:55:56AM +0300, Mike Rapoport wrote:
On Wed, Jul 01, 2026 at 04:03:48PM -0500, Brian King wrote:
On 7/1/26 4:52 AM, Mike Rapoport wrote:
On Wed, Jul 01, 2026 at 09:03:06AM +0200, Hannes Reinecke wrote:
On 6/30/26 12:54 PM, Mike Rapoport (Microsoft) wrote:
IPR dump machinery allocates memory to save adapter's crash dump using
__get_free_page().
This memory can be allocated with kmalloc() as there's nothing special
about it to go directly to the page allocator.
kmalloc() provides a better API that does not require ugly casts and
kfree() does not need to know the size of the freed object.
Replace use of __get_free_page() with kmalloc().
Link: https://lore.kernel.org/all/635405e4-9423-4a25-a6e7-e03c8ea0bcbe@xxxxxxxxxx
Signed-off-by: Mike Rapoport (Microsoft) <rppt@xxxxxxxxxx>
---
drivers/scsi/ipr.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/scsi/ipr.c b/drivers/scsi/ipr.c
index d207e5e81afe..5a212bfdeec2 100644
--- a/drivers/scsi/ipr.c
+++ b/drivers/scsi/ipr.c
@@ -2893,7 +2893,7 @@ static int ipr_sdt_copy(struct ipr_ioa_cfg *ioa_cfg,
(ioa_dump->hdr.len + bytes_copied) < max_dump_size) {
if (ioa_dump->page_offset >= PAGE_SIZE ||
ioa_dump->page_offset == 0) {
- page = (__be32 *)__get_free_page(GFP_ATOMIC);
+ page = kmalloc(PAGE_SIZE, GFP_ATOMIC);
if (!page) {
ipr_trace;
@@ -3226,7 +3226,7 @@ static void ipr_release_dump(struct kref *kref)
spin_unlock_irqrestore(ioa_cfg->host->host_lock, lock_flags);
for (i = 0; i < dump->ioa_dump.next_page_index; i++)
- free_page((unsigned long) dump->ioa_dump.ioa_data[i]);
+ kfree(dump->ioa_dump.ioa_data[i]);
vfree(dump->ioa_dump.ioa_data);
kfree(dump);
I _think_ we can replace this with kvmalloc, and allocate the entire
dump buffer in one go. Once switched to kmalloc() it's kinda pointless
to allocate separate page-sized buffers here.
kmalloc() performance is on par with __get_free_page(), but kvmalloc()
would be slower if it falls back to vmalloc().
I'm not familiar with the driver to say if this could be an issue here.
This code only runs when the adapter has hit a fatal error, so should be
extremely rare. The memory is getting allocated while the storage adapter
is in a failed state, so anything running on the system at the time could
be stalled until recovery is completed. This memory is allocated and should
be freed soon after the adapter recovers. In order for this code to
run, the iprdump daemon must be running, which will then read out the dump
after the adapter is recovered, and write it to disk, after which time, the
ipr driver will free the kernel memory.
Thanks for the explanation!
So the allocation is not on the hot path and it seems to me that
performance differences between kmalloc() and slower vmalloc() won't move a
needle relatively to the entire dump procedure.
I looked more closely at the code. Currently the dump continues as long as
it can allocate memory and even if memory for the entire buffer is not
available there still will be some data in the dump.
With a single allocation that may be as large as 80M
(IPR_FMT3_MAX_IOA_DUMP_SIZE), an allocation failure would mean no data
would be dumped at all.
I think the current model is better than using a single kvmalloc()
allocation for the entire buffer.
I'm going to send v2 that keeps kmalloc() and only changes GFP_ATOMIC to
GFP_NOIO if there are no objections.
Fair point.
You can add:
Reviewed-by: Hannes Reinecke <hare@xxxxxxxxxx>
Cheers,
Hannes
--
Dr. Hannes Reinecke Kernel Storage Architect
hare@xxxxxxxx +49 911 74053 688
SUSE Software Solutions GmbH, Frankenstr. 146, 90461 Nürnberg
HRB 36809 (AG Nürnberg), GF: I. Totev, A. McDonald, W. Knoblich