Re: [dpdk-dev] [PATCH v2 3/6] vhost: log used vring changes

[Yuanhan Liu <yuanhan.liu@linux.intel.com>]

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