[dpdk-dev,v4,01/14] virtio: Introduce config RTE_VIRTIO_INC_VECTOR
Commit Message
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.
Signed-off-by: Santosh Shukla <sshukla@mvista.com>
---
config/common_linuxapp | 1 +
drivers/net/virtio/Makefile | 2 +-
drivers/net/virtio/virtio_rxtx.c | 7 +++++++
3 files changed, 9 insertions(+), 1 deletion(-)
Comments
On Thu, Jan 14, 2016 at 06:58:24PM +0530, Santosh Shukla wrote:
> 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.
>
> Signed-off-by: Santosh Shukla <sshukla@mvista.com>
> ---
> 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.
--yliu
On Fri, Jan 15, 2016 at 12:21 PM, Yuanhan Liu
<yuanhan.liu@linux.intel.com> wrote:
> On Thu, Jan 14, 2016 at 06:58:24PM +0530, Santosh Shukla wrote:
>> 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.
>>
>> Signed-off-by: Santosh Shukla <sshukla@mvista.com>
>> ---
>> 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
@@ -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
@@ -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
@@ -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
return 0;
}
@@ -464,7 +466,10 @@ virtio_dev_tx_queue_setup(struct rte_eth_dev *dev,
const struct rte_eth_txconf *tx_conf)
{
uint8_t vtpci_queue_idx = 2 * queue_idx + VTNET_SQ_TQ_QUEUE_IDX;
+
+#ifdef RTE_VIRTIO_INC_VECTOR
struct virtio_hw *hw = dev->data->dev_private;
+#endif
struct virtqueue *vq;
uint16_t tx_free_thresh;
int ret;
@@ -477,6 +482,7 @@ virtio_dev_tx_queue_setup(struct rte_eth_dev *dev,
return -EINVAL;
}
+#ifdef RTE_VIRTIO_INC_VECTOR
/* Use simple rx/tx func if single segment and no offloads */
if ((tx_conf->txq_flags & VIRTIO_SIMPLE_FLAGS) == VIRTIO_SIMPLE_FLAGS &&
!vtpci_with_feature(hw, VIRTIO_NET_F_MRG_RXBUF)) {
@@ -485,6 +491,7 @@ virtio_dev_tx_queue_setup(struct rte_eth_dev *dev,
dev->rx_pkt_burst = virtio_recv_pkts_vec;
use_simple_rxtx = 1;
}
+#endif
ret = virtio_dev_queue_setup(dev, VTNET_TQ, queue_idx, vtpci_queue_idx,
nb_desc, socket_id, &vq);