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 1C996A00C2; Wed, 22 Apr 2020 14:24:27 +0200 (CEST) Received: from [92.243.14.124] (localhost [127.0.0.1]) by dpdk.org (Postfix) with ESMTP id 8251A1D5FE; Wed, 22 Apr 2020 14:24:26 +0200 (CEST) Received: from new1-smtp.messagingengine.com (new1-smtp.messagingengine.com [66.111.4.221]) by dpdk.org (Postfix) with ESMTP id F34011D5FD for ; Wed, 22 Apr 2020 14:24:24 +0200 (CEST) Received: from compute7.internal (compute7.nyi.internal [10.202.2.47]) by mailnew.nyi.internal (Postfix) with ESMTP id 5E25E58052E; Wed, 22 Apr 2020 08:24:24 -0400 (EDT) Received: from mailfrontend1 ([10.202.2.162]) by compute7.internal (MEProxy); Wed, 22 Apr 2020 08:24:24 -0400 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=monjalon.net; h= from:to:cc:subject:date:message-id:in-reply-to:references :mime-version:content-transfer-encoding:content-type; s=fm1; bh= 8JtSS8qXc9yVWh5DuJwQQxRj//h5UGGAhEgAZ44ESIQ=; b=DUsGlgSlzpxv5XKX PivfrqTYVRvdKKhTa5mkj24Cs9jo1WWiFts7bBpKC1zt7PMM91+z3YDwKOL8tx8R 5NIpqoihWME7MPWVqFKgan0qJ5ReR+5qPqZ26YrBO7Nv7VFd6U9+ftRL4ZD/Rzc1 iIbo5DF2CKQJZWQSeHyhvAlueBezjnOwup781x83T3+6MTRaY8EXGS8thUXfWTjR HKKvbc41s7mbqGtowaOP+IOW/z9dIXkfAJVknIELdlOHESF6MY2uLDi+33qIrOlf kaptOreaytMQV05g8DEMdOfpbEY+LQu3uQAgxhIi8f6UJXidCeN1bIu9XyLv7QBt hUKTFg== DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d= messagingengine.com; h=cc:content-transfer-encoding:content-type :date:from:in-reply-to:message-id:mime-version:references :subject:to:x-me-proxy:x-me-proxy:x-me-sender:x-me-sender :x-sasl-enc; s=fm2; bh=8JtSS8qXc9yVWh5DuJwQQxRj//h5UGGAhEgAZ44ES IQ=; b=nWVrejT6sEnYr5Fot42jaPHnXoxekarMiD0o3xE3T1SBlRQoWKLePilwp n4peCmitBuSNWrN7E1SdSr0i9w1aAWNWLznFuyxElvoogfD1lrPXiV0NBCr9/ibk SVXj4C3bjte7Yo/Xj29ZNJFzAvwzlZ8kP+NRADTXtoeVyiUPFH9ozEAgvXS6IIAw yrvrgy31/CauUsvz0tMG1vBpjRfkM1HAdtBeZL3aDIAJ/EaUUrz7ZDhZUDLiW3NW QZ1U/R2XY767DQxhbI0Qn3jQejUbFNAcZYYA+A/bDQPqrnwNS9YlAWGkn6SNGjPr zgD+OL0zosWpXL/k8X8syhS3ZhzWg== X-ME-Sender: X-ME-Proxy-Cause: gggruggvucftvghtrhhoucdtuddrgeduhedrgeejgdegkecutefuodetggdotefrodftvf curfhrohhfihhlvgemucfhrghsthforghilhdpqfgfvfdpuffrtefokffrpgfnqfghnecu uegrihhlohhuthemuceftddtnecusecvtfgvtghiphhivghnthhsucdlqddutddtmdenuc fjughrpefhvffufffkjghfggfgtgesthfuredttddtvdenucfhrhhomhepvfhhohhmrghs ucfoohhnjhgrlhhonhcuoehthhhomhgrshesmhhonhhjrghlohhnrdhnvghtqeenucfkph epjeejrddufeegrddvtdefrddukeegnecuvehluhhsthgvrhfuihiivgeptdenucfrrghr rghmpehmrghilhhfrhhomhepthhhohhmrghssehmohhnjhgrlhhonhdrnhgvth X-ME-Proxy: Received: from xps.localnet (184.203.134.77.rev.sfr.net [77.134.203.184]) by mail.messagingengine.com (Postfix) with ESMTPA id 73B473280059; Wed, 22 Apr 2020 08:24:18 -0400 (EDT) From: Thomas Monjalon To: Lukasz Wojciechowski , "Richardson, Bruce" , "Ananyev, Konstantin" Cc: "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" Date: Wed, 22 Apr 2020 14:24:16 +0200 Message-ID: <2172874.ECZNHGQPT7@thomas> In-Reply-To: References: <20200417215739.23180-1-l.wojciechow@partner.samsung.com> <26026576.gRfpFWEtPU@thomas> MIME-Version: 1.0 Content-Transfer-Encoding: 7Bit Content-Type: text/plain; charset="us-ascii" 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" 22/04/2020 13:29, Ananyev, Konstantin: > > 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. > > > > > > >> 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 > > > > This is what I had in mind. > > The question is about performance impact in the case > > we enable RTE_DEBUG at compilation time, and don't enable a > > specific debug at runtime: is this check overhead acceptable? > > if rte_log_can_log(...) > > We probably wouldn't know the answer before trying. > Probably best way to make such changes for rte_mbuf and see how > big the drop will be. > I suppose mbuf will be the one mostly affected, > as we'll call rte_log_can_log() for nearly every mbuf op (free/detach/attach/reset, etc.) Yes I like the idea of trying with mbuf, and see how much a compilation flag for global debugging affects the mbuf performance. Note that we should also manage RTE_ENABLE_ASSERT with the global meson option.