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

[Peter Xu <peterx@redhat.com>]

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.

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:

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?

Peter

>  
>  		res_cur_idx++;
>  		packet_success++;
> @@ -236,6 +249,9 @@ virtio_dev_rx(struct virtio_net *dev, uint16_t queue_id,
>  
>  	*(volatile uint16_t *)&vq->used->idx += count;
>  	vq->last_used_idx = res_end_idx;
> +	vhost_log_used_vring(dev, vq,
> +		offsetof(struct vring_used, idx),
> +		sizeof(vq->used->idx));
>  
>  	/* flush used->idx update before we read avail->flags. */
>  	rte_mb();
> @@ -265,6 +281,7 @@ copy_from_mbuf_to_vring(struct virtio_net *dev, uint32_t queue_id,
>  	uint32_t seg_avail;
>  	uint32_t vb_avail;
>  	uint32_t cpy_len, entry_len;
> +	uint16_t idx;
>  
>  	if (pkt == NULL)
>  		return 0;
> @@ -302,16 +319,18 @@ copy_from_mbuf_to_vring(struct virtio_net *dev, uint32_t queue_id,
>  	entry_len = vq->vhost_hlen;
>  
>  	if (vb_avail == 0) {
> -		uint32_t desc_idx =
> -			vq->buf_vec[vec_idx].desc_idx;
> +		uint32_t desc_idx = vq->buf_vec[vec_idx].desc_idx;
> +
> +		if ((vq->desc[desc_idx].flags & VRING_DESC_F_NEXT) == 0) {
> +			idx = cur_idx & (vq->size - 1);
>  
> -		if ((vq->desc[desc_idx].flags
> -			& VRING_DESC_F_NEXT) == 0) {
>  			/* Update used ring with desc information */
> -			vq->used->ring[cur_idx & (vq->size - 1)].id
> -				= vq->buf_vec[vec_idx].desc_idx;
> -			vq->used->ring[cur_idx & (vq->size - 1)].len
> -				= entry_len;
> +			vq->used->ring[idx].id = vq->buf_vec[vec_idx].desc_idx;
> +			vq->used->ring[idx].len = entry_len;
> +
> +			vhost_log_used_vring(dev, vq,
> +					offsetof(struct vring_used, ring[idx]),
> +					sizeof(vq->used->ring[idx]));
>  
>  			entry_len = 0;
>  			cur_idx++;
> @@ -354,10 +373,13 @@ copy_from_mbuf_to_vring(struct virtio_net *dev, uint32_t queue_id,
>  			if ((vq->desc[vq->buf_vec[vec_idx].desc_idx].flags &
>  				VRING_DESC_F_NEXT) == 0) {
>  				/* Update used ring with desc information */
> -				vq->used->ring[cur_idx & (vq->size - 1)].id
> +				idx = cur_idx & (vq->size - 1);
> +				vq->used->ring[idx].id
>  					= vq->buf_vec[vec_idx].desc_idx;
> -				vq->used->ring[cur_idx & (vq->size - 1)].len
> -					= entry_len;
> +				vq->used->ring[idx].len = entry_len;
> +				vhost_log_used_vring(dev, vq,
> +					offsetof(struct vring_used, ring[idx]),
> +					sizeof(vq->used->ring[idx]));
>  				entry_len = 0;
>  				cur_idx++;
>  				entry_success++;
> @@ -390,16 +412,18 @@ copy_from_mbuf_to_vring(struct virtio_net *dev, uint32_t queue_id,
>  
>  					if ((vq->desc[desc_idx].flags &
>  						VRING_DESC_F_NEXT) == 0) {
> -						uint16_t wrapped_idx =
> -							cur_idx & (vq->size - 1);
> +						idx = cur_idx & (vq->size - 1);
>  						/*
>  						 * Update used ring with the
>  						 * descriptor information
>  						 */
> -						vq->used->ring[wrapped_idx].id
> +						vq->used->ring[idx].id
>  							= desc_idx;
> -						vq->used->ring[wrapped_idx].len
> +						vq->used->ring[idx].len
>  							= entry_len;
> +						vhost_log_used_vring(dev, vq,
> +							offsetof(struct vring_used, ring[idx]),
> +							sizeof(vq->used->ring[idx]));
>  						entry_success++;
>  						entry_len = 0;
>  						cur_idx++;
> @@ -422,10 +446,13 @@ copy_from_mbuf_to_vring(struct virtio_net *dev, uint32_t queue_id,
>  				 * This whole packet completes.
>  				 */
>  				/* Update used ring with desc information */
> -				vq->used->ring[cur_idx & (vq->size - 1)].id
> +				idx = cur_idx & (vq->size - 1);
> +				vq->used->ring[idx].id
>  					= vq->buf_vec[vec_idx].desc_idx;
> -				vq->used->ring[cur_idx & (vq->size - 1)].len
> -					= entry_len;
> +				vq->used->ring[idx].len = entry_len;
> +				vhost_log_used_vring(dev, vq,
> +					offsetof(struct vring_used, ring[idx]),
> +					sizeof(vq->used->ring[idx]));
>  				entry_success++;
>  				break;
>  			}
> @@ -653,6 +680,9 @@ rte_vhost_dequeue_burst(struct virtio_net *dev, uint16_t queue_id,
>  		/* Update used index buffer information. */
>  		vq->used->ring[used_idx].id = head[entry_success];
>  		vq->used->ring[used_idx].len = 0;
> +		vhost_log_used_vring(dev, vq,
> +				offsetof(struct vring_used, ring[used_idx]),
> +				sizeof(vq->used->ring[used_idx]));
>  
>  		/* Allocate an mbuf and populate the structure. */
>  		m = rte_pktmbuf_alloc(mbuf_pool);
> @@ -773,6 +803,8 @@ rte_vhost_dequeue_burst(struct virtio_net *dev, uint16_t queue_id,
>  
>  	rte_compiler_barrier();
>  	vq->used->idx += entry_success;
> +	vhost_log_used_vring(dev, vq, offsetof(struct vring_used, idx),
> +			sizeof(vq->used->idx));
>  	/* Kick guest if required. */
>  	if (!(vq->avail->flags & VRING_AVAIL_F_NO_INTERRUPT))
>  		eventfd_write(vq->callfd, (eventfd_t)1);
> diff --git a/lib/librte_vhost/virtio-net.c b/lib/librte_vhost/virtio-net.c
> index de78a0f..03044f6 100644
> --- a/lib/librte_vhost/virtio-net.c
> +++ b/lib/librte_vhost/virtio-net.c
> @@ -666,12 +666,16 @@ set_vring_addr(struct vhost_device_ctx ctx, struct vhost_vring_addr *addr)
>  		return -1;
>  	}
>  
> +	vq->log_guest_addr = addr->log_guest_addr;
> +
>  	LOG_DEBUG(VHOST_CONFIG, "(%"PRIu64") mapped address desc: %p\n",
>  			dev->device_fh, vq->desc);
>  	LOG_DEBUG(VHOST_CONFIG, "(%"PRIu64") mapped address avail: %p\n",
>  			dev->device_fh, vq->avail);
>  	LOG_DEBUG(VHOST_CONFIG, "(%"PRIu64") mapped address used: %p\n",
>  			dev->device_fh, vq->used);
> +	LOG_DEBUG(VHOST_CONFIG, "(%"PRIu64") log_guest_addr: %"PRIx64"\n",
> +			dev->device_fh, vq->log_guest_addr);
>  
>  	return 0;
>  }
> -- 
> 1.9.0
>