Re: [PATCH net-next] xsk: introduce xsk_dma_ops

[Alexander Lobakin <aleksander.lobakin@intel.com>]

From: Christoph Hellwig <hch@infradead.org>
Date: Wed, 19 Apr 2023 23:16:23 -0700

> On Wed, Apr 19, 2023 at 03:14:48PM +0200, Alexander Lobakin wrote:
>>>>> dma addresses and thus dma mappings are completely driver specific.
>>>>> Upper layers have no business looking at them.
>>
>> Here it's not an "upper layer". XSk core doesn't look at them or pass
>> them between several drivers.
> 
> Same for upper layers :)  The just do abstract operations that can sit
> on top of a variety of drivers.
> 
>> It maps DMA solely via the struct device
>> passed from the driver and then just gets-sets addresses for this driver
>> only. Just like Page Pool does for regular Rx buffers. This got moved to
>> the XSk core to not repeat the same code pattern in each driver.
> 
> Which assumes that:
> 
>  a) a DMA mapping needs to be done at all
>  b) it can be done using a struct device exposed to it
>  c) that DMA mapping is actually at the same granularity that it
>     operates on
> 
> all of which might not be true.
> 
>>> >From the original patchset I suspect it is dma mapping something very
>>> long term and then maybe doing syncs on it as needed?
>>
>> As I mentioned, XSk provides some handy wrappers to map DMA for drivers.
>> Previously, XSk was supported by real hardware drivers only, but here
>> the developer tries to add support to virtio-net. I suspect he needs to
>> use DMA mapping functions different from which the regular driver use.
> 
> Yes,  For actual hardware virtio and some more complex virtualized
> setups it works just like real hardware.  For legacy virtio there is 
> no DMA maping involved at all.  Because of that all DMA mapping needs
> to be done inside of virtio.
> 
>> So this is far from dma_map_ops, the author picked wrong name :D
>> And correct, for XSk we map one big piece of memory only once and then
>> reuse it for buffers, no inflight map/unmap on hotpath (only syncs when
>> needed). So this mapping is longterm and is stored in XSk core structure
>> assigned to the driver which this mapping was done for.
>> I think Jakub thinks of something similar, but for the "regular" Rx/Tx,
>> not only XDP sockets :)
> 
> FYI, dma_map_* is not intended for long term mappings, can lead
> to starvation issues.  You need to use dma_alloc_* instead.  And
> "you" in that case is as I said the driver, not an upper layer.
> If it's just helper called by drivers and never from core code,
> that's of course fine.

Hmm, currently almost all Ethernet drivers map Rx pages once and then
just recycle them, keeping the original DMA mapping. Which means pages
can have the same first mapping for very long time, often even for the
lifetime of the struct device. Same for XDP sockets, the lifetime of DMA
mappings equals the lifetime of sockets.
Does it mean we'd better review that approach and try switching to
dma_alloc_*() family (non-coherent/caching in our case)?
Also, I remember I tried to do that for one my driver, but the thing
that all those functions zero the whole page(s) before returning them to
the driver ruins the performance -- we don't need to zero buffers for
receiving packets and spend a ton of cycles on it (esp. in cases when 4k
gets zeroed each time, but your main body of traffic is 64-byte frames).

Thanks,
Olek