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 907D7A00C2; Wed, 22 Apr 2020 13:30:34 +0200 (CEST) Received: from [92.243.14.124] (localhost [127.0.0.1]) by dpdk.org (Postfix) with ESMTP id 674931D5B0; Wed, 22 Apr 2020 13:30:34 +0200 (CEST) Received: from mga18.intel.com (mga18.intel.com [134.134.136.126]) by dpdk.org (Postfix) with ESMTP id 2CAA81C1C4 for ; Wed, 22 Apr 2020 13:30:32 +0200 (CEST) IronPort-SDR: dlNrqCj9iynuAtR7NK8w7P4JYzb0Zo6chxvt0l8Iq7BDUgVvtOKjzldyZX4HzxpXasIliP5k3l UOGN42e688wQ== X-Amp-Result: SKIPPED(no attachment in message) X-Amp-File-Uploaded: False Received: from orsmga008.jf.intel.com ([10.7.209.65]) by orsmga106.jf.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 22 Apr 2020 04:30:31 -0700 IronPort-SDR: VNZknWIU5fgOwSDOXrgeF/K3l2Pj9aVBzzwtv7dfv/pJozL67vUcubWj0o/Xw8WkB2j9NPLLw5 5hz3bY092qUQ== X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.72,414,1580803200"; d="scan'208";a="291915871" Received: from fmsmsx106.amr.corp.intel.com ([10.18.124.204]) by orsmga008.jf.intel.com with ESMTP; 22 Apr 2020 04:30:30 -0700 Received: from FMSEDG002.ED.cps.intel.com (10.1.192.134) by FMSMSX106.amr.corp.intel.com (10.18.124.204) with Microsoft SMTP Server (TLS) id 14.3.439.0; Wed, 22 Apr 2020 04:30:29 -0700 Received: from NAM12-DM6-obe.outbound.protection.outlook.com (104.47.59.176) by edgegateway.intel.com (192.55.55.69) with Microsoft SMTP Server (TLS) id 14.3.439.0; Wed, 22 Apr 2020 04:30:03 -0700 ARC-Seal: i=1; a=rsa-sha256; s=arcselector9901; d=microsoft.com; cv=none; b=Z5CI//1CrgdsKSRhg0Jvl7XwnJvOQ3ry3kt4Xd0AI0gRoVU+3xJgZBV/yqr8CmX7njlaWsjJZv9rMkF9BOYyyG6zWS+VXr/uK34MOQD9DhAQHZqroF08nOBkQyLdyw1UO9c1DQY2ra4ywgJ/AXppU1wzfBWC2wO7+uf8jIa2bFpi4psRyTVqjYRwl7+3H4sbgVFhLwqfJ81xuty0wuw5IMB6CK1NQhGANUrDYmneABgOPKxX2D3vn+kfNMCaoG5IDD6KD71cZ09F1wiNNpi/IxfxsCW0Y/Q/m3eVCrb0WzJ5tRsgQPWX5nALGcjbsicpSaoNx2jzDxOshvwf5YVx9w== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=microsoft.com; s=arcselector9901; h=From:Date:Subject:Message-ID:Content-Type:MIME-Version:X-MS-Exchange-SenderADCheck; bh=0XXZUMT8qX7V7k2FBNJ0CWvXxYoUuAAV1M1R25dEwiY=; b=Xqt7sVMV9RkKq+C7f7NFaFR7Xp/DxHbFtDtjutxUhRvsJjHjXOkTMcTzdZvUzzQVi7InMznbeSkFejz/HMUvQgcqxlA6Qp9oaH+4MDbpfs5wtDATfpFcF7ZWU1jd536EQUHdZ8jb8alzDXlpz/TvOacL0wfpbCvUf9QoHBEAXlfPGHZc4yzm3vb3D3KUVneuE/jeexJCMv11i6D24gDIwfottJaX9gDh8fkPrkG7Wf216MxtMYwU2oXU7I5QZPVU3nsgyJjwHRW/16ah2tcX5SfkkYjrJltRoJMwOevQMc0riPh59gdAgFDGJK5GpWaDEnW7Ig3IG2sjSHcs7JnXfA== ARC-Authentication-Results: i=1; mx.microsoft.com 1; spf=pass smtp.mailfrom=intel.com; dmarc=pass action=none header.from=intel.com; dkim=pass header.d=intel.com; arc=none DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=intel.onmicrosoft.com; s=selector2-intel-onmicrosoft-com; h=From:Date:Subject:Message-ID:Content-Type:MIME-Version:X-MS-Exchange-SenderADCheck; bh=0XXZUMT8qX7V7k2FBNJ0CWvXxYoUuAAV1M1R25dEwiY=; b=uqHn7Nbr+ABXztUG6rWLoNDjOJ6QECL1VD6la2nnLnCshU++Jww7d3Ug2Ss9j4nJJXXloZy2rI5wp3F6B9Ag3uOXaW5hYHjLIi2x9lF4TYHMiUlhtZ3y5l+oaJoCIbfxyiFAXgl3sOPvR9i4yEwl5+NbDFBJ6rRYAVhLovx4dLo= Received: from BYAPR11MB3301.namprd11.prod.outlook.com (2603:10b6:a03:7f::26) by BYAPR11MB3015.namprd11.prod.outlook.com (2603:10b6:a03:86::14) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id 15.20.2937.13; Wed, 22 Apr 2020 11:29:59 +0000 Received: from BYAPR11MB3301.namprd11.prod.outlook.com ([fe80::f8cb:58cd:e958:fff4]) by BYAPR11MB3301.namprd11.prod.outlook.com ([fe80::f8cb:58cd:e958:fff4%6]) with mapi id 15.20.2937.012; Wed, 22 Apr 2020 11:29:59 +0000 From: "Ananyev, Konstantin" To: Thomas Monjalon , Lukasz Wojciechowski , "Richardson, Bruce" 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" Thread-Topic: [dpdk-dev] [PATCH v1 03/17] ethdev: replace library debug flag with global one Thread-Index: AQHWFQNglnRcCbrjHUKnbi8uf4bdj6iBu2+AgABDdSCAABUSAIAABhaAgAApZACAAALOAIAAAm+AgAABfwCAABcBAIAATYdggAFmXgCAAAtiAIAA2saAgAADxYCAAAIQAIAABZCQ Date: Wed, 22 Apr 2020 11:29:59 +0000 Message-ID: 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> In-Reply-To: <26026576.gRfpFWEtPU@thomas> Accept-Language: en-GB, en-US Content-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: dlp-product: dlpe-windows dlp-reaction: no-action dlp-version: 11.2.0.6 authentication-results: spf=none (sender IP is ) smtp.mailfrom=konstantin.ananyev@intel.com; x-originating-ip: [192.198.151.168] x-ms-publictraffictype: Email x-ms-office365-filtering-correlation-id: ee305aa3-529b-4239-3920-08d7e6b07dcd x-ms-traffictypediagnostic: BYAPR11MB3015: x-ld-processed: 46c98d88-e344-4ed4-8496-4ed7712e255d,ExtAddr x-ms-exchange-transport-forked: True x-microsoft-antispam-prvs: x-ms-oob-tlc-oobclassifiers: OLM:10000; x-forefront-prvs: 03818C953D x-forefront-antispam-report: CIP:255.255.255.255; CTRY:; LANG:en; SCL:1; SRV:; IPV:NLI; SFV:NSPM; H:BYAPR11MB3301.namprd11.prod.outlook.com; PTR:; CAT:NONE; SFTY:; SFS:(396003)(376002)(366004)(39860400002)(346002)(136003)(478600001)(33656002)(6506007)(55016002)(6636002)(54906003)(52536014)(9686003)(110136005)(5660300002)(316002)(4326008)(7696005)(76116006)(66446008)(26005)(66476007)(66556008)(64756008)(66946007)(8936002)(2906002)(186003)(71200400001)(7416002)(8676002)(86362001)(30864003)(81156014)(21314003); DIR:OUT; SFP:1102; x-ms-exchange-senderadcheck: 1 x-microsoft-antispam: BCL:0; x-microsoft-antispam-message-info: cEGDR7T9U5j9Cs+EvMq6+ZUtXTcdVfBbtcbO1qSP1RMyiMktINPu8WIHy1bxlqkuo6vPfpb2SbNQ04jxHqPFRur1C76Q2lFK4fEeBbAiZUwoEXCT5lBMMdo1/mmRsmqQ7Hx3sYdGJhV2qqmWQQN83scWY48tLUtNi+v90J5h6OowQVr3zjz0D/toTNzI6xyEXkbVjRyU3JBP2fGw3tq/mv+8SgtfXcZksEbZpkzPIU5zWM0q3HxzlmLXS4JG4EMperEbfBQ11QWTlCFhiTFamG/m+ZJExH6wPMMmLZxCxwXrB/Tq0pc8cb/Wbh1QfGsQDmSuWgxsLhlTzZgADeXaZZuqU5IMZ0V8c0gkM5Yc5tfg0HPqyfi40a99R86xrSEY+bEV7hXm0jZcWB+QhX9FZoMyTq220vRLFBeRn7YHhzQf4MUFZGsww+Xld9rV9MpK5tresDb8Wi0BchDC5OT7xwKp3QI0OQ3BzgXeecdOf/XQEhbQNosOdvoOzhZ1dZW0 x-ms-exchange-antispam-messagedata: rkoiJCwOMv10158IQqx5v3AaF4xTM0SIwFqU3qN28bgzxd5meZe3DKJ4kvBf310QKKQMUtzUaYVaztKPK6/Nw1DWSdKalA8bK/68lp47bVmPYryBdzFhCohyRNo1RSync70macUgsUTfLCuhRwxP6Q== Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable MIME-Version: 1.0 X-MS-Exchange-CrossTenant-Network-Message-Id: ee305aa3-529b-4239-3920-08d7e6b07dcd X-MS-Exchange-CrossTenant-originalarrivaltime: 22 Apr 2020 11:29:59.3549 (UTC) X-MS-Exchange-CrossTenant-fromentityheader: Hosted X-MS-Exchange-CrossTenant-id: 46c98d88-e344-4ed4-8496-4ed7712e255d X-MS-Exchange-CrossTenant-mailboxtype: HOSTED X-MS-Exchange-CrossTenant-userprincipalname: ZrEPAJlqUA1M+Fk9Uh/L6ri0y09EegbwWyFN6WjpBQ7iz+M5dQ3pU7gk6CfgIfmZLkofBsStI1TEhoDv0tsxTPWjAiWOaLLqav4WJ/sOiIE= X-MS-Exchange-Transport-CrossTenantHeadersStamped: BYAPR11MB3015 X-OriginatorOrg: 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" >=20 > 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 p= urpose is to > > > >>>>>>>>>>> minimize number of config compile options, I wonder can't= it be done > > > >>>>>>>>>>> in a slightly different way: 1. introduce gloabal RTE_DEB= UG 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 partic= ular debug > > > >>>>>>>>>>> flag for these libs. Something like: If dpdk_conf.get('R= TE_DEBUG') =3D=3D > > > >>>>>>>>>>> true dpdk_conf.set('RTE_LIBRTE_XXX_DEBUG ', 1) > > > >>>>>>>>>>> > > > >>>>>>>>>>> defines that are used by multiple libs, probably can be s= et in upper > > > >>>>>>>>>>> layer meson.build. > > > >>>>>>>>>>> > > > >>>>>>>>>>> That way will have global 'debug' flag, but users will st= ill have an > > > >>>>>>>>>>> ability to enable/disable debug flags on a per lib basis = via > > > >>>>>>>>>>> CFLAGS=3D"-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 fla= gs 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, s= ince it's > > > >>>>>>>>>> always e.g. RTE_DEBUG_ + library-base-name, e.g. RTE_DEBU= G_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: * us= age of global > > > >>>>>>>>> "debug" flag for meson build stays * we standardize names o= f debug flags > > > >>>>>>>>> in the components to RTE_DEBUG_ + components name * debug f= lag 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_co= nf meson > > > >>>>>>>>> object and written then later to rte_build_config.h before = compilation > > > >>>>>>>>> stage. All the other modules will be able to use these fla= gs. > > > >>>>>>>>> > > > >>>>>>>> Sounds good to me (obviously!), but I'd like other feedback = to ensure > > > >>>>>>>> others are ok with this before you spend too much effort imp= lementing it. > > > >>>>>>>> > > > >>>>>>>> For the drivers, the flag probably needs to include the cate= gory as well as > > > >>>>>>>> the name, e.g. RTE_DEBUG_NET_IXGBE, RTE_DEBUG_RAW_IOAT, to a= void possible > > > >>>>>>>> name confusion. Those flags can then be checked inside indiv= idual > > > >>>>>>>> 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 +=3D '-DRTE_LIBRTE_IXGBE_DEBUG_RX' > > > >>>>>>>> cflags +=3D '-DRTE_LIBRTE_IXGBE_DEBUG_TX' > > > >>>>>>>> ... > > > >>>>>>>> endif > > > >>>>>>>> > > > >>>>>>>> to enable more fine-grained control if so desired, and to ma= intain > > > >>>>>>>> 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 fac= ilities > > > >>>>>>> should be enabled. Then at runtime it is possible to enable/d= isable > > > >>>>>>> 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_l= og_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 hea= der > > > >>> and enforce extra checking, stats collection. > > > >>> etc. > > > >>> Probably that's ok for some cases to enable all that extra valida= tion we have at once. > > > >>> But I suppose in many cases people just interested to enable debu= g on one > > > >>> (ok might be two/three) particular libraries, not the whole syste= m. > > > >>> Right now there is such ability, we are going to remove it withou= t > > > >>> 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 an= d the users. > > > >>>>>>> Note: I am strong on this position. > > > >>>>>>> > > > >>>>>> Note, this means that you need to ensure all debug printouts f= rom libs and > > > >>>>>> drivers are using the RTE_LOG macros so can be runtime control= led. I think > > > >>>>>> that may be some distance from reality right now. > > > >>>>> Perfect! Let's expose those nasty logs which are not (yet) cont= rollable. > > > >>>>> And next step is to block any patch in those drivers or libs, > > > >>>>> until it is fixed. Dynamic logging should have been complete fo= r long. > > > >>>>> > > > >>>> I can live with that, I suppose. Do we have any idea of the magn= itude 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 runt= ime performance? > > > >>>>> > > > >>>> I think that would be wise, yes. We can call the option "rte_deb= ug" 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 al= l agree: > > > >> > > > >> 1) The global debug flag is required to enable all the sanity chec= ks 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 introdu= ce 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=3D"-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 enab= led > > > >> when rte_debug is set. Standardized names would allow easy usage i= n > > > >> 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 the= y > > > 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. >=20 > In order to maintain a good global user experience, > we need to drive such change with a roadmap and deadlines. >=20 > > > >> Should they? Or should we leave the current debug macros? Please s= hare > > > >> 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/drive= rs. > > > > 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 hard= er. > > > > > > > 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. With= out > > RTE_DEBUG, the whole block is skipped. > > > > #ifdef RTE_DEBUG > > if rte_log_can_log(...) { > > /* do debug stuff */ > > } > > #endif >=20 > 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.=20 I suppose mbuf will be the one mostly affected,=20 as we'll call rte_log_can_log() for nearly every mbuf op (free/detach/attac= h/reset, etc.) =20