From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga04.intel.com (mga04.intel.com [192.55.52.120]) by dpdk.org (Postfix) with ESMTP id 1E10E4C9D for ; Wed, 6 Mar 2019 12:20:14 +0100 (CET) X-Amp-Result: UNKNOWN X-Amp-Original-Verdict: FILE UNKNOWN X-Amp-File-Uploaded: False Received: from fmsmga001.fm.intel.com ([10.253.24.23]) by fmsmga104.fm.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 06 Mar 2019 03:20:14 -0800 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.58,447,1544515200"; d="scan'208";a="152518039" Received: from bricha3-mobl.ger.corp.intel.com ([10.237.221.36]) by fmsmga001.fm.intel.com with SMTP; 06 Mar 2019 03:20:10 -0800 Received: by (sSMTP sendmail emulation); Wed, 06 Mar 2019 11:20:09 +0000 Date: Wed, 6 Mar 2019 11:20:09 +0000 From: Bruce Richardson To: Thomas Monjalon Cc: Anand Rawat , dev@dpdk.org, pallavi.kadam@intel.com, ranjit.menon@intel.com, jeffrey.b.shaw@intel.com Message-ID: <20190306112006.GA134252@bricha3-MOBL.ger.corp.intel.com> References: <20190306041634.12976-1-anand.rawat@intel.com> <20190306041634.12976-2-anand.rawat@intel.com> <37871127.gZq9nUeyVu@xps> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <37871127.gZq9nUeyVu@xps> User-Agent: Mutt/1.11.2 (2019-01-07) Subject: Re: [dpdk-dev] [PATCH v2 1/6] eal: eal stub to add windows support X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Wed, 06 Mar 2019 11:20:15 -0000 On Wed, Mar 06, 2019 at 11:03:24AM +0100, Thomas Monjalon wrote: > Hi, > > 06/03/2019 05:16, Anand Rawat: > > -# some libs depend on maths lib > > -add_project_link_arguments('-lm', language: 'c') > > -dpdk_extra_ldflags += '-lm' > > +if cc.find_library('lm', required : false).found() > > + # some libs depend on maths lib > > + add_project_link_arguments('-lm', language: 'c') > > + dpdk_extra_ldflags += '-lm' > > +endif > > Either libmath is required or not. > I don't think it can be optional. > Why is it changed? > I think these come as part of libc, it's just on Linux they are not in the main libc library but need to be linked in separately. https://en.wikipedia.org/wiki/C_mathematical_functions#libm Therefore, this looks the best way of dealing with this. > > # get binutils version for the workaround of Bug 97 > > -ldver = run_command('ld', '-v').stdout().strip() > > -if ldver.contains('2.30') > > - if cc.has_argument('-mno-avx512f') > > - march_opt += '-mno-avx512f' > > - message('Binutils 2.30 detected, disabling AVX512 support as workaround for bug #97') > > +if host_machine.system() != 'windows' > > + ldver = run_command('ld', '-v').stdout().strip() > > + if ldver.contains('2.30') > > + if cc.has_argument('-mno-avx512f') > > + march_opt += '-mno-avx512f' > > + message('Binutils 2.30 detected, disabling AVX512 support as workaround for bug #97') > > + endif > > endif > > endif > > Same comment as in v1, you should check which linker is used. > This check is only for GNU ld 2.30. > I'm not aware of any better way to implement this in meson right now, sadly. Through the compiler object we can get information about linker flags, but not about the specific linker itself, since it is only called through the compiler. > > +if host_machine.system() != 'windows' > > + common_sources = files( > > The definitive solution should be to compile all common EAL files. > Please explain what are the issues in the common files. > I think we should not remove them and fix them one by one. > You could provide a separate patch to skip some files for > making helloworld working. > I believe that is exactly what this patch is trying to do - it's skipping the files unneeded to get helloworld working, and the intention is to fix them one by one and add them back in later. Perhaps this sort of change should be a separate (precursor) patch where the cover letter can call this out explicitly? > > -subdir(join_paths('arch', arch_subdir)) > > + > > +common_headers += files('include/rte_common.h') > > +if host_machine.system() != 'windows' > > + subdir(join_paths('arch', arch_subdir)) > > +endif > > Same comment as above, it should not be changed. > > > -common_headers = files( > > +common_headers += files( > > 'include/rte_alarm.h', > > 'include/rte_branch_prediction.h', > > 'include/rte_bus.h', > > 'include/rte_bitmap.h', > > 'include/rte_class.h', > > - 'include/rte_common.h', > > Why rte_common.h is moved first in the list? > > > -deps += 'kvargs' > > +if host_machine.system() != 'windows' > > + deps += 'kvargs' > > +endif > > Why kvargs is removed? > Again, I believe these actions are to disable the parts of DPDK that are not needed to enable helloworld, allowing later patches to come in and fix them. > > --- /dev/null > > +++ b/lib/librte_eal/windows/eal/eal_lcore.c > > + /* Get the cpu core id value */ > > +unsigned int > > +eal_cpu_core_id(unsigned int lcore_id) > > +{ > > + return lcore_id; > > +} > > For this function and the others in this file, > please add a comment explaining this is a stub, not the expected result. > You can add a TODO or FIXME tag. > > > --- a/lib/meson.build > > +++ b/lib/meson.build > > +if host_machine.system() == 'windows' > > + libraries = ['eal'] # override libraries for windows > > +endif > > Instead of simply "override", you could explain that the other libraries > are not yet supported on Windows. > > > --- a/meson.build > > +++ b/meson.build > > -# build libs and drivers > > +# build libs > > subdir('lib') > > -subdir('buildtools') > > -subdir('drivers') > > > > -# build binaries and installable tools > > -subdir('usertools') > > -subdir('app') > > +if host_machine.system() != 'windows' > > + # build buildtools and drivers > > + subdir('buildtools') > > + subdir('drivers') > > > > -# build docs > > -subdir('doc') > > + # build binaries and installable tools > > + subdir('usertools') > > + subdir('app') > > + subdir('test') > > + > > + # build kernel modules if enabled > > + if get_option('enable_kmods') > > + subdir('kernel') > > + endif > > + > > + # build docs > > + subdir('doc') > > +endif > > I don't like modifying this file. > Can we skip not supported directories inside the sub meson files? > Since we now mandate meson 0.47 we can use the "subdir_done()" function to this. /Bruce