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

[Brian King]

On 7/3/26 2: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.

Sounds good to me


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