On Wed, 19 Apr 2023 15:22:39 +0200, Alexander Lobakin <aleksander.lobakin at
intel.com> wrote:> From: Xuan Zhuo <xuanzhuo at linux.alibaba.com>
> Date: Mon, 17 Apr 2023 11:27:50 +0800
>
> > The purpose of this patch is to allow driver pass the own dma_ops to
> > xsk.
> >
> > This is to cope with the scene of virtio-net. If virtio does not have
> > VIRTIO_F_ACCESS_PLATFORM, then virtio cannot use DMA API. In this
case,
> > XSK cannot use DMA API directly to achieve DMA address. Based on this
> > scene, we must let XSK support driver to use the driver's dma_ops.
> >
> > On the other hand, the implementation of XSK as a highlevel code
> > should put the underlying operation of DMA to the driver layer.
> > The driver layer determines the implementation of the final DMA. XSK
> > should not make such assumptions. Everything will be simplified if DMA
> > is done at the driver level.
> >
> > More is here:
> >
https://lore.kernel.org/virtualization/1681265026.6082013-1-xuanzhuo at
linux.alibaba.com/
> >
> > Signed-off-by: Xuan Zhuo <xuanzhuo at linux.alibaba.com>
>
> [...]
>
> > struct xsk_buff_pool {
> > /* Members only used in the control path first. */
> > struct device *dev;
> > @@ -85,6 +102,7 @@ struct xsk_buff_pool {
> > * sockets share a single cq when the same netdev and queue id is
shared.
> > */
> > spinlock_t cq_lock;
> > + struct xsk_dma_ops dma_ops;
>
> Why full struct, not a const pointer? You'll have indirect calls either
> way, copying the full struct won't reclaim you much performance.
>
> > struct xdp_buff_xsk *free_heads[];
> > };
> >
>
> [...]
>
> > @@ -424,18 +426,29 @@ int xp_dma_map(struct xsk_buff_pool *pool,
struct device *dev,
> > return 0;
> > }
> >
> > + if (!dma_ops) {
> > + pool->dma_ops.map_page = dma_map_page_attrs;
> > + pool->dma_ops.mapping_error = dma_mapping_error;
> > + pool->dma_ops.need_sync = dma_need_sync;
> > + pool->dma_ops.sync_single_range_for_device =
dma_sync_single_range_for_device;
> > + pool->dma_ops.sync_single_range_for_cpu =
dma_sync_single_range_for_cpu;
> > + dma_ops = &pool->dma_ops;
> > + } else {
> > + pool->dma_ops = *dma_ops;
> > + }
>
> If DMA syncs are not needed on your x86_64 DMA-coherent system, it
> doesn't mean we all don't need it. Instead of filling pointers with
> "default" callbacks, you could instead avoid indirect calls at
all when
> no custom DMA ops are specified. Pls see how for example Christoph did
> that for direct DMA. It would cost only one if-else for case without
> custom DMA ops here instead of an indirect call each time.
>
> (I *could* suggest using INDIRECT_CALL_WRAPPER(), but I won't, since
> it's more expensive than direct checking and I feel like it's more
> appropriate to check directly here)
OK, I will fix it in next version.
Thanks.
>
> > +
> > dma_map = xp_create_dma_map(dev, pool->netdev, nr_pages,
pool->umem);
> > if (!dma_map)
> > return -ENOMEM;
> [...]
>
> Thanks,
> Olek