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 B780EA0561; Tue, 21 Apr 2020 02:32:55 +0200 (CEST) Received: from [92.243.14.124] (localhost [127.0.0.1]) by dpdk.org (Postfix) with ESMTP id 934331C299; Tue, 21 Apr 2020 02:32:55 +0200 (CEST) Received: from mga02.intel.com (mga02.intel.com [134.134.136.20]) by dpdk.org (Postfix) with ESMTP id 8FB9A1C243 for ; Tue, 21 Apr 2020 02:32:53 +0200 (CEST) IronPort-SDR: uJm9kwLIZAZBGFY4MMtATo28MEdiTKAXLVoox8TBa6AlW01SLdfsHj1jwUH0TqejeuWOX9DWbI bXhlUp1iyYWA== X-Amp-Result: SKIPPED(no attachment in message) X-Amp-File-Uploaded: False Received: from fmsmga006.fm.intel.com ([10.253.24.20]) by orsmga101.jf.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 20 Apr 2020 17:32:52 -0700 IronPort-SDR: e2qjhSjNIzZj9P+hwKQdrogDJVcujWUw7vxmTsV4+m8QosVJtQx5PvDbqZxeJODw1MFzO+m73/ ELWYTKkp01TQ== X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.72,408,1580803200"; d="scan'208";a="456570684" Received: from fmsmsx105.amr.corp.intel.com ([10.18.124.203]) by fmsmga006.fm.intel.com with ESMTP; 20 Apr 2020 17:32:52 -0700 Received: from FMSEDG001.ED.cps.intel.com (10.1.192.133) by FMSMSX105.amr.corp.intel.com (10.18.124.203) with Microsoft SMTP Server (TLS) id 14.3.439.0; Mon, 20 Apr 2020 17:32:52 -0700 Received: from NAM10-BN7-obe.outbound.protection.outlook.com (104.47.70.104) by edgegateway.intel.com (192.55.55.68) with Microsoft SMTP Server (TLS) id 14.3.439.0; Mon, 20 Apr 2020 17:32:50 -0700 ARC-Seal: i=1; a=rsa-sha256; s=arcselector9901; d=microsoft.com; cv=none; b=XanZ5Hi8Uu7k8qwD63ii2WP2him4Np7isNknHcpZW3+VeRI/kPxcBUXFmMkg3358yrGrt42oY2OE5R7ePDtpwUQtCo7J8g5FY9hE22BvOaVkRQ1BMsS0J2Yz+P6ShpPuBAePDd/99/w210flYd1It7Ei6pvBNip9e2RV2+uuw+qDvQL9Z48lZmuHUDamPznjaiIAnqNLpK6k3v6VqPHUM9nX6Wh8QURZ8ylTcVQsykpot2/StDyVs1zeCPP5B0e7uVh2jMK+KXhEsv1R7nV64zD0IhjrvKRaiDRq7oivC9z1gJcKLXhFW8J4lh1DPot6t7W5gvo8bPMJqyLoyEExYw== 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=DE2gnktHA9+xq4IY7GxVQPmdDatAkvH0Shyps0+X5Qc=; b=OAyzEaOtgYOaTE0s0VjowQibnm6QX79CrbPeXNe8m8nfhV8Zqh6ycXBKEFCKp8ns6l5yJbzrajaUUYiLbBoKvYiMy9Hj33wySYK6Zp8sx3KmU3gBZnHb5qWa4CjXg/M+oTQwjlPTTKbg0OR8cfvPHCw5OT4+7VIu4QzaUWwypvj5RwDx3GQ2LZ3OTlOkBEb7hxd9QkWEzO5bf7fT1fgdXE/KT8G3VgRBC1PK3+3GIfINXpPfJgLsSUXZ7oW+G7Srh+rYmLSGqsQNWqfpU+ufb2eeBzX07u4zRI4ethmgMfarLyeUsMxtpRxOZ5F9HVRFbOlhDmvOmr+13vyL+M3azQ== 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=DE2gnktHA9+xq4IY7GxVQPmdDatAkvH0Shyps0+X5Qc=; b=MXZgfp8faHGOTF+SG0h5SOYVrKSNo66sDFzO0CkQMJe6KzMt7oWq2krOxtNVjDgjktg/XF6qAUdXe+1xAgc3RuXxSN+/f5qaLVfKveY+ZKNdn7qvMLN0x2NP9+aK6a/YcDvm2LbIlZ5qMwQhH34sNUcA4eDjdIxl0A/KO4GCUp4= Received: from BYAPR11MB3301.namprd11.prod.outlook.com (2603:10b6:a03:7f::26) by BYAPR11MB3352.namprd11.prod.outlook.com (2603:10b6:a03:1d::26) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id 15.20.2921.25; Tue, 21 Apr 2020 00:32:30 +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.2921.027; Tue, 21 Apr 2020 00:32:30 +0000 From: "Ananyev, Konstantin" To: "Richardson, Bruce" , Thomas Monjalon CC: Lukasz Wojciechowski , "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+AgAABfwCAABcBAIAATYdg Date: Tue, 21 Apr 2020 00:32:29 +0000 Message-ID: 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> In-Reply-To: <20200420185756.GA1725@bricha3-MOBL.ger.corp.intel.com> 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.166] x-ms-publictraffictype: Email x-ms-office365-filtering-correlation-id: 3e158208-fdcc-4199-caa7-08d7e58b79d9 x-ms-traffictypediagnostic: BYAPR11MB3352: 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: 038002787A 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:(10019020)(396003)(366004)(136003)(346002)(376002)(39860400002)(8936002)(4326008)(6506007)(26005)(110136005)(7416002)(316002)(54906003)(81156014)(66476007)(8676002)(66946007)(64756008)(66556008)(66446008)(71200400001)(2906002)(186003)(7696005)(5660300002)(55016002)(9686003)(76116006)(478600001)(86362001)(33656002)(52536014)(21314003); DIR:OUT; SFP:1102; x-ms-exchange-senderadcheck: 1 x-microsoft-antispam: BCL:0; x-microsoft-antispam-message-info: LA7grxgBNoiXJHa8xu1OmjX4Q5DNpA6MCD2v/DCM6YCkTD2zqDAe9CJoxzTjk6sxWoO0NWu1xPzv1AuLHnjop4IbpEfP0BaM6r81QoS/SO0EDAQtqYah7fOloTJvUGiZbI1brzFzw/X8FnWgzTErZxjnMze/owRcPozHZhdXEel1uZm8eg2cdYX7uhSxyo0f2MDlH0Z4DS57QnerWk9VR9xDGfDEHdMNn+Oyf0KMdBjxaOyXI8QcoomDRiGPgwwwzKUowZDY8VfZDYP+jOJQ1702p7Wwxe8Ol7n1tT9GBW5RUnVYN1kNqyvy77a7WJP2tSOorurKfOth9rFeV7YePc55oKS3Gkpnm7Y9yITpNAVv7OyQDe0/btNQpHefkfa+IB6QSmfKO8ngDdLF4AV6T/+5p+/VYF56tmhYplsQ71yThVEq8eZD78BsW95WIRlgKUK1LpXNnFn/2LPJWoOYKNxkOuG80J5JiNeINbJHiaiG4NVIvb0ydwctCFAKrEEp x-ms-exchange-antispam-messagedata: w+llFmkZl+3o3c5i43EHoTONtndK7Xv1sBH4ldeeLMu7ae3Uq84SeVi7d+8qU6fpCnEg0g6NYpTKSfOcggWGCToWWaMxw879zfb/F8zD6WBHaFFsmMfsKICKawJLTqIJU98vHxOqcN2r74pdL0xwoA== Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable MIME-Version: 1.0 X-MS-Exchange-CrossTenant-Network-Message-Id: 3e158208-fdcc-4199-caa7-08d7e58b79d9 X-MS-Exchange-CrossTenant-originalarrivaltime: 21 Apr 2020 00:32:29.8928 (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: 7g20Ak/LB0y+R8CLoQUq9XvRyRJTKmuLC14bwU7vp+bi5plNRnkC5UMkVB+9P13UpxbrjVgZZOJ2Tt3jU82TVeP1COphYolOWFl/T2SZI24= X-MS-Exchange-Transport-CrossTenantHeadersStamped: BYAPR11MB3352 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" > > > > > > >> I am agree with Cristian concern here: that patch removes ab= ility to > > > > > > >> enable/disable debug on particular library/PMD. If the purp= ose 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.bui= ld file > > > > > > >> check if RTE_DEBUG is enabled, If yes, then enable particula= r debug > > > > > > >> flag for these libs. Something like: If dpdk_conf.get('RTE_= DEBUG') =3D=3D > > > > > > >> 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=3D"-D..." > > > > > > >> > > > > > > >> Konstantin > > > > > > >> > > > > > > > That seems a reasonable idea to me. > > > > > > > > > > > > > > However, in this case, we don't need the RTE_DEBUG flag at al= l, 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 fo= r each > > > > > > > library or driver to standardize the naming. [This would sav= e anyone > > > > > > > working on it from having to lookup what the define was, sinc= e it's > > > > > > > always e.g. RTE_DEBUG_ + library-base-name, e.g. RTE_DEBUG_L= PM, > > > > > > > RTE_DEBUG_SCHED etc] > > > > > > > > > > > > > > Theoretically we can also do both, have the standard ones def= ined and > > > > > > > then allow a component to provide extra flags itself if so de= sired. > > > > > > > > > > > > > > /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 de= bug 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 fro= m 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 m= eson > > > > > > object and written then later to rte_build_config.h before comp= ilation > > > > > > stage. All the other modules will be able to use these flags. > > > > > > > > > > > Sounds good to me (obviously!), but I'd like other feedback to en= sure > > > > > others are ok with this before you spend too much effort implemen= ting 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. i= n 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 maintai= n > > > > > 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 facilitie= s > > > > 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 ha= ve 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. =20 Approach with rte_log_can_log() seems workable, but right now these patches don't implement it.=20 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 lib= s 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? >=20 > > > 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 onl= y 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 perf= ormance? > > > I think that would be wise, yes. We can call the option "rte_debug" or > something instead. >=20 > /Bruce