[dpdk-dev] [PATCH] net/virtio: fix Rx and Tx handler selection for arm32

[dpdk-dev] [PATCH] net/virtio: fix Rx and Tx handler selection for arm32

[Olivier Matz]

From: Samuel Gauthier <samuel.gauthier@6wind.com>

On arm32, we were always selecting the simple handler, but it is only
available if neon is present.

This is due to a typo in the name of the config option.
CONFIG_RTE_ARCH_ARM is for Makefiles. One should use RTE_ARCH_ARM.

Fixes: 2d7c37194ee4 ("net/virtio: add NEON based Rx handler")
Cc: stable@dpdk.org

Signed-off-by: Samuel Gauthier <samuel.gauthier@6wind.com>
---
 drivers/net/virtio/virtio_ethdev.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/virtio/virtio_ethdev.c b/drivers/net/virtio/virtio_ethdev.c
index e0328f61d..c0ba83b06 100644
--- a/drivers/net/virtio/virtio_ethdev.c
+++ b/drivers/net/virtio/virtio_ethdev.c
@@ -1784,7 +1784,7 @@ virtio_dev_configure(struct rte_eth_dev *dev)
 	hw->use_simple_rx = 1;
 	hw->use_simple_tx = 1;
 
-#if defined RTE_ARCH_ARM64 || defined CONFIG_RTE_ARCH_ARM
+#if defined RTE_ARCH_ARM64 || defined RTE_ARCH_ARM
 	if (!rte_cpu_get_flag_enabled(RTE_CPUFLAG_NEON)) {
 		hw->use_simple_rx = 0;
 		hw->use_simple_tx = 0;
-- 
2.11.0

Re: [dpdk-dev] [PATCH] net/virtio: fix Rx and Tx handler selection for arm32

[Maxime Coquelin]

Hi Olivier,

On 12/14/2017 03:32 PM, Olivier Matz wrote:
> From: Samuel Gauthier<samuel.gauthier@6wind.com>
> 
> On arm32, we were always selecting the simple handler, but it is only
> available if neon is present.
> 
> This is due to a typo in the name of the config option.
> CONFIG_RTE_ARCH_ARM is for Makefiles. One should use RTE_ARCH_ARM.
> 
> Fixes: 2d7c37194ee4 ("net/virtio: add NEON based Rx handler")
> Cc:stable@dpdk.org
> 
> Signed-off-by: Samuel Gauthier<samuel.gauthier@6wind.com>
> ---
>   drivers/net/virtio/virtio_ethdev.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)

Reviewed-by: Maxime Coquelin <maxime.coquelin@redhat.com>

Thanks!
Maxime

Re: [dpdk-dev] [PATCH] net/virtio: fix Rx and Tx handler selection for arm32

[Yuanhan Liu]

On Thu, Dec 14, 2017 at 03:45:04PM +0100, Maxime Coquelin wrote:
> Hi Olivier,
> 
> On 12/14/2017 03:32 PM, Olivier Matz wrote:
> >From: Samuel Gauthier<samuel.gauthier@6wind.com>
> >
> >On arm32, we were always selecting the simple handler, but it is only
> >available if neon is present.
> >
> >This is due to a typo in the name of the config option.
> >CONFIG_RTE_ARCH_ARM is for Makefiles. One should use RTE_ARCH_ARM.
> >
> >Fixes: 2d7c37194ee4 ("net/virtio: add NEON based Rx handler")
> >Cc:stable@dpdk.org
> >
> >Signed-off-by: Samuel Gauthier<samuel.gauthier@6wind.com>
> >---
> >  drivers/net/virtio/virtio_ethdev.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> Reviewed-by: Maxime Coquelin <maxime.coquelin@redhat.com>

Applied to dpdk-next-virtio.

Thanks.

	--yliu

Re: [dpdk-dev] [PATCH] net/virtio: fix Rx and Tx handler selection for arm32

[Tiwei Bie]

