[dpdk-dev] [PATCH v2 3/6] vhost: log used vring changes
Yuanhan Liu
yuanhan.liu at linux.intel.com
Tue Dec 22 08:13:49 CET 2015
On Tue, Dec 22, 2015 at 02:55:52PM +0800, Peter Xu wrote:
> On Thu, Dec 17, 2015 at 11:11:58AM +0800, Yuanhan Liu wrote:
> > +static inline void __attribute__((always_inline))
> > +vhost_log_used_vring(struct virtio_net *dev, struct vhost_virtqueue *vq,
> > + uint64_t offset, uint64_t len)
> > +{
>
> One thing optional: I feel it a little bit confusing regarding to
> the helper function name here, for the reasons:
>
> 1. It more sounds like "logging all the vrings we used", however,
> what I understand is that, here we are logging dirty pages for
> guest memory. Or say, there is merely nothing to do directly with
> vring (although many vring ops might call this function, we are
> only passing [buf, len] pairs).
>
> 2. It may also let people think of "vring_used", which is part of
> virtio protocol. However, it does not mean it too.
Yes, it does. Here log_guest_addr is set to physical address of
vring_used. (check code at vhost_virtqueue_set_addr() of qemu).
627 static int vhost_virtqueue_set_addr(struct vhost_dev *dev,
628 struct vhost_virtqueue *vq,
629 unsigned idx, bool enable_log)
630 {
631 struct vhost_vring_addr addr = {
632 .index = idx,
633 .desc_user_addr = (uint64_t)(unsigned long)vq->desc,
634 .avail_user_addr = (uint64_t)(unsigned long)vq->avail,
635 .used_user_addr = (uint64_t)(unsigned long)vq->used,
==> 636 .log_guest_addr = vq->used_phys,
637 .flags = enable_log ? (1 << VHOST_VRING_F_LOG) : 0,
638 };
So, this function does log changes to used vring.
>
> I would suggest a better name without confusion, like
> vhost_log_dirty_range() or anything else to avoid those keywords.
>
> > + uint64_t addr;
> > +
> > + addr = vq->log_guest_addr + offset;
> > + vhost_log_write(dev, addr, len);
>
> Optional too: since addr is only used once, would it cleaner using
> one line? Like:
Yes, it is. Will fix it.
>
> vhost_log_write(dev, vq->log_guest_addr + offset, len);
>
> > +}
> > +
> > /**
> > * This function adds buffers to the virtio devices RX virtqueue. Buffers can
> > * be received from the physical port or from another virtio device. A packet
> > @@ -129,6 +139,7 @@ virtio_dev_rx(struct virtio_net *dev, uint16_t queue_id,
> > uint32_t offset = 0, vb_offset = 0;
> > uint32_t pkt_len, len_to_cpy, data_len, total_copied = 0;
> > uint8_t hdr = 0, uncompleted_pkt = 0;
> > + uint16_t idx;
> >
> > /* Get descriptor from available ring */
> > desc = &vq->desc[head[packet_success]];
> > @@ -200,16 +211,18 @@ virtio_dev_rx(struct virtio_net *dev, uint16_t queue_id,
> > }
> >
> > /* Update used ring with desc information */
> > - vq->used->ring[res_cur_idx & (vq->size - 1)].id =
> > - head[packet_success];
> > + idx = res_cur_idx & (vq->size - 1);
> > + vq->used->ring[idx].id = head[packet_success];
> >
> > /* Drop the packet if it is uncompleted */
> > if (unlikely(uncompleted_pkt == 1))
> > - vq->used->ring[res_cur_idx & (vq->size - 1)].len =
> > - vq->vhost_hlen;
> > + vq->used->ring[idx].len = vq->vhost_hlen;
> > else
> > - vq->used->ring[res_cur_idx & (vq->size - 1)].len =
> > - pkt_len + vq->vhost_hlen;
> > + vq->used->ring[idx].len = pkt_len + vq->vhost_hlen;
> > +
> > + vhost_log_used_vring(dev, vq,
> > + offsetof(struct vring_used, ring[idx]),
> > + sizeof(vq->used->ring[idx]));
>
> Got a question here:
>
> I see that we are logging down changes when we are marking
> used_vring. Do we need to log down buffer copy in rte_memcpy() too?
> I am not sure whether I understand it correctly, it seems that this
> is part of DPDK API ops to deliver data to the guest (from, e.g.,
> OVS?), when we do rte_memcpy(), we seems to be modifying guest
> memory too. Am I wrong?
It's done in the next patch.
--yliu
More information about the dev
mailing list