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 AFD1FA00C2; Wed, 22 Apr 2020 13:52:31 +0200 (CEST) Received: from [92.243.14.124] (localhost [127.0.0.1]) by dpdk.org (Postfix) with ESMTP id 84CA01D5C9; Wed, 22 Apr 2020 13:52:30 +0200 (CEST) Received: from mailout2.w1.samsung.com (mailout2.w1.samsung.com [210.118.77.12]) by dpdk.org (Postfix) with ESMTP id 9DB6A1D5C8 for ; Wed, 22 Apr 2020 13:52:29 +0200 (CEST) Received: from eucas1p2.samsung.com (unknown [182.198.249.207]) by mailout2.w1.samsung.com (KnoxPortal) with ESMTP id 20200422115228euoutp02b9271d6e2966094971c5f843ac3a522d~IIhaBxEr00507205072euoutp02G; Wed, 22 Apr 2020 11:52:28 +0000 (GMT) DKIM-Filter: OpenDKIM Filter v2.11.0 mailout2.w1.samsung.com 20200422115228euoutp02b9271d6e2966094971c5f843ac3a522d~IIhaBxEr00507205072euoutp02G DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=samsung.com; s=mail20170921; t=1587556348; bh=T0wsjbf7TEs1JSQ91BZmQP+vy3CWrRCWrjWktLY2C0U=; h=Subject:To:Cc:From:Date:In-Reply-To:References:From; b=Nhi3UDN9bH2/rF6JqBYjKmPgaoao1fVaD4GfAFnHXStjq7XIMvQWdgqBpZjPdDfHK 7zGzhmhq+AIuSbVaKxb/vBW6FaD8K0QQsNUGS2neAwxQBvCIIhR4EPW/HNiQEAFSub wR2wZIFMaLZCOOVNfgStDTklhKXPrwf70Bajb55o= Received: from eusmges2new.samsung.com (unknown [203.254.199.244]) by eucas1p1.samsung.com (KnoxPortal) with ESMTP id 20200422115228eucas1p128693720fb3bdf58853c18bb0e1514c1~IIhZ5pop-1230812308eucas1p1o; Wed, 22 Apr 2020 11:52:28 +0000 (GMT) Received: from eucas1p2.samsung.com ( [182.198.249.207]) by eusmges2new.samsung.com (EUCPMTA) with SMTP id 92.EA.60679.CFF20AE5; Wed, 22 Apr 2020 12:52:28 +0100 (BST) Received: from eusmtrp1.samsung.com (unknown [182.198.249.138]) by eucas1p2.samsung.com (KnoxPortal) with ESMTPA id 20200422115228eucas1p2edd94167fa2884cae159cc84049ffc01~IIhZlJIJ_0887308873eucas1p2w; Wed, 22 Apr 2020 11:52:28 +0000 (GMT) Received: from eusmgms2.samsung.com (unknown [182.198.249.180]) by eusmtrp1.samsung.com (KnoxPortal) with ESMTP id 20200422115228eusmtrp1066a4b729ea38eef11e3dd7839b48998~IIhZjwl0Y2950929509eusmtrp1J; Wed, 22 Apr 2020 11:52:28 +0000 (GMT) X-AuditID: cbfec7f4-0e5ff7000001ed07-87-5ea02ffcaebb Received: from eusmtip1.samsung.com ( [203.254.199.221]) by eusmgms2.samsung.com (EUCPMTA) with SMTP id CF.0B.07950.CFF20AE5; Wed, 22 Apr 2020 12:52:28 +0100 (BST) Received: from [106.210.88.70] (unknown [106.210.88.70]) by eusmtip1.samsung.com (KnoxPortal) with ESMTPA id 20200422115225eusmtip1b9fbe83fbd3e908480ddf6c16888442f~IIhW8shfv2000920009eusmtip1B; Wed, 22 Apr 2020 11:52:25 +0000 (GMT) To: Thomas Monjalon , Bruce Richardson Cc: "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" From: Lukasz Wojciechowski Message-ID: <1ae782a9-10cc-8cdd-8815-4e0e7d36e466@partner.samsung.com> Date: Wed, 22 Apr 2020 13:52:21 +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: <26026576.gRfpFWEtPU@thomas> Content-Transfer-Encoding: 8bit Content-Language: en-US X-Brightmail-Tracker: H4sIAAAAAAAAA01Sf0xTVxTO7bvv9dGt5FHduEGTrs2MmZkw3ZacBcZ+aOYzWbJlmsWRyKz6 BDNA1idO3D+Na7bR4XAFYa2JrSDIjxmRQUWGEmtpy6rDbgPqhgFDDbDZOgEdlApr+zDjv++c 833nO9/NZSnVCJPG7i86KOiLdAVaRoEd7rn+9dEMe+5LLutzMFp1m4Fv/jHKIND8BtQbHyEw jxhkEJ66KIPff/0bw/BlnxyM7jsMOB9MIDjWO4ThZF0NDYu2AphyjsvBYp9G0B3xYrgfrcXg LuvGUD81RsFvE700dBy9IIPp8g+gprWCgapLnQjuOWop8FxsoGFqNKbquP4YwdjtJgQ949fk EL33Klz9ykRBXbOPgcgPZzD86DfLIFRWht98nvdXDCO+8aaJ5m+cDdB81fwFmo/Y62ne6ArR fF33pIyvPO2nePetajl//8oAw0+2F/MtwVmGD9tCzPvKHEXWXqFg/yFBn5G9S5HvuvmQKZ79 8HB4spUyoPAWE0piCfcKqbzRQJuQglVxjYiYz1iQVMwgsrhQhqViGpH2fkOMxiYkkfARqX8W kdCVW0vyECItTedQfO8Kbif5PvgzjuOVXA651N0ri5MoLqIgfw400fEBw71OXJaHCazk3iH+ 9kEmjjG3hvhO25i42zOxRTWD2yVKCumzBBM7k7gXiCs8lPCiODX5ouMkJeFU8kfQlvAi3Pkk 0vWtGUtBN5OZ0cElvIL85WmXS3g18VWWY0ngQGRgfg5JRQ8iQ8cal1iZ5NrCfOIiKmZ9vitD eoq3SPVdhQSTSSCUIt2QTMyOGkpqK8nXX6qkHenkbvkJ9MQ1ei6IjyOtdVky67I01mVprP/b 2hFuRqlCiViYJ4gbi4TP0kVdoVhSlJe+50BhG4p9e9+CZ6YTdUV3OxHHIu3Tyh0ae66K1h0S SwudiLCUdqWy9c6pXJVyr670iKA/8LG+pEAQnWgVi7WpypdrJ3equDzdQeETQSgW9E+mMjYp zYBeK+UaIqOax8lqUhF4its478gKGFsf+LrW/GQYftb9buZW7exx9afXd9s9I5pf5rYZ3m57 cUvUO7517L3P1303VJ3Z52/Z9C+3fVVLtqUtvbN+Okv+qD/b5N1Xt0mz/sSOno86iehdzFir 7ttwOD/H41VHDJrNaXt2HZ3ITLm62tamxWK+bsM6Si/q/gNeA7O+8gMAAA== X-Brightmail-Tracker: H4sIAAAAAAAAA02Sa0hTYRjHec9ts1ydluGLBtW6YEGzaeZjaBeCOgSFFESZaSsPWm3OdmZU H0JqUa7CNLuo4Ga2LqtIU9PKbkvXxS6u0iyylFZT0pmLlS3Ttlbgtx/v8/89f154xKS0nw4T b8nU8dpMpUrGjKKahh60zx6MNKXMyb9EQUdhOwOHvuoJaLMsBLP+O4KCDzkEuNy1BLx68YWC d7eaRKC3dTJg7e9CcKTxNQUl5SdpGDaqwG11iqDI9A1BvfchBX2Dpymw5dZTYHZ/JOFlVyMN NXsrCfh2eBWcrMhjoPB6HYKea6dJeFB7lgZ3h8+qefIbwcf2CwjuOO+LYLAnBu4dMJBQbmli wHvpDAVV9gICenNzqUXTOHveO8SdbzbQ3NNzbTRX+KuS5rwmM83pG3pprry+m+COldlJzvbm hIjru93CcN3VWdxFxwDDuYy9TKIkSR6v1WTr+MkZGkGXIFuvgCi5Ig7kUXPj5Iro2A3zo2Jk kQvi03jVlh28NnLBRnlGQ7OHyRpYs9PVXUHmINcyAxKLMTsXe127DWiUWMqaER6u6xEF3ifi ry9YAwry4Xg82GpgApkvCL86/wT5B+PZDfiU4zHl5xA2CXuqCpA/RLL60fhntYMKGFcIXNBj Ifwphk3ADUUe2s8Sdim2V7cyfqbY6bipzMj4myf4turtMwORcfhRkeNvQRA7Eze4Xv8tJtl5 uLSqkwzwJLyvpuQfh+K3DiNxFEmLR+jFI5TiEUrxCMWEKAsK4bMFdbpaiJILSrWQnZku36xR X0W+c7tm+1ldhwyu1VbEipEsWLJ2iilFSit3CLvUVoTFpCxEUtFZmiKVpCl37ea1mlRttooX rCjG97d8MmzCZo3veDN1qYoYRSzEKWKjY6PngSxUcpC9lyxl05U6fhvPZ/Ha/x4hDgrLQcfU TvuNH3vWdhmPngluS4lfWVSGnycONXcsub+iuTR4XPopW+i5tqHlHxIap04Kv/IpXN3bssfx KznLMCMt73hlRP+6zm2p1NiBm89QhNetf353lqVyTbjnvdkZWqtaPOwU4maPHfPGs+lzx/Xu kpZ8U+uq/K2Jn7Yn941hw27svyyTUUKGUjGL1ArKPzHnFdSEAwAA X-CMS-MailID: 20200422115228eucas1p2edd94167fa2884cae159cc84049ffc01 X-Msg-Generator: CA Content-Type: text/plain; charset="utf-8" X-RootMTR: 20200422110252eucas1p247a0a6d1efcf6499e1c15cf845ac0da9 X-EPHeader: CA CMS-TYPE: 201P X-CMS-RootMailID: 20200422110252eucas1p247a0a6d1efcf6499e1c15cf845ac0da9 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> 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 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. > 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(...) I guess that with rte_debug enabled you must accept drop of performance, but let libraries and drivers maintainers share their opinion. > > > -- Lukasz Wojciechowski Principal Software Engineer Samsung R&D Institute Poland Samsung Electronics Office +48 22 377 88 25 l.wojciechow@partner.samsung.com