From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga09.intel.com (mga09.intel.com [134.134.136.24]) by dpdk.org (Postfix) with ESMTP id 3D36F12A8 for ; Sun, 31 May 2015 15:07:59 +0200 (CEST) Received: from orsmga001.jf.intel.com ([10.7.209.18]) by orsmga102.jf.intel.com with ESMTP; 31 May 2015 06:07:58 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.13,526,1427785200"; d="scan'208";a="702853897" Received: from pgsmsx103.gar.corp.intel.com ([10.221.44.82]) by orsmga001.jf.intel.com with ESMTP; 31 May 2015 06:07:57 -0700 Received: from shsmsx151.ccr.corp.intel.com (10.239.6.50) by PGSMSX103.gar.corp.intel.com (10.221.44.82) with Microsoft SMTP Server (TLS) id 14.3.224.2; Sun, 31 May 2015 21:07:56 +0800 Received: from shsmsx102.ccr.corp.intel.com ([169.254.2.109]) by SHSMSX151.ccr.corp.intel.com ([169.254.3.180]) with mapi id 14.03.0224.002; Sun, 31 May 2015 21:07:55 +0800 From: "Ouyang, Changchun" To: "Xie, Huawei" , "dev@dpdk.org" Thread-Topic: [PATCH v2 5/5] lib_vhost: Add support copying scattered mbuf to vring Thread-Index: AQHQmVlcwCnIyiIhOEWOafvCGs18qJ2WEM1w Date: Sun, 31 May 2015 13:07:54 +0000 Message-ID: References: <1430720780-27525-1-git-send-email-changchun.ouyang@intel.com> <1432826207-8428-1-git-send-email-changchun.ouyang@intel.com> <1432826207-8428-6-git-send-email-changchun.ouyang@intel.com> In-Reply-To: Accept-Language: zh-CN, en-US Content-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: x-originating-ip: [10.239.127.40] Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable MIME-Version: 1.0 Subject: Re: [dpdk-dev] [PATCH v2 5/5] lib_vhost: Add support copying scattered mbuf to vring 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: Sun, 31 May 2015 13:08:00 -0000 > -----Original Message----- > From: Xie, Huawei > Sent: Sunday, May 31, 2015 5:11 PM > To: Ouyang, Changchun; dev@dpdk.org > Cc: Cao, Waterman > Subject: Re: [PATCH v2 5/5] lib_vhost: Add support copying scattered mbuf > to vring >=20 > virtio_dev_rx & scatter_rx & merge-able rx should be merged and the code > could be much simpler, unless there is special performance consideration. > Then, any specific suggestion on how to merge them? I do consider the performance influence here, so I think it deserve to have= 3 implementation for different cases. =20 >=20 > On 5/28/2015 11:17 PM, Ouyang, Changchun wrote: > > Add support copying scattered mbuf to vring which is done by > > dev_scatter_rx, and check the 'next' pointer in mbuf on the fly to sele= ct > suitable function to rx packets. > > > > Signed-off-by: Changchun Ouyang > > --- > > lib/librte_vhost/vhost_rxtx.c | 116 > > +++++++++++++++++++++++++++++++++++++++++- > > 1 file changed, 115 insertions(+), 1 deletion(-) > > > > diff --git a/lib/librte_vhost/vhost_rxtx.c > > b/lib/librte_vhost/vhost_rxtx.c index bb56ae1..3086bb4 100644 > > --- a/lib/librte_vhost/vhost_rxtx.c > > +++ b/lib/librte_vhost/vhost_rxtx.c > > @@ -46,7 +46,8 @@ > > * This function adds buffers to the virtio devices RX virtqueue. Buff= ers can > > * be received from the physical port or from another virtio device. A > packet > > * count is returned to indicate the number of packets that are > > succesfully > > - * added to the RX queue. This function works when mergeable is disabl= ed. > > + * added to the RX queue. This function works when mergeable is > > + disabled and > > + * the mbuf is not scattered. > > */ > > static inline uint32_t __attribute__((always_inline)) > > virtio_dev_rx(struct virtio_net *dev, uint16_t queue_id, @@ -447,6 > > +448,103 @@ fill_buf_vec(struct vhost_virtqueue *vq, uint16_t id, > > uint32_t *vec_idx) } > > > > /* > > + * This function works for scatter-gather RX. > > + */ > > +static inline uint32_t __attribute__((always_inline)) > > +virtio_dev_scatter_rx(struct virtio_net *dev, uint16_t queue_id, > > + struct rte_mbuf **pkts, uint32_t count) { > > + struct vhost_virtqueue *vq; > > + uint32_t pkt_idx =3D 0, entry_success =3D 0; > > + uint16_t avail_idx; > > + uint16_t res_base_idx, res_end_idx; > > + uint8_t success =3D 0; > > + > > + LOG_DEBUG(VHOST_DATA, "(%"PRIu64") virtio_dev_scatter_rx()\n", > > + dev->device_fh); > use __func__ > > + if (unlikely(queue_id !=3D VIRTIO_RXQ)) > > + LOG_DEBUG(VHOST_DATA, "mq isn't supported in this > version.\n"); > > + > > + vq =3D dev->virtqueue[VIRTIO_RXQ]; > > + count =3D RTE_MIN((uint32_t)MAX_PKT_BURST, count); > > + > > + if (count =3D=3D 0) > > + return 0; > > + > > + for (pkt_idx =3D 0; pkt_idx < count; pkt_idx++) { > > + uint32_t secure_len =3D 0; > > + uint32_t vec_idx =3D 0; > > + uint32_t pkt_len =3D pkts[pkt_idx]->pkt_len + vq->vhost_hlen; > > + > > + do { > > + /* > > + * As many data cores may want access to available > > + * buffers, they need to be reserved. > > + */ > > + res_base_idx =3D vq->last_used_idx_res; > > + avail_idx =3D *((volatile uint16_t *)&vq->avail->idx); > > + > > + if (unlikely(res_base_idx =3D=3D avail_idx)) { > > + LOG_DEBUG(VHOST_DATA, > > + "(%"PRIu64") Failed " > > + "to get enough desc from " > > + "vring\n", > > + dev->device_fh); > > + return pkt_idx; > > + } else { > > + uint16_t wrapped_idx =3D > > + (res_base_idx) & (vq->size - 1); > > + uint32_t idx =3D vq->avail->ring[wrapped_idx]; > > + > > + update_secure_len(vq, idx, &secure_len); > > + } > > + > > + if (pkt_len > secure_len) { > > + LOG_DEBUG(VHOST_DATA, > > + "(%"PRIu64") Failed " > > + "to get enough desc from " > > + "vring\n", > > + dev->device_fh); > > + return pkt_idx; > > + } > The behavior for virtio_dev_rx and virtio_dev_merge_rx is totally differe= nt. I > think they should behave in the same way. Why they have to work as same way? > virtio_dev_rx updates used->len to zero while this one returns immediatel= y. >=20 Yes, if it is uncompleted packets, I think it comes from your comments abou= t dropping the packets if the room is not Big enough to contain the whole packet. > Besides, with this implementation, if the caller retransmit the mbuf(whic= h > has pkt_len larger the secure_len), it will enter endless loop. Why the caller retransmit the mbuf? I think this is caller's bad, then endl= ess loop just catch that issue in caller. >=20 > > + > > + /* vq->last_used_idx_res is atomically updated. */ > > + success =3D rte_atomic16_cmpset(&vq- > >last_used_idx_res, > > + res_base_idx, > > + res_base_idx + 1); > > + } while (success =3D=3D 0); >=20 > Here the behavior becomes different again in reserving vring entries. >=20 > > + > > + fill_buf_vec(vq, res_base_idx, &vec_idx); > > + > > + res_end_idx =3D res_base_idx + 1; > > + > > + entry_success =3D copy_from_mbuf_to_vring(dev, > res_base_idx, > > + res_end_idx, pkts[pkt_idx]); > > + > > + rte_compiler_barrier(); > > + > > + /* > > + * Wait until it's our turn to add our buffer > > + * to the used ring. > > + */ > > + while (unlikely(vq->last_used_idx !=3D res_base_idx)) > > + rte_pause(); > > + > > + *(volatile uint16_t *)&vq->used->idx +=3D entry_success; > > + vq->last_used_idx =3D res_end_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)) > > + eventfd_write((int)vq->callfd, 1); > > + } > > + > > + return count; > > +} > > + > > +/* > > * This function works for mergeable RX. > > */ > > static inline uint32_t __attribute__((always_inline)) @@ -545,12 > > +643,28 @@ virtio_dev_merge_rx(struct virtio_net *dev, uint16_t > queue_id, > > return count; > > } > > > > +/* > > + * Return 1 if any mbuf is scattered, otherwise return 0. > > + */ > > +static inline uint32_t __attribute__((always_inline)) > > +check_scatter(struct rte_mbuf **pkts, uint16_t count) { > > + uint32_t i; > > + for (i =3D 0; i < count; i++) { > > + if (pkts[i]->next !=3D NULL) > > + return 1; > > + } > > + return 0; > > +} > > + > > uint16_t > > rte_vhost_enqueue_burst(struct virtio_net *dev, uint16_t queue_id, > > struct rte_mbuf **pkts, uint16_t count) { > > if (unlikely(dev->features & (1 << VIRTIO_NET_F_MRG_RXBUF))) > > return virtio_dev_merge_rx(dev, queue_id, pkts, count); > > + else if (unlikely(check_scatter(pkts, count) =3D=3D 1)) > > + return virtio_dev_scatter_rx(dev, queue_id, pkts, count); > > else > > return virtio_dev_rx(dev, queue_id, pkts, count); }