Re: [PATCH] powerpc/pseries/iommu: switch to Default DMA window during kdump

[Gaurav Batra <gbatra@linux.ibm.com>]

[-- Attachment #1: Type: text/plain, Size: 10946 bytes --]

Hello Ritesh,

Responses are inline

- Gaurav

On 8/1/26 12:55 AM, Ritesh Harjani (IBM) wrote:
> Gaurav Batra<gbatra@linux.ibm.com> writes:
>
>>> So looking at above, I had few questions -
>>> 1. In your kdump kernel how many nr_cpus you are booting up with?
>>>      Recently what I heard rhel/sles might be using nr_cpus=16/32. @Sourabh?
>>>      So the calculation in nvme-fc driver then becomes:
>>>        32(nr_cpus) * 64(queue_depth) * 2(cmd+resp) = 4096
>>>
>>>     If you are able to reproduce this issue 100% of the time - then can
>>>     you try kdump with nr_cpus=1 and see whether it fixes your iommu alloc
>>>     failure?
>> Initially, when I reproduced this issue, I was not passing any value to
>>
>> nr_cpus to the kdump kernel. I made changes to /etc/sysconfig/kdump to pass
>>
>> nr_cpus=1. With this the kdump was successful. I tried with
>> nr_cpus=16/32. These
>>
>> were successful as well. Though, in these cases, I did notice a few
>> iommu_alloc failures
>>
>> (maybe < 10), vmcore was gathered successfully.
>>
>> I started to see the issue with nr_cpus=64. My LPAR is configured with
>> max cpus = 64. So,
>>
>> earlier, when I was not specifying nr_cpus in the /etc/sysconfig/kdump,
>> kdump could be
>>
>> defaulting to 64 CPUs and hence allocating more resources during kdump.
>>
> Was this issue not reported via any system testing? Is this something
> that you found on your own when you attach nvme-fc?
> The reason I am interested in knowing that was - because the distro
> default for rhel/sles will be 16/32 cpus, so was wondering if it was
> repored with the default values as well and what were they?
This issue was reported during testing of of NVMe-FC device on SLES 16.1.
My understanding is that recently there has been changes in grub that 
enabled
NVMe over FC on SLES. It is not yet supported on RHEL. I am not sure 
what the
CPU configuration was in the tester's LPAR. Once of my team member, 
Wendy, was
able to reproduce the issue in her environment. This is where I debugged 
the issue.
Now, I am not sure, if this issue can be encountered in the fresh 
install of SLES 16.1
and what the kdump default parameters will be.
>
>
>>> 2. Also were there more than 1 controller attached? Which can change the
>>>     above calculation then.
>> Only 1 controller. Here is the output of lscpi
>>
>> ltcd41-lp11:~ # lspci
>> 0153:70:00.0 Fibre Channel: Emulex Corporation LPe37000/LPe38000 Series
>> 32Gb/64Gb Fibre Channel Adapter (rev 10)
>> 0153:70:00.1 Fibre Channel: Emulex Corporation LPe37000/LPe38000 Series
>> 32Gb/64Gb Fibre Channel Adapter (rev 10)
>> 0153:70:00.2 Fibre Channel: Emulex Corporation LPe37000/LPe38000 Series
>> 32Gb/64Gb Fibre Channel Adapter (rev 10)
>> 0153:70:00.3 Fibre Channel: Emulex Corporation LPe37000/LPe38000 Series
>> 32Gb/64Gb Fibre Channel Adapter (rev 10)
>>
>>> Looking at the lpfc and nvme-fc driver - a lot of the calculation are
>>> based on nr_online_cpus. I somehow think if we clamp that value of
>>> nr_online_cpus, we should stop seeing these alloc failures.
>>> Thoughts?
>>>
>>> Hopefully, if you can work on some above points further, it will also
>>> explain why are we seeing this failures only now. Is this something that
>>> has caused an issue after RHEL/SLES moved to nr_cpus=16/32?
>> it seems this got exposed because in my test LPAR, nr_cpus=1/16/32 was not
>>
>> getting passed to the kdump kernel.
>>
>>> Or was it after this commit from v6.9?
>>>      ec30b461f3d: ("blk-mq: don't change nr_hw_queues and nr_maps for kdump kernel")
>>>
>>>> Here are the driver logs and stack
>>>>
>>>> lpfc 0153:70:00.0: iommu_alloc failed,
>>>> tbl 0000000034ebcf5e vaddr 00000000d814df0b npages 1
>>>> lpfc 0153:70:00.0: FCP Op failed - cmdiu dma mapping failed.
>>>> lpfc 0153:70:00.0: iommu_alloc failed,
>>>> tbl 0000000034ebcf5e vaddr 000000009779e4d2 npages 1
>>>> lpfc 0153:70:00.0: FCP Op failed - cmdiu dma mapping failed.
>>>>
>>>> iommu_map_phys+0x1c4/0x1f0 (unreliable)
>>>> dma_iommu_map_phys+0x54/0xa0
>>>> dma_map_phys+0x3f8/0x590
>>>> __nvme_fc_init_request+0x110/0x300 [nvme_fc]
>>>> nvme_fc_init_request+0x60/0xb8 [nvme_fc]
>>>> blk_mq_alloc_map_and_rqs+0x388/0x510
>>>> blk_mq_alloc_tag_set+0x2a4/0x5f0
>>>> nvme_alloc_io_tag_set+0xe0/0x1e0 [nvme_core]
>>>> nvme_fc_connect_ctrl_work+0x85c/0xdac [nvme_fc]
>>>> process_one_work+0x1e4/0x5a0
>>>> worker_thread+0x1ec/0x3e0
>>>>
>>>> Increasing the number of free TCE entries in iommu_table_clear() will
>>>> increase the probability of hitting EEH since there could still be some
>>>> active IOs from the previous life of the kernel.
>>>>
>>>> Instead, during kdump, we can switch to default 2GB DMA window. This window
>>>> will mostly be empty. Or, could be slightly used if buffers in pmemory
>>>> were mapped for IO.
>>>>
>>> This will still remain a problem when we have SR-IOV adapter attached
>>> correct? Because in that case we only get 1 window, so we anyway can't
>>> use default window in kdump case. Correct?
>> you are right. The patch is fixing the dedicated adapter path only by
>> switching to
>>
>> default window for kdump. Before I submitted the patch, I did try SR-IOV
>> path as well.
>>
>> Here, I assigned a virtualized adapter to LPAR and gathered kdump over
>> NFS. I checked the
>>
>> footprint of DMA buffers in this path. They were not much. I think, I
>> did sent these details
>>
>> in my emails (the discussion/advice).
>>
>>
>> seems to me the least invasive fix for this very narrow problem.
> I went back and looked at the history of why do we use DDW window during
> kdump instead of default window. It looks like this commit itself
> changed the default to DDW instead of default window :) i.e.
>    Fixes: 09a3c1e46142 ("powerpc/pseries/iommu: IOMMU table is not initialized for kdump over SR-IOV")
>
> The commit msg is very detailed and explains a lof of things but somehow
> it didn't explain on why did we change the default to always use ddw
> window instead of using default window.
When I was working on that issue, it was clear, that the code path for 
SR-IOV case has
missed the DDW. This was because pci_dma_find() was specifically looking 
for default window,
which in the case of SR-IOV, will be missing.

