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 DB9A2A0540; Tue, 13 Dec 2022 21:07:10 +0100 (CET) Received: from mails.dpdk.org (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id 7D80D40395; Tue, 13 Dec 2022 21:07:10 +0100 (CET) Received: from NAM12-DM6-obe.outbound.protection.outlook.com (mail-dm6nam12on2057.outbound.protection.outlook.com [40.107.243.57]) by mails.dpdk.org (Postfix) with ESMTP id D932C4021D for ; Tue, 13 Dec 2022 21:07:08 +0100 (CET) ARC-Seal: i=1; a=rsa-sha256; s=arcselector9901; d=microsoft.com; cv=none; b=g+Els04yrZjI4rrKx+wrOHeEyzaRN4+vOCkGBmFz34cBAbOIWajTVmSv4AnX4dsYXIA4bNRmfPl1eXbfpvfAf/Jl49NrgqF5r8yO2wUZCOgxDjtMJ9msFLM2ib0Gfw481pYqD66ykyyPiGwoOzpS0DcV1hzTjKzYDYkXhtZrsr419Ha3FcLERq9zjXxNrmfVkIe2jsy3OSDIzUnl7XDUk4hUgbfLkLHpVn/Fsf8Kh9AuLHZVy2MnU1FrPxEBDrHz3ILXWpZEl51LwM5YKZ58HB6/OJ9yO790rmxmEWKmlb2N4teW336TamF+0D1t8/IhLmqaLzQkEXVT3itQTtusIw== 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=WiuYT4MWuMEypXDuUWDR/GZSBUaoEbalNwUZmzGYX1U=; b=CQ4Y6P3EXjEatBxMMaQhmuB4cHClx2cW8NYsCcwhoz21+deqY5VM/xo/YWNr9+aPEo9dIGW7ZUeF7ptS2/beVfLerFcfGjsDKwMaUVA0fUJB41d8YisON/gxj3+ItTf3Idn/4JPFeVT//702cRSuysjEzl2WWrB2NBsrHDkUyP2CEM3DqW0OaJVPoTXEnqYRBF9kTxd0aQDFstoPYps2egto4ubfKP8LZyHNMYqqFTgL3H0iE1nysoJ3SAxNJ67leSYIhgrCrGheY7SD5Yq94dDMaOMbk7YjlOcWOapS+46OoX0xUTEx1GnldxgpkfRK6g6Pz5pxZr5r+wWeGWAnJw== 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=WiuYT4MWuMEypXDuUWDR/GZSBUaoEbalNwUZmzGYX1U=; b=AcsvmnaHdJz7eTfvu8Tyciu6MgGXSRv5ICsruA8WJc6TP0RbuJnyua8N3CYbH6fCgdPvXcAFdnO2SrkckY8t9VUptmqAW6cvnoPJTprEbdFjzCABuumMeARUvGJuuGa3xVDFMUTMPd25suQRAM+kwLlbeQgf9ZbPPbcFzDX9EYk= 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 SA1PR12MB5616.namprd12.prod.outlook.com (2603:10b6:806:22a::11) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id 15.20.5880.19; Tue, 13 Dec 2022 20:06:50 +0000 Received: from CH2PR12MB4294.namprd12.prod.outlook.com ([fe80::b482:d5bd:c7d0:3842]) by CH2PR12MB4294.namprd12.prod.outlook.com ([fe80::b482:d5bd:c7d0:3842%8]) with mapi id 15.20.5880.019; Tue, 13 Dec 2022 20:06:49 +0000 Message-ID: <9a458e46-f71c-22b6-63a2-5c425624275a@amd.com> Date: Tue, 13 Dec 2022 20:06:33 +0000 User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:102.0) Gecko/20100101 Thunderbird/102.5.1 Content-Language: en-US To: Ankur Dwivedi , dev@dpdk.org Cc: thomas@monjalon.net, mdr@ashroe.eu, orika@nvidia.com, ferruh.yigit@xilinx.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, sthemmin@microsoft.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 References: <20220929102936.5490-1-adwivedi@marvell.com> <20221006151844.23483-1-adwivedi@marvell.com> <20221006151844.23483-2-adwivedi@marvell.com> From: Ferruh Yigit Subject: Re: [PATCH v3 1/4] ethdev: add trace points In-Reply-To: <20221006151844.23483-2-adwivedi@marvell.com> Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 7bit X-ClientProxiedBy: LO2P123CA0046.GBRP123.PROD.OUTLOOK.COM (2603:10a6:600::34) To CH2PR12MB4294.namprd12.prod.outlook.com (2603:10b6:610:a9::11) MIME-Version: 1.0 X-MS-PublicTrafficType: Email X-MS-TrafficTypeDiagnostic: CH2PR12MB4294:EE_|SA1PR12MB5616:EE_ X-MS-Office365-Filtering-Correlation-Id: 7a3750c9-25c6-4dda-e9ff-08dadd4591bc 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: UYCZELaWE6DnZoRKMFEHC75XTlvUWrR8U2xuCS9Bl6bqtMCtWVP7axEZKjL6YcGPACKcIHDXpxvL4+S2cHZDO431XjWgcu3XWvRZfDJWyi8jYzdB+DPiDqgrHQwNBCurgHLQUOFJEdlTIAoLilcG1uu24Yse1fyA/aWOtBcwhrK+lNdFnOXJOSufs6AIy2iqHPEVbYNGIg1u/myXjG65pDcVi/+QQqLaEpBbPn3QronHo3Io8CZ+C0VKXzJ2ZlfNUqg6886e2YeBBl/aiXxUmYi6/svmLG+z3g2mQIePi4Yb4eybER1ltOsbhryS9++9h/HnQq/rNWVTEOALsTsz99Yd+a/uJTWjj+8dt7xQEzrWv0OxTO81NBIBPDHBBVRaLp3FhWqovJ8q6L1OjPkzNBBsVdLIfwl7ok8VDeL2dr9Xptw+c1ybOoFtRuwi/zKdMm1BzK8eZh3fCwvN+mw5e/ZxfAQ990oZuWi+GOltpC4LUBs3JyeZulHBlC0wLujz5q+YScJDVgP+gd66+YnfM35qmWdJEOKwjjcTvQtwbmJ0R6o3vcoL/E9yX/8csCti4BlhUEokID7jorP6IhNzu5uCDVZnbEzpAb1KjX9U4fB0ySWzTS9IRpLS4DRzIsqUl6trihMAANo7y22zviBtw+nhgtYOshlkOQ0khRJP06HgbvJOwWwf9oQl9xaQapPWtgj7NKOS19R9fDCvFKMKf9pG5AuS1WejRw26lz+B9Ms= 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)(39860400002)(396003)(346002)(376002)(366004)(136003)(451199015)(31686004)(6512007)(6506007)(316002)(186003)(53546011)(26005)(2906002)(6666004)(2616005)(8676002)(36756003)(66476007)(66946007)(41300700001)(66556008)(4326008)(83380400001)(38100700002)(44832011)(7366002)(7416002)(7406005)(5660300002)(7336002)(30864003)(478600001)(8936002)(6486002)(86362001)(31696002)(45980500001)(43740500002); DIR:OUT; SFP:1101; X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1 X-MS-Exchange-AntiSpam-MessageData-0: =?utf-8?B?MGlMeDFBU1VzN3E4SHpqemlPNVZ5dTUyTXFWMzJrOG5rKzJpMlRsSHlYdCs5?= =?utf-8?B?RC9jSjFITElkV2RMT1ZpQWI1T2hKUHNUQzgzb2Zla0doRVY1NWZqNytkeU5z?= =?utf-8?B?cCtuWGhLbWEzRUhGRnZjaFV0ZDZWMUpacFVXZnZ6NFJUdVk5d2p5TXMzYURL?= =?utf-8?B?d0lmR2tncUdyNEJLRURUanhsS0RSYUpObnlIOGt0STJMVkVIL2lEQ2RTblhr?= =?utf-8?B?VTlnVXkramFhQ3FhdzRWWVBxR0l5L1pHcVVxTVk3MUdaSWJtaHBZcS9CakhE?= =?utf-8?B?eXU2MEhYYithdldsYXdVaUtWcDRnT3dWUUU3b1gzbFpoQ1VPSHJsTzI2N2dn?= =?utf-8?B?L1hjSVZoSHlXeGdxbllDaklDbDAwQWVic0tJaitYQWVVbEJRVGFaUUIrSUxs?= =?utf-8?B?c1BEMnRaZzgzVzk4K0RFNkRYdEhwNkpUVkZ4S0FjUjgydFBiYWkwQ1FPK1Ix?= =?utf-8?B?Qjlub0IrVTl1Nkhta0hYV2xSWldHdzVmRUNHdVJETlR4Y1VtTmdid0hKUDBx?= =?utf-8?B?WHZvNWRiYTh3R2R6N3ZnanY4WFlPUGIzVmpoQS9nYzhRMkhYdW12R2prUDZy?= =?utf-8?B?b1BVYnQzeFJZZE9TMm1ZT2hDdHZmcjRxekdRVW4waVBrQlMySnNqTG14aDdm?= =?utf-8?B?VGc4SWdVM2JzSW1LUmNmYndjQ0JTZ0tPVHIrbDl6MTYvTVhFdTE3TkMvQ3pZ?= =?utf-8?B?STd6NXd4WVdZVUhSTEE1cGV3RWVrSzU3OEhWR055eGREZW1Ta1FzZFl4TDlR?= =?utf-8?B?ZzRhb1llYWF1NVpWTlczdmJzaDJGc3NpTTZINkVtNjFPcStoWi90VUZuV3cv?= =?utf-8?B?Qm1jd0JMY1hQQU5CSHd1QU52b0ZkUFdMSERBek5Jai9pUXFrTjRTNFY4ZVZX?= =?utf-8?B?Wnp3SVdvZGJUSUdQL0gyZklHTFlJRExpM0syWFFhaXhDOG83d2crbWRha3o2?= =?utf-8?B?NXVSdkEyUWRZZDA4ZjVndDV0MElTT05ER3FjMmxGRVVweHdnL3paU1B4dnM2?= =?utf-8?B?WDR5UDRmbjFjN001aEthREZCSWxhRE9UakpEdmFqTFM0VlErUVZ5VXFBQjl5?= =?utf-8?B?Lzd1V2tXdXZHSlBjVXhXLzVuMVdwVy90Z3hoemhGeS9NSG4xT2gwTUhuRzZu?= =?utf-8?B?QnArdnJEWlFNL3N6OWdGZ3FKNmdkcnRvdzZVdVVTSjIvNG5uaVFtOFJZeWR2?= =?utf-8?B?Y1BIUHVXZDY3bmFUd040cEhaOVUwcmZvY1gvRFVleGU5ZXpkK05WMlowL1Mv?= =?utf-8?B?R2lWbjYrVktOOWxQbzJDQ0ZyZ2ZOaDVnckxWREJ1TlBKVHNRODVqaWhhNHYr?= =?utf-8?B?MmJ5c1hjc0xBUUNLVTBlczRUcTZCaVAybU1jRkFMN1JBWEJXcXpxTzk5djd4?= =?utf-8?B?cTBLdzhqRTBSWlU0NGRMYUE4MERTS2g3a0xpQzZnNDY3Uk9qRU00WTRCMG5u?= =?utf-8?B?N29xYm9CN09rbDRyNjBFdS9Yb3pPNHl2OGQ2MGZPOXFkTlY1WEQvaDA4NGcy?= =?utf-8?B?bjZ0SjhTRXJ0TVFCRzNVOHMvbkYzeXNQQy93cGNaVFc2VG53T3I4TGJEY3Br?= =?utf-8?B?MzVoYzRnckNqb0doWk1XQ0FtTXdHdndxUXB6ZU1haHYvZ1JyaGlFODd2Q2Yr?= =?utf-8?B?dVFiZThqcFNxc2dwTHEzK05zWmZBcktXSnZkU2VXR1lyY1kzbU5vWnE5Z3Rl?= =?utf-8?B?dUtsTGgwelQ4cWxBbXhoeUpYYXh5d1Y3RFJ3U2FUNHcvMjNIa2tHdnFWa0dt?= =?utf-8?B?dlRCcFhFeXkvZjFGS3lHRUhxZ0JLSnVwRFR5eWlqZklva3ZjRG1VWGo1b0gw?= =?utf-8?B?QUNLUkZUTG03OUhNSGhKU2cwQkhmUTdOb1ZiZWFseFBiMVd3dC8wOW9uYSsz?= =?utf-8?B?NDBJWlcxbit4QUNwcDZmRFBQbnVEYk5uanRtMlN2ckxYV0NvVjl4TjBBdTN3?= =?utf-8?B?WU1vMWpqei9uTnp3dUhYeGF3b293TERMY01BSnd0SUEwQlRaeWM0VFFNSFpD?= =?utf-8?B?ZFdDSEFiSDE0ajRVaDB6OWdUeFZvMkRKalgrSGFXa0l1T2R6VHFSbnZwVlhM?= =?utf-8?B?Q0pMMDJObGJpY3JSVytud3plTGcvdUhzYlBwQUEweFYxY09mS2hoY1cyUEN6?= =?utf-8?Q?o6XuQOXIQFpbbWoDebfE1l8T8?= X-OriginatorOrg: amd.com X-MS-Exchange-CrossTenant-Network-Message-Id: 7a3750c9-25c6-4dda-e9ff-08dadd4591bc X-MS-Exchange-CrossTenant-AuthSource: CH2PR12MB4294.namprd12.prod.outlook.com X-MS-Exchange-CrossTenant-AuthAs: Internal X-MS-Exchange-CrossTenant-OriginalArrivalTime: 13 Dec 2022 20:06:49.7186 (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: 3z3bfOCwsKvFZaEoVi1PnLxT/eXZ5wiaxZvT3jv/PjGXZIhasW5XV+P9NcssiZGd X-MS-Exchange-Transport-CrossTenantHeadersStamped: SA1PR12MB5616 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 10/6/2022 4:18 PM, Ankur Dwivedi wrote: Hi Ankur, Jerin, I have some major questions, I can see some already queried by Andrew as well: 1) How flexible are we on changing trace log output? Should we take trace log as kind of user interface and don't break it (as much as possible)? Or is it OK to change whenever we want? 2) What is the main purpose of the trace library. Is it to record type and frequency of the calls? Or is it to log values and state of the device/driver after each call? This can guide us to decide - which values to record (should record only pointers or values of structs) - if to record API return values and failure paths - if to add tracing all APIs or not, like 'rte_eth_dev_rx_offload_name()' kind of helper function that converts offload to a string, which doesn't have any functional impact, or 'rte_eth_speed_bitflag()', and many more... 3) What is the runtime impact. As far as I can see some values copied and some memory is used for this support, even user is not interested in tracing. For control path APIs we can ignore the additional cost by memcpy, but how big memory requirement is? 4) Why we need to export trace point variables in the .map files, like '__rte_eth_trace_allmulticast_disable' one... 5) (bonus Q) Can we have an 'rte_trace_point_emit_xx()' function to record mac address (struct rte_ether_addr) as string? And maybe another set for array types? There are bunch of small comments inline, some are related to above highlevel question. But I stopped after some point, specifically after 'rte_eth_trace_timesync_adjust_time()' :), this is a big file, splitting it may help reviewers. > > Add trace points for ethdev functions. > > Signed-off-by: Ankur Dwivedi <...> > +RTE_TRACE_POINT_REGISTER(rte_eth_trace_rx_burst_mode_get, > + lib.ethdev.rx_burst_mode_get) > + Tx counterpart of this API is missing. > +RTE_TRACE_POINT_REGISTER(rte_eth_trace_rx_queue_info_get, > + lib.ethdev.rx_queue_info_get) > + > +RTE_TRACE_POINT_REGISTER(rte_eth_trace_rx_queue_setup, > + lib.ethdev.rx_queue_setup) > + ! rte_eth_trace_rx_queue_setup() is not called. And Tx counterpart of this API is missing, these are important APIs, we should have them. <...> > @@ -331,6 +336,7 @@ rte_eth_find_next(uint16_t port_id) > if (port_id >= RTE_MAX_ETHPORTS) > return RTE_MAX_ETHPORTS; > > + rte_eth_trace_find_next(port_id); For the output 'port_id', trace function is after "return RTE_MAX_ETHPORTS;", so "there is no next port" case is ignored, need to update function to get trace on each call. <...> > @@ -636,6 +656,7 @@ rte_eth_dev_get_port_by_name(const char *name, uint16_t *port_id) > RTE_ETH_FOREACH_VALID_DEV(pid) > if (!strcmp(name, eth_dev_shared_data->data[pid].name)) { > *port_id = pid; > + rte_ethdev_trace_get_port_by_name(name, *port_id); This completely ignores the path that not able to find port_id from name. What about change the function as ``` ret = -ENODEV; *port_id = RTE_MAX_ETHPORTS; RTE_ETH_FOREACH_VALID_DEV(pid) if (!strcmp()) *port_id = pid; ret = 0; break; rte_ethdev_trace_get_port_by_name(name, *port_id, ret); return ret; ``` > return 0; > } > > @@ -737,6 +758,7 @@ rte_eth_dev_rx_queue_start(uint16_t port_id, uint16_t rx_queue_id) > return 0; > } > > + rte_ethdev_trace_rx_queue_start(port_id, rx_queue_id); Please include return value. > return eth_err(port_id, dev->dev_ops->rx_queue_start(dev, rx_queue_id)); > } > > @@ -770,6 +792,7 @@ rte_eth_dev_rx_queue_stop(uint16_t port_id, uint16_t rx_queue_id) > return 0; > } > > + rte_ethdev_trace_rx_queue_stop(port_id, rx_queue_id); Please include return value. > return eth_err(port_id, dev->dev_ops->rx_queue_stop(dev, rx_queue_id)); > } > > @@ -810,6 +833,7 @@ rte_eth_dev_tx_queue_start(uint16_t port_id, uint16_t tx_queue_id) > return 0; > } > > + rte_ethdev_trace_tx_queue_start(port_id, tx_queue_id); Please include return value. > return eth_err(port_id, dev->dev_ops->tx_queue_start(dev, tx_queue_id)); > } > > @@ -843,12 +867,14 @@ rte_eth_dev_tx_queue_stop(uint16_t port_id, uint16_t tx_queue_id) > return 0; > } > > + rte_ethdev_trace_tx_queue_stop(port_id, tx_queue_id); Please include return value. > return eth_err(port_id, dev->dev_ops->tx_queue_stop(dev, tx_queue_id)); > } > > uint32_t > rte_eth_speed_bitflag(uint32_t speed, int duplex) > { > + rte_eth_trace_speed_bitflag(speed, duplex); I think this should have return value, but I can see this requires change in the function. I am for either having return value or drop the tracing of this function completely. <...> > @@ -1552,6 +1581,7 @@ rte_eth_dev_set_link_up(uint16_t port_id) > > if (*dev->dev_ops->dev_set_link_up == NULL) > return -ENOTSUP; > + rte_ethdev_trace_set_link_up(port_id); Please include return value. > return eth_err(port_id, (*dev->dev_ops->dev_set_link_up)(dev)); > } > > @@ -1565,6 +1595,7 @@ rte_eth_dev_set_link_down(uint16_t port_id) > > if (*dev->dev_ops->dev_set_link_down == NULL) > return -ENOTSUP; > + rte_ethdev_trace_set_link_down(port_id); Please include return value. <...> > @@ -2327,6 +2371,7 @@ rte_eth_promiscuous_enable(uint16_t port_id) > diag = (*dev->dev_ops->promiscuous_enable)(dev); > dev->data->promiscuous = (diag == 0) ? 1 : 0; > > + rte_eth_trace_promiscuous_enable(port_id, dev->data->promiscuous); Above is good, but to be consistent what about returning return value of the API (diag (with record name 'return')), which is relavent with 'dev->data->promiscuous' value. > return eth_err(port_id, diag); > } > > @@ -2350,6 +2395,7 @@ rte_eth_promiscuous_disable(uint16_t port_id) > if (diag != 0) > dev->data->promiscuous = 1; > > + rte_eth_trace_promiscuous_disable(port_id, dev->data->promiscuous); Above is good, but to be consistent what about returning return value of the API (diag (with record name 'return')), which is relavent with 'dev->data->promiscuous' value. <...> > @@ -2561,6 +2616,7 @@ rte_eth_stats_reset(uint16_t port_id) > > dev->data->rx_mbuf_alloc_failed = 0; > > + rte_eth_trace_stats_reset(port_id); Please include return value. <...> > @@ -3483,6 +3561,7 @@ rte_eth_dev_set_vlan_strip_on_queue(uint16_t port_id, uint16_t rx_queue_id, > > if (*dev->dev_ops->vlan_strip_queue_set == NULL) > return -ENOTSUP; > + rte_ethdev_trace_set_vlan_strip_on_queue(port_id, rx_queue_id, on); Please include return value. > (*dev->dev_ops->vlan_strip_queue_set)(dev, rx_queue_id, on); > > return 0; > @@ -3500,6 +3579,7 @@ rte_eth_dev_set_vlan_ether_type(uint16_t port_id, > > if (*dev->dev_ops->vlan_tpid_set == NULL) > return -ENOTSUP; > + rte_ethdev_trace_set_vlan_ether_type(port_id, vlan_type, tpid); Please include return value. <...> > @@ -3632,6 +3714,7 @@ rte_eth_dev_set_vlan_pvid(uint16_t port_id, uint16_t pvid, int on) > > if (*dev->dev_ops->vlan_pvid_set == NULL) > return -ENOTSUP; > + rte_ethdev_trace_set_vlan_pvid(port_id, pvid, on); Please include return value. <...> > @@ -3702,6 +3787,7 @@ rte_eth_dev_priority_flow_ctrl_set(uint16_t port_id, > return -EINVAL; > } > > + rte_ethdev_trace_priority_flow_ctrl_set(port_id, pfc_conf); Please put the call very bottom of the function and include return value. <...> > @@ -4063,6 +4156,7 @@ rte_eth_dev_udp_tunnel_port_add(uint16_t port_id, > > if (*dev->dev_ops->udp_tunnel_port_add == NULL) > return -ENOTSUP; > + rte_ethdev_trace_udp_tunnel_port_add(port_id, udp_tunnel); Please include return value. > return eth_err(port_id, (*dev->dev_ops->udp_tunnel_port_add)(dev, > udp_tunnel)); > } > @@ -4090,6 +4184,7 @@ rte_eth_dev_udp_tunnel_port_delete(uint16_t port_id, > > if (*dev->dev_ops->udp_tunnel_port_del == NULL) > return -ENOTSUP; > + rte_ethdev_trace_udp_tunnel_port_delete(port_id, udp_tunnel); Please include return value. > return eth_err(port_id, (*dev->dev_ops->udp_tunnel_port_del)(dev, > udp_tunnel)); > } > @@ -4104,6 +4199,7 @@ rte_eth_led_on(uint16_t port_id) > > if (*dev->dev_ops->dev_led_on == NULL) > return -ENOTSUP; > + rte_eth_trace_led_on(port_id); Please include return value. > return eth_err(port_id, (*dev->dev_ops->dev_led_on)(dev)); > } > > @@ -4117,6 +4213,7 @@ rte_eth_led_off(uint16_t port_id) > > if (*dev->dev_ops->dev_led_off == NULL) > return -ENOTSUP; > + rte_eth_trace_led_off(port_id); Please include return value. <...> > @@ -4294,6 +4395,7 @@ rte_eth_dev_mac_addr_remove(uint16_t port_id, struct rte_ether_addr *addr) > } else if (index < 0) > return 0; /* Do nothing if address wasn't found */ > > + rte_ethdev_trace_mac_addr_remove(port_id, addr); better to keep trace APIs are last thing in the function for consistency, as long as it is possible, and for this case it is possible. <...> > @@ -4438,6 +4542,7 @@ rte_eth_dev_uc_all_hash_table_set(uint16_t port_id, uint8_t on) > > if (*dev->dev_ops->uc_all_hash_table_set == NULL) > return -ENOTSUP; > + rte_ethdev_trace_uc_all_hash_table_set(port_id, on); Please include return value. > return eth_err(port_id, (*dev->dev_ops->uc_all_hash_table_set)(dev, > on)); > } > @@ -4475,6 +4580,7 @@ int rte_eth_set_queue_rate_limit(uint16_t port_id, uint16_t queue_idx, > > if (*dev->dev_ops->set_queue_rate_limit == NULL) > return -ENOTSUP; > + rte_eth_trace_set_queue_rate_limit(port_id, queue_idx, tx_rate); Please include return value. <...> > @@ -4570,6 +4678,9 @@ rte_eth_dev_callback_register(uint16_t port_id, > next_port = last_port = port_id; > } > > + rte_ethdev_trace_callback_register(port_id, event, cb_fn, cb_arg, next_port, > + last_port); > + I assume this added into middle to be able to use 'next_port' & 'last_port', but they are generated from given 'port_id', so what do you think to have only 'port_id', and move call to the bottom of the function. 'rte_ethdev_trace_callback_unregister()' has same logic, and it has only 'port_id' already. And I think better to standardise to record API return value or not, because some has it, some don't. What do you think to trace all return values, is there any downside of this approach? If we need fine grain on recording return values, what about following: - All API that changes device config return values should be recorded * like, record return for set_mtu(), but ignore it for get_mtu() - If return value has not API status but useful information, record it * like rte_eth_dev_is_valid_port() return value > rte_spinlock_lock(ð_dev_cb_lock); > > do { > @@ -4665,6 +4776,7 @@ rte_eth_dev_callback_unregister(uint16_t port_id, > } while (++next_port <= last_port); > > rte_spinlock_unlock(ð_dev_cb_lock); > + rte_ethdev_trace_callback_unregister(port_id, event, cb_fn, cb_arg, ret); Just a syntax comment, what do you think to have an empty line one before and after the trace call, to separate this trace calls from regular logic. > return ret; > } > > @@ -4694,6 +4806,7 @@ rte_eth_dev_rx_intr_ctl(uint16_t port_id, int epfd, int op, void *data) > for (qid = 0; qid < dev->data->nb_rx_queues; qid++) { > vec = rte_intr_vec_list_index_get(intr_handle, qid); > rc = rte_intr_rx_ctl(intr_handle, epfd, op, vec, data); > + rte_ethdev_trace_rx_intr_ctl(port_id, epfd, op, data, rc); Why this is in the for loop, API if for port level, this loop iterates on queues? What about moving this trace to the bottom of the function, and record return value as return value of the API? > if (rc && rc != -EEXIST) { > RTE_ETHDEV_LOG(ERR, > "p %u q %u Rx ctl error op %d epfd %d vec %u\n", > @@ -4737,6 +4850,7 @@ rte_eth_dev_rx_intr_ctl_q_get_fd(uint16_t port_id, uint16_t queue_id) > (vec - RTE_INTR_VEC_RXTX_OFFSET) : vec; > fd = rte_intr_efds_index_get(intr_handle, efd_idx); > > + rte_ethdev_trace_rx_intr_ctl_q_get_fd(port_id, queue_id, fd); > return fd; > } > > @@ -4770,6 +4884,7 @@ rte_eth_dev_rx_intr_ctl_q(uint16_t port_id, uint16_t queue_id, > > vec = rte_intr_vec_list_index_get(intr_handle, queue_id); > rc = rte_intr_rx_ctl(intr_handle, epfd, op, vec, data); > + rte_ethdev_trace_rx_intr_ctl_q(port_id, queue_id, epfd, op, data, rc); Instead of trace API having in the middle, what about changing API as: ``` rc = ... if (rc && rc != -EEXIST) { ... } rte_ethdev_trace_rx_intr_ctl_q(port_id, queue_id, epfd, op, data, rc); return rc; ``` > if (rc && rc != -EEXIST) { > RTE_ETHDEV_LOG(ERR, > "p %u q %u Rx ctl error op %d epfd %d vec %u\n", > @@ -4796,6 +4911,7 @@ rte_eth_dev_rx_intr_enable(uint16_t port_id, > > if (*dev->dev_ops->rx_queue_intr_enable == NULL) > return -ENOTSUP; > + rte_ethdev_trace_rx_intr_enable(port_id, queue_id); Please include return value. > return eth_err(port_id, (*dev->dev_ops->rx_queue_intr_enable)(dev, queue_id)); > } > > @@ -4815,6 +4931,7 @@ rte_eth_dev_rx_intr_disable(uint16_t port_id, > > if (*dev->dev_ops->rx_queue_intr_disable == NULL) > return -ENOTSUP; > + rte_ethdev_trace_rx_intr_disable(port_id, queue_id); Please include return value. <...> > @@ -5447,6 +5586,7 @@ rte_eth_dev_set_eeprom(uint16_t port_id, struct rte_dev_eeprom_info *info) > > if (*dev->dev_ops->set_eeprom == NULL) > return -ENOTSUP; > + rte_ethdev_trace_set_eeprom(port_id, info); Please include return value. <...> > @@ -5562,6 +5705,7 @@ rte_eth_dev_adjust_nb_rx_tx_desc(uint16_t port_id, > if (nb_tx_desc != NULL) > eth_dev_adjust_nb_desc(nb_tx_desc, &dev_info.tx_desc_lim); > > + rte_ethdev_trace_adjust_nb_rx_tx_desc(port_id, *nb_rx_desc, *nb_rx_desc); 'nb_rx_desc' or 'nb_rx_desc' can be NULL, so we can't directly access them. <...> > @@ -5605,6 +5750,7 @@ rte_eth_dev_pool_ops_supported(uint16_t port_id, const char *pool) > if (*dev->dev_ops->pool_ops_supported == NULL) > return 1; /* all pools are supported */ > > + rte_ethdev_trace_pool_ops_supported(port_id, pool); Please include return value. <...> > +RTE_TRACE_POINT( > + rte_ethdev_trace_flow_ctrl_get, > + RTE_TRACE_POINT_ARGS(uint16_t port_id, struct rte_eth_fc_conf *fc_conf), > + rte_trace_point_emit_u16(port_id); > + rte_trace_point_emit_ptr(fc_conf); > +) 'fc_conf' is pointer for the config struct, its address is not much useful, can you expand it similar to ones in 'rte_ethdev_trace_flow_ctrl_set()' API? > + > +RTE_TRACE_POINT( > + rte_ethdev_trace_flow_ctrl_set, > + RTE_TRACE_POINT_ARGS(uint16_t port_id, struct rte_eth_fc_conf *fc_conf), > + rte_trace_point_emit_u16(port_id); > + rte_trace_point_emit_u32(fc_conf->high_water); > + rte_trace_point_emit_u32(fc_conf->low_water); > + rte_trace_point_emit_u16(fc_conf->pause_time); > + rte_trace_point_emit_u16(fc_conf->send_xon); > + rte_trace_point_emit_int(fc_conf->mode); > + rte_trace_point_emit_u8(fc_conf->mac_ctrl_frame_fwd); > + rte_trace_point_emit_u8(fc_conf->autoneg); > +) > + > +RTE_TRACE_POINT( > + rte_ethdev_trace_fw_version_get, > + RTE_TRACE_POINT_ARGS(uint16_t port_id, char *fw_version, size_t fw_size), > + rte_trace_point_emit_u16(port_id); > + rte_trace_point_emit_ptr(fw_version); > + rte_trace_point_emit_size_t(fw_size); > +) 'fw_version' is string, it help to display the string, but not sure if the pointer value has a value. Is there a way to display string in the trace point? And 'fw_size' is normally just used to know the valid data in the 'fw_version', when 'fw_version' string is displayed, not sure if there is a value to have 'fw_size' > + > +RTE_TRACE_POINT( > + rte_ethdev_trace_get_dcb_info, > + RTE_TRACE_POINT_ARGS(uint16_t port_id, struct rte_eth_dcb_info *dcb_info), > + rte_trace_point_emit_u16(port_id); > + rte_trace_point_emit_ptr(dcb_info); > +) 'dcb_info' is pointer to a info struct, not useful as pointer value, we should display content of the 'dcb_info' struct. There are arrays in the 'dcb_info', perhaps it can be possible to trace 'dcb_info->nb_tcs', also 'dcb_info->prio_tc[]' & 'dcb_info->tc_bws[]' since their size is small (8). > + > +RTE_TRACE_POINT( > + rte_ethdev_trace_get_eeprom, > + RTE_TRACE_POINT_ARGS(uint16_t port_id, struct rte_dev_eeprom_info *info), > + rte_trace_point_emit_u16(port_id); > + rte_trace_point_emit_ptr(info); > +) > + Similarly, content of the 'info' should be displayed, for '*data' it can be possible to display address of it, but values for rest. > +RTE_TRACE_POINT( > + rte_ethdev_trace_get_eeprom_length, > + RTE_TRACE_POINT_ARGS(uint16_t port_id), > + rte_trace_point_emit_u16(port_id); > +) > + This should return the size of the eeprom, existing 'rte_eth_dev_get_eeprom_length()' needs to be updated for it. <...> > +RTE_TRACE_POINT( > + rte_ethdev_trace_get_reg_info, > + RTE_TRACE_POINT_ARGS(uint16_t port_id, struct rte_dev_reg_info *info), > + rte_trace_point_emit_u16(port_id); > + rte_trace_point_emit_ptr(info); > +) What do you think to display content of the 'info', insted of pointer value which is not very useful. > + > +RTE_TRACE_POINT( > + rte_ethdev_trace_get_sec_ctx, > + RTE_TRACE_POINT_ARGS(uint16_t port_id), > + rte_trace_point_emit_u16(port_id); > +) 'security_ctx' is "void *", so can't record content, but what do you think to record pointer value of it? > + > +RTE_TRACE_POINT( > + rte_ethdev_trace_get_supported_ptypes, > + RTE_TRACE_POINT_ARGS(uint16_t port_id, uint32_t ptype_mask, > + uint32_t *ptypes, int num, int j), > + rte_trace_point_emit_u16(port_id); > + rte_trace_point_emit_u32(ptype_mask); > + rte_trace_point_emit_ptr(ptypes); > + rte_trace_point_emit_int(num); > + rte_trace_point_emit_int(j); > +) better to use 'supported_num' instead of 'j' which is not clear what it is. The real information is in 'ptypes' but that is an array, I guess not possible to record array values. > + > +RTE_TRACE_POINT( > + rte_ethdev_trace_get_vlan_offload, > + RTE_TRACE_POINT_ARGS(uint16_t port_id, int ret), > + rte_trace_point_emit_u16(port_id); > + rte_trace_point_emit_int(ret); > +) > + > +RTE_TRACE_POINT( > + rte_ethdev_trace_info_get, > + RTE_TRACE_POINT_ARGS(uint16_t port_id, > + struct rte_eth_dev_info *dev_info), > + rte_trace_point_emit_u16(port_id); > + rte_trace_point_emit_string(dev_info->driver_name); > + rte_trace_point_emit_u32(dev_info->if_index); > + rte_trace_point_emit_u16(dev_info->min_mtu); > + rte_trace_point_emit_u16(dev_info->max_mtu); > + rte_trace_point_emit_u32(dev_info->min_rx_bufsize); > + rte_trace_point_emit_u32(dev_info->max_rx_pktlen); > + rte_trace_point_emit_u64(dev_info->rx_offload_capa); > + rte_trace_point_emit_u64(dev_info->tx_offload_capa); > + rte_trace_point_emit_u64(dev_info->rx_queue_offload_capa); > + rte_trace_point_emit_u64(dev_info->tx_queue_offload_capa); > + rte_trace_point_emit_u16(dev_info->reta_size); > + rte_trace_point_emit_u8(dev_info->hash_key_size); > + rte_trace_point_emit_u16(dev_info->nb_rx_queues); > + rte_trace_point_emit_u16(dev_info->nb_tx_queues); > +) > + It is a debate what to display, but I would remove 'if_index' and add 'max_rx_queues', 'max_tx_queues', 'max_mac_addrs', 'flow_type_rss_offloads', 'rx_desc_lim', 'tx_desc_lim', 'speed_capa', 'dev_capa'. > +RTE_TRACE_POINT( > + rte_ethdev_trace_is_removed, > + RTE_TRACE_POINT_ARGS(uint16_t port_id, int ret), > + rte_trace_point_emit_u16(port_id); > + rte_trace_point_emit_int(ret); > +) > + > +RTE_TRACE_POINT( > + rte_ethdev_trace_is_valid_port, > + RTE_TRACE_POINT_ARGS(uint16_t port_id), > + rte_trace_point_emit_u16(port_id); > +) > + Should have return value, if the port is valid of not. > +RTE_TRACE_POINT( > + rte_ethdev_trace_mac_addr_add, > + RTE_TRACE_POINT_ARGS(uint16_t port_id, struct rte_ether_addr *addr, > + uint32_t pool), > + rte_trace_point_emit_u16(port_id); > + rte_trace_point_emit_ptr(addr); > + rte_trace_point_emit_u32(pool); > +) > + Isn't it possible to return 'addr' as string? I am not sure about benefit to record addr pointer value. Should have return value. > +RTE_TRACE_POINT( > + rte_ethdev_trace_mac_addr_remove, > + RTE_TRACE_POINT_ARGS(uint16_t port_id, struct rte_ether_addr *addr), > + rte_trace_point_emit_u16(port_id); > + rte_trace_point_emit_ptr(addr); > +) > + As above, can content of 'addr' be recorded, like as string? > +RTE_TRACE_POINT( > + rte_ethdev_trace_pool_ops_supported, > + RTE_TRACE_POINT_ARGS(uint16_t port_id, const char *pool), > + rte_trace_point_emit_u16(port_id); > + rte_trace_point_emit_ptr(pool); > +) > + 'pool' should be string, instead of pointer. > +RTE_TRACE_POINT( > + rte_ethdev_trace_priority_flow_ctrl_set, > + RTE_TRACE_POINT_ARGS(uint16_t port_id, struct rte_eth_pfc_conf *pfc_conf), > + rte_trace_point_emit_u16(port_id); > + rte_trace_point_emit_u32(pfc_conf->fc.high_water); > + rte_trace_point_emit_u32(pfc_conf->fc.low_water); > + rte_trace_point_emit_u16(pfc_conf->fc.pause_time); > + rte_trace_point_emit_u16(pfc_conf->fc.send_xon); > + rte_trace_point_emit_int(pfc_conf->fc.mode); > + rte_trace_point_emit_u8(pfc_conf->fc.mac_ctrl_frame_fwd); > + rte_trace_point_emit_u8(pfc_conf->fc.autoneg); > + rte_trace_point_emit_u8(pfc_conf->priority); > +) > + > +RTE_TRACE_POINT( > + rte_ethdev_trace_reset, > + RTE_TRACE_POINT_ARGS(uint16_t port_id, int ret), > + rte_trace_point_emit_u16(port_id); > + rte_trace_point_emit_int(ret); > +) > + > +RTE_TRACE_POINT( > + rte_ethdev_trace_rss_hash_conf_get, > + RTE_TRACE_POINT_ARGS(uint16_t port_id, struct rte_eth_rss_conf *rss_conf), > + rte_trace_point_emit_u16(port_id); > + rte_trace_point_emit_ptr(rss_conf); > +) > + Please record content of the 'rss_conf' instead of pointer value. > +RTE_TRACE_POINT( > + rte_ethdev_trace_rss_hash_update, > + RTE_TRACE_POINT_ARGS(uint16_t port_id, struct rte_eth_rss_conf *rss_conf), > + rte_trace_point_emit_u16(port_id); > + rte_trace_point_emit_ptr(rss_conf->rss_key); > + rte_trace_point_emit_u8(rss_conf->rss_key_len); > + rte_trace_point_emit_u64(rss_conf->rss_hf); > +) > + Please add return value. > +RTE_TRACE_POINT( > + rte_ethdev_trace_rss_reta_query, > + RTE_TRACE_POINT_ARGS(uint16_t port_id, > + struct rte_eth_rss_reta_entry64 *reta_conf, uint16_t reta_size), > + rte_trace_point_emit_u16(port_id); > + rte_trace_point_emit_ptr(reta_conf); > + rte_trace_point_emit_u16(reta_size); > +) > + > +RTE_TRACE_POINT( > + rte_ethdev_trace_rss_reta_update, > + RTE_TRACE_POINT_ARGS(uint16_t port_id, > + struct rte_eth_rss_reta_entry64 *reta_conf, uint16_t reta_size), > + rte_trace_point_emit_u16(port_id); > + rte_trace_point_emit_u64(reta_conf->mask); > + rte_trace_point_emit_u16(reta_size); > +) 'reta_conf->mask' on its own is not much usefull, and 'reta_conf->reta' is an array and hard to record. As above 'rte_ethdev_trace_rss_reta_query' trace point only record reta_config pointer, this can do the same. Overall, I think better to make get/set pair of an API record same set of values. <...> > +RTE_TRACE_POINT( > + rte_ethdev_trace_set_mc_addr_list, > + RTE_TRACE_POINT_ARGS(uint16_t port_id, > + struct rte_ether_addr *mc_addr_set, > + uint32_t nb_mc_addr), > + rte_trace_point_emit_u16(port_id); > + rte_trace_point_emit_ptr(mc_addr_set); > + rte_trace_point_emit_u32(nb_mc_addr); > +) > + Better to record content of 'mc_addr_set' but it is pointer array for mac address, so I guess we can't record these mac addresses. > +RTE_TRACE_POINT( > + rte_ethdev_trace_set_ptypes, > + RTE_TRACE_POINT_ARGS(uint16_t port_id, uint32_t ptype_mask, > + uint32_t *set_ptypes, unsigned int num), > + rte_trace_point_emit_u16(port_id); > + rte_trace_point_emit_u32(ptype_mask); > + rte_trace_point_emit_ptr(set_ptypes); > + rte_trace_point_emit_u32(num); > +) > + Similarly 'set_ptypes' is an array. I wonder if there should be an rte_trace_point_emit_ function for arrays? What do you think? <...> > +RTE_TRACE_POINT( > + rte_ethdev_trace_socket_id, > + RTE_TRACE_POINT_ARGS(uint16_t port_id), > + rte_trace_point_emit_u16(port_id); > +) Should include 'socket_id'. <...> > +RTE_TRACE_POINT( > + rte_ethdev_trace_uc_hash_table_set, > + RTE_TRACE_POINT_ARGS(uint16_t port_id, struct rte_ether_addr *addr, > + uint8_t on, int ret), > + rte_trace_point_emit_u16(port_id); > + rte_trace_point_emit_ptr(addr); > + rte_trace_point_emit_u8(on); > + rte_trace_point_emit_int(ret); > +) > + Can have 'addr' as string? Do we need a specific rte_trace_point_emit_ function for mac addr? <...> > +RTE_TRACE_POINT( > + rte_eth_trace_find_next, > + RTE_TRACE_POINT_ARGS(uint16_t port_id), > + rte_trace_point_emit_u16(port_id); > +) > + rte_eth_trace_find_next* functions gets 'port_id' as input parameter, and returns next 'port_id', so both input and output is 'port_id' and both should be recorded. For this need to update these functions to create an interim variable for input 'port_id'. Above comments are for all 'rte_eth_trace_find_next*()' functions. <...> > +RTE_TRACE_POINT( > + rte_eth_trace_link_get, > + RTE_TRACE_POINT_ARGS(uint16_t port_id, struct rte_eth_link *link), > + rte_trace_point_emit_u16(port_id); > + rte_trace_point_emit_u32(link->link_speed); > +) I think better to record other link values too. > + > +RTE_TRACE_POINT( > + rte_eth_trace_link_get_nowait, > + RTE_TRACE_POINT_ARGS(uint16_t port_id, struct rte_eth_link *link), > + rte_trace_point_emit_u16(port_id); > + rte_trace_point_emit_u32(link->link_speed); > +) > + I think better to record other link values too. <...> > +RTE_TRACE_POINT( > + rte_eth_trace_rx_burst_mode_get, > + RTE_TRACE_POINT_ARGS(uint16_t port_id, uint16_t queue_id, > + struct rte_eth_burst_mode *mode), > + rte_trace_point_emit_u16(port_id); > + rte_trace_point_emit_u16(queue_id); > + rte_trace_point_emit_ptr(mode); > +) > + Can you please record content of 'mode' inst > +RTE_TRACE_POINT( > + rte_eth_trace_rx_queue_info_get, > + RTE_TRACE_POINT_ARGS(uint16_t port_id, uint16_t queue_id, > + struct rte_eth_rxq_info *qinfo), > + rte_trace_point_emit_u16(port_id); > + rte_trace_point_emit_u16(queue_id); > + rte_trace_point_emit_ptr(qinfo->mp); > + rte_trace_point_emit_u8(qinfo->scattered_rx); > + rte_trace_point_emit_u8(qinfo->queue_state); > + rte_trace_point_emit_u16(qinfo->nb_desc); > + rte_trace_point_emit_u16(qinfo->rx_buf_size); > +) > + Can you please add 'qinfo->conf->rx_drop_en' & 'qinfo->conf->offloads'. > +RTE_TRACE_POINT( > + rte_eth_trace_rx_queue_setup, > + RTE_TRACE_POINT_ARGS(uint16_t port_id, uint16_t rx_queue_id, > + uint16_t nb_rx_desc, unsigned int socket_id, > + const struct rte_eth_rxconf *rx_conf, > + struct rte_mempool *mb_pool), > + 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_u32(socket_id); > + rte_trace_point_emit_ptr(rx_conf); > + rte_trace_point_emit_ptr(mb_pool); > +) > + Can you please add 'rx_conf->rx_drop_en' & 'rx_conf->offloads'. > +RTE_TRACE_POINT( > + rte_eth_trace_set_queue_rate_limit, > + RTE_TRACE_POINT_ARGS(uint16_t port_id, uint16_t queue_idx, > + uint16_t tx_rate), > + rte_trace_point_emit_u16(port_id); > + rte_trace_point_emit_u16(queue_idx); > + rte_trace_point_emit_u16(tx_rate); > +) > + > +RTE_TRACE_POINT( > + rte_eth_trace_speed_bitflag, > + RTE_TRACE_POINT_ARGS(uint32_t speed, int duplex), > + rte_trace_point_emit_u32(speed); > + rte_trace_point_emit_int(duplex); > +) > + > +RTE_TRACE_POINT( > + rte_eth_trace_stats_get, > + RTE_TRACE_POINT_ARGS(uint16_t port_id, struct rte_eth_stats *stats), > + rte_trace_point_emit_u16(port_id); > + rte_trace_point_emit_ptr(stats); > + rte_trace_point_emit_u64(stats->rx_nombuf); > +) > + Not sure what to record here, I think all basic stat counters can be recorded if there cost is effordable.