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 78ED0A05A0; Tue, 21 Apr 2020 22:58:14 +0200 (CEST) Received: from [92.243.14.124] (localhost [127.0.0.1]) by dpdk.org (Postfix) with ESMTP id 5486D1D51C; Tue, 21 Apr 2020 22:58:14 +0200 (CEST) Received: from mailout2.w1.samsung.com (mailout2.w1.samsung.com [210.118.77.12]) by dpdk.org (Postfix) with ESMTP id 29C4D1D44F for ; Tue, 21 Apr 2020 22:58:13 +0200 (CEST) Received: from eucas1p2.samsung.com (unknown [182.198.249.207]) by mailout2.w1.samsung.com (KnoxPortal) with ESMTP id 20200421205812euoutp02d9eaaa595b45c0742786606dfc8fe374~H8Umu44571269112691euoutp02T; Tue, 21 Apr 2020 20:58:12 +0000 (GMT) DKIM-Filter: OpenDKIM Filter v2.11.0 mailout2.w1.samsung.com 20200421205812euoutp02d9eaaa595b45c0742786606dfc8fe374~H8Umu44571269112691euoutp02T DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=samsung.com; s=mail20170921; t=1587502692; bh=BpKWQS7yUaYf7RGrJuSXJeye1L/LVRpEPjjmaIo31iY=; h=Subject:To:Cc:From:Date:In-Reply-To:References:From; b=rhb56P07MXvLF2fO9etjPip2upxlWA9Ale46yU1mMdyeRFLvxBq66mnlJiQZnFgkp zKAgaSs9fXYR26U41x4xb6L9TJ8ndO7/EGckk+fYFoMhK8a4hF/t1dkHVDxTKCU/aR PFGSKhJxBONY8QwXRay8E3umN7dFi6HTFseyojbc= Received: from eusmges2new.samsung.com (unknown [203.254.199.244]) by eucas1p2.samsung.com (KnoxPortal) with ESMTP id 20200421205811eucas1p28a464845b8e67a4afdef71f24cf0981f~H8Ulnj8CX1010610106eucas1p2S; Tue, 21 Apr 2020 20:58:11 +0000 (GMT) Received: from eucas1p1.samsung.com ( [182.198.249.206]) by eusmges2new.samsung.com (EUCPMTA) with SMTP id 98.AB.60679.36E5F9E5; Tue, 21 Apr 2020 21:58:11 +0100 (BST) Received: from eusmtrp2.samsung.com (unknown [182.198.249.139]) by eucas1p2.samsung.com (KnoxPortal) with ESMTPA id 20200421205809eucas1p2a709df0c205b017e1b74c694c03f236d~H8UkVYwM72197521975eucas1p2l; Tue, 21 Apr 2020 20:58:09 +0000 (GMT) Received: from eusmgms2.samsung.com (unknown [182.198.249.180]) by eusmtrp2.samsung.com (KnoxPortal) with ESMTP id 20200421205809eusmtrp201c4701bd309d9d689fc5cdb29458440~H8UkSPTRf2630426304eusmtrp2C; Tue, 21 Apr 2020 20:58:09 +0000 (GMT) X-AuditID: cbfec7f4-0cbff7000001ed07-6e-5e9f5e63c46e Received: from eusmtip1.samsung.com ( [203.254.199.221]) by eusmgms2.samsung.com (EUCPMTA) with SMTP id 35.41.07950.16E5F9E5; Tue, 21 Apr 2020 21:58:09 +0100 (BST) Received: from [106.210.88.70] (unknown [106.210.88.70]) by eusmtip1.samsung.com (KnoxPortal) with ESMTPA id 20200421205807eusmtip152b176599e9746565125a8f3e1092750~H8UhtCIzx2159521595eusmtip1y; Tue, 21 Apr 2020 20:58:06 +0000 (GMT) To: "Ananyev, Konstantin" , "Richardson, Bruce" , Thomas Monjalon , "Dumitrescu, Cristian" 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" From: Lukasz Wojciechowski Message-ID: <552291b0-32c1-d8db-034e-ecc599b4c940@partner.samsung.com> Date: Tue, 21 Apr 2020 22:58:04 +0200 User-Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:68.0) Gecko/20100101 Thunderbird/68.7.0 MIME-Version: 1.0 In-Reply-To: Content-Transfer-Encoding: 8bit Content-Language: en-US X-Brightmail-Tracker: H4sIAAAAAAAAA01SbUxTZxjdc7962616KTLeuWUmzbZkS4TBZvIkSjeXae4fB2FbyExgq3qD IkXWipsu2xolk1ZQhyCjfpQPOxGVgVA+DFNXoYgMtCqKxmYwy5DNwgDZZul0Lbdk/DvnPOd5 z3OSl6c1Q9xiflPOVsmYo8/Wciqm2f2ob+n6DHvG61VdPA6WeDnc82c+hQO1b6Ej/y/A4l/M FI5NtlB449ofDN79sUeB+e4hDl0T9wGLOm8xeKi6jMUn9mycdI0osLxiCrA9cInB8WAVg25L O4OOyXs0Xr/fyaJzZwOFU4WpWFa/j8OStlbAB81VNHa1fM/i5GBoy/nzv4D3vCcAz49cVGDw wTL8abeVxuraHg4Dp44x2OgpptBvsTBvvyR69t0FseaqlRV7jw+wYslMAysGKhysmN/hZ8Xq 9lFKPFDpoUX37YMKcfxcPyeONuWKJ33/cOKY3c+lqNeqVmyQsjdtk4zxuk9UG33eesjdq/u8 daKOM8NIohWUPBHeJAcC/ZQVVLxGqAHS4OhVyORhiFi8IJMpICfd56i5lYMXpiMrx4H0tTdG XH4gnoIxRdgVLaST73yXmfBgkdALxHrTOuuihQIVsdUc5cIuTkgiHeXTbBirhdXEO3xtVmeE l8kFc0sog+djQi+V3fxAtkSR7nIfE8bKkNxUfAbCmBaWkF3OQ7SMY8kdnz1yaqOS+IK8jN8l 3+7qZmUcTX7valLI+AXypM0+W4cIzUD6Zx6BTM4DuVVUE3EtJxcfz3Dhg2jhVfLD2XhZXknq Ah10WCbCAjLgj5JvWECKm8sispoUfKOR3XFkuLAU5mKDp33MftDa5jWzzWtjm9fG9n9uBTC1 ECvlmQyZkikxR/oszqQ3mPJyMuPWbzGcgdDH73nc9bAVzgbXuUDgQfuMeuDK0QwNq99m2m5w AeFp7SJ1/VBIUm/Qb98hGbd8bMzLlkwueJ5ntLHqN6pG0zVCpn6rtFmSciXj3JTilYvN8I5j 79Rzqw7XrfjQMjPu5FdbU+DLtCy67xWzJs2zZk/iF0P7bd2lu9exHj7r+qdJO59de7r076UK XfBwYsLXOr665NSvayrjDe/rihLSK3f8lsJEHTnx4qA92Tkco1wY/9XK/paYjybeS1mVXhi9 vHNJVmGqM+3pS9FPtQqB1OS25KRpLWPaqE94jTaa9P8Bqbo3gPQDAAA= X-Brightmail-Tracker: H4sIAAAAAAAAA02SbUxTdxTG879vLcyaS4XxDyOZaWaMOstaKD0Y7fy03Y9m84NBQRu9aZWW kt5Cxr6MQI1yERmMoVQDTCxqJVoVLEuYaCdF5UWKbKCzE7QbEF7kJSwUqlsrLuHbk/M8v3Ny kkdKyhfoJOnRXBtvzdWbFEws1f22K7Bdn12f/dkf/VthpDrAQNmsnYBh1+fgtP+DoOpFEQEz 8x4CBgcmKXj+S7cE7L5RBrxz4wjKO4coONd4hoZ/600w7x2TQG3DAoL25QcUvA5foMBX2k6B c/4VCU/GO2loLb5BwMKpr+CMu4KB6p/bEEzdvkBCl6eJhvmRCNXa8wbBq8AVBB1jv0ogPKWB eydEEhpd3QwsN1+k4Ja/ioDp0lJq9yecv+I54i73izTXe2mY5qpXbtDccoOT5uz3p2musX2C 4H74yU9yvqc1Eu71nd8YbqIlj7saXGK4mfppZo8sU7nTasm38RuNFsG2S7FfBWqlKgOU6rQM pSpVm7VDrVGk6HYe4U1HC3hriu6Q0hgMuFHead03bXPXmCI0phZRjBSzabjm7iIholipnHUi PD1wVSIiacRIxrMD7GpmAw7/LjKrmUmERbeIosYGNgufDT6iokY8+xjh44v971IkWxWLL668 eL/2PIk9sy5JFGHYXfh+7SId1TL2Cxz4a4CJaordhO8WeYjo6YTIWrt/y2okDj+sDVJRHRMZ t1TdfHeZZNNx3a1RclV/jEtaz73XifhZsJ74Hskda3DHGsSxBnGsQRoQ5ULxfL5gNpgFtVLQ m4X8XIPysMV8E0UKd9sXamlD4szXXsRKkWKdbPhxXbac1hcIhWYvwlJSES9zj0ZGsiP6wm95 q+WgNd/EC16kifxWSSYlHLZE6ptrO6jSqLSQodKmalPTQZEoO8neOyBnDXobn8Pzebz1f46Q xiQVoX3FnCHLuNhRkxpyPGkuwyFvb/jH5mP7N+uSv/MrD+UJcZ9Wdvh9fxZ/eXo2dmh7eeHy kiGnZKYJEs+Oa0MG195gQVvXYFPNB3+rXmq2DYU86yf6z3feSVka+VDUhfu4fYPXd4cy3Wnt kvUfnXC+HF7qWzAlZ/bF9ZTPncw21pUqKMGoV20lrYL+P8m9HnCGAwAA X-CMS-MailID: 20200421205809eucas1p2a709df0c205b017e1b74c694c03f236d X-Msg-Generator: CA Content-Type: text/plain; charset="utf-8" X-RootMTR: 20200421003259eucas1p26bc06f9052d8bcfb9095f2f514eafd8c X-EPHeader: CA CMS-TYPE: 201P X-CMS-RootMailID: 20200421003259eucas1p26bc06f9052d8bcfb9095f2f514eafd8c References: <20200417215739.23180-1-l.wojciechow@partner.samsung.com> <7682483.vcMjziH8VY@thomas> <20200420173015.GC1719@bricha3-MOBL.ger.corp.intel.com> <3171532.h4nFI6E9mP@thomas> <20200420185756.GA1725@bricha3-MOBL.ger.corp.intel.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" 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 2) The best option would be to have a single flag - not to introduce too many build options 3) This option should be separated from meson "debug" option (used for build with symbols) and can be called "rte_debug" 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. Should they? Or should we leave the current debug macros? Please share your opinions as I see both cons and pros of this idea. 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(). 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! -- Lukasz Wojciechowski Principal Software Engineer Samsung R&D Institute Poland Samsung Electronics Office +48 22 377 88 25 l.wojciechow@partner.samsung.com