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 9B81A418FD; Sun, 8 Jan 2023 16:41:15 +0100 (CET) Received: from mails.dpdk.org (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id 3AF2E40687; Sun, 8 Jan 2023 16:41:15 +0100 (CET) Received: from mx0b-0016f401.pphosted.com (mx0b-0016f401.pphosted.com [67.231.156.173]) by mails.dpdk.org (Postfix) with ESMTP id E0D9A4067C for ; Sun, 8 Jan 2023 16:41:13 +0100 (CET) Received: from pps.filterd (m0045851.ppops.net [127.0.0.1]) by mx0b-0016f401.pphosted.com (8.17.1.19/8.17.1.19) with ESMTP id 308DfdZH006889; Sun, 8 Jan 2023 07:41:13 -0800 Received: from nam12-mw2-obe.outbound.protection.outlook.com (mail-mw2nam12lp2040.outbound.protection.outlook.com [104.47.66.40]) by mx0b-0016f401.pphosted.com (PPS) with ESMTPS id 3my94tjftc-1 (version=TLSv1.2 cipher=ECDHE-RSA-AES256-GCM-SHA384 bits=256 verify=NOT); Sun, 08 Jan 2023 07:41:12 -0800 ARC-Seal: i=1; a=rsa-sha256; s=arcselector9901; d=microsoft.com; cv=none; b=bSGnmHpOykalsnMt7IiFKCbsTCURJNcdKQHJBaTL67Y0Y7AIV/U+MgVn1WeeoKjzsuKHzZgMCRRp1MzQ7DI/+G9Gl2iM81cPch+/ywaZHkbSpxWmNJdfwEHBvukPn/P6cfaIaBGB4oL1uHsdlhmslVLs2QyDw8ff+Ks3aFyMI4qa+TfukVQeTeSRAoPYa4qJ4xh2q43GXUesSnhPSKyFXjms58SeTgHKyQTBrLPZlWJ6mS5gvBCHSw+2JZVOTPajj2u21V8T003gLdRKdAuU0p2uxwmQlkQSywl37cRIX95Fv4wmrWypKeEv/KlNkERXW1TM2Gb5Wp9cXeaUxML7bw== 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=5tCbGKYyl0WgLCJKc7o8wV0bgMrXFeavU3lrCgfREzY=; b=IElPXx1PUq1DREuZ7fr8w/vONI2lU0nTYLxqGknTLQpH44mf82fwuXSQQtfNMItOMGkPc4Tv7zMyMOBWKgxpfnZso0Jx83y/xi1kzuf5Y5v/Z/jhyjNgompnq4nRqNqCE1vGmExQR/tDCycGqS10S/nyu9UUNFMPx8JdDECM4nyEWmoDkrWhQEGtrq3Zon/nAx1Xqq2tOyEZbjlig7TsNCVDrwy14p03VdZhRobxq5zFbzljqWhgPK+0mFeP7e+Ve7UZ6rFAA8WoD+in24cs0B/dVd2pu+Gau2nVSKpZ8yml8AQd9Khl9aExQjj9twGL9YzIIcBC4nt1nxoPH/kxwg== ARC-Authentication-Results: i=1; mx.microsoft.com 1; spf=pass smtp.mailfrom=marvell.com; dmarc=pass action=none header.from=marvell.com; dkim=pass header.d=marvell.com; arc=none DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=marvell.onmicrosoft.com; s=selector1-marvell-onmicrosoft-com; h=From:Date:Subject:Message-ID:Content-Type:MIME-Version:X-MS-Exchange-SenderADCheck; bh=5tCbGKYyl0WgLCJKc7o8wV0bgMrXFeavU3lrCgfREzY=; b=njUqPP7pWoezgOkSnsZY6uKu2firCpKa8wYOTWohzLu9m0zQvQDwF3Cn9B+4YqnFvew0/Tg//LkPiPVgAbiY1SG41IAoZIRAmCPDtUdpbFI/vy+bXfLuNMJijIZ88ptLAf6StFn1uZUoYDGANHp6rq97hYjVildN1rrt83eNVcc= Received: from DM4PR18MB4368.namprd18.prod.outlook.com (2603:10b6:5:39d::6) by SJ0PR18MB3834.namprd18.prod.outlook.com (2603:10b6:a03:2e5::21) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id 15.20.5986.18; Sun, 8 Jan 2023 15:41:08 +0000 Received: from DM4PR18MB4368.namprd18.prod.outlook.com ([fe80::3117:f51c:37c2:fa05]) by DM4PR18MB4368.namprd18.prod.outlook.com ([fe80::3117:f51c:37c2:fa05%9]) with mapi id 15.20.5986.018; Sun, 8 Jan 2023 15:41:08 +0000 From: Tomasz Duszynski To: =?iso-8859-1?Q?Morten_Br=F8rup?= , "dev@dpdk.org" CC: "thomas@monjalon.net" , Jerin Jacob Kollanukkaran , "zhoumin@loongson.cn" , "mattias.ronnblom@ericsson.com" Subject: RE: [PATCH v4 1/4] eal: add generic support for reading PMU events Thread-Topic: [PATCH v4 1/4] eal: add generic support for reading PMU events Thread-Index: AQHZDt/QEhRtFHSga0yHSOMIy5Zzs65rtOuAgAFVOCCAABlQ4IAjRt0AgAAVgdCAAM6BwA== Date: Sun, 8 Jan 2023 15:41:08 +0000 Message-ID: References: <20221129092821.1304853-1-tduszynski@marvell.com> <20221213104350.3218167-1-tduszynski@marvell.com> <20221213104350.3218167-2-tduszynski@marvell.com> <98CBD80474FA8B44BF855DF32C47DC35D8758C@smartserver.smartshare.dk> <98CBD80474FA8B44BF855DF32C47DC35D8759B@smartserver.smartshare.dk> A <98CBD80474FA8B44BF855DF32C47DC35D8762C@smartserver.smartshare.dk> In-Reply-To: <98CBD80474FA8B44BF855DF32C47DC35D8762C@smartserver.smartshare.dk> Accept-Language: en-US Content-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: x-ms-publictraffictype: Email x-ms-traffictypediagnostic: DM4PR18MB4368:EE_|SJ0PR18MB3834:EE_ x-ms-office365-filtering-correlation-id: e33fa554-3b7e-4371-49d1-08daf18ec2ba x-ms-exchange-senderadcheck: 1 x-ms-exchange-antispam-relay: 0 x-microsoft-antispam: BCL:0; x-microsoft-antispam-message-info: 2Ho6ncNnY8hj0pHngRlxI8cJHXA3TeKzv+Mc8FdsvdxRzAbe4BtXwzcn8Mlu9wQuLfbqGOdO/oB86ozdoSOwkgkwbZgckFwO9mPqpbn7QLyutg/n+njDdcwNKLJtcuN80G9UajOayIMW2NEm1qpDZE4AAooW5eWVTeIW/v3CWxjIAbl4MBB9zkFVIYlooC+cvpcsaO1tPpX3uxUNvIyFQwCBb4RDcdf9Dt5UIRd7SjFNXwdhqA8pwFxt8dfv9dRiZ/MCUL6NSbAlAqh9SkYhPOiK0YkZZCF15wXLsPec3oykXcyMsB2oSHUS2SqOIos3jJwtHNPgK7DG/vJMcL50pE1BQRUxhOwICt7VxmMUohEASix7PAFysO9PrlPZtxXtq3GcJjaImfmSjgjZTEiq5rlDdOdeUAfFZIJhfe07tAu0lJchotgZtja3qQj4rfaRSW0m1NQJqQ7rCS5LS1vaDm8Ui+uz6z93FLE431tAYGRnYHZOT8GHuDFO53GEy3YwwcoxoO5EFX4nFRqT/exTweGVt14thGfSQuNpPNFJZf71yfDjQLhvuvvZeWDzimckB6uJuTCnXZ8VM9C5W7JklxQ3BsNCBsKtJ+zJOO/rFp5sb4bBxtTJUBHO9E/mRraYeBhehT5E4xe2MONjofNKh3z1tmMaz1Wbo8L515uGyQScEb5pRNlyHAvXj23Y+wv0k6Urm01+6ZeAuhyVKmWhF9oEZPuJNg1JKr3PXnKuX7eKUPTjeTFHqKs3RP080z4t9Mw6uLBOuweD1jKzzz8W6TIwB+HVT1+VyHdVR611yPM= x-forefront-antispam-report: CIP:255.255.255.255; CTRY:; LANG:en; SCL:1; SRV:; IPV:NLI; SFV:NSPM; H:DM4PR18MB4368.namprd18.prod.outlook.com; PTR:; CAT:NONE; SFS:(13230022)(4636009)(366004)(136003)(396003)(376002)(346002)(39850400004)(451199015)(19627235002)(33656002)(30864003)(316002)(5660300002)(71200400001)(7696005)(26005)(186003)(9686003)(966005)(478600001)(38070700005)(41300700001)(66574015)(110136005)(66476007)(54906003)(4326008)(66946007)(66556008)(66446008)(64756008)(8676002)(76116006)(83380400001)(8936002)(52536014)(86362001)(55016003)(6506007)(122000001)(38100700002)(2906002)(41533002); DIR:OUT; SFP:1101; x-ms-exchange-antispam-messagedata-chunkcount: 1 x-ms-exchange-antispam-messagedata-0: =?iso-8859-1?Q?aYm+8ECDv6DgLU/KuL+uxO0jjrgyWU0PWHb0beyQpzRpeLxHa9bGhDw9+u?= =?iso-8859-1?Q?6qoirK1B1DN0gAMDGRv6Tcl9Yf2MvmvQR8f6W1Fl91JyX+nIfHuy4WWoCM?= =?iso-8859-1?Q?n1DmfBPuFg5gkBB0a2NvAID0fXjIEdb/9J/yugvgOPtyDpyjWlWkMQINnR?= =?iso-8859-1?Q?S7ML398BPFXVmyJRCIqKDVRmMoBSfNf1X/CSS/1Zdw0PqespXEnRjoFX+p?= =?iso-8859-1?Q?8twf52N8nMHLo/s8U7OUGH0UKBZBcYTlTzxY9+wgaA1aY9d/KQLEQ/FNXo?= =?iso-8859-1?Q?1x2WEpr4GzXkNTZaP2k7KOuL4c5p5K5wMUL2sm8dYmTI2p+NmIc4DR7lRO?= =?iso-8859-1?Q?9Sxcekf+iuq9tEKW/NhYWINNgFwMikzyzsxWef4P6g3mrxbsIdNcRPW4YM?= =?iso-8859-1?Q?hkiK86zkIJufehHM2bXiKfbjtH5vOhE2xOogsG/wZXP1pyC4sfw7SlgM1n?= =?iso-8859-1?Q?euJoREE+0dAwlRcrJI1WebVUIdi3mUC4SyCVXtMoXBRSpg9aJyYxh2IVQR?= =?iso-8859-1?Q?rGkhEQwDoPiRUDo4Mo24EmGGuLOXyBEXhPbVeTGPpZ75ETSkh0IPz1E4pX?= =?iso-8859-1?Q?G/Mddu0F6bmbrqYI8MBPyG0+x5ynATgiBDQAb3QkPi0NateBL1PM4FR1YU?= =?iso-8859-1?Q?bxuqmBi6PqRw3CjB+socbj2+RnZ9y73eMFRl7w9wLZCgysOxdy98T8RGT4?= =?iso-8859-1?Q?18iIE59Iyp+PMdMGdyUps/UbUjn90JVvZR+qtxGD/KuY05t4A8XOA3bJ3k?= =?iso-8859-1?Q?vuo3fd4ISc256U9cpLVElvzz4nxCNYxv8ItO/31wvoaMLZlh+tHt2k/9nc?= =?iso-8859-1?Q?om+5qRkjceHlss1CkSwrPNHG3ctCuqKii8YaXGslkFWg92uBfTuxwyjxEK?= =?iso-8859-1?Q?SU04ywEyvHlg8FipGmQoW3Z5+/yC8Y2iIvpIoMUk8qQrVp6/0wgxGNzp19?= =?iso-8859-1?Q?X/5hlThaSAm7C1VV0YJP05oE9VBwO+osj+iuDlYC8XAmVlGd4iXqyN0ezl?= =?iso-8859-1?Q?CFol32VfunsnPLElFqa/y4hwHZ6ssm/3N2KJNTFiqz1O/HiFtzybVY6gE1?= =?iso-8859-1?Q?LcaHPdRRcgGtaRd9hHac62gQ9tS+HznbVsXvC2DrZiOhKD/TsoU5Ioulrg?= =?iso-8859-1?Q?tKt0Lfi0bTdNs+zhiDwG+7j9iSZu0cfrASnLP1JKLaz/9J7rq+JtbkD0Z3?= =?iso-8859-1?Q?+hDS6uZsLB+NBhJULYqhP4uJuu+4Wcf7ZAV0K4+/SuXaZ3ynlGRskNSZty?= =?iso-8859-1?Q?TETEh6UDfMC2UwMlDNu7YvmGkAIxCFTuEH6epBQlX09AdutBkoTuWIxJ/i?= =?iso-8859-1?Q?BgLOmBRVo3hv7AGQURB1/3o9+nuouRi8LBLcii0OUIA4lzUyRsPQqOlSYr?= =?iso-8859-1?Q?A9A3jOUwRP0xId0HJxX8hVRJGRTlcegQYl1jRoTFBAkhNuqoqcTyylCFW+?= =?iso-8859-1?Q?LbEtMQCBxFHufWi3LfWajEPi6SqYREqbp9dHR3VGUoBOhZatP2brEhCTCZ?= =?iso-8859-1?Q?oHTK3grfyz4oOVCiBM1XYwEqbMVqKm/vj12Ak3UE1AInR0iByv/2m4tRRo?= =?iso-8859-1?Q?LH8Z50ssHQP/QkMyW4yHlzOXWlkYVs3wFwf4UcYdOZ1BZ5/NRv/KfXXeNp?= =?iso-8859-1?Q?fGnSFVw30HISXsxBcKxxtKk+BNsB8NKxqK?= Content-Type: text/plain; charset="iso-8859-1" Content-Transfer-Encoding: quoted-printable MIME-Version: 1.0 X-OriginatorOrg: marvell.com X-MS-Exchange-CrossTenant-AuthAs: Internal X-MS-Exchange-CrossTenant-AuthSource: DM4PR18MB4368.namprd18.prod.outlook.com X-MS-Exchange-CrossTenant-Network-Message-Id: e33fa554-3b7e-4371-49d1-08daf18ec2ba X-MS-Exchange-CrossTenant-originalarrivaltime: 08 Jan 2023 15:41:08.0673 (UTC) X-MS-Exchange-CrossTenant-fromentityheader: Hosted X-MS-Exchange-CrossTenant-id: 70e1fb47-1155-421d-87fc-2e58f638b6e0 X-MS-Exchange-CrossTenant-mailboxtype: HOSTED X-MS-Exchange-CrossTenant-userprincipalname: ZqrO3rC9TSSicaapc9ucj0NN+z6XTSB12QXbdF6J/eGDAPbzGGiTCnokHiuMfx7ktaIez8xVm/7OKs3z92M8mA== X-MS-Exchange-Transport-CrossTenantHeadersStamped: SJ0PR18MB3834 X-Proofpoint-ORIG-GUID: B8aOkQr9hmjMP2gS_6r_-TqoCRcEfg5d X-Proofpoint-GUID: B8aOkQr9hmjMP2gS_6r_-TqoCRcEfg5d X-Proofpoint-Virus-Version: vendor=baseguard engine=ICAP:2.0.205,Aquarius:18.0.923,Hydra:6.0.545,FMLib:17.11.122.1 definitions=2023-01-08_11,2023-01-06_01,2022-06-22_01 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 >-----Original Message----- >From: Morten Br=F8rup >Sent: Thursday, January 5, 2023 11:08 PM >To: Tomasz Duszynski ; dev@dpdk.org >Cc: thomas@monjalon.net; Jerin Jacob Kollanukkaran ; z= houmin@loongson.cn; >mattias.ronnblom@ericsson.com >Subject: [EXT] RE: [PATCH v4 1/4] eal: add generic support for reading PMU= events > >External Email > >---------------------------------------------------------------------- >> From: Tomasz Duszynski [mailto:tduszynski@marvell.com] >> Sent: Thursday, 5 January 2023 22.14 >> >> Hi Morten, >> >> A few comments inline. >> >> >From: Morten Br=F8rup >> >Sent: Wednesday, December 14, 2022 11:41 AM >> > >> >External Email >> > >> >--------------------------------------------------------------------- >> >- >> >+CC: Mattias, see my comment below about per-thread constructor for >> this >> > >> >> From: Tomasz Duszynski [mailto:tduszynski@marvell.com] >> >> Sent: Wednesday, 14 December 2022 10.39 >> >> >> >> Hello Morten, >> >> >> >> Thanks for review. Answers inline. >> >> >> >> [...] >> >> >> >> > > +/** >> >> > > + * @file >> >> > > + * >> >> > > + * PMU event tracing operations >> >> > > + * >> >> > > + * This file defines generic API and types necessary to setup >> PMU >> >> and >> >> > > + * read selected counters in runtime. >> >> > > + */ >> >> > > + >> >> > > +/** >> >> > > + * A structure describing a group of events. >> >> > > + */ >> >> > > +struct rte_pmu_event_group { >> >> > > + int *fds; /**< array of event descriptors */ >> >> > > + void **mmap_pages; /**< array of pointers to mmapped >> >> > > perf_event_attr structures */ >> >> > >> >> > There seems to be a lot of indirection involved here. Why are >> these >> >> arrays not statically sized, >> >> > instead of dynamically allocated? >> >> > >> >> >> >> Different architectures/pmus impose limits on number of >> simultaneously >> >> enabled counters. So in order relief the pain of thinking about it >> and >> >> adding macros for each and every arch I decided to allocate the >> number >> >> user wants dynamically. Also assumption holds that user knows about >> >> tradeoffs of using too many counters hence will not enable too many >> >> events at once. >> > >> >The DPDK convention is to use fixed size arrays (with a maximum size, >> e.g. RTE_MAX_ETHPORTS) in the >> >fast path, for performance reasons. >> > >> >Please use fixed size arrays instead of dynamically allocated arrays. >> > >> >> I do agree that from maintenance angle fixed arrays are much more >> convenient but when optimization kicks in then that statement does not >> necessarily hold true anymore. >> >> For example, in this case performance dropped by ~0.3% which is >> insignificant imo. So given simpler code, next patchset will use fixed >> arrays. > >I fail to understand how pointer chasing can perform better than obtaining= an address by >multiplying by a constant. Modern CPUs work in mysterious ways, and you ob= viously tested this, so I >believe your test results. But in theory, pointer chasing touches more cac= he lines, and should >perform worse in a loaded system where pointers in the chain have been evi= cted from the cache. > >Anyway, you agreed to use fixed arrays, so I am happy. :-) > >> >> >> >> >> > Also, what is the reason for hiding the type struct >> >> perf_event_mmap_page **mmap_pages opaque by >> >> > using void **mmap_pages instead? >> >> >> >> I think, that part doing mmap/munmap was written first hence void >> >> ** was chosen in the first place. >> > >> >Please update it, so the actual type is reflected here. >> > >> >> >> >> > >> >> > > + bool enabled; /**< true if group was enabled on particular >> lcore >> >> > > */ >> >> > > +}; >> >> > > + >> >> > > +/** >> >> > > + * A structure describing an event. >> >> > > + */ >> >> > > +struct rte_pmu_event { >> >> > > + char *name; /** name of an event */ >> >> > > + int index; /** event index into fds/mmap_pages */ >> >> > > + TAILQ_ENTRY(rte_pmu_event) next; /** list entry */ }; >> >> > > + >> >> > > +/** >> >> > > + * A PMU state container. >> >> > > + */ >> >> > > +struct rte_pmu { >> >> > > + char *name; /** name of core PMU listed under >> >> > > /sys/bus/event_source/devices */ >> >> > > + struct rte_pmu_event_group group[RTE_MAX_LCORE]; /**< per >> lcore >> >> > > event group data */ >> >> > > + int num_group_events; /**< number of events in a group */ >> >> > > + TAILQ_HEAD(, rte_pmu_event) event_list; /**< list of >> matching >> >> > > events */ >> > >> >The event_list is used in slow path only, so it can remain a list - >> i.e. no change requested here. >> >:-) >> > >> >> > > +}; >> >> > > + >> >> > > +/** Pointer to the PMU state container */ extern struct >> >> > > +rte_pmu *rte_pmu; >> >> > >> >> > Again, why not just extern struct rte_pmu, instead of dynamic >> >> allocation? >> >> > >> >> >> >> No strong opinions here since this is a matter of personal >> preference. >> >> Can be removed >> >> in the next version. >> > >> >Yes, please. >> > >> >> >> >> > > + >> >> > > +/** Each architecture supporting PMU needs to provide its own >> >> version >> >> > > */ >> >> > > +#ifndef rte_pmu_pmc_read >> >> > > +#define rte_pmu_pmc_read(index) ({ 0; }) #endif >> >> > > + >> >> > > +/** >> >> > > + * @internal >> >> > > + * >> >> > > + * Read PMU counter. >> >> > > + * >> >> > > + * @param pc >> >> > > + * Pointer to the mmapped user page. >> >> > > + * @return >> >> > > + * Counter value read from hardware. >> >> > > + */ >> >> > > +__rte_internal >> >> > > +static __rte_always_inline uint64_t >> rte_pmu_read_userpage(struct >> >> > > +perf_event_mmap_page *pc) { >> >> > > + uint64_t offset, width, pmc =3D 0; >> >> > > + uint32_t seq, index; >> >> > > + int tries =3D 100; >> >> > > + >> >> > > + for (;;) { >> > >> >As a matter of personal preference, I would write this loop >> differently: >> > >> >+ for (tries =3D 100; tries !=3D 0; tries--) { >> > >> >> > > + seq =3D pc->lock; >> >> > > + rte_compiler_barrier(); >> >> > > + index =3D pc->index; >> >> > > + offset =3D pc->offset; >> >> > > + width =3D pc->pmc_width; >> >> > > + >> >> > > + if (likely(pc->cap_user_rdpmc && index)) { >> > >> >Why "&& index"? The way I read [man perf_event_open], index 0 is >> perfectly valid. >> > >> >> Valid index starts at 1. 0 means that either hw counter is stopped or >> isn't active. Maybe this is not initially clear from man but there's >> example later on how to get actual number. > >OK. Thanks for the reference. > >Please add a comment about the special meaning of index 0 in the code. > >> >> >[man perf_event_open]: >> https://urldefense.proofpoint.com/v2/url?u=3Dhttps- >> 3A__man7.org_linux_man- >> >2Dpages_man2_perf-5Fevent- >> >5Fopen.2.html&d=3DDwIFAw&c=3DnKjWec2b6R0mOyPaz7xtfQ&r=3DPZNXgrbjdlXxVEE= GYkx >> >I >> xRndyEUwWU_ad5ce22YI6Is&m=3Dtny >> >gBVwOnoZDV7hItku1HtmsI8R3F6vPJdr7ON3hE5iAds96T2C9JTNcnt6ptN4Q&s=3Ds10yJ >> >o >> gwRRXHqAuIay47H- >> >aWl9SL5wpQ9tCjfiQUgrY&e=3D >> > >> >> > > + pmc =3D rte_pmu_pmc_read(index - 1); >> >> > > + pmc <<=3D 64 - width; >> >> > > + pmc >>=3D 64 - width; >> >> > > + } >> >> > > + >> >> > > + rte_compiler_barrier(); >> >> > > + >> >> > > + if (likely(pc->lock =3D=3D seq)) >> >> > > + return pmc + offset; >> >> > > + >> >> > > + if (--tries =3D=3D 0) { >> >> > > + RTE_LOG(DEBUG, EAL, "failed to get >> >> > > perf_event_mmap_page lock\n"); >> >> > > + break; >> >> > > + } >> > >> >- Remove the 4 above lines of code, and move the debug log message to >> the end of the function >> >instead. >> > >> >> > > + } >> > >> >+ RTE_LOG(DEBUG, EAL, "failed to get perf_event_mmap_page lock\n"); >> > >> >> > > + >> >> > > + return 0; >> >> > > +} >> >> > > + >> >> > > +/** >> >> > > + * @internal >> >> > > + * >> >> > > + * Enable group of events for a given lcore. >> >> > > + * >> >> > > + * @param lcore_id >> >> > > + * The identifier of the lcore. >> >> > > + * @return >> >> > > + * 0 in case of success, negative value otherwise. >> >> > > + */ >> >> > > +__rte_internal >> >> > > +int >> >> > > +rte_pmu_enable_group(int lcore_id); >> >> > > + >> >> > > +/** >> >> > > + * @warning >> >> > > + * @b EXPERIMENTAL: this API may change without prior notice >> >> > > + * >> >> > > + * Add event to the group of enabled events. >> >> > > + * >> >> > > + * @param name >> >> > > + * Name of an event listed under >> >> > > /sys/bus/event_source/devices/pmu/events. >> >> > > + * @return >> >> > > + * Event index in case of success, negative value otherwise. >> >> > > + */ >> >> > > +__rte_experimental >> >> > > +int >> >> > > +rte_pmu_add_event(const char *name); >> >> > > + >> >> > > +/** >> >> > > + * @warning >> >> > > + * @b EXPERIMENTAL: this API may change without prior notice >> >> > > + * >> >> > > + * Read hardware counter configured to count occurrences of an >> >> event. >> >> > > + * >> >> > > + * @param index >> >> > > + * Index of an event to be read. >> >> > > + * @return >> >> > > + * Event value read from register. In case of errors or lack >> of >> >> > > support >> >> > > + * 0 is returned. In other words, stream of zeros in a trace >> >> file >> >> > > + * indicates problem with reading particular PMU event >> register. >> >> > > + */ >> >> > > +__rte_experimental >> >> > > +static __rte_always_inline uint64_t rte_pmu_read(int index) >> > >> >The index type can be changed from int to uint32_t. This also >> eliminates the "(index < 0" part of >> >the comparison further below in this function. >> > >> >> That's true. >> >> >> > > +{ >> >> > > + int lcore_id =3D rte_lcore_id(); >> >> > > + struct rte_pmu_event_group *group; >> >> > > + int ret; >> >> > > + >> >> > > + if (!rte_pmu) >> >> > > + return 0; >> >> > > + >> >> > > + group =3D &rte_pmu->group[lcore_id]; >> >> > > + if (!group->enabled) { >> > >> >Optimized: if (unlikely(!group->enabled)) { >> > >> >> Compiler will optimize the branch itself correctly. Extra hint is not >> required. > >I haven't reviewed the output from this, so I'll take your word for it. I = suggested the unlikely() >because I previously tested some very simple code, and it optimized for ta= king the "if": > >void testb(bool b) >{ > if (!b) > exit(1); > > exit(99); >} > >I guess I should experiment with more realistic code, and update my optimi= zation notes! > I think this may be too simple to draw far-reaching conclusions from it. Co= mpiler will make the fall-through path more likely. If I recall Intel Optimization Reference Man= ual has some more info on this.=20 Lets take a different example. =20 int main(int argc, char *argv[]) { int *p; p =3D malloc(sizeof(*p)); if (!p) return 1; *p =3D atoi(argv[1]); if (*p < 0) return 2; free(p); return 0; } If compiled with -O3 and disassembled I got below.=20 00000000000010a0
: 10a0: f3 0f 1e fa endbr64 10a4: 55 push %rbp 10a5: bf 04 00 00 00 mov $0x4,%edi 10aa: 53 push %rbx 10ab: 48 89 f3 mov %rsi,%rbx 10ae: 48 83 ec 08 sub $0x8,%rsp 10b2: e8 d9 ff ff ff call 1090 10b7: 48 85 c0 test %rax,%rax 10ba: 74 31 je 10ed 10bc: 48 8b 7b 08 mov 0x8(%rbx),%rdi 10c0: ba 0a 00 00 00 mov $0xa,%edx 10c5: 31 f6 xor %esi,%esi 10c7: 48 89 c5 mov %rax,%rbp 10ca: e8 b1 ff ff ff call 1080 10cf: 49 89 c0 mov %rax,%r8 10d2: b8 02 00 00 00 mov $0x2,%eax 10d7: 45 85 c0 test %r8d,%r8d 10da: 78 0a js 10e6 10dc: 48 89 ef mov %rbp,%rdi 10df: e8 8c ff ff ff call 1070 10e4: 31 c0 xor %eax,%eax 10e6: 48 83 c4 08 add $0x8,%rsp 10ea: 5b pop %rbx 10eb: 5d pop %rbp 10ec: c3 ret 10ed: b8 01 00 00 00 mov $0x1,%eax 10f2: eb f2 jmp 10e6 Looking at both 10ba and 10da suggests that code was laid out in a way that= jumping is frowned upon. Also=20 potentially lest frequently executed code (at 10ed) is pushed further down = the memory hence optimizing cache line usage.=20 That said, each and every scenario needs analysis on its own.=20 >You could add the unlikely() for readability purposes. ;-) > Sure. That won't hurt performance. =20 >> >> >> > > + ret =3D rte_pmu_enable_group(lcore_id); >> >> > > + if (ret) >> >> > > + return 0; >> >> > > + >> >> > > + group->enabled =3D true; >> >> > > + } >> >> > >> >> > Why is the group not enabled in the setup function, >> >> rte_pmu_add_event(), instead of here, in the >> >> > hot path? >> >> > >> >> >> >> When this is executed for the very first time then cpu will have >> >> obviously more work to do but afterwards setup path is not taken >> hence >> >> much less cpu cycles are required. >> >> >> >> Setup is executed by main lcore solely, before lcores are executed >> >> hence some info passed to SYS_perf_event_open ioctl() is missing, >> pid >> >> (via rte_gettid()) being an example here. >> > >> >OK. Thank you for the explanation. Since impossible at setup, it has >> to be done at runtime. >> > >> >@Mattias: Another good example of something that would belong in per- >> thread constructors, as my >> >suggested feature creep in [1]. >> > >> >[1]: https://urldefense.proofpoint.com/v2/url?u=3Dhttp- >> >3A__inbox.dpdk.org_dev_98CBD80474FA8B44BF855DF32C47DC35D87553- >> >40smartserver.smartshare.dk_&d=3DDwIFAw&c=3DnKjWec2b6R0mOyPaz7xtfQ&r=3D= PZNX >> >g >> rbjdlXxVEEGYkxIxRndyEUwWU_ad5 >> >ce22YI6Is&m=3DtnygBVwOnoZDV7hItku1HtmsI8R3F6vPJdr7ON3hE5iAds96T2C9JTNcn >> >t >> 6ptN4Q&s=3DaSAnYqgVnrgDp6yyMtGC >> >uWgJjDlgqj1wHf1nGWyHCNo&e=3D >> > >> >> >> >> > > + >> >> > > + if (index < 0 || index >=3D rte_pmu->num_group_events) >> > >> >Optimized: if (unlikely(index >=3D rte_pmu.num_group_events)) >> > >> >> > > + return 0; >> >> > > + >> >> > > + return rte_pmu_read_userpage((struct perf_event_mmap_page >> >> > > *)group->mmap_pages[index]); >> >> > >> >> > Using fixed size arrays instead of multiple indirections via >> >> > pointers >> >> is faster. It could be: >> >> > >> >> > return rte_pmu_read_userpage((struct perf_event_mmap_page >> >> > *)rte_pmu.group[lcore_id].mmap_pages[index]); >> >> > >> >> > With our without suggested performance improvements... >> >> > >> >> > Series-acked-by: Morten Br=F8rup >> >> >>