Re: [PATCH v4 01/14] virtio: Introduce config RTE_VIRTIO_INC_VECTOR

On Fri, Jan 15, 2016 at 12:21 PM, Yuanhan Liu
[off-list ref] wrote:

On Thu, Jan 14, 2016 at 06:58:24PM +0530, Santosh Shukla wrote:

quoted

virtio_recv_pkts_vec and other virtio vector friend apis are written for sse/avx
instructions. For arm64 in particular, virtio vector implementation does not
exist(todo).

So virtio pmd driver wont build for targets like i686, arm64.  By making
RTE_VIRTIO_INC_VECTOR=n, Driver can build for non-sse/avx targets and will work
in non-vectored virtio mode.

While revisiting this patch, I'm thinking you may squash both patch 2
and patch 11 into this one.

Make sense!, we'll do in v5.

quoted

Signed-off-by: Santosh Shukla <redacted>
---
 config/common_linuxapp           |    1 +
 drivers/net/virtio/Makefile      |    2 +-
 drivers/net/virtio/virtio_rxtx.c |    7 +++++++
 3 files changed, 9 insertions(+), 1 deletion(-)

diff --git a/config/common_linuxapp b/config/common_linuxapp
index 74bc515..8677697 100644
--- a/config/common_linuxapp
+++ b/config/common_linuxapp

@@ -274,6 +274,7 @@ CONFIG_RTE_LIBRTE_VIRTIO_DEBUG_RX=n
 CONFIG_RTE_LIBRTE_VIRTIO_DEBUG_TX=n
 CONFIG_RTE_LIBRTE_VIRTIO_DEBUG_DRIVER=n
 CONFIG_RTE_LIBRTE_VIRTIO_DEBUG_DUMP=n
+CONFIG_RTE_VIRTIO_INC_VECTOR=y

 #
 # Compile burst-oriented VMXNET3 PMD driver

diff --git a/drivers/net/virtio/Makefile b/drivers/net/virtio/Makefile
index 43835ba..25a842d 100644
--- a/drivers/net/virtio/Makefile
+++ b/drivers/net/virtio/Makefile

@@ -50,7 +50,7 @@ SRCS-$(CONFIG_RTE_LIBRTE_VIRTIO_PMD) += virtqueue.c
 SRCS-$(CONFIG_RTE_LIBRTE_VIRTIO_PMD) += virtio_pci.c
 SRCS-$(CONFIG_RTE_LIBRTE_VIRTIO_PMD) += virtio_rxtx.c
 SRCS-$(CONFIG_RTE_LIBRTE_VIRTIO_PMD) += virtio_ethdev.c
-SRCS-$(CONFIG_RTE_LIBRTE_VIRTIO_PMD) += virtio_rxtx_simple.c
+SRCS-$(CONFIG_RTE_VIRTIO_INC_VECTOR) += virtio_rxtx_simple.c

 # this lib depends upon:
 DEPDIRS-$(CONFIG_RTE_LIBRTE_VIRTIO_PMD) += lib/librte_eal lib/librte_ether

diff --git a/drivers/net/virtio/virtio_rxtx.c b/drivers/net/virtio/virtio_rxtx.c
index 74b39ef..23be1ff 100644
--- a/drivers/net/virtio/virtio_rxtx.c
+++ b/drivers/net/virtio/virtio_rxtx.c

@@ -438,7 +438,9 @@ virtio_dev_rx_queue_setup(struct rte_eth_dev *dev,

      dev->data->rx_queues[queue_idx] = vq;

+#ifdef RTE_VIRTIO_INC_VECTOR
      virtio_rxq_vec_setup(vq);
+#endif

You should put such macros for the declaration in virtio_rxtx.h as well.


And note that you may miss one:


    325                         /******************************************
    326                         *         Enqueue allocated buffers        *
    327                         *******************************************/
    328                         if (use_simple_rxtx)
==> 329                                 error = virtqueue_enqueue_recv_refill_simple(vq, m);
    330                         else
    331                                 error = virtqueue_enqueue_recv_refill(vq, m);
    332                         if (error) {
    333                                 rte_pktmbuf_free(m);
    334                                 break;
    335                         }


virtqueue_enqueue_recv_refill_simple() is defined inside virtio_rxtx_simple.c,
which is built only when CONFIG_RTE_VIRTIO_INC_VECTOR is set. But I see no
such check here.

Note that this will not break the build, as gcc just ignores it, for use_simple_rxtx
is 0 by default, thus the "if" part code is not compiled at all. Even for that,
I think it's better to put an explicit macro check here.

we'll make changes in v5, Thanks for review comment.

        --yliu