diff mbox

[dpdk-dev,2/4] virtio: introduce RTE_LIBRTE_VIRTIO_INC_VECTOR

Message ID 1467028448-8914-3-git-send-email-jerin.jacob@caviumnetworks.com (mailing list archive)
State Superseded, archived
Delegated to: Yuanhan Liu
Headers

Commit Message

Jerin Jacob June 27, 2016, 11:54 a.m. UTC 
  like other PMD drivers, introduce RTE_LIBRTE_VIRTIO_INC_VECTOR
for vector based handler selection in virtio

Enabled by default in common config and disabled for non X86
platforms

Signed-off-by: Jerin Jacob <jerin.jacob@caviumnetworks.com>
---
 config/common_base                          | 1 +
 config/defconfig_arm-armv7a-linuxapp-gcc    | 1 +
 config/defconfig_arm64-armv8a-linuxapp-gcc  | 1 +
 config/defconfig_ppc_64-power8-linuxapp-gcc | 1 +
 config/defconfig_tile-tilegx-linuxapp-gcc   | 1 +
 drivers/net/virtio/virtio_rxtx.c            | 2 ++
 6 files changed, 7 insertions(+)

Comments

Thomas Monjalon June 27, 2016, 2:19 p.m. UTC | #1 
2016-06-27 17:24, Jerin Jacob:
> --- a/config/common_base
> +++ b/config/common_base
> @@ -267,6 +267,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_LIBRTE_VIRTIO_INC_VECTOR=y

I don't remember what means INC_VECTOR?
Why a config option is needed for vector implementations?
Jerin Jacob June 27, 2016, 2:48 p.m. UTC | #2 
On Mon, Jun 27, 2016 at 04:19:57PM +0200, Thomas Monjalon wrote:
> 2016-06-27 17:24, Jerin Jacob:
> > --- a/config/common_base
> > +++ b/config/common_base
> > @@ -267,6 +267,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_LIBRTE_VIRTIO_INC_VECTOR=y
> 
> I don't remember what means INC_VECTOR?
> Why a config option is needed for vector implementations?

I thought of adding additional configuration option(INC_VECTOR) _apart_ from
cpu flag based scheme in the patch because even though if a given platform
has cpu instruction support, in some platforms scalar version may
perform well wrt vector version(based on instruction latency, emulation required or not
etc). So a top level flag INC_VECTOR, can override the vector selection
for a given platform if required.

Regarding INC_VECTOR(INC in vector configuration name, I have no idea, I
followed the existing flags)
$ grep "INC_VECTOR" config/common_base
CONFIG_RTE_IXGBE_INC_VECTOR=y
CONFIG_RTE_LIBRTE_I40E_INC_VECTOR=y
CONFIG_RTE_LIBRTE_FM10K_INC_VECTOR=y

Jerin
Thomas Monjalon June 27, 2016, 2:59 p.m. UTC | #3 
2016-06-27 20:18, Jerin Jacob:
> On Mon, Jun 27, 2016 at 04:19:57PM +0200, Thomas Monjalon wrote:
> > 2016-06-27 17:24, Jerin Jacob:
> > > --- a/config/common_base
> > > +++ b/config/common_base
> > > @@ -267,6 +267,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_LIBRTE_VIRTIO_INC_VECTOR=y
> > 
> > I don't remember what means INC_VECTOR?
> > Why a config option is needed for vector implementations?
> 
> I thought of adding additional configuration option(INC_VECTOR) _apart_ from
> cpu flag based scheme in the patch because even though if a given platform
> has cpu instruction support, in some platforms scalar version may
> perform well wrt vector version(based on instruction latency, emulation required or not
> etc). So a top level flag INC_VECTOR, can override the vector selection
> for a given platform if required.

Isn't it a runtime driver option needed to disable vector virtio?

> Regarding INC_VECTOR(INC in vector configuration name, I have no idea, I
> followed the existing flags)
> $ grep "INC_VECTOR" config/common_base
> CONFIG_RTE_IXGBE_INC_VECTOR=y
> CONFIG_RTE_LIBRTE_I40E_INC_VECTOR=y
> CONFIG_RTE_LIBRTE_FM10K_INC_VECTOR=y

