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

[Brian King]

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,

Brian


-- 
Brian King
Power Linux I/O
IBM Linux Technology Center