From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mx1.redhat.com (mx1.redhat.com [209.132.183.28]) by dpdk.org (Postfix) with ESMTP id 83E228DAF for ; Tue, 22 Dec 2015 07:56:06 +0100 (CET) Received: from int-mx09.intmail.prod.int.phx2.redhat.com (int-mx09.intmail.prod.int.phx2.redhat.com [10.5.11.22]) by mx1.redhat.com (Postfix) with ESMTPS id E2893691; Tue, 22 Dec 2015 06:56:05 +0000 (UTC) Received: from pxdev.xzpeter.org (vpn1-6-183.pek2.redhat.com [10.72.6.183]) by int-mx09.intmail.prod.int.phx2.redhat.com (8.14.4/8.14.4) with ESMTP id tBM6trZI013547 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-GCM-SHA384 bits=256 verify=NO); Tue, 22 Dec 2015 01:55:58 -0500 Date: Tue, 22 Dec 2015 14:55:52 +0800 From: Peter Xu To: Yuanhan Liu Message-ID: <20151222065552.GC7532@pxdev.xzpeter.org> References: <1449027793-30975-1-git-send-email-yuanhan.liu@linux.intel.com> <1450321921-27799-1-git-send-email-yuanhan.liu@linux.intel.com> <1450321921-27799-4-git-send-email-yuanhan.liu@linux.intel.com> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: <1450321921-27799-4-git-send-email-yuanhan.liu@linux.intel.com> User-Agent: Mutt/1.5.24 (2015-08-30) X-Scanned-By: MIMEDefang 2.68 on 10.5.11.22 Cc: "Michael S. Tsirkin" , dev@dpdk.org, Victor Kaplansky Subject: Re: [dpdk-dev] [PATCH v2 3/6] vhost: log used vring changes X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: patches and discussions about DPDK List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Tue, 22 Dec 2015 06:56:06 -0000 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 >