If the flag is really needed I would suggest VIRTIO_VECTOR.
Jerin Jacob June 29, 2016, 11:18 a.m. UTC | #4 
On Mon, Jun 27, 2016 at 04:59:42PM +0200, Thomas Monjalon wrote:
> 2016-06-27 20:18, Jerin Jacob:
> > On Mon, Jun 27, 2016 at 04:19:57PM +0200, Thomas Monjalon wrote:
> > > 2016-06-27 17:24, Jerin Jacob:
> > > > --- a/config/common_base
> > > > +++ b/config/common_base
> > > > @@ -267,6 +267,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_LIBRTE_VIRTIO_INC_VECTOR=y
> > > 
> > > I don't remember what means INC_VECTOR?
> > > Why a config option is needed for vector implementations?
> > 
> > I thought of adding additional configuration option(INC_VECTOR) _apart_ from
> > cpu flag based scheme in the patch because even though if a given platform
> > has cpu instruction support, in some platforms scalar version may
> > perform well wrt vector version(based on instruction latency, emulation required or not
> > etc). So a top level flag INC_VECTOR, can override the vector selection
> > for a given platform if required.
> 
> Isn't it a runtime driver option needed to disable vector virtio?
> 
> > Regarding INC_VECTOR(INC in vector configuration name, I have no idea, I
> > followed the existing flags)
> > $ grep "INC_VECTOR" config/common_base
> > CONFIG_RTE_IXGBE_INC_VECTOR=y
> > CONFIG_RTE_LIBRTE_I40E_INC_VECTOR=y
> > CONFIG_RTE_LIBRTE_FM10K_INC_VECTOR=y
> 
> If the flag is really needed I would suggest VIRTIO_VECTOR.

OK I will change to VIRTIO_VECTOR
Thomas Monjalon June 29, 2016, 11:25 a.m. UTC | #5 
2016-06-29 16:48, Jerin Jacob:
> On Mon, Jun 27, 2016 at 04:59:42PM +0200, Thomas Monjalon wrote:
> > 2016-06-27 20:18, Jerin Jacob:
> > > On Mon, Jun 27, 2016 at 04:19:57PM +0200, Thomas Monjalon wrote:
> > > > 2016-06-27 17:24, Jerin Jacob:
> > > > > --- a/config/common_base
> > > > > +++ b/config/common_base
> > > > > @@ -267,6 +267,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_LIBRTE_VIRTIO_INC_VECTOR=y
> > > > 
> > > > I don't remember what means INC_VECTOR?
> > > > Why a config option is needed for vector implementations?
> > > 
> > > I thought of adding additional configuration option(INC_VECTOR) _apart_ from
> > > cpu flag based scheme in the patch because even though if a given platform
> > > has cpu instruction support, in some platforms scalar version may
> > > perform well wrt vector version(based on instruction latency, emulation required or not
> > > etc). So a top level flag INC_VECTOR, can override the vector selection
> > > for a given platform if required.
> > 
> > Isn't it a runtime driver option needed to disable vector virtio?
> > 
> > > Regarding INC_VECTOR(INC in vector configuration name, I have no idea, I
> > > followed the existing flags)
> > > $ grep "INC_VECTOR" config/common_base
> > > CONFIG_RTE_IXGBE_INC_VECTOR=y
> > > CONFIG_RTE_LIBRTE_I40E_INC_VECTOR=y
> > > CONFIG_RTE_LIBRTE_FM10K_INC_VECTOR=y
> > 
> > If the flag is really needed I would suggest VIRTIO_VECTOR.
> 
> OK I will change to VIRTIO_VECTOR

I would prefer a runtime option.
Jerin Jacob June 29, 2016, 11:40 a.m. UTC | #6 
On Wed, Jun 29, 2016 at 01:25:35PM +0200, Thomas Monjalon wrote:
> 2016-06-29 16:48, Jerin Jacob:
> > On Mon, Jun 27, 2016 at 04:59:42PM +0200, Thomas Monjalon wrote:
> > > 2016-06-27 20:18, Jerin Jacob:
> > > > On Mon, Jun 27, 2016 at 04:19:57PM +0200, Thomas Monjalon wrote:
> > > > > 2016-06-27 17:24, Jerin Jacob:
> > > > > > --- a/config/common_base
> > > > > > +++ b/config/common_base
> > > > > > @@ -267,6 +267,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_LIBRTE_VIRTIO_INC_VECTOR=y
> > > > > 
> > > > > I don't remember what means INC_VECTOR?
> > > > > Why a config option is needed for vector implementations?
> > > > 
> > > > I thought of adding additional configuration option(INC_VECTOR) _apart_ from
> > > > cpu flag based scheme in the patch because even though if a given platform
> > > > has cpu instruction support, in some platforms scalar version may
> > > > perform well wrt vector version(based on instruction latency, emulation required or not
> > > > etc). So a top level flag INC_VECTOR, can override the vector selection
> > > > for a given platform if required.
> > > 
> > > Isn't it a runtime driver option needed to disable vector virtio?
> > > 
> > > > Regarding INC_VECTOR(INC in vector configuration name, I have no idea, I
> > > > followed the existing flags)
> > > > $ grep "INC_VECTOR" config/common_base
> > > > CONFIG_RTE_IXGBE_INC_VECTOR=y
> > > > CONFIG_RTE_LIBRTE_I40E_INC_VECTOR=y
> > > > CONFIG_RTE_LIBRTE_FM10K_INC_VECTOR=y
> > > 
> > > If the flag is really needed I would suggest VIRTIO_VECTOR.
> > 
> > OK I will change to VIRTIO_VECTOR
> 
> I would prefer a runtime option.

