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

[Ritesh Harjani (IBM) <ritesh.list@gmail.com>]

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?


>> 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.

>
> 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>

>
> 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.

-ritesh