From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga18.intel.com (mga18.intel.com [134.134.136.126]) by dpdk.org (Postfix) with ESMTP id 0E138A3 for ; Fri, 1 Mar 2019 15:17:22 +0100 (CET) X-Amp-Result: UNSCANNABLE X-Amp-File-Uploaded: False Received: from orsmga006.jf.intel.com ([10.7.209.51]) by orsmga106.jf.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 01 Mar 2019 06:17:21 -0800 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.58,427,1544515200"; d="scan'208";a="120126820" Received: from bricha3-mobl.ger.corp.intel.com ([10.237.221.36]) by orsmga006.jf.intel.com with SMTP; 01 Mar 2019 06:17:19 -0800 Received: by (sSMTP sendmail emulation); Fri, 01 Mar 2019 14:17:17 +0000 Date: Fri, 1 Mar 2019 14:17:17 +0000 From: Bruce Richardson To: Thomas Monjalon Cc: Anand Rawat , dev@dpdk.org, pallavi.kadam@intel.com, jeffrey.b.shaw@intel.com, ranjit.menon@intel.com Message-ID: <20190301141717.GA325020@bricha3-MOBL.ger.corp.intel.com> References: <20190301071847.13376-1-anand.rawat@intel.com> <20190301071847.13376-2-anand.rawat@intel.com> <1633455.prhy9BIAj0@xps> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <1633455.prhy9BIAj0@xps> User-Agent: Mutt/1.11.2 (2019-01-07) Subject: Re: [dpdk-dev] [PATCH 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: Fri, 01 Mar 2019 14:17:23 -0000 On Fri, Mar 01, 2019 at 03:03:02PM +0100, Thomas Monjalon wrote: > 01/03/2019 08:18, Anand Rawat: > > Added initial stub source files for windows support and meson changes > > to build them. > > Thanks for sending some new patches based on meson. > > Let's start review with some simple considerations. > > > Signed-off-by: Anand Rawat Signed-off-by: > > Kadam, Pallavi > > Please avoid comma in names. I think the right name is Pallavi Kadam? > > > Reviewed-by: Jeffrey B Shaw Reviewed-by: > > Ranjit Menon > > Please make sure everybody involved here is in Cc of the emails. Git > should add them automatically. Please add me as Cc of next versions. I'm > interested in the start of the Windows port. Thanks > > [...] > > -# Copyright(c) 2017 Intel Corporation +# Copyright(c) 2019 Intel > > Corporation > > I'm not sure you need to update the dates. In case you need to, you > should probably keep the original date. > > > -# use pthreads -add_project_link_arguments('-pthread', language: 'c') > > -dpdk_extra_ldflags += '-pthread' +if host_machine.system() != > > 'windows' + # use pthreads + add_project_link_arguments('-pthread', > > language: 'c') + dpdk_extra_ldflags += '-pthread' > > Why pthreads is not used in Windows? > > > -# some libs depend on maths lib -add_project_link_arguments('-lm', > > language: 'c') -dpdk_extra_ldflags += '-lm' + # some libs depend > > on maths lib + add_project_link_arguments('-lm', language: 'c') + > > dpdk_extra_ldflags += '-lm' +endif > > Why libmath is not used? > > > # 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' > > The real fix should be to check which linker is selected by meson. > > > + 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 > > [...] > > --- a/lib/librte_eal/common/include/arch/x86/meson.build +++ > > b/lib/librte_eal/common/include/arch/x86/meson.build -install_headers( > > - 'rte_atomic_32.h', - 'rte_atomic_64.h', - 'rte_atomic.h', - > > 'rte_byteorder_32.h', - 'rte_byteorder_64.h', - 'rte_byteorder.h', > > - 'rte_cpuflags.h', - 'rte_cycles.h', - 'rte_io.h', - > > 'rte_memcpy.h', - 'rte_prefetch.h', - 'rte_pause.h', - > > 'rte_rtm.h', - 'rte_rwlock.h', - 'rte_spinlock.h', - > > 'rte_vect.h', - subdir: get_option('include_subdir_arch')) +if > > host_machine.system() != 'windows' + install_headers( + > > 'rte_atomic_32.h', + 'rte_atomic_64.h', + > > 'rte_atomic.h', + 'rte_byteorder_32.h', + > > 'rte_byteorder_64.h', + 'rte_byteorder.h', + > > 'rte_cpuflags.h', + 'rte_cycles.h', + 'rte_io.h', > > + 'rte_memcpy.h', + 'rte_prefetch.h', + > > 'rte_pause.h', + 'rte_rtm.h', + 'rte_rwlock.h', + > > 'rte_spinlock.h', + 'rte_vect.h', + subdir: > > get_option('include_subdir_arch') + ) +endif > > The headers should not be different for Windows. NACK for this part. > > [...] > > @@ -17,13 +17,19 @@ elif host_machine.system() == 'freebsd' > > dpdk_conf.set('RTE_EXEC_ENV_BSDAPP', 1) subdir('bsdapp/eal') > > > > +elif host_machine.system() == 'windows' + > > dpdk_conf.set('RTE_EXEC_ENV_WINDOWS', 1) > > For consistency, it should RTE_EXEC_ENV_WINAPP. > For this one you can partially blame me - early internal review suggested changing to winapp, but I suggested holding off on the change for community consensus. Personally, I would prefer a little inconsistency and keep it as windows for clarity. In future, I also think we should just rename linxuapp to linux, and bsdapp to freebsd! However, for now, if the majority prefer winapp, I'm ok with it. [Though I can't help reading it as winamp half the time! No llamas involved here though!] /Bruce