From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga01.intel.com (mga01.intel.com [192.55.52.88]) by dpdk.org (Postfix) with ESMTP id 1813A1B488 for ; Tue, 9 Oct 2018 12:37:00 +0200 (CEST) X-Amp-Result: SKIPPED(no attachment in message) X-Amp-File-Uploaded: False Received: from orsmga008.jf.intel.com ([10.7.209.65]) by fmsmga101.fm.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 09 Oct 2018 03:37:00 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.54,360,1534834800"; d="scan'208";a="79944202" Received: from irsmsx153.ger.corp.intel.com ([163.33.192.75]) by orsmga008.jf.intel.com with ESMTP; 09 Oct 2018 03:34:00 -0700 Received: from irsmsx102.ger.corp.intel.com ([169.254.2.67]) by IRSMSX153.ger.corp.intel.com ([169.254.9.121]) with mapi id 14.03.0319.002; Tue, 9 Oct 2018 11:33:59 +0100 From: "Van Haaren, Harry" To: Thomas Monjalon CC: "Laatz, Kevin" , "dev@dpdk.org" , "stephen@networkplumber.org" , "gaetan.rivet@6wind.com" , "shreyansh.jain@nxp.com" , "Richardson, Bruce" Thread-Topic: [PATCH v2 00/10] introduce telemetry library Thread-Index: AQHUWz+eJX4GKPxbOUKfYyMWY0UfzqUPCpJAgAAJrWCAABlkgIAHkIFw Date: Tue, 9 Oct 2018 10:33:58 +0000 Message-ID: References: <1535026093-101872-1-git-send-email-ciara.power@intel.com> <20181003173612.67101-1-kevin.laatz@intel.com> <4034734.dNxXkqf0MS@xps> In-Reply-To: <4034734.dNxXkqf0MS@xps> Accept-Language: en-US Content-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: x-titus-metadata-40: eyJDYXRlZ29yeUxhYmVscyI6IiIsIk1ldGFkYXRhIjp7Im5zIjoiaHR0cDpcL1wvd3d3LnRpdHVzLmNvbVwvbnNcL0ludGVsMyIsImlkIjoiYzQyNTA5M2ItOTMwZi00N2UyLWE2ZDUtZWU5YjM1MTgyNWI0IiwicHJvcHMiOlt7Im4iOiJDVFBDbGFzc2lmaWNhdGlvbiIsInZhbHMiOlt7InZhbHVlIjoiQ1RQX05UIn1dfV19LCJTdWJqZWN0TGFiZWxzIjpbXSwiVE1DVmVyc2lvbiI6IjE3LjEwLjE4MDQuNDkiLCJUcnVzdGVkTGFiZWxIYXNoIjoiQXE4RG14eFFJeHlvbVVkMEJ5Qlhjd2RlVW1ld1RtZGV6eVFFaElQU1BWRDdZWG9HY0Q0VkFSOW5rS1Q5eVJjaSJ9 x-ctpclassification: CTP_NT dlp-product: dlpe-windows dlp-version: 11.0.400.15 dlp-reaction: no-action x-originating-ip: [163.33.239.181] Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable MIME-Version: 1.0 Subject: Re: [dpdk-dev] [PATCH v2 00/10] introduce telemetry library X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Tue, 09 Oct 2018 10:37:01 -0000 > -----Original Message----- > From: Thomas Monjalon [mailto:thomas@monjalon.net] > Sent: Thursday, October 4, 2018 4:54 PM > To: Van Haaren, Harry > Cc: Laatz, Kevin ; dev@dpdk.org; > stephen@networkplumber.org; gaetan.rivet@6wind.com; shreyansh.jain@nxp.co= m; > Richardson, Bruce > Subject: Re: [PATCH v2 00/10] introduce telemetry library >=20 > 04/10/2018 15:25, Van Haaren, Harry: > > From: Van Haaren, Harry > > > From: Laatz, Kevin > > > > > > > > This patchset introduces a Telemetry library for DPDK Service > Assurance. > > > > This library provides an easy way to query DPDK Ethdev metrics. > > > > > > > > > > > > > Note: We are aware that the --telemetry flag is not working for mes= on > > > > builds, we are working on it for a future patch. Despite opterr be= ing > set > > > > to 0, --telemetry said to be 'unrecognized' as a startup print. Thi= s > is a > > > > cosmetic issue and will also be addressed. > > > > > > > > --- > > > > v2: > > > > - Reworked telemetry as part of EAL instead of using vdev (Gaeta= n) > > > > - Refactored rte_telemetry_command (Gaetan) > > > > - Added MAINTAINERS file entry (Stephen) > > > > - Updated docs to reflect vdev to eal rework > > > > - Removed collectd patch from patchset (Thomas) > > > > - General code clean up from v1 feedback > > > > > > > > > Hi Gaetan, Thomas, Stephen and Shreyansh! > > > > > > > > > goto TL_DR; // if time is short :) > > > > > > > > > In this v2 patchset, we've reworked the Telemetry to no longer use th= e > vdev > > > infrastructure, instead having EAL enable it directly. This was > requested as > > > feedback to the v1 patchset. I'll detail the approach below, and > highlight > > > some issues we identified while implementing it. > > > > > > Currently, EAL does not depend on any "DPDK" libraries (ignore kvargs > etc > > > for a minute). > > > Telemetry is a DPDK library, so it depends on EAL. In order to have E= AL > > > initialize > > > Telemetry, it must depend on it - this causes a circular dependency. > > > > > > This patchset resolves that circular dependency by using a "weak" sym= bol > for > > > telemetry init, and then the "strong" version of telemetry init will > replace > > > it when the library is compiled in. > > > > Correction: we attempted this approach - but ended up adding a TAILQ of > library > > initializers functions to EAL, which was then iterated during > rte_eal_init(). > > This also resolved the circular-dependency, but the same linking issue = as > > described below still exists. > > > > So - the same question still stands - what is the best solution for 18.= 11? > > > > > > > Although this *technically* works, it > > > requires > > > that applications *LINK* against Telemetry library explicitly - as EA= L > won't > > > pull > > > in the Telemetry .so automatically... This means application-level > build- > > > system > > > changes to enable --telemetry on the DPDK EAL command line. > > > > > > Given the complexity in enabling EAL to handle the Telemetry init() a= nd > its > > > dependencies, I'd like to ask you folks for input on how to better so= lve > > > this? >=20 > First, the telemetry feature must be enabled via a public function (API). > The application can decide to enable the feature at any time, right? > If the application wants to enable the feature at initialization > (and considers user input from the command line), > then the init function has a dependency on telemetry. > Your dependency concern is that the init function (which is high level) > is in EAL (which is the lowest layer in DPDK). Yes, and this has been resolved by allowing components to register with EAL to have their _init() function called later. V3 coming up with this approach, it seems to cover the required use-cases. > I think the command line should not be managed directly by EAL. > My suggestion in last summit was to move this code in a different library= . > We should also move the init function(s) to a new high level library. >=20 > This is my proposal to solve cyclic dependency: move rte_eal_init in a li= b > which depends on everything. I have prototyped this approach, and it is not really clean. It means splitting EAL into two halves, and due to meson library naming we have to move all eal files to eal_impl or something, and then eal.so keeps rte_e= al_init(). Removing functions from the .map files is also technically an ABI break, at which point I didn't think it was the right solution. > About the linking issue, I don't understand the problem. > If you use the DPDK makefiles, rte.app.mk should manage it. > If you use the DPDK meson, all libs are linked. > If you use your own system, of course you need to add telemetry lib. Yes agreed, in practice it should be exactly like this. In reality it can be harder to achieve the exact dependencies correctly with both Static/Shared builds and constructors etc. I believe the current approach of registering an _init() function will be acceptable, let's wait for v3 to hit the mailing list.