Re: [dpdk-dev] [PATCH v3 2/2] config: disable CONFIG_RTE_SCHED_VECTOR for arm

[Jianbo Liu <jianbo.liu@linaro.org>]

On Mon, Nov 30, 2015 at 03:52:31PM +0530, Jerin Jacob wrote:
> On Mon, Nov 30, 2015 at 12:03:21PM -0500, Jianbo Liu wrote:
> > On Mon, Nov 30, 2015 at 11:17:52AM +0530, Jerin Jacob wrote:
> > > On Sun, Nov 29, 2015 at 06:48:29PM -0500, Jianbo Liu wrote:
> > > > On Fri, Nov 27, 2015 at 07:04:28PM +0530, Jerin Jacob wrote:
> > > > > Commit 42ec27a0178a causes compiling error on arm, as RTE_SCHED_VECTOR
> > > > > does support only SSE intrinsic, so disable it till we have neon support.
> > > > > 
> > > > > Fixes: 42ec27a0178a ("sched: enable SSE optimizations in config")
> > > > > 
> > > > > Signed-off-by: Jerin Jacob <jerin.jacob@caviumnetworks.com>
> > > > > ---
> > > > >  config/common_arm64                      | 1 +
> > > > >  config/defconfig_arm-armv7a-linuxapp-gcc | 1 +
> > > > >  2 files changed, 2 insertions(+)
> > > > > 
> > > > > diff --git a/config/common_arm64 b/config/common_arm64
> > > > > index 5e5e303..d6a9cb9 100644
> > > > > --- a/config/common_arm64
> > > > > +++ b/config/common_arm64
> > > > > @@ -46,3 +46,4 @@ CONFIG_RTE_LIBRTE_I40E_PMD=n
> > > > >  CONFIG_RTE_LIBRTE_LPM=n
> > > > >  CONFIG_RTE_LIBRTE_TABLE=n
> > > > >  CONFIG_RTE_LIBRTE_PIPELINE=n
> > > > > +CONFIG_RTE_SCHED_VECTOR=n
> > > > > diff --git a/config/defconfig_arm-armv7a-linuxapp-gcc b/config/defconfig_arm-armv7a-linuxapp-gcc
> > > > > index 82143af..9924ff9 100644
> > > > > --- a/config/defconfig_arm-armv7a-linuxapp-gcc
> > > > > +++ b/config/defconfig_arm-armv7a-linuxapp-gcc
> > > > > @@ -57,6 +57,7 @@ CONFIG_RTE_LIBRTE_ACL=n
> > > > >  CONFIG_RTE_LIBRTE_LPM=n
> > > > >  CONFIG_RTE_LIBRTE_TABLE=n
> > > > >  CONFIG_RTE_LIBRTE_PIPELINE=n
> > > > > +CONFIG_RTE_SCHED_VECTOR=n
> > > > >  
> > > > >  # cannot use those on ARM
> > > > >  CONFIG_RTE_KNI_KMOD=n
> > > > > -- 
> > > > > 2.1.0
> > > > > 
> > > > 
> > > > Hi Jerin,
> > > 
> > > Hi Jianbo, Thanks for the review.
> > > Looking forward to seeing contributions to DPDK-ARM.
> > > We definitely need more hands to make best DPDK-ARM port.
> > > 
> > > > In this way, we still have to modify two files each time a new feature
> > > > is added but not verified on ARM architectures.
> > > > Since disabling those drivers and libs are common for both armv7 and
> > > > armv8, can you put them in one config file, for example: common_arm? 
> > > 
> > > I initially thought of making it a single common_arm file, Then
> > > later I realized that it may not be worth as,
> > > 
> > > 1) If a new feature added to DPDK which has the dependency on SSE then
> > > implementer has to disable on "n" platforms(tile, powerpc..).By unifying
> > > single arm config will make it "n-1" so it's like "n" vs "n-1" not "n"
> > > vs "2n"
> > 
> > I'm talking about your patch, which is for ARM platform only. And the
> > two files we need to modify are armv7 and armv8 configs. 
> > If you want to include other platforms, your patch is still incomplete :)
> > 
> 
> That was the reply for the concern you have raised for the new feature.
> Not specific to my patch. My patch is complete, as I have
> checked other platforms before sending the patch
> they have already disabled the sched library :-)
> 
> 
> > > 
> > > 2) AFAIK, PCI NIC PMD's are not yet supported in ARMv7 platform yet
> > > unlike ARMv8.
> > > Till we have PCI NIC PMD support, armv7 config needs to be updated
> > > for each and every new PMD inclusion.
> > > 
> > > 3) neon capabilities are bit different in ARMv7 and ARMv8.
> > > For instance, "vqtbl1q_u8" neon intrinsics is not defined in ARMv7 which used
> > > in implementing ACL-NEON. i.e Need additional efforts to extend
> > > the armv8 neon code to armv7(or vice versa).So it's better to
> > > have fine control on the config file to enable selective features
> > > 
> > 
> > The differences between ARMv7 and ARMv8 can't justify we only add new
> > config for ARMv8. And this file is trying to disable drivers and libs
> > which is not supported on ARM platforms for now.
> >
> 
> I thought difference and point 3 should justify the need for different config. No?
> 
>   
> > > 3) anyway we may need common_armv8 file to address the "IMPLEMENTATION
> > > DEFINED" parts of the armv8 specific in future, like frequency at cntvct_el0
> > > runs ? optional features like armv8 crypto instruction support or not?
> > > It's armv8 v1 or v2 ? atomic instruction support for not? its a long
> > > list
> > > 
> > 
> > I think these "IMPLEMENTATION DEFINED" features should be configured
> > in the different platform (machine) config files. Can this
> > common_arm64 solve your concern?
> 
> Yes, "IMPLEMENTATION DEFINED" features of armv8(default behavior in
> common_arm64). I think it makes sense not mix with armv7.
> 
> > 
> > > 4)I would like to see ARM configs as different config like i686, X86_64
> > > in DPDK
> > > 
> > 
> > Basically, we need to use the default common_linux/bsd to enable the
> > new-added features in DPDK.
> > 
> > > 
> > > > It is not like common_arm64, which is solely for armv8 platform.
> > > > Actually, the arm64 common config is defconfig_arm64-armv8a-linuxapp-gcc
> > > 
> > > I thought so, Then  I realized that we may have
> > > FreeBSD, arm compiler, clang, llvm support in future.
> > > 
> > > > you can include it in the thunderx or xgene1 config files respectively,
> > > > and overriding some special config if needed.
> > > 
> > > Agree. existing patch addresses this
> > > 
> > 
> > If there exists a defconfig_arm64-armv8a-linuxapp-gcc, why needs to
> > add a new file(common_arm64) in your patch?
> > The defconfig_arm64-armv8a-xxx-xxx can be treated as a config for a
> > common ARMv8 platform, and one which other specific ARMv8 platforms
> > can base on.
> >
> 
> I was thinking to have the common_arm64 file so that "FreeBSD, arm compiler,
> clang, llvm" future version can include it directly.
> But I agree with you. defconfig_arm64-armv8a-linuxapp-gcc can be treated as a
> config for a common file for defconfig_arm64-*-linuxapp-gcc(anyway its same,
> just the toolchain added in defconfig_arm64-*-linuxapp-gcc)
> I will send out new version with defconfig_arm64-armv8a-linuxapp-gcc
> as the base instead of common_arm64 file.
> 
> 
>  
> > > > 
> > > > On the other hand, If we support the features in the future by
> > > > replacing SSE intrinsic with NEON, we just need to remove the lines in one place.
> > > 
> > > See point 3 above,
> > > 
> > > I feel rather than coming with the framework to fix the exceptions it's
> > > better to fix the exceptions its self.
> > > I am planning to send out next patch by today for supporting
> > > CONFIG_RTE_LIBRTE_LPM,CONFIG_RTE_LIBRTE_TABLE,CONFIG_RTE_LIBRTE_PIPELINE.
> > > 
> > > i.e only a few entries will be common. Please find below the list,
> > > the reason for setting as "n" for armv7 and armv8 is different.
> > > lack of PCI PMD supports vs SIMD support.
> > > 
> > > CONFIG_RTE_IXGBE_INC_VECTOR=n
> > > CONFIG_RTE_LIBRTE_VIRTIO_PMD=n
> > > CONFIG_RTE_LIBRTE_IVSHMEM=n
> > > CONFIG_RTE_LIBRTE_FM10K_PMD=n
> > > CONFIG_RTE_LIBRTE_I40E_PMD=n
> > > 
> > 
> > Yes, but what if new SSE enhancement is added on x86, we need to add
> > it to the list.
> 
> CONFIG_RTE_IXGBE_INC_VECTOR=n
> CONFIG_RTE_LIBRTE_VIRTIO_PMD=n
> CONFIG_RTE_LIBRTE_IVSHMEM=n
> CONFIG_RTE_LIBRTE_FM10K_PMD=n
> CONFIG_RTE_LIBRTE_I40E_PMD=n
> 
> IMO, creating a config file with above as "common_arm" file
> doesn't make sense to me. It looks odd and we are not there because
> functional difference in current ARMv7 and ARMv8 port.
> Feel free to submit the RFC patch if you think it makes sense.
> 
> Let me know your plan. I can hold off my v4  for "updating
> defconfig_arm64-armv8a-linuxapp-gcc as base instead of common_arm64 file"
> 
I understand your concern about ARMv7 and ARMv8 differencs, We can talk
that later. For now, please continue your v4 patch.
Besides, I don't like "common_arm" either. If there must be, it should be
called backlists, and provided globally by DPDK for all non-x86 platform :)

> 
> > 
> > > - Jerin
> > > 
> > > > 
> > > > Regards,
> > > > Jianbo