From: "Michael S. Tsirkin" <mst@redhat.com>
To: Jason Wang <jasowang@redhat.com>
Cc: virtualization@lists.linux-foundation.org
Subject: Re: [PATCH vhost 01/10] virtio_ring: split: refactor virtqueue_add_split() for premapped
Date: Mon, 20 Feb 2023 07:12:54 -0500 [thread overview]
Message-ID: <20230220071202-mutt-send-email-mst@kernel.org> (raw)
In-Reply-To: <CACGkMEtnVwTBzwmRGGXELNkds=b1K+crAmonyjn9=rM1R2-Fkw@mail.gmail.com>
On Mon, Feb 20, 2023 at 01:37:37PM +0800, Jason Wang wrote:
> On Tue, Feb 14, 2023 at 3:27 PM Xuan Zhuo <xuanzhuo@linux.alibaba.com> wrote:
> >
> > DMA-related logic is separated from the virtqueue_add_split to prepare
> > for subsequent support for premapped.
>
> The patch seems to do more than what is described here.
>
> To simplify reviewers, I'd suggest to split this patch into three:
>
> 1) virtqueue_add_split_prepare() (could we have a better name?)
> 2) virtqueue_map_sgs()
> 3) virtqueue_add_split_vring()
>
> (Or only factor DMA parts out, I haven't gone through the reset of the patches)
>
> Thanks
>
It's pretty small, even split is not mandatary imho.
But definitely please do document what is done fully.
> >
> > Signed-off-by: Xuan Zhuo <xuanzhuo@linux.alibaba.com>
> > ---
> > drivers/virtio/virtio_ring.c | 219 ++++++++++++++++++++++++-----------
> > 1 file changed, 152 insertions(+), 67 deletions(-)
> >
> > diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c
> > index 41144b5246a8..560ee30d942c 100644
> > --- a/drivers/virtio/virtio_ring.c
> > +++ b/drivers/virtio/virtio_ring.c
> > @@ -520,29 +520,83 @@ static inline unsigned int virtqueue_add_desc_split(struct virtqueue *vq,
> > return next;
> > }
> >
> > -static inline int virtqueue_add_split(struct virtqueue *_vq,
> > - struct scatterlist *sgs[],
> > - unsigned int total_sg,
> > - unsigned int out_sgs,
> > - unsigned int in_sgs,
> > - void *data,
> > - void *ctx,
> > - gfp_t gfp)
> > +static int virtqueue_map_sgs(struct vring_virtqueue *vq,
> > + struct scatterlist *sgs[],
> > + unsigned int total_sg,
> > + unsigned int out_sgs,
> > + unsigned int in_sgs)
> > {
> > - struct vring_virtqueue *vq = to_vvq(_vq);
> > struct scatterlist *sg;
> > - struct vring_desc *desc;
> > - unsigned int i, n, avail, descs_used, prev, err_idx;
> > - int head;
> > - bool indirect;
> > + unsigned int n;
> >
> > - START_USE(vq);
> > + for (n = 0; n < out_sgs; n++) {
> > + for (sg = sgs[n]; sg; sg = sg_next(sg)) {
> > + dma_addr_t addr = vring_map_one_sg(vq, sg, DMA_TO_DEVICE);
> > +
> > + if (vring_mapping_error(vq, addr))
> > + return -ENOMEM;
> > +
> > + sg->dma_address = addr;
> > + }
> > + }
> > + for (; 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, DMA_FROM_DEVICE);
> > +
> > + if (vring_mapping_error(vq, addr))
> > + return -ENOMEM;
> > +
> > + sg->dma_address = addr;
> > + }
> > + }
> > +
> > + return 0;
> > +}
> > +
> > +static void virtqueue_unmap_sgs(struct vring_virtqueue *vq,
> > + struct scatterlist *sgs[],
> > + unsigned int total_sg,
> > + unsigned int out_sgs,
> > + unsigned int in_sgs)
> > +{
> > + struct scatterlist *sg;
> > + unsigned int n;
> > +
> > + for (n = 0; n < out_sgs; n++) {
> > + for (sg = sgs[n]; sg; sg = sg_next(sg)) {
> > + if (!sg->dma_address)
> > + return;
> > +
> > + dma_unmap_single(vring_dma_dev(vq), sg->dma_address,
> > + sg->length, DMA_TO_DEVICE);
> > + }
> > + }
> > + for (; n < (out_sgs + in_sgs); n++) {
> > + for (sg = sgs[n]; sg; sg = sg_next(sg)) {
> > + if (!sg->dma_address)
> > + return;
> > +
> > + dma_unmap_single(vring_dma_dev(vq), sg->dma_address,
> > + sg->length, DMA_FROM_DEVICE);
> > + }
> > + }
> > +}
> > +
> > +static inline int virtqueue_add_split_prepare(struct vring_virtqueue *vq,
> > + unsigned int total_sg,
> > + unsigned int out_sgs,
> > + void *data,
> > + void *ctx,
> > + gfp_t gfp,
> > + struct vring_desc **pdesc)
> > +{
> > + struct vring_desc *desc;
> > + unsigned int descs_used;
> >
> > BUG_ON(data == NULL);
> > BUG_ON(ctx && vq->indirect);
> >
> > if (unlikely(vq->broken)) {
> > - END_USE(vq);
> > return -EIO;
> > }
> >
> > @@ -550,27 +604,17 @@ static inline int virtqueue_add_split(struct virtqueue *_vq,
> >
> > BUG_ON(total_sg == 0);
> >
> > - head = vq->free_head;
> > -
> > if (virtqueue_use_indirect(vq, total_sg))
> > - desc = alloc_indirect_split(_vq, total_sg, gfp);
> > + desc = alloc_indirect_split(&vq->vq, total_sg, gfp);
> > else {
> > desc = NULL;
> > WARN_ON_ONCE(total_sg > vq->split.vring.num && !vq->indirect);
> > }
> >
> > - if (desc) {
> > - /* Use a single buffer which doesn't continue */
> > - indirect = true;
> > - /* Set up rest to use this indirect table. */
> > - i = 0;
> > + if (desc)
> > descs_used = 1;
> > - } else {
> > - indirect = false;
> > - desc = vq->split.vring.desc;
> > - i = head;
> > + else
> > descs_used = total_sg;
> > - }
> >
> > if (unlikely(vq->vq.num_free < descs_used)) {
> > pr_debug("Can't add buf len %i - avail = %i\n",
> > @@ -580,38 +624,64 @@ static inline int virtqueue_add_split(struct virtqueue *_vq,
> > * host should service the ring ASAP. */
> > if (out_sgs)
> > vq->notify(&vq->vq);
> > - if (indirect)
> > - kfree(desc);
> > - END_USE(vq);
> > + kfree(desc);
> > return -ENOSPC;
> > }
> >
> > + *pdesc = desc;
> > +
> > + return 0;
> > +}
> > +
> > +static inline int virtqueue_add_split_vring(struct vring_virtqueue *vq,
> > + struct scatterlist *sgs[],
> > + unsigned int total_sg,
> > + unsigned int out_sgs,
> > + unsigned int in_sgs,
> > + struct vring_desc *desc)
> > +{
> > + unsigned int n, i, avail, descs_used, prev;
> > + struct virtqueue *_vq = &vq->vq;
> > + struct scatterlist *sg;
> > + bool indirect;
> > + int head;
> > +
> > + head = vq->free_head;
> > +
> > + if (desc) {
> > + /* Use a single buffer which doesn't continue */
> > + indirect = true;
> > + /* Set up rest to use this indirect table. */
> > + i = 0;
> > + descs_used = 1;
> > + } else {
> > + indirect = false;
> > + desc = vq->split.vring.desc;
> > + i = head;
> > + descs_used = total_sg;
> > + }
> > +
> > for (n = 0; n < out_sgs; n++) {
> > for (sg = sgs[n]; sg; sg = sg_next(sg)) {
> > - dma_addr_t addr = vring_map_one_sg(vq, sg, DMA_TO_DEVICE);
> > - if (vring_mapping_error(vq, addr))
> > - goto unmap_release;
> > -
> > prev = i;
> > /* Note that we trust indirect descriptor
> > * table since it use stream DMA mapping.
> > */
> > - i = virtqueue_add_desc_split(_vq, desc, i, addr, sg->length,
> > + i = virtqueue_add_desc_split(_vq, desc, i,
> > + sg->dma_address,
> > + sg->length,
> > VRING_DESC_F_NEXT,
> > indirect);
> > }
> > }
> > for (; 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, DMA_FROM_DEVICE);
> > - if (vring_mapping_error(vq, addr))
> > - goto unmap_release;
> > -
> > prev = i;
> > /* Note that we trust indirect descriptor
> > * table since it use stream DMA mapping.
> > */
> > - i = virtqueue_add_desc_split(_vq, desc, i, addr,
> > + i = virtqueue_add_desc_split(_vq, desc, i,
> > + sg->dma_address,
> > sg->length,
> > VRING_DESC_F_NEXT |
> > VRING_DESC_F_WRITE,
> > @@ -630,7 +700,7 @@ static inline int virtqueue_add_split(struct virtqueue *_vq,
> > vq, desc, total_sg * sizeof(struct vring_desc),
> > DMA_TO_DEVICE);
> > if (vring_mapping_error(vq, addr))
> > - goto unmap_release;
> > + return -ENOMEM;
> >
> > virtqueue_add_desc_split(_vq, vq->split.vring.desc,
> > head, addr,
> > @@ -648,13 +718,6 @@ static inline int virtqueue_add_split(struct virtqueue *_vq,
> > else
> > vq->free_head = i;
> >
> > - /* Store token and indirect buffer state. */
> > - vq->split.desc_state[head].data = data;
> > - if (indirect)
> > - vq->split.desc_state[head].indir_desc = desc;
> > - else
> > - vq->split.desc_state[head].indir_desc = ctx;
> > -
> > /* Put entry in available array (but don't update avail->idx until they
> > * do sync). */
> > avail = vq->split.avail_idx_shadow & (vq->split.vring.num - 1);
> > @@ -677,30 +740,52 @@ static inline int virtqueue_add_split(struct virtqueue *_vq,
> > virtqueue_kick(_vq);
> >
> > return 0;
> > +}
> >
> > -unmap_release:
> > - err_idx = i;
> > +static inline int virtqueue_add_split(struct virtqueue *_vq,
> > + struct scatterlist *sgs[],
> > + unsigned int total_sg,
> > + unsigned int out_sgs,
> > + unsigned int in_sgs,
> > + void *data,
> > + void *ctx,
> > + gfp_t gfp)
> > +{
> > + struct vring_virtqueue *vq = to_vvq(_vq);
> > + struct vring_desc *desc;
> > + int head;
> > + int err;
> >
> > - if (indirect)
> > - i = 0;
> > - else
> > - i = head;
> > + START_USE(vq);
> >
> > - for (n = 0; n < total_sg; n++) {
> > - if (i == err_idx)
> > - break;
> > - if (indirect) {
> > - vring_unmap_one_split_indirect(vq, &desc[i]);
> > - i = virtio16_to_cpu(_vq->vdev, desc[i].next);
> > - } else
> > - i = vring_unmap_one_split(vq, i);
> > - }
> > + /* check vq state and try to alloc desc for indirect. */
> > + err = virtqueue_add_split_prepare(vq, total_sg, out_sgs, data, ctx, gfp, &desc);
> > + if (err)
> > + goto end;
> >
> > - if (indirect)
> > - kfree(desc);
> > + err = virtqueue_map_sgs(vq, sgs, total_sg, out_sgs, in_sgs);
> > + if (err)
> > + goto err;
> >
> > + head = vq->free_head;
> > + err = virtqueue_add_split_vring(vq, sgs, total_sg, out_sgs, in_sgs, desc);
> > + if (err)
> > + goto err;
> > +
> > + /* Store token and indirect buffer state. */
> > + vq->split.desc_state[head].data = data;
> > + vq->split.desc_state[head].indir_desc = desc ? desc : ctx;
> > +
> > + goto end;
> > +
> > +err:
> > + virtqueue_unmap_sgs(vq, sgs, total_sg, out_sgs, in_sgs);
> > +
> > + kfree(desc);
> > +
> > +end:
> > END_USE(vq);
> > - return -ENOMEM;
> > + return err;
> > }
> >
> > static bool virtqueue_kick_prepare_split(struct virtqueue *_vq)
> > --
> > 2.32.0.3.g01195cf9f
> >
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
next prev parent reply other threads:[~2023-02-20 12:13 UTC|newest]
Thread overview: 46+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-02-14 7:26 [PATCH vhost 00/10] virtio core prepares for AF_XDP Xuan Zhuo
2023-02-14 7:26 ` [PATCH vhost 01/10] virtio_ring: split: refactor virtqueue_add_split() for premapped Xuan Zhuo
2023-02-20 5:37 ` Jason Wang
2023-02-20 6:57 ` Xuan Zhuo
2023-02-20 12:12 ` Michael S. Tsirkin [this message]
2023-02-14 7:26 ` [PATCH vhost 02/10] virtio_ring: packed: separate prepare code from virtuque_add_indirect_packed() Xuan Zhuo
2023-02-20 5:37 ` Jason Wang
2023-02-20 6:56 ` Xuan Zhuo
2023-02-14 7:26 ` [PATCH vhost 03/10] virtio_ring: packed: refactor virtqueue_add_packed() for premapped Xuan Zhuo
2023-02-20 5:37 ` Jason Wang
2023-02-14 7:26 ` [PATCH vhost 04/10] virtio_ring: split: introduce virtqueue_add_split_premapped() Xuan Zhuo
2023-02-20 5:38 ` Jason Wang
2023-02-20 6:43 ` Xuan Zhuo
2023-02-21 1:49 ` Jason Wang
2023-02-14 7:26 ` [PATCH vhost 05/10] virtio_ring: packed: introduce virtqueue_add_packed_premapped() Xuan Zhuo
2023-02-14 7:27 ` [PATCH vhost 06/10] virtio_ring: introduce virtqueue_add_inbuf_premapped() Xuan Zhuo
2023-02-14 7:27 ` [PATCH vhost 07/10] virtio_ring: add api virtio_dma_map() for advance dma Xuan Zhuo
2023-02-20 5:38 ` Jason Wang
2023-02-20 6:59 ` Xuan Zhuo
2023-02-21 1:51 ` Jason Wang
2023-02-28 11:15 ` Xuan Zhuo
2023-03-02 2:04 ` Xuan Zhuo
2023-03-02 3:05 ` Jason Wang
2023-03-02 3:21 ` Xuan Zhuo
2023-03-02 3:26 ` Jason Wang
2023-03-02 6:09 ` Michael S. Tsirkin
2023-03-02 6:43 ` Xuan Zhuo
2023-03-02 6:56 ` Jason Wang
2023-03-02 7:31 ` Xuan Zhuo
2023-03-02 7:56 ` Jason Wang
2023-03-02 11:08 ` Xuan Zhuo
2023-03-01 11:47 ` Xuan Zhuo
2023-02-14 7:27 ` [PATCH vhost 08/10] virtio_ring: introduce dma sync api for virtio Xuan Zhuo
2023-02-20 5:38 ` Jason Wang
2023-02-20 7:04 ` Xuan Zhuo
2023-02-21 1:52 ` Jason Wang
2023-02-14 7:27 ` [PATCH vhost 09/10] virtio_ring: correct the expression of the description of virtqueue_resize() Xuan Zhuo
2023-02-20 5:38 ` Jason Wang
2023-02-14 7:27 ` [PATCH vhost 10/10] virtio_ring: introduce virtqueue_reset() Xuan Zhuo
2023-02-20 5:38 ` Jason Wang
2023-02-20 7:03 ` Xuan Zhuo
2023-02-21 1:51 ` Jason Wang
2023-02-16 5:27 ` [PATCH vhost 00/10] virtio core prepares for AF_XDP Jason Wang
2023-02-16 11:46 ` Xuan Zhuo
2023-02-17 5:23 ` Jason Wang
2023-02-17 9:02 ` Xuan Zhuo
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=20230220071202-mutt-send-email-mst@kernel.org \
--to=mst@redhat.com \
--cc=jasowang@redhat.com \
--cc=virtualization@lists.linux-foundation.org \
/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 an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.