From mboxrd@z Thu Jan 1 00:00:00 1970 From: "Wang, Zhihong" Subject: Re: [PATCH v4 2/6] vhost: rewrite enqueue Date: Wed, 7 Sep 2016 05:39:10 +0000 Message-ID: <8F6C2BD409508844A0EFC19955BE09414E70C1A7@SHSMSX103.ccr.corp.intel.com> References: <1472528164-54296-1-git-send-email-zhihong.wang@intel.com> <1472528164-54296-3-git-send-email-zhihong.wang@intel.com> <20160905063925.GK30752@yliu-dev.sh.intel.com> <20160907053310.GC4357@yliu-dev.sh.intel.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable Cc: "dev@dpdk.org" , "maxime.coquelin@redhat.com" , "thomas.monjalon@6wind.com" To: Yuanhan Liu Return-path: Received: from mga04.intel.com (mga04.intel.com [192.55.52.120]) by dpdk.org (Postfix) with ESMTP id 27DD37EEF for ; Wed, 7 Sep 2016 07:39:19 +0200 (CEST) In-Reply-To: <20160907053310.GC4357@yliu-dev.sh.intel.com> Content-Language: en-US List-Id: patches and discussions about DPDK List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org Sender: "dev" > -----Original Message----- > From: Yuanhan Liu [mailto:yuanhan.liu@linux.intel.com] > Sent: Wednesday, September 7, 2016 1:33 PM > To: Wang, Zhihong > Cc: dev@dpdk.org; maxime.coquelin@redhat.com; > thomas.monjalon@6wind.com > Subject: Re: [PATCH v4 2/6] vhost: rewrite enqueue >=20 > Hmmm, yet another email didn't send out successfully. Resend. >=20 > BTW, please work out v5 on top of the latest next-virtio tree. >=20 > Thanks. Okay. Thanks. >=20 > --yliu >=20 > On Mon, Sep 05, 2016 at 02:39:25PM +0800, Yuanhan Liu wrote: > ---- > On Mon, Aug 29, 2016 at 11:36:00PM -0400, Zhihong Wang wrote: > > This patch implements the vhost logic from scratch into a single functi= on > > designed for high performance and better maintainability. > > > > This is the baseline version of the new code, more optimization will be > > added in the following patches in this patch set. > > > > --- > > Changes in v4: > > > > 1. Refactor the code for clearer logic. > > > > 2. Add PRINT_PACKET for debugging. > > > > --- > > Changes in v3: > > > > 1. Rewrite enqueue and delete the obsolete in the same patch. >=20 > Change log should go ----> >=20 > > Signed-off-by: Zhihong Wang > > --- >=20 > ... here, after the SoB. >=20 > > lib/librte_vhost/vhost_rxtx.c | 525 ++++++++++++----------------------= ------- > - > > 1 file changed, 145 insertions(+), 380 deletions(-) > > > > diff --git a/lib/librte_vhost/vhost_rxtx.c b/lib/librte_vhost/vhost_rxt= x.c > > index 5806f99..629e8ae 100644 > > --- a/lib/librte_vhost/vhost_rxtx.c > > +++ b/lib/librte_vhost/vhost_rxtx.c > > @@ -91,7 +91,7 @@ is_valid_virt_queue_idx(uint32_t idx, int is_tx, > uint32_t qp_nb) > > return (is_tx ^ (idx & 1)) =3D=3D 0 && idx < qp_nb * VIRTIO_QNUM; > > } > > > > -static void > > +static inline void __attribute__((always_inline)) > > virtio_enqueue_offload(struct rte_mbuf *m_buf, struct virtio_net_hdr > *net_hdr) > > { > > if (m_buf->ol_flags & PKT_TX_L4_MASK) { > > @@ -112,6 +112,10 @@ virtio_enqueue_offload(struct rte_mbuf *m_buf, > struct virtio_net_hdr *net_hdr) > > cksum)); > > break; > > } > > + } else { > > + net_hdr->flags =3D 0; > > + net_hdr->csum_start =3D 0; > > + net_hdr->csum_offset =3D 0; > > } > > > > if (m_buf->ol_flags & PKT_TX_TCP_SEG) { > > @@ -122,437 +126,198 @@ virtio_enqueue_offload(struct rte_mbuf > *m_buf, struct virtio_net_hdr *net_hdr) > > net_hdr->gso_size =3D m_buf->tso_segsz; > > net_hdr->hdr_len =3D m_buf->l2_len + m_buf->l3_len > > + m_buf->l4_len; > > + } else { > > + net_hdr->gso_type =3D 0; > > + net_hdr->hdr_len =3D 0; > > + net_hdr->gso_size =3D 0; > > } > > } > > > > -static inline void > > -copy_virtio_net_hdr(struct virtio_net *dev, uint64_t desc_addr, > > - struct virtio_net_hdr_mrg_rxbuf hdr) > > +static inline void __attribute__((always_inline)) > > +update_used_ring(struct virtio_net *dev, struct vhost_virtqueue *vq, > > + uint32_t desc_chain_head, uint32_t desc_chain_len) > > { > > - if (dev->vhost_hlen =3D=3D sizeof(struct virtio_net_hdr_mrg_rxbuf)) > > - *(struct virtio_net_hdr_mrg_rxbuf *)(uintptr_t)desc_addr =3D > hdr; > > - else > > - *(struct virtio_net_hdr *)(uintptr_t)desc_addr =3D hdr.hdr; > > + uint32_t used_idx_round =3D vq->last_used_idx & (vq->size - 1); >=20 > I'd suggest to use "used_idx", instead of "used_idx_round". >=20 > > + > > + vq->used->ring[used_idx_round].id =3D desc_chain_head; > > + vq->used->ring[used_idx_round].len =3D desc_chain_len; > > + vhost_log_used_vring(dev, vq, offsetof(struct vring_used, > > + ring[used_idx_round]), > > + sizeof(vq->used->ring[used_idx_round])); > > } > > > > -static inline int __attribute__((always_inline)) > > -copy_mbuf_to_desc(struct virtio_net *dev, struct vhost_virtqueue *vq, > > - struct rte_mbuf *m, uint16_t desc_idx) > > +static inline uint32_t __attribute__((always_inline)) > > +enqueue_packet(struct virtio_net *dev, struct vhost_virtqueue *vq, > > + uint16_t avail_idx, struct rte_mbuf *mbuf, > > + uint32_t is_mrg_rxbuf) > > { > > - uint32_t desc_avail, desc_offset; > > - uint32_t mbuf_avail, mbuf_offset; > > - uint32_t cpy_len; > > + struct virtio_net_hdr_mrg_rxbuf *virtio_hdr; > > struct vring_desc *desc; > > uint64_t desc_addr; > > - struct virtio_net_hdr_mrg_rxbuf virtio_hdr =3D {{0, 0, 0, 0, 0, 0}, 0= }; > > - > > - desc =3D &vq->desc[desc_idx]; > > + uint32_t desc_chain_head; > > + uint32_t desc_chain_len; > > + uint32_t desc_current; > > + uint32_t desc_offset; > > + uint32_t mbuf_len; > > + uint32_t mbuf_avail; > > + uint32_t copy_len; > > + uint32_t extra_buffers =3D 0; >=20 > I'd name it to "num_buffers", to keep consistent with the virito hdr > naming style. >=20 > > + > > + /* start with the first mbuf of the packet */ > > + mbuf_len =3D rte_pktmbuf_data_len(mbuf); > > + mbuf_avail =3D mbuf_len; > > + > > + /* get the current desc */ > > + desc_current =3D vq->avail->ring[(vq->last_used_idx) & (vq->size - 1)= ]; > > + desc_chain_head =3D desc_current; > > + desc =3D &vq->desc[desc_current]; > > desc_addr =3D gpa_to_vva(dev, desc->addr); > > - /* > > - * Checking of 'desc_addr' placed outside of 'unlikely' macro to avoi= d > > - * performance issue with some versions of gcc (4.8.4 and 5.3.0) > which > > - * otherwise stores offset on the stack instead of in a register. > > - */ > > - if (unlikely(desc->len < dev->vhost_hlen) || !desc_addr) > > - return -1; > > + if (unlikely(!desc_addr)) > > + goto error; > > > > - rte_prefetch0((void *)(uintptr_t)desc_addr); > > + /* handle virtio header */ > > + virtio_hdr =3D (struct virtio_net_hdr_mrg_rxbuf > *)(uintptr_t)desc_addr; > > + virtio_enqueue_offload(mbuf, &(virtio_hdr->hdr)); > > + if (is_mrg_rxbuf) > > + virtio_hdr->num_buffers =3D extra_buffers + 1; > > > > - virtio_enqueue_offload(m, &virtio_hdr.hdr); > > - copy_virtio_net_hdr(dev, desc_addr, virtio_hdr); > > vhost_log_write(dev, desc->addr, dev->vhost_hlen); > > PRINT_PACKET(dev, (uintptr_t)desc_addr, dev->vhost_hlen, 0); > > - > > desc_offset =3D dev->vhost_hlen; > > - desc_avail =3D desc->len - dev->vhost_hlen; > > - > > - mbuf_avail =3D rte_pktmbuf_data_len(m); > > - mbuf_offset =3D 0; > > - while (mbuf_avail !=3D 0 || m->next !=3D NULL) { > > - /* done with current mbuf, fetch next */ > > - if (mbuf_avail =3D=3D 0) { > > - m =3D m->next; > > - > > - mbuf_offset =3D 0; > > - mbuf_avail =3D rte_pktmbuf_data_len(m); > > + desc_chain_len =3D desc_offset; > > + desc_addr +=3D desc_offset; > > + > > + /* start copy from mbuf to desc */ > > + while (mbuf_avail || mbuf->next) { > > + /* get the next mbuf if the current done */ > > + if (!mbuf_avail) { > > + mbuf =3D mbuf->next; > > + mbuf_len =3D rte_pktmbuf_data_len(mbuf); > > + mbuf_avail =3D mbuf_len; > > } > > > > - /* done with current desc buf, fetch next */ > > - if (desc_avail =3D=3D 0) { > > - if ((desc->flags & VRING_DESC_F_NEXT) =3D=3D 0) { > > - /* Room in vring buffer is not enough */ > > - return -1; > > - } > > - if (unlikely(desc->next >=3D vq->size)) > > - return -1; > > + /* get the next desc if the current done */ > > + if (desc->len <=3D desc_offset) { > > + if (desc->flags & VRING_DESC_F_NEXT) { > > + /* go on with the current desc chain */ > > + desc_offset =3D 0; > > + desc_current =3D desc->next; > > + desc =3D &vq->desc[desc_current]; > > + desc_addr =3D gpa_to_vva(dev, desc->addr); > > + if (unlikely(!desc_addr)) > > + goto error; > > + } else if (is_mrg_rxbuf) { > > + /* start with the next desc chain */ > > + update_used_ring(dev, vq, > desc_chain_head, > > + desc_chain_len); > > + vq->last_used_idx++; >=20 > Why not putting "vq->last_used_idx++" into update_used_ring()? >=20 > > + extra_buffers++; > > + virtio_hdr->num_buffers++; > > + if (avail_idx =3D=3D vq->last_used_idx) > > + goto error; > > + > > + desc_current =3D > > + vq->avail->ring[(vq->last_used_idx) > & > > + (vq->size - 1)]; > > + desc_chain_head =3D desc_current; > > + desc =3D &vq->desc[desc_current]; > > + desc_addr =3D gpa_to_vva(dev, desc->addr); > > + if (unlikely(!desc_addr)) > > + goto error; > > > > - desc =3D &vq->desc[desc->next]; > > - desc_addr =3D gpa_to_vva(dev, desc->addr); > > - if (unlikely(!desc_addr)) > > - return -1; > > - > > - desc_offset =3D 0; > > - desc_avail =3D desc->len; > > + desc_chain_len =3D 0; > > + desc_offset =3D 0; > > + } else > > + goto error; > > } > > > > - cpy_len =3D RTE_MIN(desc_avail, mbuf_avail); > > - rte_memcpy((void *)((uintptr_t)(desc_addr + desc_offset)), > > - rte_pktmbuf_mtod_offset(m, void *, mbuf_offset), > > - cpy_len); > > - vhost_log_write(dev, desc->addr + desc_offset, cpy_len); > > - PRINT_PACKET(dev, (uintptr_t)(desc_addr + desc_offset), > > - cpy_len, 0); > > - > > - mbuf_avail -=3D cpy_len; > > - mbuf_offset +=3D cpy_len; > > - desc_avail -=3D cpy_len; > > - desc_offset +=3D cpy_len; > > + /* copy mbuf data */ > > + copy_len =3D RTE_MIN(desc->len - desc_offset, mbuf_avail); >=20 > TBH, I'm okay with copy_len (actually, I prefer it slightly). However, > the old code uses cpy_len, the current dequeue function also uses cpy_len= , > I then see no good reason to use copy_len here. It's really not a good > idea to me to use two different naming styles in one source file. >=20 >=20 > > + rte_memcpy((void *)(uintptr_t)desc_addr, > > + rte_pktmbuf_mtod_offset(mbuf, void *, > > + mbuf_len - mbuf_avail), >=20 > I would keep the old var "mbuf_offset" and do not introduce "mbuf_len". > This could avoid above calculation and make it straightforward. >=20 > > + copy_len); > > + vhost_log_write(dev, desc->addr + desc_offset, copy_len); > > + PRINT_PACKET(dev, (uintptr_t)desc_addr, copy_len, 0); > > + mbuf_avail -=3D copy_len; > > + desc_offset +=3D copy_len; > > + desc_addr +=3D copy_len; > > + desc_chain_len +=3D copy_len; >=20 > Vertical alighment[0] is not a must, but as you can see, it's a style I > prefer. Meaning, if possible, please follow it. >=20 > [0]: https://en.wikipedia.org/wiki/Programming_style#Vertical_alignment >=20 > > } > > > > - return 0; > > -} > > + update_used_ring(dev, vq, desc_chain_head, desc_chain_len); > > + vq->last_used_idx++; > > > ... > > - rte_prefetch0(&vq->desc[desc_indexes[0]]); > > - for (i =3D 0; i < count; i++) { > > - uint16_t desc_idx =3D desc_indexes[i]; > > - int err; > > + return 0; > > > > - err =3D copy_mbuf_to_desc(dev, vq, pkts[i], desc_idx); > > - if (unlikely(err)) { > > - used_idx =3D (start_idx + i) & (vq->size - 1); > > - vq->used->ring[used_idx].len =3D dev->vhost_hlen; > > - vhost_log_used_vring(dev, vq, > > - offsetof(struct vring_used, ring[used_idx]), > > - sizeof(vq->used->ring[used_idx])); > > - } > > +error: > > + /* rollback on any error if last_used_idx update on-the-fly */ > > + vq->last_used_idx -=3D extra_buffers; > > > > - if (i + 1 < count) > > - rte_prefetch0(&vq->desc[desc_indexes[i+1]]); > > - } > > + return 1; >=20 > We normally returns -1 (but not 1) on error. >=20 > > > > +static inline void __attribute__((always_inline)) > > +notify_guest(struct virtio_net *dev, struct vhost_virtqueue *vq) > > +{ > > rte_smp_wmb(); > > - > > - *(volatile uint16_t *)&vq->used->idx +=3D count; > > - vq->last_used_idx +=3D count; > > - vhost_log_used_vring(dev, vq, > > - offsetof(struct vring_used, idx), > > - sizeof(vq->used->idx)); > > - > > - /* flush used->idx update before we read avail->flags. */ > > + vq->used->idx =3D vq->last_used_idx; >=20 > I will not drop the "volatile" cast here, sliently. You know this kind of > stuff is tricky and would be painful to debug if it cause any issue. Such > removal deserves a patch, as well as some explanations. >=20 > > + vhost_log_used_vring(dev, vq, offsetof(struct vring_used, idx), > > + sizeof(vq->used->idx)); > > rte_mb(); > > - > > - /* Kick the guest if necessary. */ > > if (!(vq->avail->flags & VRING_AVAIL_F_NO_INTERRUPT) > > && (vq->callfd >=3D 0)) > > eventfd_write(vq->callfd, (eventfd_t)1); > > - return count; > > } > > > ... > > + /* start enqueuing packets 1 by 1 */ > > + avail_idx =3D *((volatile uint16_t *)&vq->avail->idx); > > + while (pkt_left && avail_idx !=3D vq->last_used_idx) { > > + if (enqueue_packet(dev, vq, avail_idx, pkts[pkt_idx], > > + is_mrg_rxbuf)) > > break; > > - } > > - > > - nr_used =3D copy_mbuf_to_desc_mergeable(dev, vq, end, > > - pkts[pkt_idx], buf_vec); > > - rte_smp_wmb(); > > - > > - *(volatile uint16_t *)&vq->used->idx +=3D nr_used; > > - vhost_log_used_vring(dev, vq, offsetof(struct vring_used, > idx), > > - sizeof(vq->used->idx)); > > - vq->last_used_idx +=3D nr_used; > > - } > > - > > - if (likely(pkt_idx)) { > > - /* flush used->idx update before we read avail->flags. */ > > - rte_mb(); > > > > - /* Kick the guest if necessary. */ > > - if (!(vq->avail->flags & VRING_AVAIL_F_NO_INTERRUPT) > > - && (vq->callfd >=3D 0)) > > - eventfd_write(vq->callfd, (eventfd_t)1); > > + pkt_idx++; > > + pkt_sent++; >=20 > pkt_idx and pkt_sent is duplicate here. >=20 > --yliu >=20 > > + pkt_left--; > > } > > > > - return pkt_idx; > > -} > > - > > -uint16_t > > -rte_vhost_enqueue_burst(int vid, uint16_t queue_id, > > - struct rte_mbuf **pkts, uint16_t count) > > -{ > > - struct virtio_net *dev =3D get_device(vid); > > - > > - if (!dev) > > - return 0; > > + /* update used idx and kick the guest if necessary */ > > + if (pkt_sent) > > + notify_guest(dev, vq); > > > > - if (dev->features & (1 << VIRTIO_NET_F_MRG_RXBUF)) > > - return virtio_dev_merge_rx(dev, queue_id, pkts, count); > > - else > > - return virtio_dev_rx(dev, queue_id, pkts, count); > > + return pkt_sent; > > } > > > > static void > > -- > > 2.7.4