From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga14.intel.com (mga14.intel.com [192.55.52.115]) by dpdk.org (Postfix) with ESMTP id 3729C8DED for ; Tue, 22 Dec 2015 08:12:37 +0100 (CET) Received: from fmsmga001.fm.intel.com ([10.253.24.23]) by fmsmga103.fm.intel.com with ESMTP; 21 Dec 2015 23:12:36 -0800 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.20,463,1444719600"; d="scan'208";a="867063954" Received: from yliu-dev.sh.intel.com (HELO yliu-dev) ([10.239.66.49]) by fmsmga001.fm.intel.com with ESMTP; 21 Dec 2015 23:12:34 -0800 Date: Tue, 22 Dec 2015 15:13:49 +0800 From: Yuanhan Liu To: Peter Xu Message-ID: <20151222071349.GQ18863@yliu-dev.sh.intel.com> 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> <20151222065552.GC7532@pxdev.xzpeter.org> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20151222065552.GC7532@pxdev.xzpeter.org> User-Agent: Mutt/1.5.23 (2014-03-12) 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 07:12:37 -0000 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