* [dpdk-stable] [PATCH] net/virtio: fix Rx and Tx handler selection for arm32
@ 2017-12-14 14:32 Olivier Matz
2017-12-14 14:45 ` Maxime Coquelin
2017-12-18 2:50 ` [dpdk-stable] [dpdk-dev] " Tiwei Bie
0 siblings, 2 replies; 6+ messages in thread
From: Olivier Matz @ 2017-12-14 14:32 UTC (permalink / raw)
To: dev, Yuanhan Liu, Maxime Coquelin; +Cc: Samuel Gauthier, stable
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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [dpdk-stable] [PATCH] net/virtio: fix Rx and Tx handler selection for arm32
2017-12-14 14:32 [dpdk-stable] [PATCH] net/virtio: fix Rx and Tx handler selection for arm32 Olivier Matz
@ 2017-12-14 14:45 ` Maxime Coquelin
2018-01-26 15:28 ` Yuanhan Liu
2017-12-18 2:50 ` [dpdk-stable] [dpdk-dev] " Tiwei Bie
1 sibling, 1 reply; 6+ messages in thread
From: Maxime Coquelin @ 2017-12-14 14:45 UTC (permalink / raw)
To: Olivier Matz, dev, Yuanhan Liu; +Cc: Samuel Gauthier, stable
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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [dpdk-stable] [dpdk-dev] [PATCH] net/virtio: fix Rx and Tx handler selection for arm32
2017-12-14 14:32 [dpdk-stable] [PATCH] net/virtio: fix Rx and Tx handler selection for arm32 Olivier Matz
2017-12-14 14:45 ` Maxime Coquelin
@ 2017-12-18 2:50 ` Tiwei Bie
2017-12-19 10:50 ` Olivier MATZ
1 sibling, 1 reply; 6+ messages in thread
From: Tiwei Bie @ 2017-12-18 2:50 UTC (permalink / raw)
To: Olivier Matz; +Cc: dev, Yuanhan Liu, Maxime Coquelin, Samuel Gauthier, stable
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
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [dpdk-stable] [dpdk-dev] [PATCH] net/virtio: fix Rx and Tx handler selection for arm32
2017-12-18 2:50 ` [dpdk-stable] [dpdk-dev] " Tiwei Bie
@ 2017-12-19 10:50 ` Olivier MATZ
2017-12-20 1:31 ` Tiwei Bie
0 siblings, 1 reply; 6+ messages in thread
From: Olivier MATZ @ 2017-12-19 10:50 UTC (permalink / raw)
To: Tiwei Bie; +Cc: dev, Yuanhan Liu, Maxime Coquelin, Samuel Gauthier, stable
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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [dpdk-stable] [dpdk-dev] [PATCH] net/virtio: fix Rx and Tx handler selection for arm32
2017-12-19 10:50 ` Olivier MATZ
@ 2017-12-20 1:31 ` Tiwei Bie
0 siblings, 0 replies; 6+ messages in thread
From: Tiwei Bie @ 2017-12-20 1:31 UTC (permalink / raw)
To: Olivier MATZ; +Cc: dev, Yuanhan Liu, Maxime Coquelin, Samuel Gauthier, stable
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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [dpdk-stable] [PATCH] net/virtio: fix Rx and Tx handler selection for arm32
2017-12-14 14:45 ` Maxime Coquelin
@ 2018-01-26 15:28 ` Yuanhan Liu
0 siblings, 0 replies; 6+ messages in thread
From: Yuanhan Liu @ 2018-01-26 15:28 UTC (permalink / raw)
To: Maxime Coquelin; +Cc: Olivier Matz, dev, Samuel Gauthier, stable
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
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2018-01-26 15:28 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-12-14 14:32 [dpdk-stable] [PATCH] net/virtio: fix Rx and Tx handler selection for arm32 Olivier Matz
2017-12-14 14:45 ` Maxime Coquelin
2018-01-26 15:28 ` Yuanhan Liu
2017-12-18 2:50 ` [dpdk-stable] [dpdk-dev] " Tiwei Bie
2017-12-19 10:50 ` Olivier MATZ
2017-12-20 1:31 ` Tiwei Bie
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).