lkml.org 
[lkml]   [2022]   [Sep]   [1]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
SubjectRe: [PATCH v2] Revert "swiotlb: panic if nslabs is too small"
From
Date
Hi Robin,

On 9/1/22 5:24 AM, Robin Murphy wrote:
> On 2022-09-01 03:18, Dongli Zhang wrote:
>> Hi Yu,
>>
>> On 8/31/22 5:24 PM, Yu Zhao wrote:
>>> On Wed, Aug 31, 2022 at 4:20 PM Dongli Zhang <dongli.zhang@oracle.com> wrote:
>>>>
>>>> Hi Yu,
>>>>
>>>> As we discussed in the past, the swiotlb panic on purpose
>>>
>>> We should not panic() at all, especially on a platform that has been
>>> working well since at least 4.14.
>>>
>>> Did you check out this link I previously shared with you?
>>> https://urldefense.com/v3/__https://lore.kernel.org/r/CAHk-=wit-DmhMfQErY29JSPjFgebx_Ld*pnerc4J2Ag990WwAA@mail.gmail.com/__;Kw!!ACWV5N9M2RV99hQ!PXzSLurBv7VqxI1451TV4zO3_-BYj4grk-HYBsXzSnA6nZcXaBzdsQ-rF2DAqlICSRPMt-efYv_Uu2A2CQ$
>>>
>>
>> Thanks for sharing! I used to read that in the past. To be honest I am still
>> confused on when to use BUG/panic and when to not, as I still see many usage in
>> some patches.
>>
>> Just about swiotlb, it may panic in many cases during boot, e.g.,:
>>
>> https://urldefense.com/v3/__https://bugs.launchpad.net/ubuntu/*source/linux/*bug/1955655__;Kys!!ACWV5N9M2RV99hQ!MO4S9r0PisrW6Z-kZqUitAMbuGNMX6aceQd5VApqXllP6f1jaPCyL9x50um7chsn2uGL_pNqdBzTjZK5GebeKrI$ 
>
>
> That's really a different thing, but frankly that panic is also bogus anyway -
> there is no good reason to have different behaviour for failing to allocate a
> buffer slot because the buffer is full vs. failing to allocate a buffer slot
> because there is no buffer. If we can fail gracefully some of the time we should
> fail gracefully all of the time. Yes, there's a slight difference in that one

Thank you very much for the feedback!

Currently the swiotlb remap logic is more gracefully to handle failure, to
re-try with smaller nslabs (line 352), but will still panic at the end if the
value is reduced to smaller than IO_TLB_MIN_SLABS.

337 retry:
338 bytes = PAGE_ALIGN(nslabs << IO_TLB_SHIFT);
339 if (flags & SWIOTLB_ANY)
340 tlb = memblock_alloc(bytes, PAGE_SIZE);
341 else
342 tlb = memblock_alloc_low(bytes, PAGE_SIZE);
343 if (!tlb) {
344 pr_warn("%s: Failed to allocate %zu bytes tlb structure\n",
345 __func__, bytes);
346 return;
347 }
348
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 }

> case has a chance of succeeding if retried in future while the other definitely
> doesn't, but it's not SWIOTLB's place to decide that the entire system is
> terminally unusable just because some device can't make a DMA mapping.
Currently the swiotlb panic at line 735 on purpose if swiotlb initialization is
failed, but when any DMA requires a swiotlb mapped slot.

724 phys_addr_t swiotlb_tbl_map_single(struct device *dev, phys_addr_t orig_addr,
725 size_t mapping_size, size_t alloc_size,
726 unsigned int alloc_align_mask, enum dma_data_direction dir,
727 unsigned long attrs)
728 {
729 struct io_tlb_mem *mem = dev->dma_io_tlb_mem;
730 unsigned int offset = swiotlb_align_offset(dev, orig_addr);
731 unsigned int i;
732 int index;
733 phys_addr_t tlb_addr;
734
735 if (!mem || !mem->nslabs)
736 panic("Can not allocate SWIOTLB buffer earlier and can't
now provide you with the DMA bounce buffer");

I agree it is a very interesting point whether it is a good idea to leave the
decision to swiotlb for a panic. One thing to take care is the driver should
gracefully handle the error returned by swiotlb. Otherwise, there may be IO hang
(e.g., hung_task) instead of an IO failure.

>
> Similarly for the other panics at init time, which seem to have originated from
> a mechanical refactoring of the memblock API with the expectation of preserving
> the existing behaviour at the time. Those have then just been moved around
> without anyone thinking to question why they're there, and if memblock *does*
> now return usable error information, why don't we start handling that error
> properly like we do in other init paths?
>
> Really there is no reason to panic anywhere in SWIOTLB. This is old code, things
> have moved on over the last 20 years, and we can and should do much better. I'll
> add this to my list of things to look at for cleanup once I find a bit of free
> time, unless anyone else fancies taking it on.

Thank you very much for the feedback. Looking forward to how this can be improved!

Dongli Zhang

\
 
 \ /
  Last update: 2022-09-01 18:48    [W:0.055 / U:0.348 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site