On Mon, Jan 29, 2024 at 11:33 AM Xuan Zhuo <xuanzhuo@xxxxxxxxxxxxxxxxx> wrote:
>
> On Mon, 29 Jan 2024 11:07:50 +0800, Jason Wang <jasowang@xxxxxxxxxx> wrote:
> > On Thu, Jan 25, 2024 at 1:58 PM Xuan Zhuo <xuanzhuo@xxxxxxxxxxxxxxxxx> wrote:
> > >
> > > On Thu, 25 Jan 2024 11:39:03 +0800, Jason Wang <jasowang@xxxxxxxxxx> wrote:
> > > > On Tue, Jan 16, 2024 at 3:59 PM Xuan Zhuo <xuanzhuo@xxxxxxxxxxxxxxxxx> wrote:
> > > > >
> > > > > Introduce helper virtqueue_get_dma_premapped(), then the driver
> > > > > can know whether dma unmap is needed.
> > > > >
> > > > > Signed-off-by: Xuan Zhuo <xuanzhuo@xxxxxxxxxxxxxxxxx>
> > > > > ---
> > > > > drivers/net/virtio/main.c | 22 +++++++++-------------
> > > > > drivers/net/virtio/virtio_net.h | 3 ---
> > > > > drivers/virtio/virtio_ring.c | 22 ++++++++++++++++++++++
> > > > > include/linux/virtio.h | 1 +
> > > > > 4 files changed, 32 insertions(+), 16 deletions(-)
> > > > >
> > > > > diff --git a/drivers/net/virtio/main.c b/drivers/net/virtio/main.c
> > > > > index 186b2cf5d8fc..4fbf612da235 100644
> > > > > --- a/drivers/net/virtio/main.c
> > > > > +++ b/drivers/net/virtio/main.c
> > > > > @@ -483,7 +483,7 @@ static void *virtnet_rq_get_buf(struct virtnet_rq *rq, u32 *len, void **ctx)
> > > > > void *buf;
> > > > >
> > > > > buf = virtqueue_get_buf_ctx(rq->vq, len, ctx);
> > > > > - if (buf && rq->do_dma)
> > > > > + if (buf && virtqueue_get_dma_premapped(rq->vq))
> > > > > virtnet_rq_unmap(rq, buf, *len);
> > > > >
> > > > > return buf;
> > > > > @@ -496,7 +496,7 @@ static void virtnet_rq_init_one_sg(struct virtnet_rq *rq, void *buf, u32 len)
> > > > > u32 offset;
> > > > > void *head;
> > > > >
> > > > > - if (!rq->do_dma) {
> > > > > + if (!virtqueue_get_dma_premapped(rq->vq)) {
> > > > > sg_init_one(rq->sg, buf, len);
> > > > > return;
> > > > > }
> > > > > @@ -526,7 +526,7 @@ static void *virtnet_rq_alloc(struct virtnet_rq *rq, u32 size, gfp_t gfp)
> > > > >
> > > > > head = page_address(alloc_frag->page);
> > > > >
> > > > > - if (rq->do_dma) {
> > > > > + if (virtqueue_get_dma_premapped(rq->vq)) {
> > > > > dma = head;
> > > > >
> > > > > /* new pages */
> > > > > @@ -580,12 +580,8 @@ static void virtnet_rq_set_premapped(struct virtnet_info *vi)
> > > > > if (!vi->mergeable_rx_bufs && vi->big_packets)
> > > > > return;
> > > > >
> > > > > - for (i = 0; i < vi->max_queue_pairs; i++) {
> > > > > - if (virtqueue_set_dma_premapped(vi->rq[i].vq))
> > > > > - continue;
> > > > > -
> > > > > - vi->rq[i].do_dma = true;
> > > > > - }
> > > > > + for (i = 0; i < vi->max_queue_pairs; i++)
> > > > > + virtqueue_set_dma_premapped(vi->rq[i].vq);
> > > > > }
> > > > >
> > > > > static void free_old_xmit(struct virtnet_sq *sq, bool in_napi)
> > > > > @@ -1643,7 +1639,7 @@ static int add_recvbuf_small(struct virtnet_info *vi, struct virtnet_rq *rq,
> > > > >
> > > > > err = virtqueue_add_inbuf_ctx(rq->vq, rq->sg, 1, buf, ctx, gfp);
> > > > > if (err < 0) {
> > > > > - if (rq->do_dma)
> > > > > + if (virtqueue_get_dma_premapped(rq->vq))
> > > > > virtnet_rq_unmap(rq, buf, 0);
> > > > > put_page(virt_to_head_page(buf));
> > > > > }
> > > > > @@ -1758,7 +1754,7 @@ static int add_recvbuf_mergeable(struct virtnet_info *vi,
> > > > > ctx = mergeable_len_to_ctx(len + room, headroom);
> > > > > err = virtqueue_add_inbuf_ctx(rq->vq, rq->sg, 1, buf, ctx, gfp);
> > > > > if (err < 0) {
> > > > > - if (rq->do_dma)
> > > > > + if (virtqueue_get_dma_premapped(rq->vq))
> > > > > virtnet_rq_unmap(rq, buf, 0);
> > > > > put_page(virt_to_head_page(buf));
> > > > > }
> > > > > @@ -4007,7 +4003,7 @@ static void free_receive_page_frags(struct virtnet_info *vi)
> > > > > int i;
> > > > > for (i = 0; i < vi->max_queue_pairs; i++)
> > > > > if (vi->rq[i].alloc_frag.page) {
> > > > > - if (vi->rq[i].do_dma && vi->rq[i].last_dma)
> > > > > + if (virtqueue_get_dma_premapped(vi->rq[i].vq) && vi->rq[i].last_dma)
> > > > > virtnet_rq_unmap(&vi->rq[i], vi->rq[i].last_dma, 0);
> > > > > put_page(vi->rq[i].alloc_frag.page);
> > > > > }
> > > > > @@ -4035,7 +4031,7 @@ static void virtnet_rq_free_unused_bufs(struct virtqueue *vq)
> > > > > rq = &vi->rq[i];
> > > > >
> > > > > while ((buf = virtqueue_detach_unused_buf(vq)) != NULL) {
> > > > > - if (rq->do_dma)
> > > > > + if (virtqueue_get_dma_premapped(rq->vq))
> > > > > virtnet_rq_unmap(rq, buf, 0);
> > > > >
> > > > > virtnet_rq_free_buf(vi, rq, buf);
> > > > > diff --git a/drivers/net/virtio/virtio_net.h b/drivers/net/virtio/virtio_net.h
> > > > > index b28a4d0a3150..066a2b9d2b3c 100644
> > > > > --- a/drivers/net/virtio/virtio_net.h
> > > > > +++ b/drivers/net/virtio/virtio_net.h
> > > > > @@ -115,9 +115,6 @@ struct virtnet_rq {
> > > > >
> > > > > /* Record the last dma info to free after new pages is allocated. */
> > > > > struct virtnet_rq_dma *last_dma;
> > > > > -
> > > > > - /* Do dma by self */
> > > > > - bool do_dma;
> > > > > };
> > > > >
> > > > > struct virtnet_info {
> > > > > diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c
> > > > > index 2c5089d3b510..9092bcdebb53 100644
> > > > > --- a/drivers/virtio/virtio_ring.c
> > > > > +++ b/drivers/virtio/virtio_ring.c
> > > > > @@ -2905,6 +2905,28 @@ int virtqueue_set_dma_premapped(struct virtqueue *_vq)
> > > > > }
> > > > > EXPORT_SYMBOL_GPL(virtqueue_set_dma_premapped);
> > > > >
> > > > > +/**
> > > > > + * virtqueue_get_dma_premapped - get the vring premapped mode
> > > > > + * @_vq: the struct virtqueue we're talking about.
> > > > > + *
> > > > > + * Get the premapped mode of the vq.
> > > > > + *
> > > > > + * Returns bool for the vq premapped mode.
> > > > > + */
> > > > > +bool virtqueue_get_dma_premapped(struct virtqueue *_vq)
> > > > > +{
> > > > > + struct vring_virtqueue *vq = to_vvq(_vq);
> > > > > + bool premapped;
> > > > > +
> > > > > + START_USE(vq);
> > > > > + premapped = vq->premapped;
> > > > > + END_USE(vq);
> > > >
> > > > Why do we need to protect premapped like this? Is the user allowed to
> > > > change it on the fly?
> > >
> > >
> > > Just protect before accessing vq.
> >
> > I meant how did that differ from other booleans? E.g use_dma_api, do_unmap etc.
>
> Sorry, maybe I misunderstanded you.
>
> Do you mean, should we put "premapped" to the struct virtqueue?
> Then the user can read/write by the struct virtqueue directly?
>
> If that, the reason is that when set premapped, we must check
> use_dma_api.
I may not be very clear.
I meant why we should protect premapped with START_USE()/END_USER() here.
If it is set once during init_vqs we should not need that.
Thanks
