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

[Wen Xiong]

On 2026-07-02 14:56, Brian King wrote:
> On 7/2/26 1:55 AM, 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.
> >
> > 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.
>
> Agree that GFP_ATOMIC is not needed here, but we probably do need
> GFP_NOIO since we don't want these allocations starting I/O as the
> storage adapter is not in a state where it can handle I/O until
> the card is reset.
>
> Thanks,
>
> Brian

iprdump works with both of GFP_ATOMIC and GFP_NOIO.

Tested-by: Wen Xiong <wenxiong@xxxxxxxxxxxxx>