Hi Ilya,
Few comments inline:
On Sat, 20 Feb 2016 14:28:52 +0300
Ilya Maximets <i.maxim...@samsung.com> wrote:
> Currently virtio driver in guest operating system have to be configured
> to use exactly same number of queues. If number of queues will be less,
> some packets will get stuck in queues unused by guest and will not be
> received.
>
> Fix that by using new 'vring_state_changed' callback, which is
> available for vhost-user since DPDK 2.2.
> Implementation uses additional mapping from configured tx queues to
> enabled by virtio driver. This requires mandatory locking of TX queues
> in __netdev_dpdk_vhost_send(), but this locking was almost always anyway
> because of calling set_multiq with n_txq = 'ovs_numa_get_n_cores() + 1'.
>
> Fixes: 4573fbd38fa1 ("netdev-dpdk: Add vhost-user multiqueue support")
> Signed-off-by: Ilya Maximets <i.maxim...@samsung.com>
> Reviewed-by: Aaron Conole <acon...@redhat.com>
> Acked-by: Flavio Leitner <f...@sysclose.org>
If you do small changes to the patch that doesn't alter its logic you
can preserve the signature from others (e.g.: typos, indentation,
comments...). However, in this case you changed the logic so you can't
preserve those anymore.
The procedure would be to take them out and add the previous reviewers
to the CC in the hope that they will review the new patch again.
> ---
>
> Version 3:
> * Fixed size in dpdk_rte_mzalloc() for enabled_queues.
> * Fixed possible segfault because of unallocated tx_q[].
> * Added remap of tx queues after real_n_txq modification
> in netdev_dpdk_vhost_set_queues().
>
> lib/netdev-dpdk.c | 112
> ++++++++++++++++++++++++++++++++++++++++++++++--------
> 1 file changed, 97 insertions(+), 15 deletions(-)
>
> diff --git a/lib/netdev-dpdk.c b/lib/netdev-dpdk.c
> index e4f789b..3e69e50 100644
> --- a/lib/netdev-dpdk.c
> +++ b/lib/netdev-dpdk.c
> @@ -173,6 +173,8 @@ struct dpdk_tx_queue {
> * from concurrent access. It is used
> only
> * if the queue is shared among different
> * pmd threads (see 'txq_needs_locking').
> */
> + int map; /* Mapping of configured vhost-user queues
> + * to enabled by guest. */
> uint64_t tsc;
> struct rte_mbuf *burst_pkts[MAX_TX_QUEUE_LEN];
> };
> @@ -572,6 +574,9 @@ netdev_dpdk_alloc_txq(struct netdev_dpdk *netdev,
> unsigned int n_txqs)
> /* Queues are shared among CPUs. Always flush */
> netdev->tx_q[i].flush_tx = true;
> }
> +
> + /* Initialize map for vhost devices. */
> + netdev->tx_q[i].map = -1;
> rte_spinlock_init(&netdev->tx_q[i].tx_lock);
> }
> }
> @@ -623,6 +628,8 @@ netdev_dpdk_init(struct netdev *netdev_, unsigned int
> port_no,
> if (err) {
> goto unlock;
> }
> + } else {
> + netdev_dpdk_alloc_txq(netdev, VHOST_MAX_QUEUE_PAIRS);
The VHOST_MAX_QUEUE_PAIRS is 0x8000, so we are allocating 32768 queue.
Also that the struct dpdk_tx_queue has 3096 bytes so in the end it is
allocating 101MB for each vhost-user port.
Why do we need to pre-allocate all TX queues?
> }
>
> list_push_back(&dpdk_list, &netdev->list_node);
> @@ -889,10 +896,8 @@ netdev_dpdk_vhost_set_multiq(struct netdev *netdev_,
> unsigned int n_txq,
> ovs_mutex_lock(&dpdk_mutex);
> ovs_mutex_lock(&netdev->mutex);
>
> - rte_free(netdev->tx_q);
> netdev->up.n_txq = n_txq;
> netdev->up.n_rxq = n_rxq;
> - netdev_dpdk_alloc_txq(netdev, netdev->up.n_txq);
>
> ovs_mutex_unlock(&netdev->mutex);
> ovs_mutex_unlock(&dpdk_mutex);
> @@ -1117,17 +1122,16 @@ __netdev_dpdk_vhost_send(struct netdev *netdev, int
> qid,
> unsigned int total_pkts = cnt;
> uint64_t start = 0;
>
> - if (OVS_UNLIKELY(!is_vhost_running(virtio_dev))) {
> + qid = vhost_dev->tx_q[qid % vhost_dev->real_n_txq].map;
> +
> + if (OVS_UNLIKELY(!is_vhost_running(virtio_dev) || qid == -1)) {
> rte_spinlock_lock(&vhost_dev->stats_lock);
> vhost_dev->stats.tx_dropped+= cnt;
> rte_spinlock_unlock(&vhost_dev->stats_lock);
> goto out;
> }
>
> - if (vhost_dev->txq_needs_locking) {
> - qid = qid % vhost_dev->real_n_txq;
> - rte_spinlock_lock(&vhost_dev->tx_q[qid].tx_lock);
> - }
> + rte_spinlock_lock(&vhost_dev->tx_q[qid].tx_lock);
>
> do {
> int vhost_qid = qid * VIRTIO_QNUM + VIRTIO_RXQ;
> @@ -1165,9 +1169,7 @@ __netdev_dpdk_vhost_send(struct netdev *netdev, int qid,
> }
> } while (cnt);
>
> - if (vhost_dev->txq_needs_locking) {
> - rte_spinlock_unlock(&vhost_dev->tx_q[qid].tx_lock);
> - }
> + rte_spinlock_unlock(&vhost_dev->tx_q[qid].tx_lock);
>
> rte_spinlock_lock(&vhost_dev->stats_lock);
> netdev_dpdk_vhost_update_tx_counters(&vhost_dev->stats, pkts, total_pkts,
> @@ -1827,6 +1829,46 @@ set_irq_status(struct virtio_net *dev)
> }
> }
>
> +/*
> + * Fixes mapping for vhost-user tx queues. Must be called after each
> + * enabling/disabling of queues and real_n_txq modifications.
> + */
> +static void
> +netdev_dpdk_remap_txqs(struct netdev_dpdk *netdev)
> + OVS_REQUIRES(netdev->mutex)
> +{
> + int *enabled_queues, n_enabled = 0;
> + int i, k, total_txqs = netdev->real_n_txq;
> +
> + enabled_queues = dpdk_rte_mzalloc(total_txqs * sizeof *enabled_queues);
> +
> + for (i = 0; i < total_txqs; i++) {
> + /* Enabled queues always mapped to themselves. */
> + if (netdev->tx_q[i].map == i) {
> + enabled_queues[n_enabled++] = i;
> + }
> + }
> +
> + if (n_enabled == 0 && total_txqs != 0) {
> + enabled_queues[0] = -1;
> + n_enabled = 1;
> + }
> +
> + k = 0;
> + for (i = 0; i < total_txqs; i++) {
> + if (netdev->tx_q[i].map != i) {
> + netdev->tx_q[i].map = enabled_queues[k];
> + k = (k + 1) % n_enabled;
> + }
> + }
> +
> + VLOG_DBG("TX queue mapping for %s\n", netdev->vhost_id);
> + for (i = 0; i < total_txqs; i++) {
> + VLOG_DBG("%2d --> %2d", i, netdev->tx_q[i].map);
> + }
> +
> + rte_free(enabled_queues);
> +}
>
> static int
> netdev_dpdk_vhost_set_queues(struct netdev_dpdk *netdev, struct virtio_net
> *dev)
> @@ -1843,11 +1885,9 @@ netdev_dpdk_vhost_set_queues(struct netdev_dpdk
> *netdev, struct virtio_net *dev)
>
> netdev->real_n_rxq = qp_num;
> netdev->real_n_txq = qp_num;
> - if (netdev->up.n_txq > netdev->real_n_txq) {
> - netdev->txq_needs_locking = true;
> - } else {
> - netdev->txq_needs_locking = false;
> - }
> + netdev->txq_needs_locking = true;
> +
> + netdev_dpdk_remap_txqs(netdev);
>
> return 0;
> }
> @@ -1941,6 +1981,47 @@ destroy_device(volatile struct virtio_net *dev)
>
> }
>
> +static int
> +vring_state_changed(struct virtio_net *dev, uint16_t queue_id, int enable)
> +{
> + struct netdev_dpdk *vhost_dev;
> + bool exists = false;
> + int qid = queue_id / VIRTIO_QNUM;
> +
> + if (queue_id % VIRTIO_QNUM == VIRTIO_TXQ) {
> + return 0;
> + }
> +
> + ovs_mutex_lock(&dpdk_mutex);
> + LIST_FOR_EACH (vhost_dev, list_node, &dpdk_list) {
> + if (strncmp(dev->ifname, vhost_dev->vhost_id, IF_NAME_SZ) == 0) {
> + ovs_mutex_lock(&vhost_dev->mutex);
> + if (enable) {
> + vhost_dev->tx_q[qid].map = qid;
> + } else {
> + vhost_dev->tx_q[qid].map = -1;
> + }
> + netdev_dpdk_remap_txqs(vhost_dev);
> + exists = true;
> + ovs_mutex_unlock(&vhost_dev->mutex);
> + break;
> + }
> + }
> + ovs_mutex_unlock(&dpdk_mutex);
> +
> + if (exists) {
> + VLOG_INFO("State of queue %d ( tx_qid %d ) of vhost device '%s' %"
> + PRIu64" changed to \'%s\'", queue_id, qid, dev->ifname,
> + dev->device_fh, (enable == 1) ? "enabled" : "disabled");
> + } else {
> + VLOG_INFO("vHost Device '%s' %"PRIu64" not found", dev->ifname,
> + dev->device_fh);
> + return -1;
> + }
> +
> + return 0;
> +}
> +
> struct virtio_net *
> netdev_dpdk_get_virtio(const struct netdev_dpdk *dev)
> {
> @@ -1955,6 +2036,7 @@ static const struct virtio_net_device_ops
> virtio_net_device_ops =
> {
> .new_device = new_device,
> .destroy_device = destroy_device,
> + .vring_state_changed = vring_state_changed
> };
>
> static void *
--
fbl
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
