From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mails.dpdk.org (mails.dpdk.org [217.70.189.124]) by inbox.dpdk.org (Postfix) with ESMTP id E20464246D; Mon, 23 Jan 2023 18:29:06 +0100 (CET) Received: from mails.dpdk.org (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id D12BA40E5A; Mon, 23 Jan 2023 18:29:06 +0100 (CET) Received: from NAM10-DM6-obe.outbound.protection.outlook.com (mail-dm6nam10on2045.outbound.protection.outlook.com [40.107.93.45]) by mails.dpdk.org (Postfix) with ESMTP id 31ABD400EF for ; Mon, 23 Jan 2023 18:29:05 +0100 (CET) ARC-Seal: i=1; a=rsa-sha256; s=arcselector9901; d=microsoft.com; cv=none; b=ZjJrLHrHT7bg07TRWx9jTn93q43H6U+NvaV4sU3Nl8D6c5qcb/MDVGC5ULXM8tKOz9pKK132rllQ3u0hqm4BV1l3GiTowsytwDuWOjtvIyxpg5KRIlEhAD2Ab+Eqhks6JDZQ/tlQfTON1Rli89pGiq1gL6VxSz7r4RsIr32lLEu3ngnT/s1+bRAGfKdn8ATSWUqe1jEFysULFmCXcAc5sq0ZUWRvyXX6+gKJUlfPwLCsXZNbbT2byTTPY+KrTfrOvv39/992ZlV++0Dt/HtRkHQe2IyO24P3Ky+ACJ68070BgymLs3kg+vm/AcITvEa3Ht4VzcZb+wDaxeYPDiUk2w== 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-AntiSpam-MessageData-ChunkCount:X-MS-Exchange-AntiSpam-MessageData-0:X-MS-Exchange-AntiSpam-MessageData-1; bh=362MD8vN2qn0CDS6KQbfU8Whi23bQTj+uRrs4YiGKwg=; b=JoBVdmeAO9qfF8a+2eXk1FXvIS+R/CjrnxleKjuU+qMffbR1wtnGYu7wHgbOx7u8PMMMhnqCXnBw+0IkQ/avesrpqQkcLozhr9wA3VEZoZLm4mMSDZhz648F452d8DooNru5zAOFx2WMuIi+ASnOtL24tiSuXHSBL+e1mmB6QsdB0dHLOh6P0grVQlDPYO7gIgFGDR20N5TyigXI1ly6PaL6fzofNSKvdxLSi2KRWBGBXJTs1ovR/J2q8hUmCDvEMZGPUkxH//8a95+WAFKF7qiS0XSThkxTvZb0vTD9iXCsR3xGduR+x8LH0MqfbkrSIKyrPLst1X7wd7bBqzQ/Ag== ARC-Authentication-Results: i=1; mx.microsoft.com 1; spf=pass smtp.mailfrom=amd.com; dmarc=pass action=none header.from=amd.com; dkim=pass header.d=amd.com; arc=none DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=amd.com; s=selector1; h=From:Date:Subject:Message-ID:Content-Type:MIME-Version:X-MS-Exchange-SenderADCheck; bh=362MD8vN2qn0CDS6KQbfU8Whi23bQTj+uRrs4YiGKwg=; b=FVT0/n84zm4iuDbrqTvQiwJWKtTTOFMTFIDbMpuIOCddFfljUKLEryilxnEnw13H+qglE82jenxGIw6O4fAB0+NXjNKaK4IfsYrOXNyR06b4atRaJyj/P5AAtPVeACUfQi/8wWdSlcjS2NaNLBMsLqGZ72jHmYdVP3twUvFQf1g= Authentication-Results: dkim=none (message not signed) header.d=none;dmarc=none action=none header.from=amd.com; Received: from CH2PR12MB4294.namprd12.prod.outlook.com (2603:10b6:610:a9::11) by DM4PR12MB5149.namprd12.prod.outlook.com (2603:10b6:5:390::14) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id 15.20.6002.33; Mon, 23 Jan 2023 17:29:02 +0000 Received: from CH2PR12MB4294.namprd12.prod.outlook.com ([fe80::4807:1f44:5e04:e05a]) by CH2PR12MB4294.namprd12.prod.outlook.com ([fe80::4807:1f44:5e04:e05a%7]) with mapi id 15.20.6002.033; Mon, 23 Jan 2023 17:29:02 +0000 Message-ID: <614e22c6-8485-0e8d-742e-b3d100f96468@amd.com> Date: Mon, 23 Jan 2023 17:28:46 +0000 User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:102.0) Gecko/20100101 Thunderbird/102.6.1 From: Ferruh Yigit Subject: Re: [PATCH v6 2/6] ethdev: add trace points for ethdev (part one) To: Ankur Dwivedi , dev@dpdk.org, David Marchand Cc: thomas@monjalon.net, david.marchand@redhat.com, mdr@ashroe.eu, orika@nvidia.com, chas3@att.com, humin29@huawei.com, linville@tuxdriver.com, ciara.loftus@intel.com, qi.z.zhang@intel.com, mw@semihalf.com, mk@semihalf.com, shaibran@amazon.com, evgenys@amazon.com, igorch@amazon.com, chandu@amd.com, irusskikh@marvell.com, shepard.siegel@atomicrules.com, ed.czeck@atomicrules.com, john.miller@atomicrules.com, ajit.khaparde@broadcom.com, somnath.kotur@broadcom.com, jerinj@marvell.com, mczekaj@marvell.com, sthotton@marvell.com, srinivasan@marvell.com, hkalra@marvell.com, rahul.lakkireddy@chelsio.com, johndale@cisco.com, hyonkim@cisco.com, liudongdong3@huawei.com, yisen.zhuang@huawei.com, xuanziyang2@huawei.com, cloud.wangxiaoyun@huawei.com, zhouguoyang@huawei.com, simei.su@intel.com, wenjun1.wu@intel.com, qiming.yang@intel.com, Yuying.Zhang@intel.com, beilei.xing@intel.com, xiao.w.wang@intel.com, jingjing.wu@intel.com, junfeng.guo@intel.com, rosen.xu@intel.com, ndabilpuram@marvell.com, kirankumark@marvell.com, skori@marvell.com, skoteshwar@marvell.com, lironh@marvell.com, zr@semihalf.com, radhac@marvell.com, vburru@marvell.com, sedara@marvell.com, matan@nvidia.com, viacheslavo@nvidia.com, longli@microsoft.com, spinler@cesnet.cz, chaoyong.he@corigine.com, niklas.soderlund@corigine.com, hemant.agrawal@nxp.com, sachin.saxena@oss.nxp.com, g.singh@nxp.com, apeksha.gupta@nxp.com, sachin.saxena@nxp.com, aboyer@pensando.io, rmody@marvell.com, shshaikh@marvell.com, dsinghrawat@marvell.com, andrew.rybchenko@oktetlabs.ru, jiawenwu@trustnetic.com, jianwang@trustnetic.com, jbehrens@vmware.com, maxime.coquelin@redhat.com, chenbo.xia@intel.com, steven.webster@windriver.com, matt.peters@windriver.com, bruce.richardson@intel.com, mtetsuyah@gmail.com, grive@u256.net, jasvinder.singh@intel.com, cristian.dumitrescu@intel.com, jgrajcia@cisco.com, mb@smartsharesystems.com References: <20230112112140.807233-1-adwivedi@marvell.com> <20230120084059.2926575-1-adwivedi@marvell.com> <20230120084059.2926575-3-adwivedi@marvell.com> Content-Language: en-US In-Reply-To: <20230120084059.2926575-3-adwivedi@marvell.com> Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 7bit X-ClientProxiedBy: LO2P265CA0263.GBRP265.PROD.OUTLOOK.COM (2603:10a6:600:8a::35) To CH2PR12MB4294.namprd12.prod.outlook.com (2603:10b6:610:a9::11) MIME-Version: 1.0 X-MS-PublicTrafficType: Email X-MS-TrafficTypeDiagnostic: CH2PR12MB4294:EE_|DM4PR12MB5149:EE_ X-MS-Office365-Filtering-Correlation-Id: 3698b8c7-d59d-403a-5e13-08dafd6751cb X-LD-Processed: 3dd8961f-e488-4e60-8e11-a82d994e183d,ExtAddr X-MS-Exchange-SenderADCheck: 1 X-MS-Exchange-AntiSpam-Relay: 0 X-Microsoft-Antispam: BCL:0; X-Microsoft-Antispam-Message-Info: BY8qxZ7c0cAUXIe3tncFzTx1f0/tywwQAJ7EYsYEGSlcRCtYO6006l7WGQ4pZMXAHfJKxjbCRx55OA96Lsc/IJh2dAj6Q+Dz4N5tGU4GarcYBdwwqsrp06z8Wwo7TEyXhF6Sg3OrZNagiA93bTzmGW9m7yerggo/qbXA1BVXNyNO+6j7UguEZqJu5W6BAz7tqtHyJAz9H9/DtAtEzmhlGwNiz9ybwi20b6YKIJTZFkAgz8USgEUqV2HBLL2QBUPkQzrfvYvL/GHU0p4oP7p26EoVzX5xLxJpOIDAXsro466+NvjSrWFyoiD3xTf1cT6qCzUTCQuU7Xp8XierjFEEnFyrFL7NWxFUT0IpwXtJeuUDFhda6aNyDrdhcxo/m65UEAeI6njzn/C1n1SXCAOrP380gh5+B7ms21zVzCfe/Tjd27j4iE4PC+mrKw0tjPaZpbmZjEIzqU50ldJhDcBJwvZfKPbq/eCshW/ahb1+UgYyoMvV2EF5nCwOAN4OqAEZaodg7Kn8zvhrXSAG9rrLpqddbNtlULWd67oNtD4LBoSC/R5GQRX0LILZfr+VUf10tWeZFya5Y8UhAY9GOhj4DSa+QrrHidEXthWdbjqg1wMHRLTXh8nm3OjF4QedG7t8kkYP9uwrU0I25mTdQYPix2VGJsj31ysGNOljx6V6QdKtn1SswqvVXNmgh9V3RNTc6Nlo5Hq+voFfzbJBzdtgYGBstzo8q6B+A1e5SBJiHTlxY8odyC1xrctzF2kDeV+0 X-Forefront-Antispam-Report: CIP:255.255.255.255; CTRY:; LANG:en; SCL:1; SRV:; IPV:NLI; SFV:NSPM; H:CH2PR12MB4294.namprd12.prod.outlook.com; PTR:; CAT:NONE; SFS:(13230022)(4636009)(346002)(136003)(376002)(396003)(366004)(39860400002)(451199015)(31686004)(53546011)(31696002)(86362001)(2906002)(7366002)(44832011)(7336002)(7406005)(7416002)(30864003)(8936002)(38100700002)(36756003)(5660300002)(110136005)(83380400001)(6512007)(186003)(26005)(6666004)(6506007)(316002)(478600001)(6486002)(41300700001)(66946007)(66556008)(4326008)(8676002)(66476007)(2616005)(21314003)(45980500001)(43740500002); DIR:OUT; SFP:1101; X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1 X-MS-Exchange-AntiSpam-MessageData-0: =?utf-8?B?N1JqWnZ1QUQvK25nbEx2blMxc0xqZTBicTZvUmZreHZjTmtNVXArQWdoQW4z?= =?utf-8?B?SENPTzlNb2lScFcxWEhDdzh4ZTlJOTBzTnNrQXJJUVp3T0gvV0FSVUFUMWRQ?= =?utf-8?B?R09tSzh5U2U3Y2hYRzJxT1pQdjc2TFFTc29WTlpyc0hTdVAxM3FZOXJJaGtI?= =?utf-8?B?RENSdnBXaDlqZUl1bGZJZnZwenI3TEltNUZEZ01QNlpWUVBOb2x2NXpHdTN5?= =?utf-8?B?OUsvTmNyRGFVdGxXbDFSRlRmWitNNkVXSmhGakkrVS9UL1BOODhFRGdtdVhJ?= =?utf-8?B?WFRrZjBqczVRVlA3cXV3cE9aYVBId2FOdjFVdDRXMU0wclg4emZBakgzUFV0?= =?utf-8?B?Y3ZMck9lZEg1T081K1NhR0Z5Z0tNdHUxa0F3bGR6OTVuTm5HbTFGTWliWjF3?= =?utf-8?B?SUdUVjhpcjVSQVdSYW5NYmxidThscWNjM2RLdVI0NXVPRTBTQitnZUtMNGRm?= =?utf-8?B?KzZWc3VsN2NXc28yZE9ic1pMQ204UHlLWU9Ca3VIbzNnMzNiRm9mYzJwMmxU?= =?utf-8?B?OFpqbFMrQkcveC95WDZQNGFncytHeTk5MW9jaEdybGVHT3VEajZWdGphS2Ra?= =?utf-8?B?ck5LSWlveDhvaHRFMkVHOWE0ZXVzT1ppV1lRVXVzbDZSWnc1WGNpcHhoTW1q?= =?utf-8?B?bGRRS29ZUGMvZHkrY3RTbGlBZHF5MmFOK2p1anFmZlRqUTZkcEYrcERERUhM?= =?utf-8?B?ZCt3NXpNV2IyZGR3VWxUalJoWHpudDdkWDU2clRGUWVDVktvVzN6WDA1NW05?= =?utf-8?B?d1B3Y296aEZoUlRsRktIUmlIRkVHNFZwTXFYQk4rS0FnVVU2Qjc5b05GWWpl?= =?utf-8?B?dXRtWWlkV3FNSFJwMEc5ditBS1JxRUtrakJmL0ZGR0MvYVhKaEVDbmlPWWtH?= =?utf-8?B?eFNybHJscndPSUV2Yk9vOXJUT0poUzc5SndTU3VFQ0VwVGZqZlpoNUZ6dE5I?= =?utf-8?B?TzNyYW0zUXdmR3M0VDd6WDdKRlBONFlHYU9GdzV3UVhwZkIvb1dRMHRWMnZx?= =?utf-8?B?TFRYSHhNbDI5Z3pIQ25SSTlXVkdKRUJpSkNJQ1RNN2hERVRpVEdpblRBM050?= =?utf-8?B?NXRDcy9OTEJISk8xL0ZiWE9MalJmZjdITWNvejRpNDlJdTNUcG1aT0xscjB2?= =?utf-8?B?L3dVbUFubVBnWXhEWnFqd1VCU0Q1cXVZRytMSVhsNWZUVGJRdk5RSkdjYkJQ?= =?utf-8?B?alRudzF2L1kzNXBxQVdkNXNCTVBadXhUYm5OeXJMQWNOZDM5RWhyMThJdVY2?= =?utf-8?B?dE9rdlp1Mld6bFloMlVJaGRLTFNwRmVuMTQwRkJxQmU1KzNsWitNck01aU9k?= =?utf-8?B?aVVlNWxtdmNqK3J6V0VZSzdhSWZkSWNCRW01V3JXR05SOHQ0WW5pdkd0N2h4?= =?utf-8?B?ZTZaUURzRjB3MmJMRjNGYnlCUSs5Y3h1NEdlcGlCNURBZ2dBdG9lOWRlOEtH?= =?utf-8?B?YUZQZElEajEvT0NJd2ZtTnJxaGFoQXhxL1dhaGhDcWFiUGlQN1Z4WXlMZXdY?= =?utf-8?B?VlBPblZ1aVUwNVkxN0wxR1l5UEZjcEVoZkpjTVhhdHlxU2JXazB3elE0eERq?= =?utf-8?B?UWtRSnNrbU83bHliRWE3V3RSd09ibHhEcThZdnVuVmNuS1p1VG5ENkx6eWIz?= =?utf-8?B?N3pqNEdrYktqNSt2THdQeUtFbjU2UGtVTzgySFRjRy9MQndFOXljckVSejJh?= =?utf-8?B?cWtnalNpT0hCUXdITG13TFFqTWlmb1FSb1pWNnphR3VQUksvOHBkNWQ5U2Yr?= =?utf-8?B?MXpOd0dHblhiNGplL3Z1KzZsblhIVHJlbjZsR0pTRG9xVHh3QW1vb3h1a202?= =?utf-8?B?bEY1YXhtNlVPVGw2aTlNbXRheXgybnJHNVdKY3l4YnZDTWs1TnlBOXk5L3BJ?= =?utf-8?B?RXJmSy9ISi9kUGtGdldBMGVqbk4zNkJzWG4zcjBBMmNpZHBWU2FEd2E3MFVE?= =?utf-8?B?M0dTajFUMDFTN3BSb2J1NTYwMjhmWlFQUWVUSXl1TDcvc2lGaE4ySmZONC96?= =?utf-8?B?NUY1QTdwSitoNHljNFRUcU15SFVtdEp2Z1UzWVF4QlNaeGFaRnAwRG9xSndZ?= =?utf-8?B?WjNLV0hQVHZpTmxENnpONHJ3ZkJhek12WTFlTU90V1hoKzRqTkFUNjBqUGov?= =?utf-8?Q?y8eL/SNBt4qsHZ5wTDLak4j+v?= X-OriginatorOrg: amd.com X-MS-Exchange-CrossTenant-Network-Message-Id: 3698b8c7-d59d-403a-5e13-08dafd6751cb X-MS-Exchange-CrossTenant-AuthSource: CH2PR12MB4294.namprd12.prod.outlook.com X-MS-Exchange-CrossTenant-AuthAs: Internal X-MS-Exchange-CrossTenant-OriginalArrivalTime: 23 Jan 2023 17:29:02.5055 (UTC) X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted X-MS-Exchange-CrossTenant-Id: 3dd8961f-e488-4e60-8e11-a82d994e183d X-MS-Exchange-CrossTenant-MailboxType: HOSTED X-MS-Exchange-CrossTenant-UserPrincipalName: gxp8PnjcHedbhlyXQ2e3rV53UukqYmpvNhZIYV1gk56StbQ1vaSGYr/DI6vGjVur X-MS-Exchange-Transport-CrossTenantHeadersStamped: DM4PR12MB5149 X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org On 1/20/2023 8:40 AM, Ankur Dwivedi wrote: > Adds trace points for ethdev functions. > Moved the rte_ethdev_trace_rx_burst and rte_ethdev_trace_tx_burst to > a new file rte_ethdev_trace_fp_burst.h. This is needed to resolve > cyclic dependency between rte_ethdev.h and rte_ethdev_trace_fp.h. > > Signed-off-by: Ankur Dwivedi <...> > +RTE_TRACE_POINT_REGISTER(rte_eth_trace_rx_hairpin_queue_setup, > + lib.ethdev.rx.hairpin_queue_setup) > + s/rx.hairpin_queue_setup/rx_hairpin_queue_setup/ ? > +RTE_TRACE_POINT_REGISTER(rte_eth_trace_tx_hairpin_queue_setup, > + lib.ethdev.tx.hairpin_queue_setup) > + s/tx.hairpin_queue_setup/tx_hairpin_queue_setup/ ? <...> > diff --git a/lib/ethdev/meson.build b/lib/ethdev/meson.build > index 39250b5da1..f5c0865023 100644 > --- a/lib/ethdev/meson.build > +++ b/lib/ethdev/meson.build > @@ -24,6 +24,7 @@ headers = files( > 'rte_ethdev.h', > 'rte_ethdev_trace.h', > 'rte_ethdev_trace_fp.h', > + 'rte_ethdev_trace_fp_burst.h', Why these trace headers are public? Aren't trace points only used by the APIs, so I expect them to be internal, so applications shouldn't need them. Why they are expsed to user. @David, what do you think? <...> > @@ -258,6 +259,7 @@ rte_eth_iterator_init(struct rte_dev_iterator *iter, const char *devargs_str) > > end: > iter->cls = rte_class_find_by_name("eth"); > + rte_eth_trace_iterator_init(devargs_str); > rte_devargs_reset(&devargs); > return 0; Why not add trace call just before return, as a separate block, like: `` end: iter->cls = rte_class_find_by_name("eth"); rte_devargs_reset(&devargs); rte_eth_trace_iterator_init(devargs_str); return 0; `` > > @@ -274,6 +276,8 @@ rte_eth_iterator_init(struct rte_dev_iterator *iter, const char *devargs_str) > uint16_t > rte_eth_iterator_next(struct rte_dev_iterator *iter) > { > + uint16_t id; > + > if (iter == NULL) { > RTE_ETHDEV_LOG(ERR, > "Cannot get next device from NULL iterator\n"); > @@ -297,8 +301,11 @@ rte_eth_iterator_next(struct rte_dev_iterator *iter) > /* A device is matching bus part, need to check ethdev part. */ > iter->class_device = iter->cls->dev_iterate( > iter->class_device, iter->cls_str, iter); > - if (iter->class_device != NULL) > - return eth_dev_to_id(iter->class_device); /* match */ > + if (iter->class_device != NULL) { > + id = eth_dev_to_id(iter->class_device); > + rte_eth_trace_iterator_next(iter, id); > + return id; /* match */ please move 'id' declaration within the if block, and again put trace call into separate block to highlight it, otherwise easy to miss, like: `` if (iter->class_device != NULL) { uint16_t id = eth_dev_to_id(iter->class_device); rte_eth_trace_iterator_next(iter, id); return id; /* match */ } > + } > } while (iter->bus != NULL); /* need to try next rte_device */ > > /* No more ethdev port to iterate. */ > @@ -316,6 +323,7 @@ rte_eth_iterator_cleanup(struct rte_dev_iterator *iter) > > if (iter->bus_str == NULL) > return; /* nothing to free in pure class filter */ > + rte_eth_trace_iterator_cleanup(iter); Can you please make trace call a separate block, I won't comment same for bellow, can you please apply this to all. > free(RTE_CAST_FIELD(iter, bus_str, char *)); /* workaround const */ > free(RTE_CAST_FIELD(iter, cls_str, char *)); /* workaround const */ > memset(iter, 0, sizeof(*iter)); > @@ -324,12 +332,18 @@ rte_eth_iterator_cleanup(struct rte_dev_iterator *iter) > uint16_t > rte_eth_find_next(uint16_t port_id) > { > + rte_eth_trace_find_next(port_id); > + Why tracing previous port_id, and other one below records next port_id, won't it be confusing to have both with same name. I suggest just keep last one, (next port_id). > while (port_id < RTE_MAX_ETHPORTS && > rte_eth_devices[port_id].state == RTE_ETH_DEV_UNUSED) > port_id++; > > - if (port_id >= RTE_MAX_ETHPORTS) > + if (port_id >= RTE_MAX_ETHPORTS) { > + rte_eth_trace_find_next(RTE_MAX_ETHPORTS); Is there a specific reason to trace all paths, why not just keep the last one? > return RTE_MAX_ETHPORTS; > + } > + > + rte_eth_trace_find_next(port_id); > > return port_id; > } > @@ -347,10 +361,15 @@ uint16_t > rte_eth_find_next_of(uint16_t port_id, const struct rte_device *parent) > { > port_id = rte_eth_find_next(port_id); > + > + rte_eth_trace_find_next_of(port_id); > + > while (port_id < RTE_MAX_ETHPORTS && > rte_eth_devices[port_id].device != parent) > port_id = rte_eth_find_next(port_id + 1); > > + rte_eth_trace_find_next_of(port_id); > + Same here, lets keep only the last one. > return port_id; > } > > @@ -358,6 +377,9 @@ uint16_t > rte_eth_find_next_sibling(uint16_t port_id, uint16_t ref_port_id) > { > RTE_ETH_VALID_PORTID_OR_ERR_RET(ref_port_id, RTE_MAX_ETHPORTS); > + > + rte_eth_trace_find_next_sibling(port_id, ref_port_id); > + This doesn't record the return values, function should be updated to keep the interim return value of 'rte_eth_find_next_of()' and trace functions should record it. > return rte_eth_find_next_of(port_id, > rte_eth_devices[ref_port_id].device); > } > @@ -372,10 +394,13 @@ int > rte_eth_dev_is_valid_port(uint16_t port_id) > { > if (port_id >= RTE_MAX_ETHPORTS || > - (rte_eth_devices[port_id].state == RTE_ETH_DEV_UNUSED)) > + (rte_eth_devices[port_id].state == RTE_ETH_DEV_UNUSED)) { > + rte_ethdev_trace_is_valid_port(port_id, 0); > return 0; > - else > + } else { > + rte_ethdev_trace_is_valid_port(port_id, 1); > return 1; > + } What about to create an interim 'is_valid' variable and record it with single trace call? <...> > uint32_t > rte_eth_speed_bitflag(uint32_t speed, int duplex) > { > + rte_eth_trace_speed_bitflag(speed, duplex); Let's create an interim return value and record it for the trace function, and please move trace function to the bottom of the function. <...> > @@ -2433,6 +2529,7 @@ void > rte_eth_tx_buffer_drop_callback(struct rte_mbuf **pkts, uint16_t unsent, > void *userdata __rte_unused) > { > + rte_eth_trace_tx_buffer_drop_callback(pkts, unsent); Since only pointer value is recorded, function can be moved down, please put emtpy line in between. <...> > @@ -2495,7 +2598,12 @@ rte_eth_tx_done_cleanup(uint16_t port_id, uint16_t queue_id, uint32_t free_cnt) > /* Call driver to free pending mbufs. */ > ret = (*dev->dev_ops->tx_done_cleanup)(dev->data->tx_queues[queue_id], > free_cnt); > - return eth_err(port_id, ret); > + > + ret = eth_err(port_id, ret); Please don't add new empty line _before_ this functions. <...> > @@ -2700,6 +2834,8 @@ rte_eth_link_to_str(char *str, size_t len, const struct rte_eth_link *eth_link) > return -EINVAL; > } > > + rte_eth_trace_link_to_str(len, eth_link); > + Why not record return value and move trace function to the end of the function and record 'str' too. > if (eth_link->link_status == RTE_ETH_LINK_DOWN) > return snprintf(str, len, "Link down"); > else > @@ -2715,6 +2851,7 @@ int > rte_eth_stats_get(uint16_t port_id, struct rte_eth_stats *stats) > { > struct rte_eth_dev *dev; > + int ret; > > RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -ENODEV); > dev = &rte_eth_devices[port_id]; > @@ -2730,7 +2867,12 @@ rte_eth_stats_get(uint16_t port_id, struct rte_eth_stats *stats) > if (*dev->dev_ops->stats_get == NULL) > return -ENOTSUP; > stats->rx_nombuf = dev->data->rx_mbuf_alloc_failed; > - return eth_err(port_id, (*dev->dev_ops->stats_get)(dev, stats)); > + > + ret = eth_err(port_id, (*dev->dev_ops->stats_get)(dev, stats)); Pleaes don't add empyt line above. <...> > @@ -3229,13 +3384,19 @@ int > rte_eth_xstats_reset(uint16_t port_id) > { > struct rte_eth_dev *dev; > + int ret; > > RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -ENODEV); > dev = &rte_eth_devices[port_id]; > > /* implemented by the driver */ > - if (dev->dev_ops->xstats_reset != NULL) > - return eth_err(port_id, (*dev->dev_ops->xstats_reset)(dev)); > + if (dev->dev_ops->xstats_reset != NULL) { > + ret = eth_err(port_id, (*dev->dev_ops->xstats_reset)(dev)); Can you please move 'ret' decleration in if block. `` int ret = eth_err(port_id, (*dev->dev_ops->xstats_reset)(dev)); `` > + > + rte_eth_trace_xstats_reset(port_id, ret); > + > + return ret; > + } > > /* fallback to default */ > return rte_eth_stats_reset(port_id); > @@ -3268,24 +3429,37 @@ int > rte_eth_dev_set_tx_queue_stats_mapping(uint16_t port_id, uint16_t tx_queue_id, > uint8_t stat_idx) > { > - return eth_err(port_id, eth_dev_set_queue_stats_mapping(port_id, > + int ret; > + > + ret = eth_err(port_id, eth_dev_set_queue_stats_mapping(port_id, > tx_queue_id, > stat_idx, STAT_QMAP_TX)); > + > + rte_ethdev_trace_set_tx_queue_stats_mapping(port_id, tx_queue_id, stat_idx, ret); > + In below function 'rte_ethdev_trace_set_rx_queue_stats_mapping()' call wrapped to fit 80 lines, but this one not, please be consistent and if possible break both to fit as done below. <...> > +RTE_TRACE_POINT( > + rte_eth_trace_iterator_next, > + RTE_TRACE_POINT_ARGS(struct rte_dev_iterator *iter, uint16_t id), > + rte_trace_point_emit_ptr(iter); > + rte_trace_point_emit_string(iter->bus_str); > + rte_trace_point_emit_string(iter->cls_str); > + rte_trace_point_emit_u16(id); > +) > + Trace functions don't chage parameters, what do you think to update all parameters with 'const' keywords for this: `` RTE_TRACE_POINT( rte_eth_trace_iterator_next, RTE_TRACE_POINT_ARGS(const struct rte_dev_iterator *iter, uint16_t id), rte_trace_point_emit_ptr(iter); rte_trace_point_emit_string(iter->bus_str); rte_trace_point_emit_string(iter->cls_str); rte_trace_point_emit_u16(id); ) `` This comment is not just for this trace point, but for *all* trace points. <...> > +RTE_TRACE_POINT( > + rte_eth_trace_rx_hairpin_queue_setup, > + RTE_TRACE_POINT_ARGS(uint16_t port_id, uint16_t rx_queue_id, > + uint16_t nb_rx_desc, const struct rte_eth_hairpin_conf *conf, > + int ret), > + uint32_t peer_count = conf->peer_count; > + uint32_t tx_explicit = conf->tx_explicit; > + uint32_t manual_bind = conf->manual_bind; > + uint32_t use_locked_device_memory = conf->use_locked_device_memory; > + uint32_t use_rte_memory = conf->use_rte_memory; > + uint32_t force_memory = conf->force_memory; > + > + rte_trace_point_emit_u16(port_id); > + rte_trace_point_emit_u16(rx_queue_id); > + rte_trace_point_emit_u16(nb_rx_desc); > + rte_trace_point_emit_ptr(conf); > + rte_trace_point_emit_u32(peer_count); > + rte_trace_point_emit_u32(tx_explicit); > + rte_trace_point_emit_u32(manual_bind); > + rte_trace_point_emit_u32(use_locked_device_memory); > + rte_trace_point_emit_u32(use_rte_memory); > + rte_trace_point_emit_u32(force_memory); > + rte_trace_point_emit_int(ret); Do we need temporary variables like 'peer_count', why not directly use them: `` rte_trace_point_emit_u32(conf->peer_count); `` > +) > + > +RTE_TRACE_POINT( > + rte_eth_trace_tx_hairpin_queue_setup, > + RTE_TRACE_POINT_ARGS(uint16_t port_id, uint16_t tx_queue_id, > + uint16_t nb_tx_desc, const struct rte_eth_hairpin_conf *conf, > + int ret), > + uint32_t peer_count = conf->peer_count; > + uint32_t tx_explicit = conf->tx_explicit; > + uint32_t manual_bind = conf->manual_bind; > + uint32_t use_locked_device_memory = conf->use_locked_device_memory; > + uint32_t use_rte_memory = conf->use_rte_memory; > + uint32_t force_memory = conf->force_memory; > + > + rte_trace_point_emit_u16(port_id); > + rte_trace_point_emit_u16(tx_queue_id); > + rte_trace_point_emit_u16(nb_tx_desc); > + rte_trace_point_emit_ptr(conf); > + rte_trace_point_emit_u32(peer_count); > + rte_trace_point_emit_u32(tx_explicit); > + rte_trace_point_emit_u32(manual_bind); > + rte_trace_point_emit_u32(use_locked_device_memory); > + rte_trace_point_emit_u32(use_rte_memory); > + rte_trace_point_emit_u32(force_memory); > + rte_trace_point_emit_int(ret); > +) Same as above. <...> > +RTE_TRACE_POINT( > + rte_eth_trace_allmulticast_disable, > + RTE_TRACE_POINT_ARGS(uint16_t port_id, int all_multicast, int diag), > + rte_trace_point_emit_u16(port_id); > + rte_trace_point_emit_int(all_multicast); > + rte_trace_point_emit_int(diag); > +) > + Can you replace 'diag' with 'ret' for consistency, same for '*promiscuous/allmulticast_enable/disable'. <...> > +RTE_TRACE_POINT_FP( > + rte_eth_trace_find_next, > + RTE_TRACE_POINT_ARGS(uint16_t port_id), > + rte_trace_point_emit_u16(port_id); > +) > + Why 'rte_eth_trace_find_next' added as fast path? Can you please add comment for all why it is added as fast path, this help to evaluate/review this later. > +RTE_TRACE_POINT_FP( > + rte_eth_trace_find_next_of, > + RTE_TRACE_POINT_ARGS(uint16_t port_id), > + rte_trace_point_emit_u16(port_id); > +) > + Why not record second parameter of the API, "struct rte_device *parent" too? <...> > +RTE_TRACE_POINT_FP( > + rte_eth_trace_hairpin_get_peer_ports, > + RTE_TRACE_POINT_ARGS(uint16_t port_id, uint16_t *peer_ports, > + size_t len, uint32_t direction, int ret), > + rte_trace_point_emit_u16(port_id); > + rte_trace_point_emit_ptr(peer_ports); > + rte_trace_point_emit_size_t(len); > + rte_trace_point_emit_u32(direction); > + rte_trace_point_emit_int(ret); > +) > + Some of these functions I am not clear why fast path trace point is used, 'rte_eth_trace_hairpin_get_peer_ports' is one of them, can you please comment the justification to the code as suggested above. <...> > +RTE_TRACE_POINT_FP( > + rte_eth_trace_link_get, > + RTE_TRACE_POINT_ARGS(uint16_t port_id, struct rte_eth_link *link), > + uint16_t link_duplex = link->link_duplex; > + uint16_t link_autoneg = link->link_autoneg; > + uint16_t link_status = link->link_status; > + > + rte_trace_point_emit_u16(port_id); > + rte_trace_point_emit_u32(link->link_speed); > + rte_trace_point_emit_u16(link_duplex); > + rte_trace_point_emit_u16(link_autoneg); > + rte_trace_point_emit_u16(link_status); > +) Why creating varible for 'link_duplex' for 'link->link_duplex' but using 'link->link_speed', why not use all as 'link->xxx'? <...> > +RTE_TRACE_POINT_FP( > + rte_eth_trace_xstats_get_names, > + RTE_TRACE_POINT_ARGS(uint16_t port_id, > + struct rte_eth_xstat_name *xstats_names, > + unsigned int size, int cnt_used_entries), > + rte_trace_point_emit_u16(port_id); > + rte_trace_point_emit_string(xstats_names->name); > + rte_trace_point_emit_u32(size); > + rte_trace_point_emit_int(cnt_used_entries); > +) > + 'xstats_names' is an array, whose size is 'cnt_used_entries', just printing 'xstats_names->name' for first element can be misleading, can print all values as done in 'rte_eth_xstats_get()' <...> > +RTE_TRACE_POINT_FP( > + rte_eth_trace_xstats_get, > + RTE_TRACE_POINT_ARGS(uint16_t port_id, struct rte_eth_xstat xstats, > + int i), > + rte_trace_point_emit_u16(port_id); > + rte_trace_point_emit_u64(xstats.id); > + rte_trace_point_emit_u64(xstats.value); > + rte_trace_point_emit_u32(i); > +) > + as far as I can see "id == i", so maybe 'i' can be dropped. <...> > @@ -298,6 +298,72 @@ EXPERIMENTAL { > rte_flow_get_q_aged_flows; > rte_mtr_meter_policy_get; > rte_mtr_meter_profile_get; > + > + # added in 23.03 > + __rte_eth_trace_allmulticast_disable; > + __rte_eth_trace_allmulticast_enable; > + __rte_eth_trace_allmulticast_get; > + __rte_eth_trace_call_rx_callbacks; > + __rte_eth_trace_call_tx_callbacks; > + __rte_eth_trace_find_next; > + __rte_eth_trace_find_next_of; > + __rte_eth_trace_find_next_owned_by; > + __rte_eth_trace_find_next_sibling; > + __rte_eth_trace_hairpin_bind; > + __rte_eth_trace_hairpin_get_peer_ports; > + __rte_eth_trace_hairpin_unbind; > + __rte_eth_trace_iterator_cleanup; > + __rte_eth_trace_iterator_init; > + __rte_eth_trace_iterator_next; > + __rte_eth_trace_link_get; > + __rte_eth_trace_link_get_nowait; > + __rte_eth_trace_link_speed_to_str; > + __rte_eth_trace_link_to_str; > + __rte_eth_trace_promiscuous_disable; > + __rte_eth_trace_promiscuous_enable; > + __rte_eth_trace_promiscuous_get; > + __rte_eth_trace_rx_hairpin_queue_setup; > + __rte_eth_trace_speed_bitflag; > + __rte_eth_trace_stats_get; > + __rte_eth_trace_stats_reset; > + __rte_eth_trace_tx_buffer_count_callback; > + __rte_eth_trace_tx_buffer_drop_callback; > + __rte_eth_trace_tx_buffer_init; > + __rte_eth_trace_tx_buffer_set_err_callback; > + __rte_eth_trace_tx_done_cleanup; > + __rte_eth_trace_tx_hairpin_queue_setup; > + __rte_eth_trace_xstats_get; > + __rte_eth_trace_xstats_get_by_id; > + __rte_eth_trace_xstats_get_id_by_name; > + __rte_eth_trace_xstats_get_names; > + __rte_eth_trace_xstats_get_names_by_id; > + __rte_eth_trace_xstats_reset; > + __rte_ethdev_trace_capability_name; > + __rte_ethdev_trace_count_avail; > + __rte_ethdev_trace_count_total; > + __rte_ethdev_trace_fw_version_get; > + __rte_ethdev_trace_get_name_by_port; > + __rte_ethdev_trace_get_port_by_name; > + __rte_ethdev_trace_get_sec_ctx; > + __rte_ethdev_trace_is_removed; > + __rte_ethdev_trace_is_valid_port; > + __rte_ethdev_trace_owner_delete; > + __rte_ethdev_trace_owner_get; > + __rte_ethdev_trace_owner_new; > + __rte_ethdev_trace_owner_set; > + __rte_ethdev_trace_owner_unset; > + __rte_ethdev_trace_reset; > + __rte_ethdev_trace_rx_offload_name; > + __rte_ethdev_trace_rx_queue_start; > + __rte_ethdev_trace_rx_queue_stop; > + __rte_ethdev_trace_set_link_down; > + __rte_ethdev_trace_set_link_up; > + __rte_ethdev_trace_set_rx_queue_stats_mapping; > + __rte_ethdev_trace_set_tx_queue_stats_mapping; > + __rte_ethdev_trace_socket_id; > + __rte_ethdev_trace_tx_offload_name; > + __rte_ethdev_trace_tx_queue_start; > + __rte_ethdev_trace_tx_queue_stop; Can you please drop these trace objects?