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 A0F78A05A0; Tue, 21 Apr 2020 23:38:57 +0200 (CEST) Received: from [92.243.14.124] (localhost [127.0.0.1]) by dpdk.org (Postfix) with ESMTP id A2D0F1D16B; Tue, 21 Apr 2020 23:38:56 +0200 (CEST) Received: from new4-smtp.messagingengine.com (new4-smtp.messagingengine.com [66.111.4.230]) by dpdk.org (Postfix) with ESMTP id 66EAA1D14A for ; Tue, 21 Apr 2020 23:38:55 +0200 (CEST) Received: from compute7.internal (compute7.nyi.internal [10.202.2.47]) by mailnew.nyi.internal (Postfix) with ESMTP id CE8F2580389; Tue, 21 Apr 2020 17:38:54 -0400 (EDT) Received: from mailfrontend1 ([10.202.2.162]) by compute7.internal (MEProxy); Tue, 21 Apr 2020 17:38:54 -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= 4Q/upnX/M8o+PLpmi9/iyGgp9AIJx99xgpwTfsxgVx0=; b=Mptoj1ZqUNmHOG1z pnI6SsaEzzUMAC2lT2RkUxfJUaRTirv9RdMBGG/zKI7LR31MIcit3hk4H4p3TFjB iWRzj9gpFH3RCNZ6ZrwiJ6NpNYqDbxuCtk39TognZolYnOWQegKTtAG/0DgVd9np juFDMq3uKpIzGmtX9+76HlcaZSFySb81eYIgovxc+BtiKfLjN2t2VLLf9DWkA9cc HeqKnuOt7i6pi8kb8ovZK3UlXIdU7BEJZqTdzb8gUHnnzS1QnJptKA2J19eUgnjA ZCpu2C7XRmggxXj0GVTh8ABPuebyWFD0YmNwbgJhZgB1J2o6IX92UfVw+2ksx/MU pwrK0g== 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=4Q/upnX/M8o+PLpmi9/iyGgp9AIJx99xgpwTfsxgV x0=; b=FDO8NYDxa5o8viO7305j+vLD46Fkp9WtDPRx2teqA/zOExXpOfvcBiHdy cXiWBnvGCRx2Gx+ECNZxQYDxH/P0sZOFpYhSkHLS3pX4CyswFmaVcJoa3MYDOgPt iwvkEvgkt/oQGMQrAqfnfRFMshmfZw4B0Arh3mmq96FldXHy6we9L1kJf7Oq4hEB 6giXTwPwETv4sTOf0savQkHd1hPXScKtGL8MHRmmrAnnwCBfrV9lgPxqYaKHhgq9 uZO7y1VW52z1A4rVT8H5ofQIx/CdDn5B2pNCb59G8blfn/cERS7VFEEvDx2yBfoV 6Yc4wZNP/I9y5Sn7bYLxhRV1RUlZQ== X-ME-Sender: X-ME-Proxy-Cause: gggruggvucftvghtrhhoucdtuddrgeduhedrgeeiucetufdoteggodetrfdotffvucfrrh hofhhilhgvmecuhfgrshhtofgrihhlpdfqfgfvpdfurfetoffkrfgpnffqhgenuceurghi lhhouhhtmecufedttdenucesvcftvggtihhpihgvnhhtshculddquddttddmnecujfgurh ephffvufffkfgjfhgggfgtsehtufertddttddvnecuhfhrohhmpefvhhhomhgrshcuofho nhhjrghlohhnuceothhhohhmrghssehmohhnjhgrlhhonhdrnhgvtheqnecukfhppeejje drudefgedrvddtfedrudekgeenucevlhhushhtvghrufhiiigvpedtnecurfgrrhgrmhep mhgrihhlfhhrohhmpehthhhomhgrshesmhhonhhjrghlohhnrdhnvght 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 C97EB328006C; Tue, 21 Apr 2020 17:38:49 -0400 (EDT) From: Thomas Monjalon To: "Ananyev, Konstantin" , "Richardson, Bruce" , "Dumitrescu, Cristian" , Lukasz Wojciechowski Cc: 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: Tue, 21 Apr 2020 23:38:48 +0200 Message-ID: <1868765.PIDvDuAF1L@thomas> In-Reply-To: <552291b0-32c1-d8db-034e-ecc599b4c940@partner.samsung.com> References: <20200417215739.23180-1-l.wojciechow@partner.samsung.com> <552291b0-32c1-d8db-034e-ecc599b4c940@partner.samsung.com> 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" 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). > 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()? > 6) To avoid logs flood using this option, logging functionality in the > debug section of the code should use valid logtype, so they can be > filtered out by rte_log_can_log(). rte_log_can_log() is not for logs. It allows to enable/disable some code following the same rule as a logtype. > This can be done in 2nd stage, when all logs not using proper logtype > will be exposed in rte_debug mode. > > 7) For the drivers, which have multiple debug flags ..._TX, ..._RX, etc. > in case of rt_debug all can be enabled in driver's internal meson.build > (as Bruce proposed above) > > > There is also an alternative proposed: defining options like > debug_drivers or debug_libraries that would use globs and work similar > way to the disable_drivers. > > > Please share you opinion about this plan!