| 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
| ||
|
Date: Tue, 18 Jun 2024 09:04:03 +0800
From: Jason Wang <jasowang@...hat.com>
To: Xuan Zhuo <xuanzhuo@...ux.alibaba.com>
Cc: netdev@...r.kernel.org, "Michael S. Tsirkin" <mst@...hat.com>,
Eugenio Pérez <eperezma@...hat.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>, virtualization@...ts.linux.dev,
bpf@...r.kernel.org
Subject: Re: [PATCH net-next v5 09/15] virtio_net: xsk: bind/unbind xsk
On Mon, Jun 17, 2024 at 3:49 PM Xuan Zhuo <xuanzhuo@...ux.alibaba.com> wrote:
>
> On Mon, 17 Jun 2024 14:19:10 +0800, Jason Wang <jasowang@...hat.com> wrote:
> > On Fri, Jun 14, 2024 at 2:39 PM Xuan Zhuo <xuanzhuo@...ux.alibaba.com> wrote:
> > >
> > > This patch implement the logic of bind/unbind xsk pool to sq and rq.
> > >
> > > Signed-off-by: Xuan Zhuo <xuanzhuo@...ux.alibaba.com>
> > > ---
> > > drivers/net/virtio_net.c | 201 ++++++++++++++++++++++++++++++++++++++-
> > > 1 file changed, 200 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
> > > index 88ab9ea1646f..35fd8bca7fcf 100644
> > > --- a/drivers/net/virtio_net.c
> > > +++ b/drivers/net/virtio_net.c
> > > @@ -26,6 +26,7 @@
> > > #include <net/netdev_rx_queue.h>
> > > #include <net/netdev_queues.h>
> > > #include <uapi/linux/virtio_ring.h>
> > > +#include <net/xdp_sock_drv.h>
> > >
> > > static int napi_weight = NAPI_POLL_WEIGHT;
> > > module_param(napi_weight, int, 0444);
> > > @@ -57,6 +58,8 @@ DECLARE_EWMA(pkt_len, 0, 64)
> > >
> > > #define VIRTNET_DRIVER_VERSION "1.0.0"
> > >
> > > +static struct virtio_net_hdr_mrg_rxbuf xsk_hdr;
> >
> > Does this mean AF_XDP only supports virtio_net_hdr_mrg_rxbuf but not others?
>
> Sorry, this is the old code.
>
> Should be virtio_net_common_hdr.
>
> Here we should use the max size of the virtio-net header.
Better but still suboptimal, for example it could be extended as we're
adding more features?
>
> >
> > > +
> > > static const unsigned long guest_offloads[] = {
> > > VIRTIO_NET_F_GUEST_TSO4,
> > > VIRTIO_NET_F_GUEST_TSO6,
> > > @@ -321,6 +324,12 @@ struct send_queue {
> > > bool premapped;
> > >
> > > struct virtnet_sq_dma_info dmainfo;
> > > +
> > > + struct {
> > > + struct xsk_buff_pool *pool;
> > > +
> > > + dma_addr_t hdr_dma_address;
> > > + } xsk;
> > > };
> > >
> > > /* Internal representation of a receive virtqueue */
> > > @@ -372,6 +381,13 @@ struct receive_queue {
> > >
> > > /* Record the last dma info to free after new pages is allocated. */
> > > struct virtnet_rq_dma *last_dma;
> > > +
> > > + struct {
> > > + struct xsk_buff_pool *pool;
> > > +
> > > + /* xdp rxq used by xsk */
> > > + struct xdp_rxq_info xdp_rxq;
> > > + } xsk;
> > > };
> > >
> > > /* This structure can contain rss message with maximum settings for indirection table and keysize
> > > @@ -695,7 +711,7 @@ static void virtnet_sq_free_dma_meta(struct send_queue *sq)
> > > /* This function must be called immediately after creating the vq, or after vq
> > > * reset, and before adding any buffers to it.
> > > */
> > > -static __maybe_unused int virtnet_sq_set_premapped(struct send_queue *sq, bool premapped)
> > > +static int virtnet_sq_set_premapped(struct send_queue *sq, bool premapped)
> > > {
> > > if (premapped) {
> > > int r;
> > > @@ -5177,6 +5193,187 @@ static int virtnet_restore_guest_offloads(struct virtnet_info *vi)
> > > return virtnet_set_guest_offloads(vi, offloads);
> > > }
> > >
> > > +static int virtnet_rq_bind_xsk_pool(struct virtnet_info *vi, struct receive_queue *rq,
> > > + struct xsk_buff_pool *pool)
> > > +{
> > > + int err, qindex;
> > > +
> > > + qindex = rq - vi->rq;
> > > +
> > > + if (pool) {
> > > + err = xdp_rxq_info_reg(&rq->xsk.xdp_rxq, vi->dev, qindex, rq->napi.napi_id);
> > > + if (err < 0)
> > > + return err;
> > > +
> > > + err = xdp_rxq_info_reg_mem_model(&rq->xsk.xdp_rxq,
> > > + MEM_TYPE_XSK_BUFF_POOL, NULL);
> > > + if (err < 0) {
> > > + xdp_rxq_info_unreg(&rq->xsk.xdp_rxq);
> > > + return err;
> > > + }
> > > +
> > > + xsk_pool_set_rxq_info(pool, &rq->xsk.xdp_rxq);
> > > + }
> > > +
> > > + virtnet_rx_pause(vi, rq);
> > > +
> > > + err = virtqueue_reset(rq->vq, virtnet_rq_unmap_free_buf);
> > > + if (err) {
> > > + netdev_err(vi->dev, "reset rx fail: rx queue index: %d err: %d\n", qindex, err);
> > > +
> > > + pool = NULL;
> > > + }
> > > +
> > > + if (!pool)
> > > + xdp_rxq_info_unreg(&rq->xsk.xdp_rxq);
> >
> > Let's use err label instead of duplicating xdp_rxq_info_unreg() here?
> >
> > > +
> > > + rq->xsk.pool = pool;
> > > +
> > > + virtnet_rx_resume(vi, rq);
> > > +
> > > + return err;
> > > +}
> > > +
> > > +static int virtnet_sq_bind_xsk_pool(struct virtnet_info *vi,
> > > + struct send_queue *sq,
> > > + struct xsk_buff_pool *pool)
> > > +{
> > > + int err, qindex;
> > > +
> > > + qindex = sq - vi->sq;
> > > +
> > > + virtnet_tx_pause(vi, sq);
> > > +
> > > + err = virtqueue_reset(sq->vq, virtnet_sq_free_unused_buf);
> > > + if (err)
> > > + netdev_err(vi->dev, "reset tx fail: tx queue index: %d err: %d\n", qindex, err);
> > > + else
> > > + err = virtnet_sq_set_premapped(sq, !!pool);
> > > +
> > > + if (err)
> > > + pool = NULL;
> > > +
> > > + sq->xsk.pool = pool;
> > > +
> > > + virtnet_tx_resume(vi, sq);
> > > +
> > > + return err;
> > > +}
> > > +
> > > +static int virtnet_xsk_pool_enable(struct net_device *dev,
> > > + struct xsk_buff_pool *pool,
> > > + u16 qid)
> > > +{
> > > + struct virtnet_info *vi = netdev_priv(dev);
> > > + struct receive_queue *rq;
> > > + struct send_queue *sq;
> > > + struct device *dma_dev;
> > > + dma_addr_t hdr_dma;
> > > + int err;
> > > +
> > > + /* In big_packets mode, xdp cannot work, so there is no need to
> > > + * initialize xsk of rq.
> > > + *
> > > + * Support for small mode firstly.
> >
> > This comment is kind of confusing, I think mergeable mode is also
> > supported. If it's true, we can simply remove it.
>
> For the commit num limit of the net-next, I have to remove some commits.
>
> So the mergeable mode is not supported by this patch set.
>
> I plan to support the merge mode after this patch set.
Then, I'd suggest to split the patches into two series:
1) AF_XDP TX zerocopy
2) AF_XDP RX zerocopy
And implement both small and mergeable in series 2).
>
>
> >
> > > + */
> > > + if (vi->big_packets)
> > > + return -ENOENT;
> > > +
> > > + if (qid >= vi->curr_queue_pairs)
> > > + return -EINVAL;
> > > +
> > > + sq = &vi->sq[qid];
> > > + rq = &vi->rq[qid];
> > > +
> > > + /* xsk tx zerocopy depend on the tx napi.
> > > + *
> > > + * All xsk packets are actually consumed and sent out from the xsk tx
> > > + * queue under the tx napi mechanism.
> > > + */
> > > + if (!sq->napi.weight)
> > > + return -EPERM;
> > > +
> > > + /* For the xsk, the tx and rx should have the same device. But
> > > + * vq->dma_dev allows every vq has the respective dma dev. So I check
> > > + * the dma dev of vq and sq is the same dev.
> > > + */
> > > + if (virtqueue_dma_dev(rq->vq) != virtqueue_dma_dev(sq->vq))
> > > + return -EPERM;
> >
> > I don't understand how a different DMA device matters here. It looks
> > like the code is using per virtqueue DMA below.
>
> The af-xdp may use one buffer to receive from the rx and reuse this buffer to
> send by the tx. So the dma dev of sq and rq should be the same one.
Right, let's tweak the comment to say something like this.
Thanks
Powered by blists - more mailing lists