OK

The platform I test their was NO need for additional VIRTIO_VECTOR
configuration as NEON versions outperforms than scalar version.

I thought of adding this option to override for any platform if it need
to accommodate such platform differences NEON vs scalar versions.

I will change completely to run-time detection based on cpuflags for IA and ARM.
Any objections?

Jerin
Yuanhan Liu June 30, 2016, 5:44 a.m. UTC | #7 
On Wed, Jun 29, 2016 at 05:10:31PM +0530, Jerin Jacob wrote:
> On Wed, Jun 29, 2016 at 01:25:35PM +0200, Thomas Monjalon wrote:
> > 2016-06-29 16:48, Jerin Jacob:
> > > On Mon, Jun 27, 2016 at 04:59:42PM +0200, Thomas Monjalon wrote:
> > > > 2016-06-27 20:18, Jerin Jacob:
> > > > > On Mon, Jun 27, 2016 at 04:19:57PM +0200, Thomas Monjalon wrote:
> > > > > > 2016-06-27 17:24, Jerin Jacob:
> > > > > > > --- a/config/common_base
> > > > > > > +++ b/config/common_base
> > > > > > > @@ -267,6 +267,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_LIBRTE_VIRTIO_INC_VECTOR=y
> > > > > > 
> > > > > > I don't remember what means INC_VECTOR?
> > > > > > Why a config option is needed for vector implementations?
> > > > > 
> > > > > I thought of adding additional configuration option(INC_VECTOR) _apart_ from
> > > > > cpu flag based scheme in the patch because even though if a given platform
> > > > > has cpu instruction support, in some platforms scalar version may
> > > > > perform well wrt vector version(based on instruction latency, emulation required or not
> > > > > etc). So a top level flag INC_VECTOR, can override the vector selection
> > > > > for a given platform if required.
> > > > 
> > > > Isn't it a runtime driver option needed to disable vector virtio?
> > > > 
> > > > > Regarding INC_VECTOR(INC in vector configuration name, I have no idea, I
> > > > > followed the existing flags)
> > > > > $ grep "INC_VECTOR" config/common_base
> > > > > CONFIG_RTE_IXGBE_INC_VECTOR=y
> > > > > CONFIG_RTE_LIBRTE_I40E_INC_VECTOR=y
> > > > > CONFIG_RTE_LIBRTE_FM10K_INC_VECTOR=y
> > > > 
> > > > If the flag is really needed I would suggest VIRTIO_VECTOR.
> > > 
> > > OK I will change to VIRTIO_VECTOR
> > 
> > I would prefer a runtime option.

+1

> 
> OK
> 
> The platform I test their was NO need for additional VIRTIO_VECTOR
> configuration as NEON versions outperforms than scalar version.
> 
> I thought of adding this option to override for any platform if it need
> to accommodate such platform differences NEON vs scalar versions.
> 
> I will change completely to run-time detection based on cpuflags for IA and ARM.
> Any objections?

Nope from me.

	--yliu
diff mbox

Patch

diff --git a/config/common_base b/config/common_base
index 3a04fba..f6ce168 100644
--- a/config/common_base
+++ b/config/common_base
@@ -267,6 +267,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_LIBRTE_VIRTIO_INC_VECTOR=y
 
 #
 # Compile burst-oriented VMXNET3 PMD driver
