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 2AF07A00C2; Wed, 22 Apr 2020 14:45:11 +0200 (CEST) Received: from [92.243.14.124] (localhost [127.0.0.1]) by dpdk.org (Postfix) with ESMTP id C099A1D61E; Wed, 22 Apr 2020 14:45:09 +0200 (CEST) Received: from mga18.intel.com (mga18.intel.com [134.134.136.126]) by dpdk.org (Postfix) with ESMTP id 320E11D619 for ; Wed, 22 Apr 2020 14:45:06 +0200 (CEST) IronPort-SDR: dbPV1if5016MqDgr/Fd71+aBhhzmqAecyTdf9JQghC3ciNPn3kI4CSYKAaKIUVou8gKCtwaQRF wkXf05pKY1PQ== X-Amp-Result: SKIPPED(no attachment in message) X-Amp-File-Uploaded: False Received: from orsmga004.jf.intel.com ([10.7.209.38]) by orsmga106.jf.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 22 Apr 2020 05:45:06 -0700 IronPort-SDR: plWUB/5h2ZMp6i0DgzkSTVU+qbr2kr5ov8l3eqMHn7YJdrZbZIqZ+iVQ7gqfBH1lCMo9EXQwC/ BPb3ZvtNfv0Q== X-IronPort-AV: E=Sophos;i="5.72,414,1580803200"; d="scan'208";a="402534772" Received: from bricha3-mobl.ger.corp.intel.com ([10.214.196.230]) by orsmga004-auth.jf.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-SHA; 22 Apr 2020 05:44:54 -0700 Date: Wed, 22 Apr 2020 13:44:49 +0100 From: Bruce Richardson To: Lukasz Wojciechowski Cc: Thomas Monjalon , "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: <20200422124449.GE1402@bricha3-MOBL.ger.corp.intel.com> References: <20200417215739.23180-1-l.wojciechow@partner.samsung.com> <237f3d8b-4040-15a2-a6c5-979aae9df8f2@partner.samsung.com> <20200422105519.GB1402@bricha3-MOBL.ger.corp.intel.com> <26026576.gRfpFWEtPU@thomas> <1ae782a9-10cc-8cdd-8815-4e0e7d36e466@partner.samsung.com> MIME-Version: 1.0 Content-Type: text/plain; charset=iso-8859-1 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: <1ae782a9-10cc-8cdd-8815-4e0e7d36e466@partner.samsung.com> 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 Wed, Apr 22, 2020 at 01:52:21PM +0200, Lukasz Wojciechowski wrote: > > W dniu 22.04.2020 o 13:02, Thomas Monjalon pisze: > > 22/04/2020 12:55, Bruce Richardson: > >> On Wed, Apr 22, 2020 at 12:41:49PM +0200, Lukasz Wojciechowski wrote: > >>> W dniu 21.04.2020 o 23:38, Thomas Monjalon pisze: > >>>> 21/04/2020 22:58, Lukasz Wojciechowski: > >>>>> W dniu 21.04.2020 o 02:32, Ananyev, Konstantin pisze: > >>>>>>>>>>>>>> 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. > >>>>>> For many libs these flags mean much more than just logging. > >>>>>> Let say RTE_LIBRTE_ETHDEV_DEBUG changes behaviour of tx_prepare() for many > >>>>>> drivers - extra validation performed. > >>>>>> RTE_LIBRTE_MBUF_DEBUG makes __rte_mbuf_sanity_check() a call to real > >>>>>> rte_mbuf_sanity_check() instead of just NOP. > >>>>>> Which means performance would be greatly affected. > >>>>>> RTE_LIBRTE_MEMPOOL_DEBUG changes format of the mempool object header > >>>>>> and enforce extra checking, stats collection. > >>>>>> etc. > >>>>>> Probably that's ok for some cases to enable all that extra validation we have at once. > >>>>>> But I suppose in many cases people just interested to enable debug on one > >>>>>> (ok might be two/three) particular libraries, not the whole system. > >>>>>> Right now there is such ability, we are going to remove it without > >>>>>> providing adequate replacement. > >>>>>> Approach with rte_log_can_log() seems workable, > >>>>>> but right now these patches don't implement it. > >>>>>> Konstantin > >>>>>> > >>>>>>>>>> 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 > >>>>> OK, lets's summarize current opinions and requirements to make a > >>>>> proposal for version2 of these patches, that I can implement if all agree: > >>>>> > >>>>> 1) The global debug flag is required to enable all the sanity checks and > >>>>> validations that are normally not used due to performance reasons > >>>> Yes > >>>> > >>>>> 2) The best option would be to have a single flag - not to introduce too > >>>>> many build options > >>>> Yes > >>>> > >>>>> 3) This option should be separated from meson "debug" option (used for > >>>>> build with symbols) and can be called "rte_debug" > >>>> Yes, it looks to be the consensus. > >>>> > >>>>> 4) The currently existing DEBUG macros should not be replaced with a > >>>>> RTE_DEBUG macro. This would allow to still enable them using > >>>>> CFLAGS="-D..." to test a single module (library, driver). > >>>>> > >>>>> 5) Currently existing options' names should be standardized to > >>>>> RTE_DEBUG_{library/driver name}, so they can be automatically enabled > >>>>> when rte_debug is set. Standardized names would allow easy usage in > >>>>> other modules. > >>>> I don't understand difference between 4) et 5). > >>> In current version of patches, I replaced all the DEBUG macros with > >>> RTE_DEBUG. It would be much better to keep fine-grained debugs as they > >>> are defined currently in dpdk. This is what I have on mind in 4) > >>> > >>> However the currently used debug macros have different naming > >>> conventions: some use RTE_LIBRTE_{name}_DEBUG convention, other > >>> RTE_{name}_DEBUG, some just {name}_DEBUG. > >>> So in 5) I follow Bruce's proposal to standardize them to one form > >>> RTE_DEBUG_{name}. However this will change the existing macros and > >>> someone might not like it, so I ask for the opinion about it. > >>> > >> My thought is to standardize in the build and then leave it to module > >> owners to either change their macros to use those standard ones as time > >> goes on. > > In order to maintain a good global user experience, > > we need to drive such change with a roadmap and deadlines. > What is the process of documenting new wanted feature and adding it the > roadmap? > > > >>>>> Should they? Or should we leave the current debug macros? Please share > >>>>> your opinions as I see both cons and pros of this idea. > >>>> I am not sure we need to keep fine-grain debug flags per libs/drivers. > >>>> In case RTE_DEBUG is enabled, which kind of debug processing > >>>> (except logs) do we want to be able to disable? > >>>> Is it possible to decide based on a call to rte_log_can_log()? > >>> I think it's rather opposite way round. Sometimes we would like to > >>> enable just some debug processing, e.g. when working on single lib or > >>> driver. > >>> If we will use rte_debug - every debug processing would be enabled, we > >>> won't disable anything, but without rte_debug we will still have the > >>> possibility of enabling debugs on a single module. > >>> > >>> I believe it is possible to do it with rte_log_can_log, but changing > >>> build time enabled options into runtime enabled options might affect > >>> performance. It might make working on a single library or driver harder. > >>> > >> I think the idea is to use both. When RTE_DEBUG is enabled, then the > >> rte_log_can_log() calls will be used to control the actual output. Without > >> RTE_DEBUG, the whole block is skipped. > >> > >> #ifdef RTE_DEBUG > >> if rte_log_can_log(...) { > >> /* do debug stuff */ > >> } > >> #endif > I thought that we are closer to agree to remain old macros, like: > > #ifdef RET_DEBUG_SOMELIBRARY >         if rte_log_can_log(...) { > > with enabling RET_DEBUG_SOMELIBRARY in general library meson.build when > rte_debug option is set. > Yes, looks better to me, since it does allow use of CFLAGS if you *really* want to limit the debug to just one module.