From: "Michael S. Tsirkin" <mst@redhat.com>
To: Xuan Zhuo <xuanzhuo@linux.alibaba.com>
Cc: netdev@vger.kernel.org, "David S. Miller" <davem@davemloft.net>,
"Eric Dumazet" <edumazet@google.com>,
"Jakub Kicinski" <kuba@kernel.org>,
"Paolo Abeni" <pabeni@redhat.com>,
"Jason Wang" <jasowang@redhat.com>,
"Björn Töpel" <bjorn@kernel.org>,
"Magnus Karlsson" <magnus.karlsson@intel.com>,
"Maciej Fijalkowski" <maciej.fijalkowski@intel.com>,
"Jonathan Lemon" <jonathan.lemon@gmail.com>,
"Alexei Starovoitov" <ast@kernel.org>,
"Daniel Borkmann" <daniel@iogearbox.net>,
"Jesper Dangaard Brouer" <hawk@kernel.org>,
"John Fastabend" <john.fastabend@gmail.com>,
"Sebastian Andrzej Siewior" <bigeasy@linutronix.de>,
"Menglong Dong" <imagedong@tencent.com>,
"Kuniyuki Iwashima" <kuniyu@amazon.com>,
"Petr Machata" <petrm@nvidia.com>,
virtualization@lists.linux-foundation.org, bpf@vger.kernel.org
Subject: Re: [PATCH 03/33] virtio_ring: packed: virtqueue_add_packed() support premapped
Date: Fri, 3 Feb 2023 04:16:45 -0500 [thread overview]
Message-ID: <20230203041006-mutt-send-email-mst@kernel.org> (raw)
In-Reply-To: <20230202110058.130695-4-xuanzhuo@linux.alibaba.com>
On Thu, Feb 02, 2023 at 07:00:28PM +0800, Xuan Zhuo wrote:
> virtqueue_add_packed() only supports virtual addresses, dma is completed
> in virtqueue_add_packed().
>
> In some scenarios (such as the AF_XDP scenario), the memory is allocated
> and DMA is completed in advance, so it is necessary for us to support
> passing the DMA address to virtqueue_add_packed().
>
> Record this information in desc_state, we can skip unmap based on this
> when executing dma unmap.
>
> Signed-off-by: Xuan Zhuo <xuanzhuo@linux.alibaba.com>
> ---
> drivers/virtio/virtio_ring.c | 71 +++++++++++++++++++++++++-----------
> 1 file changed, 50 insertions(+), 21 deletions(-)
>
> diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c
> index ec622403cbd5..25027a35fcf8 100644
> --- a/drivers/virtio/virtio_ring.c
> +++ b/drivers/virtio/virtio_ring.c
> @@ -78,6 +78,7 @@ struct vring_desc_state_packed {
> struct vring_packed_desc *indir_desc; /* Indirect descriptor, if any. */
> u16 num; /* Descriptor list length. */
> u16 last; /* The last desc state in a list. */
> + bool premapped;
> };
>
> struct vring_desc_extra {
That's an extra cache line.
> @@ -1200,7 +1201,8 @@ static inline u16 packed_last_used(u16 last_used_idx)
> }
>
> static void vring_unmap_extra_packed(const struct vring_virtqueue *vq,
> - struct vring_desc_extra *extra)
> + struct vring_desc_extra *extra,
> + bool premapped)
> {
> u16 flags;
>
> @@ -1215,6 +1217,9 @@ static void vring_unmap_extra_packed(const struct vring_virtqueue *vq,
> (flags & VRING_DESC_F_WRITE) ?
> DMA_FROM_DEVICE : DMA_TO_DEVICE);
> } else {
> + if (premapped)
> + return;
> +
> dma_unmap_page(vring_dma_dev(vq),
> extra->addr, extra->len,
> (flags & VRING_DESC_F_WRITE) ?
> @@ -1262,7 +1267,8 @@ static int virtqueue_add_indirect_packed(struct vring_virtqueue *vq,
> unsigned int out_sgs,
> unsigned int in_sgs,
> void *data,
> - gfp_t gfp)
> + gfp_t gfp,
> + bool premapped)
> {
> struct vring_packed_desc *desc;
> struct scatterlist *sg;
> @@ -1288,10 +1294,15 @@ static int virtqueue_add_indirect_packed(struct vring_virtqueue *vq,
>
> for (n = 0; n < out_sgs + in_sgs; n++) {
> for (sg = sgs[n]; sg; sg = sg_next(sg)) {
> - addr = vring_map_one_sg(vq, sg, n < out_sgs ?
> - DMA_TO_DEVICE : DMA_FROM_DEVICE);
> - if (vring_mapping_error(vq, addr))
> - goto unmap_release;
> + if (premapped) {
> + addr = sg_dma_address(sg);
> +
> + } else {
> + addr = vring_map_one_sg(vq, sg, n < out_sgs ?
> + DMA_TO_DEVICE : DMA_FROM_DEVICE);
> + if (vring_mapping_error(vq, addr))
> + goto unmap_release;
> + }
>
> desc[i].flags = cpu_to_le16(n < out_sgs ?
> 0 : VRING_DESC_F_WRITE);
> @@ -1350,6 +1361,7 @@ static int virtqueue_add_indirect_packed(struct vring_virtqueue *vq,
> vq->packed.desc_state[id].data = data;
> vq->packed.desc_state[id].indir_desc = desc;
> vq->packed.desc_state[id].last = id;
> + vq->packed.desc_state[id].premapped = premapped;
>
> vq->num_added += 1;
>
> @@ -1359,10 +1371,11 @@ static int virtqueue_add_indirect_packed(struct vring_virtqueue *vq,
> return 0;
>
> unmap_release:
> - err_idx = i;
> -
> - for (i = 0; i < err_idx; i++)
> - vring_unmap_desc_packed(vq, &desc[i]);
> + if (!premapped) {
> + err_idx = i;
> + for (i = 0; i < err_idx; i++)
> + vring_unmap_desc_packed(vq, &desc[i]);
> + }
>
> kfree(desc);
>
> @@ -1377,6 +1390,7 @@ static inline int virtqueue_add_packed(struct virtqueue *_vq,
> unsigned int in_sgs,
> void *data,
> void *ctx,
> + bool premapped,
> gfp_t gfp)
> {
> struct vring_virtqueue *vq = to_vvq(_vq);
> @@ -1403,7 +1417,7 @@ static inline int virtqueue_add_packed(struct virtqueue *_vq,
>
> if (virtqueue_use_indirect(vq, total_sg)) {
> err = virtqueue_add_indirect_packed(vq, sgs, total_sg, out_sgs,
> - in_sgs, data, gfp);
> + in_sgs, data, gfp, premapped);
> if (err != -ENOMEM) {
> END_USE(vq);
> return err;
> @@ -1435,10 +1449,17 @@ static inline int virtqueue_add_packed(struct virtqueue *_vq,
> c = 0;
> for (n = 0; n < out_sgs + in_sgs; n++) {
> for (sg = sgs[n]; sg; sg = sg_next(sg)) {
> - dma_addr_t addr = vring_map_one_sg(vq, sg, n < out_sgs ?
> - DMA_TO_DEVICE : DMA_FROM_DEVICE);
> - if (vring_mapping_error(vq, addr))
> - goto unmap_release;
> + dma_addr_t addr;
> +
> + if (premapped) {
> + addr = sg_dma_address(sg);
> +
drop this empty line pls.
> + } else {
> + addr = vring_map_one_sg(vq, sg, n < out_sgs ?
> + DMA_TO_DEVICE : DMA_FROM_DEVICE);
> + if (vring_mapping_error(vq, addr))
> + goto unmap_release;
> + }
>
> flags = cpu_to_le16(vq->packed.avail_used_flags |
> (++c == total_sg ? 0 : VRING_DESC_F_NEXT) |
> @@ -1485,6 +1506,7 @@ static inline int virtqueue_add_packed(struct virtqueue *_vq,
> vq->packed.desc_state[id].data = data;
> vq->packed.desc_state[id].indir_desc = ctx;
> vq->packed.desc_state[id].last = prev;
> + vq->packed.desc_state[id].premapped = premapped;
>
> /*
> * A driver MUST NOT make the first descriptor in the list
> @@ -1501,22 +1523,26 @@ static inline int virtqueue_add_packed(struct virtqueue *_vq,
> return 0;
>
> unmap_release:
> + vq->packed.avail_used_flags = avail_used_flags;
> +
> + if (premapped)
> + goto unmap_free;
> +
This goto branching inside error handling is too much like spaghetti code.
See Documentation/process/coding-style.rst for when goto is ok -
basically exit/error handling. This is not error handling.
Pls find a way to avoid.
> err_idx = i;
> i = head;
> curr = vq->free_head;
>
> - vq->packed.avail_used_flags = avail_used_flags;
> -
> for (n = 0; n < total_sg; n++) {
> if (i == err_idx)
> break;
> - vring_unmap_extra_packed(vq, &vq->packed.desc_extra[curr]);
> + vring_unmap_extra_packed(vq, &vq->packed.desc_extra[curr], false);
> curr = vq->packed.desc_extra[curr].next;
> i++;
> if (i >= vq->packed.vring.num)
> i = 0;
> }
>
> +unmap_free:
> END_USE(vq);
> return -EIO;
> }
> @@ -1576,8 +1602,10 @@ static void detach_buf_packed(struct vring_virtqueue *vq,
> struct vring_desc_state_packed *state = NULL;
> struct vring_packed_desc *desc;
> unsigned int i, curr;
> + bool premapped;
>
> state = &vq->packed.desc_state[id];
> + premapped = state->premapped;
>
> /* Clear data ptr. */
> state->data = NULL;
> @@ -1590,7 +1618,8 @@ static void detach_buf_packed(struct vring_virtqueue *vq,
> curr = id;
> for (i = 0; i < state->num; i++) {
> vring_unmap_extra_packed(vq,
> - &vq->packed.desc_extra[curr]);
> + &vq->packed.desc_extra[curr],
> + premapped);
> curr = vq->packed.desc_extra[curr].next;
> }
> }
> @@ -1603,7 +1632,7 @@ static void detach_buf_packed(struct vring_virtqueue *vq,
> if (!desc)
> return;
>
> - if (vq->use_dma_api) {
> + if (vq->use_dma_api && !premapped) {
> len = vq->packed.desc_extra[id].len;
> for (i = 0; i < len / sizeof(struct vring_packed_desc);
> i++)
> @@ -2122,7 +2151,7 @@ static inline int virtqueue_add(struct virtqueue *_vq,
> struct vring_virtqueue *vq = to_vvq(_vq);
>
> return vq->packed_ring ? virtqueue_add_packed(_vq, sgs, total_sg,
> - out_sgs, in_sgs, data, ctx, gfp) :
> + out_sgs, in_sgs, data, ctx, premapped, gfp) :
> virtqueue_add_split(_vq, sgs, total_sg,
> out_sgs, in_sgs, data, ctx, premapped, gfp);
> }
Too much if !premapped all over the place. Pls refactor so we
get common code and then have premapped and non premapped
versions call that.
> --
> 2.32.0.3.g01195cf9f
next prev parent reply other threads:[~2023-02-03 9:18 UTC|newest]
Thread overview: 79+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-02-02 11:00 [PATCH 00/33] virtio-net: support AF_XDP zero copy Xuan Zhuo
2023-02-02 11:00 ` [PATCH 01/33] virtio_ring: virtqueue_add() support premapped Xuan Zhuo
2023-02-02 11:00 ` [PATCH 02/33] virtio_ring: split: virtqueue_add_split() " Xuan Zhuo
2023-02-02 11:00 ` [PATCH 03/33] virtio_ring: packed: virtqueue_add_packed() " Xuan Zhuo
2023-02-03 9:16 ` Michael S. Tsirkin [this message]
2023-02-02 11:00 ` [PATCH 04/33] virtio_ring: introduce virtqueue_add_outbuf_premapped() Xuan Zhuo
2023-02-02 11:00 ` [PATCH 05/33] virtio_ring: introduce virtqueue_add_inbuf_premapped() Xuan Zhuo
2023-02-02 11:00 ` [PATCH 06/33] virtio_ring: introduce virtqueue_reset() Xuan Zhuo
2023-02-03 9:05 ` Michael S. Tsirkin
2023-02-03 9:09 ` Xuan Zhuo
2023-02-13 12:15 ` Michael S. Tsirkin
2023-02-14 1:53 ` Xuan Zhuo
2023-02-02 11:00 ` [PATCH 07/33] virtio_ring: add api virtio_dma_map() for advance dma Xuan Zhuo
2023-02-03 9:07 ` Michael S. Tsirkin
2023-02-02 11:00 ` [PATCH 08/33] virtio_ring: introduce dma sync api for virtio Xuan Zhuo
2023-02-02 12:44 ` Magnus Karlsson
2023-02-03 9:24 ` Michael S. Tsirkin
2023-02-02 11:00 ` [PATCH 09/33] xsk: xsk_buff_pool add callback for dma_sync Xuan Zhuo
2023-02-02 12:51 ` Magnus Karlsson
2023-02-03 7:01 ` Xuan Zhuo
2023-02-02 11:00 ` [PATCH 10/33] xsk: support virtio DMA map Xuan Zhuo
2023-02-05 22:04 ` kernel test robot
2023-02-02 11:00 ` [PATCH 11/33] virtio_net: rename free_old_xmit_skbs to free_old_xmit Xuan Zhuo
2023-02-02 11:00 ` [PATCH 12/33] virtio_net: unify the code for recycling the xmit ptr Xuan Zhuo
2023-02-02 11:00 ` [PATCH 13/33] virtio_net: virtnet_poll_tx support rescheduled Xuan Zhuo
2023-02-02 11:00 ` [PATCH 14/33] virtio_net: independent directory Xuan Zhuo
2023-02-02 11:00 ` [PATCH 15/33] virtio_net: move to virtio_net.h Xuan Zhuo
2023-02-03 8:53 ` Michael S. Tsirkin
2023-02-03 9:04 ` Xuan Zhuo
2023-02-03 9:26 ` Michael S. Tsirkin
2023-02-02 11:00 ` [PATCH 16/33] virtio_net: introduce virtnet_xdp_handler() to seprate the logic of run xdp Xuan Zhuo
2023-02-03 8:55 ` Michael S. Tsirkin
2023-02-03 9:01 ` Xuan Zhuo
2023-02-02 11:00 ` [PATCH 17/33] virtio_net: receive_small() use virtnet_xdp_handler() Xuan Zhuo
2023-02-02 11:00 ` [PATCH 18/33] virtio_net: receive_merageable() " Xuan Zhuo
2023-02-02 17:16 ` Michael S. Tsirkin
2023-02-02 11:00 ` [PATCH 19/33] virtio_net: introduce virtnet_tx_reset() Xuan Zhuo
2023-02-02 17:23 ` Michael S. Tsirkin
2023-02-03 4:35 ` Xuan Zhuo
2023-02-02 11:00 ` [PATCH 20/33] virtio_net: xsk: introduce virtnet_rq_bind_xsk_pool() Xuan Zhuo
2023-02-03 8:48 ` Michael S. Tsirkin
2023-02-03 8:52 ` Xuan Zhuo
2023-02-03 9:28 ` Michael S. Tsirkin
2023-02-02 11:00 ` [PATCH 21/33] virtio_net: xsk: introduce virtnet_xsk_pool_enable() Xuan Zhuo
2023-02-02 11:00 ` [PATCH 22/33] virtio_net: xsk: introduce xsk disable Xuan Zhuo
2023-02-02 23:02 ` kernel test robot
2023-02-12 7:56 ` kernel test robot
2023-02-02 11:00 ` [PATCH 23/33] virtio_net: xsk: support xsk setup Xuan Zhuo
2023-02-02 11:00 ` [PATCH 24/33] virtio_net: xsk: stop disable tx napi Xuan Zhuo
2023-02-02 17:25 ` Michael S. Tsirkin
2023-02-03 3:24 ` Xuan Zhuo
2023-02-03 8:33 ` Michael S. Tsirkin
2023-02-03 8:49 ` Xuan Zhuo
2023-02-03 9:29 ` Michael S. Tsirkin
2023-02-02 11:00 ` [PATCH 25/33] virtio_net: xsk: __free_old_xmit distinguishes xsk buffer Xuan Zhuo
2023-02-02 11:00 ` [PATCH 26/33] virtio_net: virtnet_sq_free_unused_buf() check " Xuan Zhuo
2023-02-02 11:00 ` [PATCH 27/33] virtio_net: virtnet_rq_free_unused_buf() " Xuan Zhuo
2023-02-02 11:00 ` [PATCH 28/33] net: introduce napi_tx_raise() Xuan Zhuo
2023-02-02 11:00 ` [PATCH 29/33] virtio_net: xsk: tx: support tx Xuan Zhuo
2023-02-03 8:39 ` Maciej Fijalkowski
2023-02-03 8:55 ` Xuan Zhuo
2023-02-02 11:00 ` [PATCH 30/33] virtio_net: xsk: tx: support wakeup Xuan Zhuo
2023-02-02 11:00 ` [PATCH 31/33] virtio_net: xsk: tx: auto wakeup when free old xmit Xuan Zhuo
2023-02-02 11:00 ` [PATCH 32/33] virtio_net: xsk: rx: introduce add_recvbuf_xsk() Xuan Zhuo
2023-02-03 8:43 ` Maciej Fijalkowski
2023-02-03 8:56 ` Xuan Zhuo
2023-02-02 11:00 ` [PATCH 33/33] virtio_net: xsk: rx: introduce receive_xsk() to recv xsk buffer Xuan Zhuo
2023-02-02 11:08 ` [PATCH 00/33] virtio-net: support AF_XDP zero copy Michael S. Tsirkin
2023-02-02 11:44 ` Xuan Zhuo
2023-02-03 9:08 ` Michael S. Tsirkin
2023-02-03 9:09 ` Xuan Zhuo
2023-02-02 14:41 ` Paolo Abeni
2023-02-03 3:33 ` Xuan Zhuo
2023-02-03 8:37 ` Michael S. Tsirkin
2023-02-03 8:46 ` Maciej Fijalkowski
2023-02-03 9:09 ` Michael S. Tsirkin
2023-02-03 9:17 ` Michael S. Tsirkin
2023-02-06 2:41 ` Xuan Zhuo
2023-02-13 12:14 ` Michael S. Tsirkin
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20230203041006-mutt-send-email-mst@kernel.org \
--to=mst@redhat.com \
--cc=ast@kernel.org \
--cc=bigeasy@linutronix.de \
--cc=bjorn@kernel.org \
--cc=bpf@vger.kernel.org \
--cc=daniel@iogearbox.net \
--cc=davem@davemloft.net \
--cc=edumazet@google.com \
--cc=hawk@kernel.org \
--cc=imagedong@tencent.com \
--cc=jasowang@redhat.com \
--cc=john.fastabend@gmail.com \
--cc=jonathan.lemon@gmail.com \
--cc=kuba@kernel.org \
--cc=kuniyu@amazon.com \
--cc=maciej.fijalkowski@intel.com \
--cc=magnus.karlsson@intel.com \
--cc=netdev@vger.kernel.org \
--cc=pabeni@redhat.com \
--cc=petrm@nvidia.com \
--cc=virtualization@lists.linux-foundation.org \
--cc=xuanzhuo@linux.alibaba.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).