On Thu, Dec 14, 2017 at 03:32:13PM +0100, Olivier Matz wrote:
> From: Samuel Gauthier <samuel.gauthier@6wind.com>
> 
> On arm32, we were always selecting the simple handler, but it is only
> available if neon is present.
> 
> This is due to a typo in the name of the config option.
> CONFIG_RTE_ARCH_ARM is for Makefiles. One should use RTE_ARCH_ARM.
> 
> Fixes: 2d7c37194ee4 ("net/virtio: add NEON based Rx handler")
> Cc: stable@dpdk.org

Hi Olivier,

My comment isn't really related to this patch, but related
to the commit it fixes and some related commits from you.

The commit (2d7c37194ee4) specified by the fixline doesn't
really cause the problem described in the commit log:

"On arm32, we were always selecting the simple handler, ..."

Actually, it will cause the simple handler won't be chosen
on arm32. Below is the relevant part (use_simple_rxtx won't
get a chance to be updated to 1 on arm32):

diff --git a/drivers/net/virtio/virtio_rxtx.c b/drivers/net/virtio/virtio_rxtx.c
index 0e369bd12..9ab441bc3 100644
--- a/drivers/net/virtio/virtio_rxtx.c
+++ b/drivers/net/virtio/virtio_rxtx.c
@@ -488,6 +488,9 @@ virtio_update_rxtx_handler(struct rte_eth_dev *dev,
 #if defined RTE_ARCH_X86
 	if (rte_cpu_get_flag_enabled(RTE_CPUFLAG_SSE3))
 		use_simple_rxtx = 1;
+#elif defined RTE_ARCH_ARM64 || defined CONFIG_RTE_ARCH_ARM
+	if (rte_cpu_get_flag_enabled(RTE_CPUFLAG_NEON))
+		use_simple_rxtx = 1;
 #endif
 	/* Use simple rx/tx func if single segment and no offloads */
 	if (use_simple_rxtx &&

It's the below commits (together with the above commit)
caused the simple handler will always be chosen on arm32:

4819eae8d94b ("net/virtio: rationalize setting of Rx/Tx handlers")
0964936308cd ("net/virtio: keep Rx handler whatever the Tx queue config")

For the above two commits, I think they have some other
problems. From my understanding, vector Rx function of
virtio PMD doesn't really follow the virtio spec. It
assumes the desc idx in the used ring will be written by
the backend in an expected order (i.e. the same order in
avail ring). So it even doesn't read the id field from
the used_elem to get the desc idx (but actually it should,
unless we change the virtio spec). It seems that simple
Tx function also has similar problem.

So IMO, we shouldn't choose the simple functions on all
platforms unless users enable them explicitly as we can't
guarantee they will work with all backends. So maybe the
problems can be fixed further? Any thoughts?

Best regards,
Tiwei Bie

> 
> Signed-off-by: Samuel Gauthier <samuel.gauthier@6wind.com>
> ---
>  drivers/net/virtio/virtio_ethdev.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/net/virtio/virtio_ethdev.c b/drivers/net/virtio/virtio_ethdev.c
> index e0328f61d..c0ba83b06 100644
> --- a/drivers/net/virtio/virtio_ethdev.c
> +++ b/drivers/net/virtio/virtio_ethdev.c
> @@ -1784,7 +1784,7 @@ virtio_dev_configure(struct rte_eth_dev *dev)
>  	hw->use_simple_rx = 1;
>  	hw->use_simple_tx = 1;
>  
> -#if defined RTE_ARCH_ARM64 || defined CONFIG_RTE_ARCH_ARM
> +#if defined RTE_ARCH_ARM64 || defined RTE_ARCH_ARM
>  	if (!rte_cpu_get_flag_enabled(RTE_CPUFLAG_NEON)) {
>  		hw->use_simple_rx = 0;
>  		hw->use_simple_tx = 0;
> -- 
> 2.11.0
> 

Re: [dpdk-dev] [PATCH] net/virtio: fix Rx and Tx handler selection for arm32

[Olivier MATZ]

Hi Tiwei,

On Mon, Dec 18, 2017 at 10:50:47AM +0800, Tiwei Bie wrote:
> On Thu, Dec 14, 2017 at 03:32:13PM +0100, Olivier Matz wrote:
> > From: Samuel Gauthier <samuel.gauthier@6wind.com>
> > 
> > On arm32, we were always selecting the simple handler, but it is only
> > available if neon is present.
> > 
> > This is due to a typo in the name of the config option.
> > CONFIG_RTE_ARCH_ARM is for Makefiles. One should use RTE_ARCH_ARM.
> > 
> > Fixes: 2d7c37194ee4 ("net/virtio: add NEON based Rx handler")
> > Cc: stable@dpdk.org
> 
> Hi Olivier,
> 
> My comment isn't really related to this patch, but related
> to the commit it fixes and some related commits from you.
> 
> The commit (2d7c37194ee4) specified by the fixline doesn't
> really cause the problem described in the commit log:
> 
> "On arm32, we were always selecting the simple handler, ..."
> 
> Actually, it will cause the simple handler won't be chosen
> on arm32. Below is the relevant part (use_simple_rxtx won't
> get a chance to be updated to 1 on arm32):
> 
> diff --git a/drivers/net/virtio/virtio_rxtx.c b/drivers/net/virtio/virtio_rxtx.c
> index 0e369bd12..9ab441bc3 100644
> --- a/drivers/net/virtio/virtio_rxtx.c
> +++ b/drivers/net/virtio/virtio_rxtx.c
> @@ -488,6 +488,9 @@ virtio_update_rxtx_handler(struct rte_eth_dev *dev,
>  #if defined RTE_ARCH_X86
>  	if (rte_cpu_get_flag_enabled(RTE_CPUFLAG_SSE3))
>  		use_simple_rxtx = 1;
> +#elif defined RTE_ARCH_ARM64 || defined CONFIG_RTE_ARCH_ARM
> +	if (rte_cpu_get_flag_enabled(RTE_CPUFLAG_NEON))
> +		use_simple_rxtx = 1;
>  #endif
>  	/* Use simple rx/tx func if single segment and no offloads */
>  	if (use_simple_rxtx &&

You are right. The commit 2d7c37194ee4 introduces the typo in the config
name, but does not trigger the same issue since the handler selection
logic was reversed later.

I think we can keep this "Fixes: 2d7c37194ee4" but also add the other
that you quote below.

> It's the below commits (together with the above commit)
> caused the simple handler will always be chosen on arm32:
> 
> 4819eae8d94b ("net/virtio: rationalize setting of Rx/Tx handlers")
> 0964936308cd ("net/virtio: keep Rx handler whatever the Tx queue config")
> 
> For the above two commits, I think they have some other
> problems. From my understanding, vector Rx function of
> virtio PMD doesn't really follow the virtio spec. It
> assumes the desc idx in the used ring will be written by
> the backend in an expected order (i.e. the same order in
> avail ring). So it even doesn't read the id field from
> the used_elem to get the desc idx (but actually it should,
> unless we change the virtio spec). It seems that simple
> Tx function also has similar problem.

Ok, I was not aware of this.

> So IMO, we shouldn't choose the simple functions on all
> platforms unless users enable them explicitly as we can't
> guarantee they will work with all backends. So maybe the
> problems can be fixed further? Any thoughts?

Yes, if the vector implementation does not follow the specifications, I
think it should be fixed (prefered solution) or disabled by default. I'm
wondering what would be the proper way to enable it, maybe it could be a
device argument (rte_devargs)?

In that case, we could replace:

   hw->use_simple_rx = 1;
   hw->use_simple_tx = 1;

by:

   hw->use_simple_rx = <value from devargs>;
   hw->use_simple_tx = <value from devargs>;


Olivier

Re: [dpdk-dev] [PATCH] net/virtio: fix Rx and Tx handler selection for arm32

[Tiwei Bie]

Hi Olivier,

On Tue, Dec 19, 2017 at 11:50:31AM +0100, Olivier MATZ wrote:
> On Mon, Dec 18, 2017 at 10:50:47AM +0800, Tiwei Bie wrote:
> > On Thu, Dec 14, 2017 at 03:32:13PM +0100, Olivier Matz wrote:
> > > From: Samuel Gauthier <samuel.gauthier@6wind.com>
> > > 
> > > On arm32, we were always selecting the simple handler, but it is only
> > > available if neon is present.
> > > 
> > > This is due to a typo in the name of the config option.
> > > CONFIG_RTE_ARCH_ARM is for Makefiles. One should use RTE_ARCH_ARM.
> > > 
> > > Fixes: 2d7c37194ee4 ("net/virtio: add NEON based Rx handler")
> > > Cc: stable@dpdk.org
> > 
> > Hi Olivier,
> > 
> > My comment isn't really related to this patch, but related
> > to the commit it fixes and some related commits from you.
> > 
> > The commit (2d7c37194ee4) specified by the fixline doesn't
> > really cause the problem described in the commit log:
> > 
> > "On arm32, we were always selecting the simple handler, ..."
> > 
> > Actually, it will cause the simple handler won't be chosen
> > on arm32. Below is the relevant part (use_simple_rxtx won't
> > get a chance to be updated to 1 on arm32):
> > 
> > diff --git a/drivers/net/virtio/virtio_rxtx.c b/drivers/net/virtio/virtio_rxtx.c
> > index 0e369bd12..9ab441bc3 100644
> > --- a/drivers/net/virtio/virtio_rxtx.c
> > +++ b/drivers/net/virtio/virtio_rxtx.c
> > @@ -488,6 +488,9 @@ virtio_update_rxtx_handler(struct rte_eth_dev *dev,
> >  #if defined RTE_ARCH_X86
> >  	if (rte_cpu_get_flag_enabled(RTE_CPUFLAG_SSE3))
> >  		use_simple_rxtx = 1;
> > +#elif defined RTE_ARCH_ARM64 || defined CONFIG_RTE_ARCH_ARM
> > +	if (rte_cpu_get_flag_enabled(RTE_CPUFLAG_NEON))
> > +		use_simple_rxtx = 1;
> >  #endif
> >  	/* Use simple rx/tx func if single segment and no offloads */
> >  	if (use_simple_rxtx &&
> 
> You are right. The commit 2d7c37194ee4 introduces the typo in the config
> name, but does not trigger the same issue since the handler selection
> logic was reversed later.
> 
> I think we can keep this "Fixes: 2d7c37194ee4" but also add the other
> that you quote below.

Looks good to me. Or I think it's also OK to mainly talk
about the typo in the commit log without talking about the
exact effect to the Tx/Rx handler selection.

> 
> > It's the below commits (together with the above commit)
> > caused the simple handler will always be chosen on arm32:
> > 
> > 4819eae8d94b ("net/virtio: rationalize setting of Rx/Tx handlers")
> > 0964936308cd ("net/virtio: keep Rx handler whatever the Tx queue config")
> > 
> > For the above two commits, I think they have some other
> > problems. From my understanding, vector Rx function of
> > virtio PMD doesn't really follow the virtio spec. It
> > assumes the desc idx in the used ring will be written by
> > the backend in an expected order (i.e. the same order in
> > avail ring). So it even doesn't read the id field from
> > the used_elem to get the desc idx (but actually it should,
> > unless we change the virtio spec). It seems that simple
> > Tx function also has similar problem.
> 
> Ok, I was not aware of this.
> 
> > So IMO, we shouldn't choose the simple functions on all
> > platforms unless users enable them explicitly as we can't
> > guarantee they will work with all backends. So maybe the
> > problems can be fixed further? Any thoughts?
> 
> Yes, if the vector implementation does not follow the specifications, I
> think it should be fixed (prefered solution) or disabled by default. I'm
> wondering what would be the proper way to enable it, maybe it could be a
> device argument (rte_devargs)?

I'm not sure how to fix it because we don't want to see big
performance drop after the fix. If it can be fixed properly
in this release, it would be great. Otherwise, we may need
to choose the 2nd solution (i.e. disabled by default) first.
Adding devargs looks good to me.

Best regards,
Tiwei Bie

> 
> In that case, we could replace:
> 
>    hw->use_simple_rx = 1;
>    hw->use_simple_tx = 1;
> 
> by:
> 
>    hw->use_simple_rx = <value from devargs>;
>    hw->use_simple_tx = <value from devargs>;
> 
> 
> Olivier