From mboxrd@z Thu Jan 1 00:00:00 1970 From: Jianbo Liu Subject: Re: [PATCH v3 2/2] config: disable CONFIG_RTE_SCHED_VECTOR for arm Date: Mon, 30 Nov 2015 13:55:45 -0500 Message-ID: <20151130185544.GA6141@qq.com> References: <1448631268-10692-1-git-send-email-jerin.jacob@caviumnetworks.com> <1448631268-10692-3-git-send-email-jerin.jacob@caviumnetworks.com> <20151129234829.GA2913@qq.com> <20151130054749.GA11512@localhost.localdomain> <20151130170038.GA3472@qq.com> <20151130102228.GA16187@localhost.localdomain> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Cc: dev@dpdk.org To: Jerin Jacob Return-path: Received: from mail-wm0-f41.google.com (mail-wm0-f41.google.com [74.125.82.41]) by dpdk.org (Postfix) with ESMTP id 664715AA0 for ; Mon, 30 Nov 2015 11:55:50 +0100 (CET) Received: by wmec201 with SMTP id c201so149228333wme.0 for ; Mon, 30 Nov 2015 02:55:50 -0800 (PST) Content-Disposition: inline In-Reply-To: <20151130102228.GA16187@localhost.localdomain> List-Id: patches and discussions about DPDK List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org Sender: "dev" 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 > > > > > --- > > > > > 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