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 7D309A00C2; Wed, 22 Apr 2020 12:41:59 +0200 (CEST) Received: from [92.243.14.124] (localhost [127.0.0.1]) by dpdk.org (Postfix) with ESMTP id 3AB561D562; Wed, 22 Apr 2020 12:41:58 +0200 (CEST) Received: from mailout1.w1.samsung.com (mailout1.w1.samsung.com [210.118.77.11]) by dpdk.org (Postfix) with ESMTP id AD01E1D55B for ; Wed, 22 Apr 2020 12:41:56 +0200 (CEST) Received: from eucas1p2.samsung.com (unknown [182.198.249.207]) by mailout1.w1.samsung.com (KnoxPortal) with ESMTP id 20200422104156euoutp0180e7c1ce09fc1e2ffb6649cc1014a207~IHj0A9OUg2861928619euoutp01Z; Wed, 22 Apr 2020 10:41:56 +0000 (GMT) DKIM-Filter: OpenDKIM Filter v2.11.0 mailout1.w1.samsung.com 20200422104156euoutp0180e7c1ce09fc1e2ffb6649cc1014a207~IHj0A9OUg2861928619euoutp01Z DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=samsung.com; s=mail20170921; t=1587552116; bh=BxcAirvl5kPYmpx+YUCFw9xy6N1xNsKtpXxinG5ujgE=; h=Subject:To:Cc:From:Date:In-Reply-To:References:From; b=jDFRzARCDYApBj3NH+bWu/4gsZ3lUqhWhMdHhqjvadIdFod2F7CQ5y5iNFMW0k2/O iy/GvAo8XdAo8RfK89+X4LB8WwFnh6S2RDo6bpCBv9wQ71V8hk0+IBROIxnseWMhuG VXBgYyaFDJhKKlGzFkW0WL9pypkcrBJRNFcBW1D0= Received: from eusmges2new.samsung.com (unknown [203.254.199.244]) by eucas1p1.samsung.com (KnoxPortal) with ESMTP id 20200422104155eucas1p155a1c67423fb747833b02ecaed907682~IHjz3njsf3124331243eucas1p1s; Wed, 22 Apr 2020 10:41:55 +0000 (GMT) Received: from eucas1p2.samsung.com ( [182.198.249.207]) by eusmges2new.samsung.com (EUCPMTA) with SMTP id A9.50.60679.37F10AE5; Wed, 22 Apr 2020 11:41:55 +0100 (BST) Received: from eusmtrp1.samsung.com (unknown [182.198.249.138]) by eucas1p1.samsung.com (KnoxPortal) with ESMTPA id 20200422104155eucas1p1c82011eb17e4b214278253b1d1c98cdc~IHjzhZXIi2249822498eucas1p1j; Wed, 22 Apr 2020 10:41:55 +0000 (GMT) Received: from eusmgms1.samsung.com (unknown [182.198.249.179]) by eusmtrp1.samsung.com (KnoxPortal) with ESMTP id 20200422104155eusmtrp1b55a5d00dcec6f924af23664daeb1d66~IHjzgTjGP1526115261eusmtrp12; Wed, 22 Apr 2020 10:41:55 +0000 (GMT) X-AuditID: cbfec7f4-0e5ff7000001ed07-e5-5ea01f736d41 Received: from eusmtip2.samsung.com ( [203.254.199.222]) by eusmgms1.samsung.com (EUCPMTA) with SMTP id AF.F3.08375.37F10AE5; Wed, 22 Apr 2020 11:41:55 +0100 (BST) Received: from [106.210.88.70] (unknown [106.210.88.70]) by eusmtip2.samsung.com (KnoxPortal) with ESMTPA id 20200422104152eusmtip2570509157156b7e2ed1b69e3b9a0fe91~IHjwe8Hx51210212102eusmtip26; Wed, 22 Apr 2020 10:41:52 +0000 (GMT) To: Thomas Monjalon , "Ananyev, Konstantin" , "Richardson, Bruce" , "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: <237f3d8b-4040-15a2-a6c5-979aae9df8f2@partner.samsung.com> Date: Wed, 22 Apr 2020 12:41:49 +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: <1868765.PIDvDuAF1L@thomas> Content-Transfer-Encoding: 8bit Content-Language: en-US X-Brightmail-Tracker: H4sIAAAAAAAAA01Sa0xTZxje1/Od00NjyaHq+KZGSMV7hKlseaduzm0Zx6jB6PyzRfEgJ0Dk lh7xsh+mYqO04tyAySgBKojWeqGOi0pQYlFwosPiDZUOjVVALWiriIButEcz/j3v+z7P+zzv l4+lNC5mApuctknUpQkpWkaFa5vetM6RwizrPj1vmAv3810M7HluUEC7bTFUGPoR5HbqFdDr PaWAG21PMXScbVGCoekBA44X3Qj2XryNoai8gIZ/S1PA6+hSQqHFh6B+8BKGvuEyDE3GegwV 3ocUXO++SENN1kkF+HJWQYF9HwP5Z04jeFZbRkHzqUM0eO+PqGquvEXw0HUEQUNXoxKGn30G 53ebKCi3tTAweOwghipnrgI8RiP+OoJ37utAvPWaieavHm6n+fyhkzQ/aKmgecMFD82X1/co +LwDTopvurNfyfedu8nwPdUZ/FH3AMP3lnqYleofVYsSxJTkzaIu6qv1qqTSmi8y7sRufTTU gPTo0BITYlnCRZOdztUmpGI1nBWRawM+LBcvESk6dpeRCx8ixn+slAkFBRS7ftFT8uAwIvf2 P34v8SCy50od7WeN5daSP9yXsR+P41oRedH9k59EcdkqYraWMP4Bw31JLhS+CgjU3Pfk73OX GX8ozE0ll3bQfjh+ZE/BrR9kRgj5q9AdWBnEzSDVnZUBJcWFkZ01RZSMQ8ldd6nCb0W4qiDS 1mXFcurvyEFnDiPjseRJc7VSxpNIS14OlgW1iNwceoPkogGR23ut71kLSeO7oUA4iptJKuui 5MdbQorzwmUYTNo9IXKGYJJbW0DJbTXJ3qWRd0SSRzm/ow+uw8fd+FekNY+6zDzqGvOoa8z/ 21oQtqFQMVNKTRSleWnilkhJSJUy0xIjN6Sn/olGPn3Lu+aXp1HdcLwDcSzSjlG3t5as09DC ZmlbqgMRltKOU9sfjLTUCcK2n0VdepwuM0WUHGgii7Wh6vllPWs1XKKwSdwoihmi7sNUwQZN 0KPqzg3qkvRXrm/WbD+ywKMfk1Tha6va8TRkilhpeCwsdXozPhY1n3w0Lf5btHLK/GXG6NiO Fa9fF2dTxVED/ZPiY7LmxOmSdY1942f/RoVNb4idGGrqn3FCCJ/6/Oxq5/Ise3mCLTwixr7G Ms02uehJRJbSZQ/+/Gq0EOOJ65jZGxOtxVKSMHcWpZOE/wBBdoiB8AMAAA== X-Brightmail-Tracker: H4sIAAAAAAAAA02SaUxTaRSG892thdjkWlE/iRHtjIkarRYsPXUnUef+m4kSQ1TQG7wBY0uZ 3pboJCRVrwtF2SQiNQMdEdBKoogVF5RYpKjoOFRH0YCA1hFFFnGJsqgtHRP+PTnnfc7JSY6c VA7QkfLtaRbBnMYbVEw41fy16dkCMcqZtCincBp0FrYzkD0gEdDqWgnl0icEBR02AvoGawl4 6OuhoO1aswwkbxcDnnfdCA43PqbgeFkRDd9KDTDoeSWDYud7BHVDtyjoHzlBgTerjoLywRck POhupMG9p5qA94fWQdG5XAYKL19C8PbiCRKaaitoGOwMWO67owhetJ9GUP+qQQYjb7Vw44Cd hDJXMwNDVScpqGkpIKA3K4ta9TPXktuGuFP/2GnuXmUrzRUOV9PckLOc5qSbvTRXVvea4I78 1UJy3idHZVz/9X8Z7vWFdO6M/zPD9ZX2Mr8pNqqXmU1WizAz1SRalqs2aSBardGDOnqxXq2J 0SUuidaqFq5Ytk0wbM8QzAtXbFWnlrr16U9+3flyuB7ZUEWcHYXJMbsY78+xkXYULley5QjX 725AdiQPNKbjAR8bykzCI4/sTCjTg7D7v6tUsDGJTcTH/HfGOIK9j3DH8JRgiGQLwvHJ4Q4i ZDQR2HmmlQimGHY5vln8kQ6ygl2L/75+hwluo9jZ+NZuOoiTA0OllrmhxER8u9g/Nj+MnYMv dJwdM0k2FpfUdJEhjsJ73cf/56n4qb+UyENKxzjdMU5xjFMc4xQnolwoQrCKxhSjqFGLvFG0 pqWok03G8yjwbRe9X2ouIV/1eg9i5Ug1QdF6vyRJSfMZ4i6jB2E5qYpQnOsKlBTb+F1/CGbT FrPVIIgepA2clk9GTk42BX43zbJFo9XoQK/RxehiYkE1VXGQvbFZyabwFmGHIKQL5h8eIQ+L tCFx5dJv8Q3xUp5jRnJGQlsD3307aXXKvlm/R9l1n31hh3O7rOukEU9mz3N/Ai/1x5f92ZPX Po39Srsyq3wJRVWd+k6yNttdcn6+Ni436+Wobf/cCHv++p8Sv9BE484rj+ZnvjnLbZJiDTkf ZL94R607NlT2ubLX5MelTnf3PY3HKkpM5TXzSLPIfwf1LpeVgwMAAA== X-CMS-MailID: 20200422104155eucas1p1c82011eb17e4b214278253b1d1c98cdc X-Msg-Generator: CA Content-Type: text/plain; charset="utf-8" X-RootMTR: 20200421213900eucas1p23913c6c3203b1dbbe07045f5db50fccf X-EPHeader: CA CMS-TYPE: 201P X-CMS-RootMailID: 20200421213900eucas1p23913c6c3203b1dbbe07045f5db50fccf References: <20200417215739.23180-1-l.wojciechow@partner.samsung.com> <552291b0-32c1-d8db-034e-ecc599b4c940@partner.samsung.com> <1868765.PIDvDuAF1L@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 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. >> 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. > >> 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. Oh, I didn't know, that it should be used this way also. Currently except for logs it's used in 5 places and are related with printing some information. So I thought it was just for logging. > >> 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