From mboxrd@z Thu Jan 1 00:00:00 1970
Return-Path: <dev-bounces@dpdk.org>
Received: from dpdk.org (dpdk.org [92.243.14.124])
by inbox.dpdk.org (Postfix) with ESMTP id D751BA00C2;
Wed, 22 Apr 2020 13:02:51 +0200 (CEST)
Received: from [92.243.14.124] (localhost [127.0.0.1])
by dpdk.org (Postfix) with ESMTP id 900AD1D57C;
Wed, 22 Apr 2020 13:02:50 +0200 (CEST)
Received: from new1-smtp.messagingengine.com (new1-smtp.messagingengine.com
[66.111.4.221]) by dpdk.org (Postfix) with ESMTP id D14F01D563
for <dev@dpdk.org>; Wed, 22 Apr 2020 13:02:48 +0200 (CEST)
Received: from compute7.internal (compute7.nyi.internal [10.202.2.47])
by mailnew.nyi.internal (Postfix) with ESMTP id 2E499580589;
Wed, 22 Apr 2020 07:02:48 -0400 (EDT)
Received: from mailfrontend2 ([10.202.2.163])
by compute7.internal (MEProxy); Wed, 22 Apr 2020 07:02:48 -0400
DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=monjalon.net; h=
from:to:cc:subject:date:message-id:in-reply-to:references
:mime-version:content-transfer-encoding:content-type; s=fm1; bh=
ILGvg3fHNxRUMq3RdmTeUxy53xsQSL65v3w9W9WWzXk=; b=OK1s44og92gmZMdh
mKNqRDjl7hiXQC57zz8+QfTLRmKyFl5/IA/ZW1+CMqk/orU2v26wOjvFIxiaSHyt
IVfrAECt9U8hNZjmE70hE+wX5D+WAMMeEzO82hFVUPnjMG5ttVGl3/pby3vkyZsW
3DEOT6xa3JphRb/XodhcJ40A4KDmgBpYL9bi6HSlSYscQkLSLF8yaE7HEEocz3z9
v9emt0ZDiy3f4Iym5ACrdgee3ZeZ4EtqwJFiEO+h6FQJDruSNGDWgGSBwwXQRs6g
pPTovRQ7M839KpQXxcPHdBfcP/kY4qBPuO1forKYi6s0U9UqU7AmM57SVUEyryC1
7oOqvg==
DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=
messagingengine.com; h=cc:content-transfer-encoding:content-type
:date:from:in-reply-to:message-id:mime-version:references
:subject:to:x-me-proxy:x-me-proxy:x-me-sender:x-me-sender
:x-sasl-enc; s=fm2; bh=ILGvg3fHNxRUMq3RdmTeUxy53xsQSL65v3w9W9WWz
Xk=; b=qBNO1cOzJQZGNrv3JoPXd4W1kZLrblD8CVwVoGNQWGrSmTi8y7uyTS+8m
fH/QkTrARnahOz/9fwU83wkfW7qNHbm1iA/dcJ/ygQ/UKp4ae45fPRouhlxMkvLb
b4/BE1pzr88a2mMa8fZK65Jzyr8xsFR65AwIMT6SZUFCVfLPmt0fbeHAURAv29OI
Bqx9tRD63Qov2EeEUY5h7byoG9+th1vDBY5IO8VoE/9h87Gpl8FOOJtgyM8vObAq
Pp97iUoem7pAi89nxV/EyQdmfZrzvAtcqqJG642kdH9+EtHF5UrXlFQ75ADeWwIs
4QH5KR+Dnn6cQBIzNg1qS2rAxmZCg==
X-ME-Sender: <xms:ViSgXj25qBikfuZYa6FGwdIkF9bQYFhNBw5GJIGuMp4Z3_MHdMolJA>
X-ME-Proxy-Cause: gggruggvucftvghtrhhoucdtuddrgeduhedrgeejgdeffecutefuodetggdotefrodftvf
curfhrohhfihhlvgemucfhrghsthforghilhdpqfgfvfdpuffrtefokffrpgfnqfghnecu
uegrihhlohhuthemuceftddtnecusecvtfgvtghiphhivghnthhsucdlqddutddtmdenuc
fjughrpefhvffufffkjghfggfgtgesthfuredttddtvdenucfhrhhomhepvfhhohhmrghs
ucfoohhnjhgrlhhonhcuoehthhhomhgrshesmhhonhhjrghlohhnrdhnvghtqeenucfkph
epjeejrddufeegrddvtdefrddukeegnecuvehluhhsthgvrhfuihiivgeptdenucfrrghr
rghmpehmrghilhhfrhhomhepthhhohhmrghssehmohhnjhgrlhhonhdrnhgvth
X-ME-Proxy: <xmx:ViSgXt9_ZLd0yfJR4e4R0CkmEtCGGo5eR0d47SrVgdzOYvO816Liiw>
<xmx:ViSgXg7nC3RUZ9ft06kWIWlMw2-GQaiHobI2aMqzDWYKjTlMplDsDg>
<xmx:ViSgXttfwfzgbTrQcwPKI7hChOxKE3ehTLSwlK57THR6Juf1ij87QQ>
<xmx:WCSgXpqofQNgWHFurrUG4qwiB7q6uMDGdjeGkrf4xBdncyZL9LwOvg>
Received: from xps.localnet (184.203.134.77.rev.sfr.net [77.134.203.184])
by mail.messagingengine.com (Postfix) with ESMTPA id 9F0D63065CBE;
Wed, 22 Apr 2020 07:02:43 -0400 (EDT)
From: Thomas Monjalon <thomas@monjalon.net>
To: Lukasz Wojciechowski <l.wojciechow@partner.samsung.com>,
Bruce Richardson <bruce.richardson@intel.com>
Cc: "Ananyev, Konstantin" <konstantin.ananyev@intel.com>, "Dumitrescu,
Cristian" <cristian.dumitrescu@intel.com>,
Igor Russkikh <igor.russkikh@aquantia.com>,
Pavel Belous <pavel.belous@aquantia.com>, "Lu,
Wenzhuo" <wenzhuo.lu@intel.com>, Marcin Wojtas <mw@semihalf.com>,
Michal Krawczyk <mk@semihalf.com>, Guy Tzalik <gtzalik@amazon.com>,
Evgeny Schemeilin <evgenys@amazon.com>,
Igor Chauskin <igorch@amazon.com>, John Daley <johndale@cisco.com>,
Hyong Youb Kim <hyonkim@cisco.com>, "Zhang, Qi Z" <qi.z.zhang@intel.com>,
"Wang, Xiao W" <xiao.w.wang@intel.com>,
Ziyang Xuan <xuanziyang2@huawei.com>,
Xiaoyun Wang <cloud.wangxiaoyun@huawei.com>,
Guoyang Zhou <zhouguoyang@huawei.com>,
"Wei Hu (Xavier)" <xavier.huwei@huawei.com>,
"Min Hu (Connor)" <humin29@huawei.com>,
Yisen Zhuang <yisen.zhuang@huawei.com>, "Xing,
Beilei" <beilei.xing@intel.com>, "Wu, Jingjing" <jingjing.wu@intel.com>,
"Yang, Qiming" <qiming.yang@intel.com>, Rasesh Mody <rmody@marvell.com>,
Shahed Shaikh <shshaikh@marvell.com>, "Singh,
Jasvinder" <jasvinder.singh@intel.com>,
Maxime Coquelin <maxime.coquelin@redhat.com>, "Wang,
Zhihong" <zhihong.wang@intel.com>, "Ye,
Xiaolong" <xiaolong.ye@intel.com>, Yong Wang <yongwang@vmware.com>,
"Yigit, Ferruh" <ferruh.yigit@intel.com>,
Andrew Rybchenko <arybchenko@solarflare.com>,
Olivier Matz <olivier.matz@6wind.com>, "dev@dpdk.org" <dev@dpdk.org>
Date: Wed, 22 Apr 2020 13:02:42 +0200
Message-ID: <26026576.gRfpFWEtPU@thomas>
In-Reply-To: <20200422105519.GB1402@bricha3-MOBL.ger.corp.intel.com>
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>
MIME-Version: 1.0
Content-Transfer-Encoding: 7Bit
Content-Type: text/plain; charset="us-ascii"
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 <dev.dpdk.org>
List-Unsubscribe: <https://mails.dpdk.org/options/dev>,
<mailto:dev-request@dpdk.org?subject=unsubscribe>
List-Archive: <http://mails.dpdk.org/archives/dev/>
List-Post: <mailto:dev@dpdk.org>
List-Help: <mailto:dev-request@dpdk.org?subject=help>
List-Subscribe: <https://mails.dpdk.org/listinfo/dev>,
<mailto:dev-request@dpdk.org?subject=subscribe>
Errors-To: dev-bounces@dpdk.org
Sender: "dev" <dev-bounces@dpdk.org>
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.
> > >> 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
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(...)