Re: [PATCH v1 4/4] swiotlb: panic if nslabs is too small

[Dongli Zhang]

Hi Yu,

On 8/22/22 4:10 PM, Yu Zhao wrote:
> On Mon, Aug 22, 2022 at 4:28 PM Dongli Zhang <dongli.zhang@xxxxxxxxxx> wrote:
>>
>> Hi Yu, Robin and Christoph,
>>
>> The mips kernel panic because the SWIOTLB buffer is adjusted to a very small
>> value (< 1MB, or < 512-slot), so that the swiotlb panic on purpose.
>>
>> software IO TLB: SWIOTLB bounce buffer size adjusted to 0MB
>> software IO TLB: area num 1.
>> Kernel panic - not syncing: swiotlb_init_remap: nslabs = 128 too small
>>
>>
>> From mips code, the 'swiotlbsize' is set to PAGE_SIZE initially. It is always
>> PAGE_SIZE unless it is used by CONFIG_PCI or CONFIG_USB_OHCI_HCD_PLATFORM.
>>
>> Finally, the swiotlb panic on purpose.
>>
>> 189 void __init plat_swiotlb_setup(void)
>> 190 {
>> ... ...
>> 211         swiotlbsize = PAGE_SIZE;
>> 212
>> 213 #ifdef CONFIG_PCI
>> 214         /*
>> 215          * For OCTEON_DMA_BAR_TYPE_SMALL, size the iotlb at 1/4 memory
>> 216          * size to a maximum of 64MB
>> 217          */
>> 218         if (OCTEON_IS_MODEL(OCTEON_CN31XX)
>> 219             || OCTEON_IS_MODEL(OCTEON_CN38XX_PASS2)) {
>> 220                 swiotlbsize = addr_size / 4;
>> 221                 if (swiotlbsize > 64 * (1<<20))
>> 222                         swiotlbsize = 64 * (1<<20);
>> 223         } else if (max_addr > 0xf0000000ul) {
>> 224                 /*
>> 225                  * Otherwise only allocate a big iotlb if there is
>> 226                  * memory past the BAR1 hole.
>> 227                  */
>> 228                 swiotlbsize = 64 * (1<<20);
>> 229         }
>> 230 #endif
>> 231 #ifdef CONFIG_USB_OHCI_HCD_PLATFORM
>> 232         /* OCTEON II ohci is only 32-bit. */
>> 233         if (OCTEON_IS_OCTEON2() && max_addr >= 0x100000000ul)
>> 234                 swiotlbsize = 64 * (1<<20);
>> 235 #endif
>> 236
>> 237         swiotlb_adjust_size(swiotlbsize);
>> 238         swiotlb_init(true, SWIOTLB_VERBOSE);
>> 239 }
>>
>>
>> Here are some thoughts. Would you mind suggesting which is the right way to go?
>>
>> 1. Will the PAGE_SIZE swiotlb be used by mips when it is only PAGE_SIZE? If it
>> is not used, why not disable swiotlb completely in the code?
>>
>> 2. The swiotlb panic on purpose when it is less then 1MB. Should we remove that
>> limitation?
>>
>> 3. ... or explicitly declare the limitation that: "swiotlb should be at least
>> 1MB, otherwise please do not use it"?
>>
>>
>> The reason I add the panic on purpose is for below case:
>>
>> The user's kernel is configured with very small swiotlb buffer. As a result, the
>> device driver may work abnormally.
> 
> Which driver? This sounds like that driver is broken, and we should
> fix that driver.

Any components that may use swiotlb. The driver is not broken.

The kernel is configured with very few swiotlb slots and obviously many drivers
will not be happy with it.

In the mips case, it is equivalent to swiotlb=2.

> 
>> As a result, the issue is reported to a
>> specific driver's developers, who spend some time to confirm it is swiotlb
>> issue.
> 
> Is this a fact or a hypothetical proposition?

I never encounter this in reality myself.

I always encounter the case that "swiotlb: No low mem" so that many drivers
cannot work well (because swiotlb is even not allocated). The OS hangs (and
reasons unknown to bug filer), e.g., the below:

https://bugs.launchpad.net/ubuntu/+source/linux/+bug/1983625

> 
>> Suppose those developers are not familiar with IOMMU/swiotlb, it takes
>> times until the root cause is identified.
> 
> Sorry but you are making quite a few assumptions in a series claimed
> to be "swiotlb: some cleanup" -- I personally expect cleanup patches
> not to have any runtime side effects.

I regarded this as cleanup because swiotlb may panic on purpose in the same
function in a different case (if statement) when the remap function is not able
to map > 1MB memory.

This patch is to sync with that part: line 353-354.

 349         if (remap && remap(tlb, nslabs) < 0) {
 350                 memblock_free(tlb, PAGE_ALIGN(bytes));
 351
 352                 nslabs = ALIGN(nslabs >> 1, IO_TLB_SEGSIZE);
 353                 if (nslabs < IO_TLB_MIN_SLABS)
 354                         panic("%s: Failed to remap %zu bytes\n",
 355                               __func__, bytes);
 356                 goto retry;
 357         }

> 
>> If we panic earlier, the issue will be reported to IOMMU/swiotlb developer.
> 
> Ok, I think we should at least revert this patch, if not the entire series.
> 
>> This
>> is also to sync with the remap failure logic in swiotlb (used by xen).
> 
> We can have it back in after we have better understood how it
> interacts with different archs/drivers, or better yet when the needs
> arise, if they arise at all.

We have already understood everything related to swiotlb. It is good to me to
revert the patch.

The questions are:

1. Is there any case that the OS may use < 1MB swiotlb buffer? E.g., the mips
only needs PAGE_SIZE buffer for DMA?

According to git log, the arch/mips/cavium-octeon/dma-octeon.c uses "swiotlb=2"
(e.g., 4KB PAGE_SIZE) dating back to 2010.

2. Should we panic pro-actively if the swiotlb user is allocating < 1MB buffer
and no one may use < 1MB buffer.

It is good to me to revert the patch. I will leave the decision to Christoph
whether to revert this patch.

Thank you very much!

Dongli Zhang