[dpdk-dev] [PATCH v4 07/12] librte_ether: extend flow director struct
Hi, Thomas
> -Original Message-
> From: Thomas Monjalon [mailto:thomas.monjalon at 6wind.com]
> Sent: Friday, March 18, 2016 7:44 PM
> To: Wu, Jingjing
> Cc: dev at dpdk.org; Richardson, Bruce
> Subject: Re: [dpdk-dev] [PATCH v4 07/12] librte_ether: extend flow director
> struct
>
> Hi Jingjing,
>
> 2016-03-10 11:25, Jingjing Wu:
> > This patch changed rte_eth_fdir_flow from union to struct to
> > support more packets formats, for example, Vxlan and GRE tunnel
> > packets with IP inner frame.
>
> I think we need a lot more explanations about how it should work.
> From this point we should collect some acknowledgements from the
> maintainers of other drivers having this kind of flow steering need.
> Maybe that a better API, more generic, is possible.
>
> > This patch also add new RTE_FDIR_TUNNEL_TYPE_GRE enum.
>
> OK to add GRE to the existing API.
>
> > Signed-off-by: Jingjing Wu
> > Acked-by: Helin Zhang
> [...]
> > /**
> > - * An union contains the inputs for all types of flow
> > + * A struct contains the inputs for all types of flow
> > */
> > -union rte_eth_fdir_flow {
> > - struct rte_eth_l2_flow l2_flow;
> > - struct rte_eth_udpv4_flow udp4_flow;
> > - struct rte_eth_tcpv4_flow tcp4_flow;
> > - struct rte_eth_sctpv4_flow sctp4_flow;
> > - struct rte_eth_ipv4_flow ip4_flow;
> > - struct rte_eth_udpv6_flow udp6_flow;
> > - struct rte_eth_tcpv6_flow tcp6_flow;
> > - struct rte_eth_sctpv6_flow sctp6_flow;
> > - struct rte_eth_ipv6_flow ipv6_flow;
> > +struct rte_eth_fdir_flow {
> > + union {
> > + struct rte_eth_l2_flow l2_flow;
> > + struct rte_eth_udpv4_flow udp4_flow;
> > + struct rte_eth_tcpv4_flow tcp4_flow;
> > + struct rte_eth_sctpv4_flow sctp4_flow;
> > + struct rte_eth_ipv4_flow ip4_flow;
> > + struct rte_eth_udpv6_flow udp6_flow;
> > + struct rte_eth_tcpv6_flow tcp6_flow;
> > + struct rte_eth_sctpv6_flow sctp6_flow;
> > + struct rte_eth_ipv6_flow ipv6_flow;
> > + };
> > struct rte_eth_mac_vlan_flow mac_vlan_flow;
> > struct rte_eth_tunnel_flow tunnel_flow;
> > };
>
> Please explain somewhere how to use this API change in order to have more
> discussions with other maintainers.
>
> I'm sorry to comment this change only now. I took time to realize that
> we need more consensus about the filtering API to make it usable by
> more drivers.
>
> For the 16.04 release, I suggest to remove this change from the series.
> Thanks for your understanding.
OK. Understand that we need to comments more on the change. And as you know,
the whole patch set actually contains two changes on filter API: One the change
is in this patch, which adds tunnel filtering using flow director, another one
is the patch "[PATCH v4,05/12] i40e: extend flow director to filter by IP "
(http://www.dpdk.org/dev/patchwork/patch/11358/ ). If you are OK to the latter
one, I will send another version which just contains the change and drops
tunnel supporting in 16.04 release.
And let's discuss more on the filtering API in future.
Thanks
Jingjing
[dpdk-dev] [PATCH v4 07/12] librte_ether: extend flow director struct
> -Original Message-
> From: Wu, Jingjing
> Sent: Sunday, March 20, 2016 4:57 PM
> To: 'Thomas Monjalon'
> Cc: dev at dpdk.org; Richardson, Bruce
> Subject: RE: [dpdk-dev] [PATCH v4 07/12] librte_ether: extend flow director
> struct
>
> Hi, Thomas
>
>
> > -Original Message-
> > From: Thomas Monjalon [mailto:thomas.monjalon at 6wind.com]
> > Sent: Friday, March 18, 2016 7:44 PM
> > To: Wu, Jingjing
> > Cc: dev at dpdk.org; Richardson, Bruce
> > Subject: Re: [dpdk-dev] [PATCH v4 07/12] librte_ether: extend flow director
> > struct
> >
> > Hi Jingjing,
> >
> > 2016-03-10 11:25, Jingjing Wu:
> > > This patch changed rte_eth_fdir_flow from union to struct to
> > > support more packets formats, for example, Vxlan and GRE tunnel
> > > packets with IP inner frame.
> >
> > I think we need a lot more explanations about how it should work.
> > From this point we should collect some acknowledgements from the
> > maintainers of other drivers having this kind of flow steering need.
> > Maybe that a better API, more generic, is possible.
> >
> > > This patch also add new RTE_FDIR_TUNNEL_TYPE_GRE enum.
> >
> > OK to add GRE to the existing API.
> >
> > > Signed-off-by: Jingjing Wu
> > > Acked-by: Helin Zhang
> > [...]
> > > /**
> > > - * An union contains the inputs for all types of flow
> > > + * A struct contains the inputs for all types of flow
> > > */
> > > -union rte_eth_fdir_flow {
> > > - struct rte_eth_l2_flow l2_flow;
> > > - struct rte_eth_udpv4_flow udp4_flow;
> > > - struct rte_eth_tcpv4_flow tcp4_flow;
> > > - struct rte_eth_sctpv4_flow sctp4_flow;
> > > - struct rte_eth_ipv4_flow ip4_flow;
> > > - struct rte_eth_udpv6_flow udp6_flow;
> > > - struct rte_eth_tcpv6_flow tcp6_flow;
> > > - struct rte_eth_sctpv6_flow sctp6_flow;
> > > - struct rte_eth_ipv6_flow ipv6_flow;
> > > +struct rte_eth_fdir_flow {
> > > + union {
> > > + struct rte_eth_l2_flow l2_flow;
> > > + struct rte_eth_udpv4_flow udp4_flow;
> > > + struct rte_eth_tcpv4_flow tcp4_flow;
> > > + struct rte_eth_sctpv4_flow sctp4_flow;
> > > + struct rte_eth_ipv4_flow ip4_flow;
> > > + struct rte_eth_udpv6_flow udp6_flow;
> > > + struct rte_eth_tcpv6_flow tcp6_flow;
> > > + struct rte_eth_sctpv6_flow sctp6_flow;
> > > + struct rte_eth_ipv6_flow ipv6_flow;
> > > + };
> > > struct rte_eth_mac_vlan_flow mac_vlan_flow;
> > > struct rte_eth_tunnel_flow tunnel_flow;
> > > };
> >
> > Please explain somewhere how to use this API change in order to have more
> > discussions with other maintainers.
> >
> > I'm sorry to comment this change only now. I took time to realize that
> > we need more consensus about the filtering API to make it usable by
> > more drivers.
> >
>
> > For the 16.04 release, I suggest to remove this change from the series.
> > Thanks for your understanding.
> OK. Understand that we need to comments more on the change. And as you know,
> the
> whole patch set actually contains two changes on filter API: One the change
> is in this patch,
> which adds tunnel filtering using flow director, another one is the patch
> "[PATCH v4,05/12]
> i40e: extend flow director to filter by IP "
> (http://www.dpdk.org/dev/patchwork/patch/11358/ ). If you are OK to the
> latter one, I will
> send another version which just contains the change and drops tunnel
> supporting in 16.04
> release.
Correct the mistake:
The other change is "[v4,01/12] ethdev: extend flow director for input
selection" http://www.dpdk.org/dev/patchwork/patch/11354/.
> And let's discuss more on the filtering API in future.
>
> Thanks
> Jingjing
[dpdk-dev] [PATCH v3 4/4] eal/arm: introduce CONFIG_RTE_ARCH_ARM_NEON_MEMCPY
On Sat, 19 Mar 2016 21:14:57 +0100 Thomas Monjalon wrote: > 2016-03-19 20:58, Jan Viktorin: > > The flag is used to enable memcpy optimizations in EAL. As it is not always > > the performance benefit, the flag allows to disable it. > > Ideally the default should be to choose the best optimization. > If it is not possible, it would help to have some comments explaining > how to choose wether enabling NEON memcpy or not. Ok, we can rename the option to CONFIG_RTE_ARCH_ARM_AVOID_NEON_MEMCPY, delete it from the defconfig and change the test in rte_memcpy_32.h to #ifndef CONFIG_RTE_ARCH_ARM_AVOID_NEON_MEMCPY Alternatively, to have a positive test like #ifdef CONFIG_RTE_ARCH_ARM_AVOID_NEON_MEMCPY I can create a bigger change that moves the non-neon-memcpy up in the file... Should I resend the whole series as v3? Regards Jan
[dpdk-dev] [PATCH v3 4/4] eal/arm: introduce CONFIG_RTE_ARCH_ARM_NEON_MEMCPY
On Sun, 20 Mar 2016 10:41:10 +0100 Jan Viktorin wrote: > On Sat, 19 Mar 2016 21:14:57 +0100 > Thomas Monjalon wrote: > > > 2016-03-19 20:58, Jan Viktorin: > > > The flag is used to enable memcpy optimizations in EAL. As it is not > > > always > > > the performance benefit, the flag allows to disable it. > > > > Ideally the default should be to choose the best optimization. > > If it is not possible, it would help to have some comments explaining > > how to choose wether enabling NEON memcpy or not. The related statistics are mentioned here: commit 04a2fde35daf5e9a271e72331a70b48b951d7568 Author: Vlastimil Kosar Date: Tue Nov 3 00:47:20 2015 +0100 eal/arm: add vector memcpy for ARMv7 It's quite difficult to easily summarize it, especially for so many CPUs... > > Ok, we can rename the option to CONFIG_RTE_ARCH_ARM_AVOID_NEON_MEMCPY, > delete it from the defconfig and change the test in rte_memcpy_32.h to > > #ifndef CONFIG_RTE_ARCH_ARM_AVOID_NEON_MEMCPY > > Alternatively, to have a positive test like > > #ifdef CONFIG_RTE_ARCH_ARM_AVOID_NEON_MEMCPY > > I can create a bigger change that moves the non-neon-memcpy up in the > file... > > Should I resend the whole series as v3? > > Regards > Jan -- Jan ViktorinE-mail: Viktorin at RehiveTech.com System ArchitectWeb:www.RehiveTech.com RehiveTech Brno, Czech Republic
[dpdk-dev] [PATCH v3 4/4] eal/arm: introduce CONFIG_RTE_ARCH_ARM_NEON_MEMCPY
2016-03-20 10:41, Jan Viktorin: > On Sat, 19 Mar 2016 21:14:57 +0100 > Thomas Monjalon wrote: > > > 2016-03-19 20:58, Jan Viktorin: > > > The flag is used to enable memcpy optimizations in EAL. As it is not > > > always > > > the performance benefit, the flag allows to disable it. > > > > Ideally the default should be to choose the best optimization. > > If it is not possible, it would help to have some comments explaining > > how to choose wether enabling NEON memcpy or not. > > Ok, we can rename the option to CONFIG_RTE_ARCH_ARM_AVOID_NEON_MEMCPY, > delete it from the defconfig and change the test in rte_memcpy_32.h to > > #ifndef CONFIG_RTE_ARCH_ARM_AVOID_NEON_MEMCPY > > Alternatively, to have a positive test like > > #ifdef CONFIG_RTE_ARCH_ARM_AVOID_NEON_MEMCPY > > I can create a bigger change that moves the non-neon-memcpy up in the > file... > > Should I resend the whole series as v3? No, I don't think changing the name of the config or moving code will change anything. We just need to understand when it must be enabled or disabled.
[dpdk-dev] [PATCH v3 4/4] eal/arm: introduce CONFIG_RTE_ARCH_ARM_NEON_MEMCPY
2016-03-20 10:46, Jan Viktorin: > On Sun, 20 Mar 2016 10:41:10 +0100 > Jan Viktorin wrote: > > > On Sat, 19 Mar 2016 21:14:57 +0100 > > Thomas Monjalon wrote: > > > > > 2016-03-19 20:58, Jan Viktorin: > > > > The flag is used to enable memcpy optimizations in EAL. As it is not > > > > always > > > > the performance benefit, the flag allows to disable it. > > > > > > Ideally the default should be to choose the best optimization. > > > If it is not possible, it would help to have some comments explaining > > > how to choose wether enabling NEON memcpy or not. > > The related statistics are mentioned here: > > commit 04a2fde35daf5e9a271e72331a70b48b951d7568 > Author: Vlastimil Kosar > Date: Tue Nov 3 00:47:20 2015 +0100 > > eal/arm: add vector memcpy for ARMv7 > > It's quite difficult to easily summarize it, especially for so many > CPUs... If it is difficult for you, it will be impossible for the users of this config option. When someone will ask what is the best value for his CPU, what will you answer? At least, we can add a comment explaining that the performance is not always better, depending of the buffer size and the CPU.
[dpdk-dev] [PATCH v4 07/12] librte_ether: extend flow director struct
2016-03-20 09:02, Wu, Jingjing:
> From: Wu, Jingjing
> > From: Thomas Monjalon [mailto:thomas.monjalon at 6wind.com]
> > > 2016-03-10 11:25, Jingjing Wu:
> > > > This patch changed rte_eth_fdir_flow from union to struct to
> > > > support more packets formats, for example, Vxlan and GRE tunnel
> > > > packets with IP inner frame.
> > >
> > > I think we need a lot more explanations about how it should work.
> > > From this point we should collect some acknowledgements from the
> > > maintainers of other drivers having this kind of flow steering need.
> > > Maybe that a better API, more generic, is possible.
> > >
> > > > This patch also add new RTE_FDIR_TUNNEL_TYPE_GRE enum.
> > >
> > > OK to add GRE to the existing API.
> > >
> > > > Signed-off-by: Jingjing Wu
> > > > Acked-by: Helin Zhang
> > > [...]
> > > > /**
> > > > - * An union contains the inputs for all types of flow
> > > > + * A struct contains the inputs for all types of flow
> > > > */
> > > > -union rte_eth_fdir_flow {
> > > > - struct rte_eth_l2_flow l2_flow;
> > > > - struct rte_eth_udpv4_flow udp4_flow;
> > > > - struct rte_eth_tcpv4_flow tcp4_flow;
> > > > - struct rte_eth_sctpv4_flow sctp4_flow;
> > > > - struct rte_eth_ipv4_flow ip4_flow;
> > > > - struct rte_eth_udpv6_flow udp6_flow;
> > > > - struct rte_eth_tcpv6_flow tcp6_flow;
> > > > - struct rte_eth_sctpv6_flow sctp6_flow;
> > > > - struct rte_eth_ipv6_flow ipv6_flow;
> > > > +struct rte_eth_fdir_flow {
> > > > + union {
> > > > + struct rte_eth_l2_flow l2_flow;
> > > > + struct rte_eth_udpv4_flow udp4_flow;
> > > > + struct rte_eth_tcpv4_flow tcp4_flow;
> > > > + struct rte_eth_sctpv4_flow sctp4_flow;
> > > > + struct rte_eth_ipv4_flow ip4_flow;
> > > > + struct rte_eth_udpv6_flow udp6_flow;
> > > > + struct rte_eth_tcpv6_flow tcp6_flow;
> > > > + struct rte_eth_sctpv6_flow sctp6_flow;
> > > > + struct rte_eth_ipv6_flow ipv6_flow;
> > > > + };
> > > > struct rte_eth_mac_vlan_flow mac_vlan_flow;
> > > > struct rte_eth_tunnel_flow tunnel_flow;
> > > > };
> > >
> > > Please explain somewhere how to use this API change in order to have more
> > > discussions with other maintainers.
> > >
> > > I'm sorry to comment this change only now. I took time to realize that
> > > we need more consensus about the filtering API to make it usable by
> > > more drivers.
> > >
> >
> > > For the 16.04 release, I suggest to remove this change from the series.
> > > Thanks for your understanding.
> > OK. Understand that we need to comments more on the change. And as you
> > know, the
> > whole patch set actually contains two changes on filter API: One the change
> > is in this patch,
> > which adds tunnel filtering using flow director, another one is the patch
> > "[PATCH v4,05/12]
> > i40e: extend flow director to filter by IP "
> > (http://www.dpdk.org/dev/patchwork/patch/11358/ ). If you are OK to the
> > latter one, I will
> > send another version which just contains the change and drops tunnel
> > supporting in 16.04
> > release.
> Correct the mistake:
> The other change is "[v4,01/12] ethdev: extend flow director for input
> selection" http://www.dpdk.org/dev/patchwork/patch/11354/.
I'm OK with patch 01 and "RTE_FDIR_TUNNEL_TYPE_GRE" add in patch 07.
> > And let's discuss more on the filtering API in future.
Thanks
[dpdk-dev] DPDK and HW offloads
> -Original Message- > From: Thomas Monjalon [mailto:thomas.monjalon at 6wind.com] > Sent: Saturday, March 19, 2016 2:01 AM > To: Stephen Hemminger > Cc: Zhang, Helin ; dev at dpdk.org > Subject: Re: DPDK and HW offloads > > 2016-03-18 10:16, Stephen Hemminger: > > As I look at how the ethernet device interface in DPDK has exploded in > > complexity; > > Yes I would like to start addressing this complexity in 16.07. > > > it makes life very hard for end users. The goal has been to enable > > all the cool hardware features, but it has put blinders on the driver > > devlopers; they are ignoring the fact that real applications can't just > > work on > one kind of hardware. > > +1 Agree that DPDK needs to add common APIs from application's point of view. > > > The DPDK is doing a terrible job at providing abstractions. There > > needs to be a real generic set of operations, and every hardware offload > feature must: > > * have a clear well defined API > > +1 > > > * if feature is not available in software, then the DPDK must provide > > a software equivalent feature. > > I'm not against this idea. Looking for more opinions. > > > * any difference in API must be hidden from application. > > * no compile config options about offload. > > * tests and documentation must work for both hw and sw version > > > > Right now, all those offload features are pretty much unusable in a > > real product without lots and lots of extra codes and huge bug > > surface. It bothers me enough that I would recommend removing much of the > filter/offload/ptype stuff from DPDK! > > One of the biggest challenge is to think about a good filtering API. > The offloading has some interaction with the mbuf struct. > > I would like to suggest rewriting ethdev API by keeping it as is for some > time for > compatibility while creating a new one. What about the prefix dpdk_netdev_ to > progressively replace rte_eth_dev? I totally agree with to add new and generic APIs for user applications. But I don't think we need to remove all current APIs. Generic APIs may not support all advanced hardware features, while specific APIs can. Why not support all? One generic APIs for common users, and others APIs for advanced users.
[dpdk-dev] [PATCH v3 2/4] arm: detect NEON cpu feature by checking __ARM_NEON
On Sat, Mar 19, 2016 at 08:58:03PM +0100, Jan Viktorin wrote: > The __ARM_NEON declares that the arm_neon.h is available which is not true for > the __ARM_NEON_FP. The __ARM_NEON_FP is not provided by aarch64 gcc. It depends on specific aarch64 compiler builds. Some aarch64 gcc versions do provide __ARM_NEON_FP. [~] $ aarch64-thunderx-linux-gnu-gcc -dM -E - < /dev/null |grep "NEON\|FP" #define __ARM_FP 12 #define __ARM_NEON_FP 12 #define __FP_FAST_FMAF 1 #define __ARM_NEON 1 #define __FP_FAST_FMA 1 However, This patch is correct, we should use __ARM_NEON. For this series, Acked-by: Jerin Jacob > > $ arm-linux-gnueabi-gcc -dM -E - < /dev/null | grep "_FP\|_NEON" > #define __ARM_FP 12 > #define __ARM_NEON_FP 4 > #define __VFP_FP__ 1 > > $ arm-linux-gnueabi-gcc -mfpu=neon -dM -E - < /dev/null | grep "_FP\|_NEON" > #define __ARM_FP 12 > #define __ARM_NEON_FP 4 > #define __ARM_NEON__ 1 > #define __VFP_FP__ 1 > #define __ARM_NEON 1 > > $ aarch64-linux-gnu-gcc -dM -E - < /dev/null | grep "NEON\|FP" > #define __FP_FAST_FMAF 1 > #define __ARM_NEON 1 > #define __FP_FAST_FMA 1 > > Signed-off-by: Jan Viktorin > --- > mk/rte.cpuflags.mk | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/mk/rte.cpuflags.mk b/mk/rte.cpuflags.mk > index 19a3e7e..529bcef 100644 > --- a/mk/rte.cpuflags.mk > +++ b/mk/rte.cpuflags.mk > @@ -111,7 +111,7 @@ CPUFLAGS += VSX > endif > > # ARM flags > -ifneq ($(filter $(AUTO_CPUFLAGS),__ARM_NEON_FP),) > +ifneq ($(filter $(AUTO_CPUFLAGS),__ARM_NEON),) > CPUFLAGS += NEON > endif > > -- > 2.7.0 >
[dpdk-dev] [PATCH v3 4/4] eal/arm: introduce CONFIG_RTE_ARCH_ARM_NEON_MEMCPY
On Sun, Mar 20, 2016 at 11:29:48AM +0100, Thomas Monjalon wrote: > 2016-03-20 10:41, Jan Viktorin: > > On Sat, 19 Mar 2016 21:14:57 +0100 > > Thomas Monjalon wrote: > > > > > 2016-03-19 20:58, Jan Viktorin: > > > > The flag is used to enable memcpy optimizations in EAL. As it is not > > > > always > > > > the performance benefit, the flag allows to disable it. > > > > > > Ideally the default should be to choose the best optimization. > > > If it is not possible, it would help to have some comments explaining > > > how to choose wether enabling NEON memcpy or not. > > > > Ok, we can rename the option to CONFIG_RTE_ARCH_ARM_AVOID_NEON_MEMCPY, > > delete it from the defconfig and change the test in rte_memcpy_32.h to > > > > #ifndef CONFIG_RTE_ARCH_ARM_AVOID_NEON_MEMCPY > > > > Alternatively, to have a positive test like > > > > #ifdef CONFIG_RTE_ARCH_ARM_AVOID_NEON_MEMCPY > > > > I can create a bigger change that moves the non-neon-memcpy up in the > > file... > > > > Should I resend the whole series as v3? > > No, I don't think changing the name of the config or moving code > will change anything. > We just need to understand when it must be enabled or disabled. By default, NEON implementation should be enabled in default config file, if a given arm target/cpu has issue with NEON specific implementation at target/cpu config level it can be disabled. IMO, Its inline with Jan's Patch. The factors like NEON instruction execution cycles and pipelines supported etc highly depend on the ARM target vendor implementation. (ie arm specification does not mandate those fine-grained details) so let target/cpu configuration decides any expectation is required or not. Jerin
[dpdk-dev] DPDK and HW offloads
2016-03-20 14:17, Zhang, Helin: > From: Thomas Monjalon [mailto:thomas.monjalon at 6wind.com] > > 2016-03-18 10:16, Stephen Hemminger: > > > Right now, all those offload features are pretty much unusable in a > > > real product without lots and lots of extra codes and huge bug > > > surface. It bothers me enough that I would recommend removing much of the > > filter/offload/ptype stuff from DPDK! > > > > One of the biggest challenge is to think about a good filtering API. > > The offloading has some interaction with the mbuf struct. > > > > I would like to suggest rewriting ethdev API by keeping it as is for some > > time for > > compatibility while creating a new one. What about the prefix dpdk_netdev_ > > to > > progressively replace rte_eth_dev? > > I totally agree with to add new and generic APIs for user applications. But I > don't > think we need to remove all current APIs. Generic APIs may not support all > advanced > hardware features, while specific APIs can. Why not support all? One generic > APIs for > common users, and others APIs for advanced users. Yes we cannot access to every features of a device through generic API. Until now we were trying to add an ethdev API for every features even if it is used by only one driver. I think we should allow a direct access to the driver by the applications and work on generic API only for common features.
