From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga02.intel.com (mga02.intel.com [134.134.136.20]) by dpdk.org (Postfix) with ESMTP id B9EAF137C for ; Fri, 4 Mar 2016 03:19:44 +0100 (CET) Received: from orsmga002.jf.intel.com ([10.7.209.21]) by orsmga101.jf.intel.com with ESMTP; 03 Mar 2016 18:19:43 -0800 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.22,534,1449561600"; d="scan'208";a="926492933" Received: from yliu-dev.sh.intel.com (HELO yliu-dev) ([10.239.66.49]) by orsmga002.jf.intel.com with ESMTP; 03 Mar 2016 18:19:42 -0800 Date: Fri, 4 Mar 2016 10:21:18 +0800 From: Yuanhan Liu To: "Xie, Huawei" Message-ID: <20160304022118.GU14300@yliu-dev.sh.intel.com> References: <1449122773-25510-1-git-send-email-yuanhan.liu@linux.intel.com> <1455803352-5518-1-git-send-email-yuanhan.liu@linux.intel.com> <1455803352-5518-2-git-send-email-yuanhan.liu@linux.intel.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: User-Agent: Mutt/1.5.23 (2014-03-12) Cc: "Michael S. Tsirkin" , "dev@dpdk.org" , Victor Kaplansky Subject: Re: [dpdk-dev] [PATCH v2 1/7] vhost: refactor rte_vhost_dequeue_burst 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: Fri, 04 Mar 2016 02:19:45 -0000 On Thu, Mar 03, 2016 at 04:21:19PM +0000, Xie, Huawei wrote: > On 2/18/2016 9:48 PM, Yuanhan Liu wrote: > > The current rte_vhost_dequeue_burst() implementation is a bit messy > > and logic twisted. And you could see repeat code here and there: it > > invokes rte_pktmbuf_alloc() three times at three different places! > > > > However, rte_vhost_dequeue_burst() acutally does a simple job: copy > > the packet data from vring desc to mbuf. What's tricky here is: > > > > - desc buff could be chained (by desc->next field), so that you need > > fetch next one if current is wholly drained. > > > > - One mbuf could not be big enough to hold all desc buff, hence you > > need to chain the mbuf as well, by the mbuf->next field. > > > > Even though, the logic could be simple. Here is the pseudo code. > > > > while (this_desc_is_not_drained_totally || has_next_desc) { > > if (this_desc_has_drained_totally) { > > this_desc = next_desc(); > > } > > > > if (mbuf_has_no_room) { > > mbuf = allocate_a_new_mbuf(); > > } > > > > COPY(mbuf, desc); > > } > > > > And this is how I refactored rte_vhost_dequeue_burst. > > > > Note that the old patch does a special handling for skipping virtio > > header. However, that could be simply done by adjusting desc_avail > > and desc_offset var: > > > > desc_avail = desc->len - vq->vhost_hlen; > > desc_offset = vq->vhost_hlen; > > > > This refactor makes the code much more readable (IMO), yet it reduces > > binary code size (nearly 2K). > > > > Signed-off-by: Yuanhan Liu > > --- > > > > v2: - fix potential NULL dereference bug of var "prev" and "head" > > --- > > lib/librte_vhost/vhost_rxtx.c | 297 +++++++++++++++++------------------------- > > 1 file changed, 116 insertions(+), 181 deletions(-) > > > > diff --git a/lib/librte_vhost/vhost_rxtx.c b/lib/librte_vhost/vhost_rxtx.c > > index 5e7e5b1..d5cd0fa 100644 > > --- a/lib/librte_vhost/vhost_rxtx.c > > +++ b/lib/librte_vhost/vhost_rxtx.c > > @@ -702,21 +702,104 @@ vhost_dequeue_offload(struct virtio_net_hdr *hdr, struct rte_mbuf *m) > > } > > } > > > > +static inline struct rte_mbuf * > > +copy_desc_to_mbuf(struct virtio_net *dev, struct vhost_virtqueue *vq, > > + uint16_t desc_idx, struct rte_mempool *mbuf_pool) > > +{ > > + struct vring_desc *desc; > > + uint64_t desc_addr; > > + uint32_t desc_avail, desc_offset; > > + uint32_t mbuf_avail, mbuf_offset; > > + uint32_t cpy_len; > > + struct rte_mbuf *head = NULL; > > + struct rte_mbuf *cur = NULL, *prev = NULL; > > + struct virtio_net_hdr *hdr; > > + > > + desc = &vq->desc[desc_idx]; > > + desc_addr = gpa_to_vva(dev, desc->addr); > > + rte_prefetch0((void *)(uintptr_t)desc_addr); > > + > > + /* Retrieve virtio net header */ > > + hdr = (struct virtio_net_hdr *)((uintptr_t)desc_addr); > > + desc_avail = desc->len - vq->vhost_hlen; > > There is a serious bug here, desc->len - vq->vhost_len could overflow. > VM could easily create this case. Let us fix it here. Nope, this issue has been there since the beginning, and this patch is a refactor: we should not bring any functional changes. Therefore, we should not fix it here. And actually, it's been fixed in the 6th patch in this series: [PATCH v2 6/7] vhost: do sanity check for desc->len --yliu