From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from dpdk.org (dpdk.org [92.243.14.124]) by inbox.dpdk.org (Postfix) with ESMTP id 00968A0561; Mon, 20 Apr 2020 20:58:12 +0200 (CEST) Received: from [92.243.14.124] (localhost [127.0.0.1]) by dpdk.org (Postfix) with ESMTP id BFE251D58F; Mon, 20 Apr 2020 20:58:11 +0200 (CEST) Received: from mga04.intel.com (mga04.intel.com [192.55.52.120]) by dpdk.org (Postfix) with ESMTP id 4EF0A1D588 for ; Mon, 20 Apr 2020 20:58:09 +0200 (CEST) IronPort-SDR: 1z8+urFZKuh1UBbT/paEKFoZvzbauGtlLyn+tf2E39JXsCpj9AtLTGx73XhjjhMIdhzNSGFlLo Rs3yL83bWPtw== X-Amp-Result: SKIPPED(no attachment in message) X-Amp-File-Uploaded: False Received: from orsmga005.jf.intel.com ([10.7.209.41]) by fmsmga104.fm.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 20 Apr 2020 11:58:08 -0700 IronPort-SDR: zQN6jcn3YsRP+KfFfaKxIeqLqfeg8Q+UBVjB1JeW/PDFkrSDm58ueIZxf6EUePQb40KfY3fuS+ 2WtsHH9QNs1Q== X-IronPort-AV: E=Sophos;i="5.72,407,1580803200"; d="scan'208";a="429237236" Received: from bricha3-mobl.ger.corp.intel.com ([10.249.46.74]) by orsmga005-auth.jf.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-SHA; 20 Apr 2020 11:57:59 -0700 Date: Mon, 20 Apr 2020 19:57:56 +0100 From: Bruce Richardson To: Thomas Monjalon Cc: Lukasz Wojciechowski , "Ananyev, Konstantin" , "Dumitrescu, Cristian" , Igor Russkikh , Pavel Belous , "Lu, Wenzhuo" , Marcin Wojtas , Michal Krawczyk , Guy Tzalik , Evgeny Schemeilin , Igor Chauskin , John Daley , Hyong Youb Kim , "Zhang, Qi Z" , "Wang, Xiao W" , Ziyang Xuan , Xiaoyun Wang , Guoyang Zhou , "Wei Hu (Xavier)" , "Min Hu (Connor)" , Yisen Zhuang , "Xing, Beilei" , "Wu, Jingjing" , "Yang, Qiming" , Rasesh Mody , Shahed Shaikh , "Singh, Jasvinder" , Maxime Coquelin , "Wang, Zhihong" , "Ye, Xiaolong" , Yong Wang , "Yigit, Ferruh" , Andrew Rybchenko , Olivier Matz , "dev@dpdk.org" Message-ID: <20200420185756.GA1725@bricha3-MOBL.ger.corp.intel.com> References: <20200417215739.23180-1-l.wojciechow@partner.samsung.com> <7682483.vcMjziH8VY@thomas> <20200420173015.GC1719@bricha3-MOBL.ger.corp.intel.com> <3171532.h4nFI6E9mP@thomas> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <3171532.h4nFI6E9mP@thomas> Subject: Re: [dpdk-dev] [PATCH v1 03/17] ethdev: replace library debug flag with global one 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: , Errors-To: dev-bounces@dpdk.org Sender: "dev" On Mon, Apr 20, 2020 at 07:35:36PM +0200, Thomas Monjalon wrote: > 20/04/2020 19:30, Bruce Richardson: > > On Mon, Apr 20, 2020 at 07:21:32PM +0200, Thomas Monjalon wrote: > > > 20/04/2020 19:11, Bruce Richardson: > > > > On Mon, Apr 20, 2020 at 04:43:21PM +0200, Lukasz Wojciechowski wrote: > > > > > W dniu 20.04.2020 o 16:21, Bruce Richardson pisze: > > > > > > On Mon, Apr 20, 2020 at 01:37:12PM +0000, Ananyev, Konstantin wrote: > > > > > >>> > > > > > > > > > >> I am agree with Cristian concern here: that patch removes ability to > > > > > >> enable/disable debug on particular library/PMD. If the purpose is to > > > > > >> minimize number of config compile options, I wonder can't it be done > > > > > >> in a slightly different way: 1. introduce gloabal RTE_DEBUG 2. keep > > > > > >> actual .[c,h] files intact 3. In actual librte_xxx/meson.build file > > > > > >> check if RTE_DEBUG is enabled, If yes, then enable particular debug > > > > > >> flag for these libs. Something like: If dpdk_conf.get('RTE_DEBUG') == > > > > > >> true dpdk_conf.set('RTE_LIBRTE_XXX_DEBUG ', 1) > > > > > >> > > > > > >> defines that are used by multiple libs, probably can be set in upper > > > > > >> layer meson.build. > > > > > >> > > > > > >> That way will have global 'debug' flag, but users will still have an > > > > > >> ability to enable/disable debug flags on a per lib basis via > > > > > >> CFLAGS="-D..." > > > > > >> > > > > > >> Konstantin > > > > > >> > > > > > > That seems a reasonable idea to me. > > > > > > > > > > > > However, in this case, we don't need the RTE_DEBUG flag at all, we can > > > > > > either: > > > > > > > > > > > > * allow each component meson.build file define its own flags after > > > > > > checking get_option('debug') * have lib/meson.build and > > > > > > drivers/meson.build automatically define a specific define for each > > > > > > library or driver to standardize the naming. [This would save anyone > > > > > > working on it from having to lookup what the define was, since it's > > > > > > always e.g. RTE_DEBUG_ + library-base-name, e.g. RTE_DEBUG_LPM, > > > > > > RTE_DEBUG_SCHED etc] > > > > > > > > > > > > Theoretically we can also do both, have the standard ones defined and > > > > > > then allow a component to provide extra flags itself if so desired. > > > > > > > > > > > > /Bruce > > > > > OK, so let's summarize how the patches should be redo: * usage of global > > > > > "debug" flag for meson build stays * we standardize names of debug flags > > > > > in the components to RTE_DEBUG_ + components name * debug flag enables al > > > > > the RTE_DEBUG_... flags > > > > > > > > > > This allow to easily use both: * the debug flag - to enable all debugs * > > > > > or define manually RTE_DEBUG+component name, just for debug from a single > > > > > component > > > > > > > > > > I like Bruce's idea of adding it to the lib/meson.build and > > > > > drivers/meson.build. This way they will be added to dpdk_conf meson > > > > > object and written then later to rte_build_config.h before compilation > > > > > stage. All the other modules will be able to use these flags. > > > > > > > > > Sounds good to me (obviously!), but I'd like other feedback to ensure > > > > others are ok with this before you spend too much effort implementing it. > > > > > > > > For the drivers, the flag probably needs to include the category as well as > > > > the name, e.g. RTE_DEBUG_NET_IXGBE, RTE_DEBUG_RAW_IOAT, to avoid possible > > > > name confusion. Those flags can then be checked inside individual > > > > meson.build files to enable other debug flags if necessary e.g. in ixgbe, > > > > you could theoretically do: > > > > > > > > if dpdk_conf.has('RTE_DEBUG_NET_IXGBE') > > > > cflags += '-DRTE_LIBRTE_IXGBE_DEBUG_RX' > > > > cflags += '-DRTE_LIBRTE_IXGBE_DEBUG_TX' > > > > ... > > > > endif > > > > > > > > to enable more fine-grained control if so desired, and to maintain > > > > compatibility with existing defines, again if so desired. > > > > > > Nak the nak from Cristian. > > > > > > We don't need all these flags. > > > If the user choose to compile DPDK for debug, every debug facilities > > > should be enabled. Then at runtime it is possible to enable/disable > > > the interesting logs. > > > If you need to disable something which is not a log, > > > you can align with the log level thanks to the function rte_log_can_log. > > > > > > Please let's stop complicating things for the contributors and the users. > > > Note: I am strong on this position. > > > > > Note, this means that you need to ensure all debug printouts from libs and > > drivers are using the RTE_LOG macros so can be runtime controlled. I think > > that may be some distance from reality right now. > > Perfect! Let's expose those nasty logs which are not (yet) controllable. > And next step is to block any patch in those drivers or libs, > until it is fixed. Dynamic logging should have been complete for long. > I can live with that, I suppose. Do we have any idea of the magnitude of the work required here? > > Even if we do want all debug enabled from one flag, I'm still not 100% > > convinced that the existing debug flag is the way to do, since it only adds > > debug info to binary without affecting code generation. > > OK, we want to keep this flag for gdb symbols only? > And add a new flag for debugging facilities which hurt the runtime performance? > I think that would be wise, yes. We can call the option "rte_debug" or something instead. /Bruce