On 06/10/2021 16:23, Robin Murphy wrote:
> On 2021-10-06 14:10, Gerald Schaefer wrote:
>> On Fri, 1 Oct 2021 14:52:56 +0200
>> Gerald Schaefer wrote:
>>
>>> On Thu, 30 Sep 2021 15:37:33 +0200
>>> Karsten Graul wrote:
>>>
>>>> On 14/09/2021 17:45, Ioana Ciornei wrote:
>>>>> On Wed, Sep 08, 2021 at 10:33:26PM -0500, Jeremy Linton wrote:
>>>>>> +DPAA2, netdev maintainers
>>>>>> Hi,
>>>>>>
>>>>>> On 5/18/21 7:54 AM, Hamza Mahfooz wrote:
>>>>>>> Since, overlapping mappings are not supported by the DMA API we should
>>>>>>> report an error if active_cacheline_insert returns -EEXIST.
>>>>>>
>>>>>> It seems this patch found a victim. I was trying to run iperf3 on a
>>>>>> honeycomb (5.14.0, fedora 35) and the console is blasting this error
>>>>>> message
>>>>>> at 100% cpu. So, I changed it to a WARN_ONCE() to get the call trace,
>>>>>> which
>>>>>> is attached below.
>>>>>>
>>>>>
>>>>> These frags are allocated by the stack, transformed into a scatterlist
>>>>> by skb_to_sgvec and then DMA mapped with dma_map_sg. It was not the
>>>>> dpaa2-eth's decision to use two fragments from the same page (that will
>>>>> also end un in the same cacheline) in two different in-flight skbs.
>>>>>
>>>>> Is this behavior normal?
>>>>>
>>>>
>>>> We see the same problem here and it started with 5.15-rc2 in our nightly
>>>> CI runs.
>>>> The CI has panic_on_warn enabled so we see the panic every day now.
>>>
>>> Adding a WARN for a case that be detected false-positive seems not
>>> acceptable, exactly for this reason (kernel panic on unaffected
>>> systems).
>>>
>>> So I guess it boils down to the question if the behavior that Ioana
>>> described is legit behavior, on a system that is dma coherent. We
>>> are apparently hitting the same scenario, although it could not yet be
>>> reproduced with debug printks for some reason.
>>>
>>> If the answer is yes, than please remove at lease the WARN, so that
>>> it will not make systems crash that behave valid, and have
>>> panic_on_warn set. Even a normal printk feels wrong to me in that
>>> case, it really sounds rather like you want to fix / better refine
>>> the overlap check, if you want to report anything here.
>>
>> Dan, Christoph, any opinion?
>>
>> So far it all looks a lot like a false positive, so could you please
>> see that those patches get reverted? I do wonder a bit why this is
>> not an issue for others, we surely cannot be the only ones running
>> CI with panic_on_warn.
>
> What convinces you it's a false-positive? I'm hardly familiar with most of
> that callstack, but it appears to be related to mlx5, and I know that exists
> on expansion cards which could be plugged into a system with non-coherent
> PCIe where partial cacheline overlap *would* be a real issue. Of course it's
> dubious that there are many real use-cases for plugging a NIC with a 4-figure
> price tag into a little i.MX8 or whatever, but the point is that it *should*
> still work correctly.
>
>> We would need to disable DEBUG_DMA if this WARN stays in, which
>> would be a shame. Of course, in theory, this might also indicate
>> some real bug, but there really is no sign of that so far.
>
> The whole point of DMA debug is to flag up things that you *do* get away with
> on the vast majority of systems, precisely because most testing happens on
> those systems rather than more esoteric embedded setups. Say your system only
> uses dma-direct and a driver starts triggering the warning for not calling
> dma_mapping_error(), would you argue for removing that warning as well since
> dma_map_single() can't fail on your machine so it's "not a bug"?
>
>> Having multiple sg elements in the same page (or cacheline) is
>> valid, correct? And this is also not a decision of the driver
>> IIUC, so if it was bug, it should be addressed in common code,
>> correct?
>
> According to the streaming DMA API documentation, it is *not* valid:
>
> ".. warning::
>
> Memory coherency operates at a granularity called the cache
> line width. In order for memory mapped by this API to operate
> correctly, the mapped region must begin exactly on a cache line
> boundary and end ex
