From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga01.intel.com (mga01.intel.com [192.55.52.88]) by dpdk.org (Postfix) with ESMTP id ADC662C66 for ; Mon, 7 Mar 2016 09:36:38 +0100 (CET) Received: from orsmga002.jf.intel.com ([10.7.209.21]) by fmsmga101.fm.intel.com with ESMTP; 07 Mar 2016 00:36:37 -0800 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.22,550,1449561600"; d="scan'208";a="928359119" Received: from yliu-dev.sh.intel.com (HELO yliu-dev) ([10.239.66.49]) by orsmga002.jf.intel.com with ESMTP; 07 Mar 2016 00:36:34 -0800 Date: Mon, 7 Mar 2016 16:38:45 +0800 From: Yuanhan Liu To: "Xie, Huawei" Message-ID: <20160307083845.GF14300@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-4-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 3/7] vhost: refactor virtio_dev_merge_rx 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: Mon, 07 Mar 2016 08:36:39 -0000 On Mon, Mar 07, 2016 at 07:52:22AM +0000, Xie, Huawei wrote: > On 2/18/2016 9:48 PM, Yuanhan Liu wrote: > > Current virtio_dev_merge_rx() implementation just looks like the > > old rte_vhost_dequeue_burst(), full of twisted logic, that you > > can see same code block in quite many different places. > > > > However, the logic of virtio_dev_merge_rx() is quite similar to > > virtio_dev_rx(). The big difference is that the mergeable one > > could allocate more than one available entries to hold the data. > > Fetching all available entries to vec_buf at once makes the > [...] > > - } > > +static inline uint32_t __attribute__((always_inline)) > > +copy_mbuf_to_desc_mergeable(struct virtio_net *dev, struct vhost_virtqueue *vq, > > + uint16_t res_start_idx, uint16_t res_end_idx, > > + struct rte_mbuf *m) > > +{ > > + struct virtio_net_hdr_mrg_rxbuf virtio_hdr = {{0, 0, 0, 0, 0, 0}, 0}; > > + uint32_t vec_idx = 0; > > + uint16_t cur_idx = res_start_idx; > > + uint64_t desc_addr; > > + uint32_t mbuf_offset, mbuf_avail; > > + uint32_t desc_offset, desc_avail; > > + uint32_t cpy_len; > > + uint16_t desc_idx, used_idx; > > + uint32_t nr_used = 0; > > > > - cpy_len = RTE_MIN(vb_avail, seg_avail); > > + if (m == NULL) > > + return 0; > > Is this inherited from old code? Yes. > Let us remove the unnecessary check. > Caller ensures it is not NULL. ... > > + desc_avail = vq->buf_vec[vec_idx].buf_len - vq->vhost_hlen; > > + desc_offset = vq->vhost_hlen; > > As we know we are in merge-able path, use sizeof(virtio_net_hdr) to save > one load for the header len. Please, it's a refactor patch series. You have mentioned quite many trivial issues here and there, which I don't care too much and I don't think they would matter somehow. In addition, they are actually from the old code. > > > + > > + mbuf_avail = rte_pktmbuf_data_len(m); > > + mbuf_offset = 0; > > + while (1) { > > + /* done with current desc buf, get the next one */ > > + > [...] > > + if (reserve_avail_buf_mergeable(vq, pkt_len, &start, &end) < 0) > > + break; > > > > + nr_used = copy_mbuf_to_desc_mergeable(dev, vq, start, end, > > + pkts[pkt_idx]); > > In which case couldn't we get nr_used from start and end? When pkts[pkt_idx] is NULL, though you suggest to remove it, the check is here. --yliu