Both dedicated adapter and SR-IOV will have DDW. That is why I switched 
to DDW in both the case
(dedicated and SR-IOV). Also, iommu_table_clear() was making space in 
DDW by clearing 2K worth of
TCEs, and the fact that this is not a performance path, I switched to 
DDW. I have detailed all this
reasoning in the commit: 09a3c1e46142 ("powerpc/pseries/iommu: IOMMU 
table is not initialized for kdump over SR-IOV")
>
>> Your insight and thoughts?
>>
> Somehow in the current commit msg we didn't mention that this commit is
> just a partial revert of that previous fixes commit - i.e. we are
> switching back to using 2GB default DMA window as the preferred window
> for kdump case.
>
> IMO, let's please also _add_ something like this to the existing commit
> msg, so that it becomes clear for others.
>
> <snip>
>    (after this) ...kdump path and DMA window is needed for IO to the device.
>
> Although this commit fixed an issue during kdump with SR-IOV case,
>    09a3c1e46142 ("powerpc/pseries/iommu: IOMMU table is not initialized for kdump over SR-IOV")
> but this also made the kdump prefer DDW over the default DMA window when
> both are present (dedicated adapter case). Since the DDW is fully mapped
> by the previous kernel, iommu_table_clear() can free only
> KDUMP_MIN_TCE_ENTRIES (2048) TCEs for use by kdump kernel.
>
> This is not enough when the dump device is NVMe over Fibre Channel.
> Because nvme-fc driver DMA-maps the cmds and resp IUs of every
> pre-allocated request and each such mapping consumes roughly:
>         32(IO queues, one per cpus = nr_cpus) * 64(queue_depth, blk-mq kdump limit) * 2(cmd+resp) = 4096
>
> This is already double of what we have without counting admin queues and
> lpfc driver's own allocations / mapping requirement. Hence this results
> into iommu_alloc failures like -
>
> lpfc 0153:70:00.0: iommu_alloc failed,
> tbl 0000000034ebcf5e vaddr 00000000d814df0b npages 1
> lpfc 0153:70:00.0: FCP Op failed - cmdiu dma mapping failed.
> lpfc 0153:70:00.0: iommu_alloc failed,
> tbl 0000000034ebcf5e vaddr 000000009779e4d2 npages 1
> lpfc 0153:70:00.0: FCP Op failed - cmdiu dma mapping failed.
>
> iommu_map_phys+0x1c4/0x1f0 (unreliable)
> dma_iommu_map_phys+0x54/0xa0
> dma_map_phys+0x3f8/0x590
> __nvme_fc_init_request+0x110/0x300 [nvme_fc]
> nvme_fc_init_request+0x60/0xb8 [nvme_fc]
> blk_mq_alloc_map_and_rqs+0x388/0x510
> blk_mq_alloc_tag_set+0x2a4/0x5f0
> nvme_alloc_io_tag_set+0xe0/0x1e0 [nvme_core]
> nvme_fc_connect_ctrl_work+0x85c/0xdac [nvme_fc]
> process_one_work+0x1e4/0x5a0
> worker_thread+0x1ec/0x3e0
>
> Increasing the number of free TCE entries in iommu_table_clear() will
> increase the probability of hitting EEH since there could still be some
> active IOs from the previous life of the kernel.
>
> Hence this patch partially reverts the previous fixes commit and
> switches the kdump's default back to 2GB default DMA window instead of
> DDW window. This window will mostly be empty. Or, could be slightly used
> if buffers in pmemory were mapped for IO.
>
> Fixes: 09a3c1e46142 ("powerpc/pseries/iommu: IOMMU table is not initialized for kdump over SR-IOV")
> Cc:stable@vger.kernel.org
>
> (before this) ...Signed-off-by:...
> </snip>
>
>
> With that added to the commit msg - please also feel free to add:
> Reviewed-by: Ritesh Harjani (IBM)<ritesh.list@gmail.com>

I will add the above in my next version of the patch. Thanks a lot for 
digging into this and
providing a much better and detailed commit message.
>> Thanks a lot
>>
>> Gaurav
>>
> Thanks again for adding detailed info. After looking at the history, I
> agree we can partially revert the previous patch to prefer the default
> DMA window for kdump case.
>
>> As of now SR-IOV path doesn't seems to be of concern. But, I think, the
>> correct overall fix
>>
>> should be to maintain the DDW state --> if it is pre-mapped DDW,
>> transfer this knowledge to kdump.
>>
>> With this, the DDW will be intact and buffers pre-mapped, as before.
>> But, this requires more work
>>
>> and thorough testing by FVT/ISST. So, I kept this for later. For now,
>> switching to default window
> I guess for kdump using 2GB default DMA window is not an issue, however
> I agree that for kexec case we should find a way to fix this.
> Because IIUC - kexec path on pseries is not performant today. It doesn't
> use pre-mapped TCEs. So if someone is doing any I/O performance
> measurement, we have to go via the full reboot cycle instead of just
> using kexec.
Totally agree with you. I will work on a patch that fixes this and 
retains the
DDW as pre-mapped in the kexec case. There are some nuances that I would 
need to
take care of. I will list all those when I sent that patch for review.
>
> -ritesh

[-- Attachment #2: Type: text/html, Size: 13304 bytes --]