netdev - Re: [PATCH net-next] xsk: introduce xsk_dma

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Wed, 19 Apr 2023 15:22:39 +0200
From:   Alexander Lobakin <aleksander.lobakin@...el.com>
To:     Xuan Zhuo <xuanzhuo@...ux.alibaba.com>
CC:     <netdev@...r.kernel.org>,
        Björn Töpel <bjorn@...nel.org>,
        Magnus Karlsson <magnus.karlsson@...el.com>,
        "Maciej Fijalkowski" <maciej.fijalkowski@...el.com>,
        Jonathan Lemon <jonathan.lemon@...il.com>,
        "David S. Miller" <davem@...emloft.net>,
        "Eric Dumazet" <edumazet@...gle.com>,
        Jakub Kicinski <kuba@...nel.org>,
        Paolo Abeni <pabeni@...hat.com>,
        Alexei Starovoitov <ast@...nel.org>,
        Daniel Borkmann <daniel@...earbox.net>,
        Jesper Dangaard Brouer <hawk@...nel.org>,
        "John Fastabend" <john.fastabend@...il.com>, <bpf@...r.kernel.org>,
        <virtualization@...ts.linux-foundation.org>,
        Jason Wang <jasowang@...hat.com>,
        "Michael S. Tsirkin" <mst@...hat.com>,
        Guenter Roeck <linux@...ck-us.net>,
        Gerd Hoffmann <kraxel@...hat.com>,
        Christoph Hellwig <hch@...radead.org>
Subject: Re: [PATCH net-next] xsk: introduce xsk_dma_ops

From: Xuan Zhuo <xuanzhuo@...ux.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@linux.alibaba.com/
> 
> Signed-off-by: Xuan Zhuo <xuanzhuo@...ux.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)

> +
>  	dma_map = xp_create_dma_map(dev, pool->netdev, nr_pages, pool->umem);
>  	if (!dma_map)
>  		return -ENOMEM;
[...]

Thanks,
Olek