diff --git a/config/defconfig_arm-armv7a-linuxapp-gcc b/config/defconfig_arm-armv7a-linuxapp-gcc
index bde6acd..a249ad5 100644
--- a/config/defconfig_arm-armv7a-linuxapp-gcc
+++ b/config/defconfig_arm-armv7a-linuxapp-gcc
@@ -75,3 +75,4 @@  CONFIG_RTE_LIBRTE_VMXNET3_PMD=n
 CONFIG_RTE_LIBRTE_PMD_XENVIRT=n
 CONFIG_RTE_LIBRTE_PMD_BNX2X=n
 CONFIG_RTE_LIBRTE_QEDE_PMD=n
+CONFIG_RTE_LIBRTE_VIRTIO_INC_VECTOR=n
diff --git a/config/defconfig_arm64-armv8a-linuxapp-gcc b/config/defconfig_arm64-armv8a-linuxapp-gcc
index a786562..95ed30e 100644
--- a/config/defconfig_arm64-armv8a-linuxapp-gcc
+++ b/config/defconfig_arm64-armv8a-linuxapp-gcc
@@ -48,5 +48,6 @@  CONFIG_RTE_IXGBE_INC_VECTOR=n
 CONFIG_RTE_LIBRTE_IVSHMEM=n
 CONFIG_RTE_LIBRTE_FM10K_PMD=n
 CONFIG_RTE_LIBRTE_I40E_PMD=n
+CONFIG_RTE_LIBRTE_VIRTIO_INC_VECTOR=n
 
 CONFIG_RTE_SCHED_VECTOR=n
diff --git a/config/defconfig_ppc_64-power8-linuxapp-gcc b/config/defconfig_ppc_64-power8-linuxapp-gcc
index bef8f49..1eca73a 100644
--- a/config/defconfig_ppc_64-power8-linuxapp-gcc
+++ b/config/defconfig_ppc_64-power8-linuxapp-gcc
@@ -51,6 +51,7 @@  CONFIG_RTE_LIBRTE_EAL_VMWARE_TSC_MAP_SUPPORT=n
 CONFIG_RTE_LIBRTE_IXGBE_PMD=n
 CONFIG_RTE_LIBRTE_I40E_PMD=n
 CONFIG_RTE_LIBRTE_VIRTIO_PMD=y
+CONFIG_RTE_LIBRTE_VIRTIO_INC_VECTOR=n
 CONFIG_RTE_LIBRTE_VMXNET3_PMD=n
 CONFIG_RTE_LIBRTE_PMD_BOND=n
 CONFIG_RTE_LIBRTE_ENIC_PMD=n
diff --git a/config/defconfig_tile-tilegx-linuxapp-gcc b/config/defconfig_tile-tilegx-linuxapp-gcc
index 5a50793..0d6fe1e 100644
--- a/config/defconfig_tile-tilegx-linuxapp-gcc
+++ b/config/defconfig_tile-tilegx-linuxapp-gcc
@@ -59,6 +59,7 @@  CONFIG_RTE_LIBRTE_IXGBE_PMD=n
 CONFIG_RTE_LIBRTE_I40E_PMD=n
 CONFIG_RTE_LIBRTE_FM10K_PMD=n
 CONFIG_RTE_LIBRTE_VIRTIO_PMD=n
+CONFIG_RTE_LIBRTE_VIRTIO_INC_VECTOR=n
 CONFIG_RTE_LIBRTE_VMXNET3_PMD=n
 CONFIG_RTE_LIBRTE_ENIC_PMD=n
 
diff --git a/drivers/net/virtio/virtio_rxtx.c b/drivers/net/virtio/virtio_rxtx.c
index 63b53f7..e9b42f3 100644
--- a/drivers/net/virtio/virtio_rxtx.c
+++ b/drivers/net/virtio/virtio_rxtx.c
@@ -499,6 +499,7 @@  virtio_dev_tx_queue_setup(struct rte_eth_dev *dev,
 		return -EINVAL;
 	}
 
+#ifdef RTE_LIBRTE_VIRTIO_INC_VECTOR
 #ifdef RTE_MACHINE_CPUFLAG_SSSE3
 	struct virtio_hw *hw = dev->data->dev_private;
 	/* Use simple rx/tx func if single segment and no offloads */
@@ -510,6 +511,7 @@  virtio_dev_tx_queue_setup(struct rte_eth_dev *dev,
 		hw->use_simple_rxtx = 1;
 	}
 #endif
+#endif
 
 	ret = virtio_dev_queue_setup(dev, VTNET_TQ, queue_idx, vtpci_queue_idx,
 			nb_desc, socket_id, (void **)&txvq);