Re: [PATCH 3/4] scsi: ipr: use kmalloc() to allocate IPR dump buffer memory

[Mike Rapoport]

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.

And it seems that GFP_ATOMIC here is a historical artifact rather than
actual necessity. ipr_get_ioa_dump() runs in a workqueue context without
locks held so nothing warrants GFP_ATOMIC.
 
> Thanks,
> 
> Brian
> 
> 
> -- 
> Brian King
> Power Linux I/O
> IBM Linux Technology Center
> 

-- 
Sincerely yours,
Mike.