From: "Ouyang, Changchun" <changchun.ouyang@intel.com>
To: "Xie, Huawei" <huawei.xie@intel.com>, "dev@dpdk.org" <dev@dpdk.org>
Subject: Re: [PATCH v2 5/5] lib_vhost: Add support copying scattered mbuf to vring
Date: Sun, 31 May 2015 13:07:54 +0000 [thread overview]
Message-ID: <F52918179C57134FAEC9EA62FA2F962511B732A9@shsmsx102.ccr.corp.intel.com> (raw)
In-Reply-To: <C37D651A908B024F974696C65296B57B0F4D042B@SHSMSX101.ccr.corp.intel.com>
> -----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
>
> 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.
>
> 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 select
> suitable function to rx packets.
> >
> > Signed-off-by: Changchun Ouyang <changchun.ouyang@intel.com>
> > ---
> > 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. Buffers 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 disabled.
> > + * 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 = 0, entry_success = 0;
> > + uint16_t avail_idx;
> > + uint16_t res_base_idx, res_end_idx;
> > + uint8_t success = 0;
> > +
> > + LOG_DEBUG(VHOST_DATA, "(%"PRIu64") virtio_dev_scatter_rx()\n",
> > + dev->device_fh);
> use __func__
> > + if (unlikely(queue_id != VIRTIO_RXQ))
> > + LOG_DEBUG(VHOST_DATA, "mq isn't supported in this
> version.\n");
> > +
> > + vq = dev->virtqueue[VIRTIO_RXQ];
> > + count = RTE_MIN((uint32_t)MAX_PKT_BURST, count);
> > +
> > + if (count == 0)
> > + return 0;
> > +
> > + for (pkt_idx = 0; pkt_idx < count; pkt_idx++) {
> > + uint32_t secure_len = 0;
> > + uint32_t vec_idx = 0;
> > + uint32_t pkt_len = 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 = vq->last_used_idx_res;
> > + avail_idx = *((volatile uint16_t *)&vq->avail->idx);
> > +
> > + if (unlikely(res_base_idx == 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 =
> > + (res_base_idx) & (vq->size - 1);
> > + uint32_t idx = 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 different. 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 immediately.
>
Yes, if it is uncompleted packets, I think it comes from your comments about 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(which
> 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 endless loop just catch that issue in caller.
>
> > +
> > + /* vq->last_used_idx_res is atomically updated. */
> > + success = rte_atomic16_cmpset(&vq-
> >last_used_idx_res,
> > + res_base_idx,
> > + res_base_idx + 1);
> > + } while (success == 0);
>
> Here the behavior becomes different again in reserving vring entries.
>
> > +
> > + fill_buf_vec(vq, res_base_idx, &vec_idx);
> > +
> > + res_end_idx = res_base_idx + 1;
> > +
> > + entry_success = 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 != res_base_idx))
> > + rte_pause();
> > +
> > + *(volatile uint16_t *)&vq->used->idx += entry_success;
> > + vq->last_used_idx = 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 = 0; i < count; i++) {
> > + if (pkts[i]->next != 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) == 1))
> > + return virtio_dev_scatter_rx(dev, queue_id, pkts, count);
> > else
> > return virtio_dev_rx(dev, queue_id, pkts, count); }
next prev parent reply other threads:[~2015-05-31 13:07 UTC|newest]
Thread overview: 52+ messages / expand[flat|nested] mbox.gz Atom feed top
2015-05-04 6:26 [PATCH] virtio: Fix enqueue/dequeue can't handle chained vring descriptors Ouyang Changchun
2015-05-12 10:00 ` Thomas Monjalon
2015-05-18 9:39 ` Xie, Huawei
2015-05-18 13:23 ` Ouyang, Changchun
2015-05-20 5:26 ` Xie, Huawei
2015-05-28 15:16 ` [PATCH v2 0/5] Fix vhost enqueue/dequeue issue Ouyang Changchun
2015-05-28 15:16 ` [PATCH v2 1/5] lib_vhost: Fix enqueue/dequeue can't handle chained vring descriptors Ouyang Changchun
2015-05-31 5:03 ` Xie, Huawei
2015-05-31 13:20 ` Ouyang, Changchun
2015-05-31 8:40 ` Xie, Huawei
2015-05-31 12:59 ` Ouyang, Changchun
2015-05-31 13:22 ` Ouyang, Changchun
2015-05-31 13:33 ` Ouyang, Changchun
2015-05-28 15:16 ` [PATCH v2 2/5] lib_vhost: Refine code style Ouyang Changchun
2015-05-28 15:16 ` [PATCH v2 3/5] lib_vhost: Extract function Ouyang Changchun
2015-05-28 15:16 ` [PATCH v2 4/5] lib_vhost: Remove unnecessary vring descriptor length updating Ouyang Changchun
2015-05-28 15:16 ` [PATCH v2 5/5] lib_vhost: Add support copying scattered mbuf to vring Ouyang Changchun
2015-05-31 9:10 ` Xie, Huawei
2015-05-31 13:07 ` Ouyang, Changchun [this message]
2015-06-01 8:25 ` [PATCH v3 0/4] Fix vhost enqueue/dequeue issue Ouyang Changchun
2015-06-01 8:25 ` [PATCH v3 1/4] lib_vhost: Fix enqueue/dequeue can't handle chained vring descriptors Ouyang Changchun
2015-06-02 7:51 ` Xie, Huawei
2015-06-02 8:10 ` Ouyang, Changchun
2015-06-01 8:25 ` [PATCH v3 2/4] lib_vhost: Refine code style Ouyang Changchun
2015-06-01 8:25 ` [PATCH v3 3/4] lib_vhost: Extract function Ouyang Changchun
2015-06-01 8:25 ` [PATCH v3 4/4] lib_vhost: Remove unnecessary vring descriptor length updating Ouyang Changchun
2015-06-02 8:51 ` [PATCH v4 0/4] Fix vhost enqueue/dequeue issue Ouyang Changchun
2015-06-02 8:51 ` [PATCH v4 1/4] lib_vhost: Fix enqueue/dequeue can't handle chained vring descriptors Ouyang Changchun
2015-06-02 8:51 ` [PATCH v4 2/4] lib_vhost: Refine code style Ouyang Changchun
2015-06-02 8:51 ` [PATCH v4 3/4] lib_vhost: Extract function Ouyang Changchun
2015-06-02 8:51 ` [PATCH v4 4/4] lib_vhost: Remove unnecessary vring descriptor length updating Ouyang Changchun
2015-06-03 6:02 ` [PATCH v5 0/4] Fix vhost enqueue/dequeue issue Ouyang Changchun
2015-06-03 6:02 ` [PATCH v5 1/4] lib_vhost: Fix enqueue/dequeue can't handle chained vring descriptors Ouyang Changchun
2015-06-03 6:02 ` [PATCH v5 2/4] lib_vhost: Refine code style Ouyang Changchun
2015-06-03 6:02 ` [PATCH v5 3/4] lib_vhost: Extract function Ouyang Changchun
2015-06-03 6:02 ` [PATCH v5 4/4] lib_vhost: Remove unnecessary vring descriptor length updating Ouyang Changchun
2015-06-03 7:50 ` [PATCH v5 0/4] Fix vhost enqueue/dequeue issue Xu, Qian Q
2015-06-08 3:18 ` [PATCH v6 " Ouyang Changchun
2015-06-08 3:18 ` [PATCH v6 1/4] lib_vhost: Fix enqueue/dequeue can't handle chained vring descriptors Ouyang Changchun
2015-06-08 3:18 ` [PATCH v6 2/4] lib_vhost: Refine code style Ouyang Changchun
2015-06-08 3:18 ` [PATCH v6 3/4] lib_vhost: Extract function Ouyang Changchun
2015-06-08 3:18 ` [PATCH v6 4/4] lib_vhost: Remove unnecessary vring descriptor length updating Ouyang Changchun
2015-06-09 1:03 ` [PATCH v7 0/4] Fix vhost enqueue/dequeue issue Ouyang Changchun
2015-06-09 1:03 ` [PATCH v7 1/4] lib_vhost: Fix enqueue/dequeue can't handle chained vring descriptors Ouyang Changchun
2015-06-09 1:03 ` [PATCH v7 2/4] lib_vhost: Refine code style Ouyang Changchun
2015-06-09 1:03 ` [PATCH v7 3/4] lib_vhost: Extract function Ouyang Changchun
2015-06-09 1:03 ` [PATCH v7 4/4] lib_vhost: Remove unnecessary vring descriptor length updating Ouyang Changchun
2015-06-10 1:40 ` [PATCH v7 0/4] Fix vhost enqueue/dequeue issue Xie, Huawei
2015-06-10 6:49 ` Xie, Huawei
2015-06-17 14:57 ` Thomas Monjalon
2015-06-15 9:42 ` Thomas Monjalon
2015-06-16 1:01 ` Ouyang, Changchun
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=F52918179C57134FAEC9EA62FA2F962511B732A9@shsmsx102.ccr.corp.intel.com \
--to=changchun.ouyang@intel.com \
--cc=dev@dpdk.org \
--cc=huawei.xie@intel.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.