From mboxrd@z Thu Jan 1 00:00:00 1970 From: =?iso-8859-1?Q?Ga=EBtan?= Rivet Subject: Re: [PATCH 01/11] telemetry: initial telemetry infrastructure Date: Wed, 29 Aug 2018 10:23:11 +0200 Message-ID: <20180829082311.6cknwcvua2gsazb3@bidouze.vm.6wind.com> References: <1535026093-101872-1-git-send-email-ciara.power@intel.com> <1535026093-101872-2-git-send-email-ciara.power@intel.com> <20180828114633.vky27x53tu7eyag2@bidouze.vm.6wind.com> Mime-Version: 1.0 Content-Type: text/plain; charset=iso-8859-1 Content-Transfer-Encoding: 8bit Cc: "Power, Ciara" , "Archbold, Brian" , "Kenny, Emma" , "dev@dpdk.org" To: "Van Haaren, Harry" Return-path: Received: from mail-wr1-f66.google.com (mail-wr1-f66.google.com [209.85.221.66]) by dpdk.org (Postfix) with ESMTP id B20282B92 for ; Wed, 29 Aug 2018 10:23:28 +0200 (CEST) Received: by mail-wr1-f66.google.com with SMTP id a108-v6so3901480wrc.13 for ; Wed, 29 Aug 2018 01:23:28 -0700 (PDT) Content-Disposition: inline In-Reply-To: List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org Sender: "dev" On Tue, Aug 28, 2018 at 04:54:33PM +0000, Van Haaren, Harry wrote: > Hi Gaetan, > > > -----Original Message----- > > From: Gaëtan Rivet [mailto:gaetan.rivet@6wind.com] > > Sent: Tuesday, August 28, 2018 12:47 PM > > To: Power, Ciara > > Cc: Van Haaren, Harry ; Archbold, Brian > > ; Kenny, Emma ; dev@dpdk.org > > Subject: Re: [dpdk-dev] [PATCH 01/11] telemetry: initial telemetry > > infrastructure > > > > Hi Ciara, > > > > On Thu, Aug 23, 2018 at 01:08:03PM +0100, Ciara Power wrote: > > > This patch adds the infrastructure and initial code for the > > > telemetry library. > > > > > > A virtual device is used for telemetry, which is included in > > > this patch. To use telemetry, a command-line argument must be > > > used - "--vdev=telemetry". > > > > > > > Why use a virtual device? > > > > It seems that you are only using the device semantic as a way to carry > > around a tag telling the DPDK framework to init your library once it has > > finished its initialization. > > > > I guess you wanted to avoid having to add the call to rte_telemetry_init > > to all applications. In the absence of a proper EAL option framework, > > you can workaround by adding a --telemetry EAL parameter, setting a flag > > on, and checking this flag from librte_telemetry, within a routine > > declared with RTE_INIT_PRIO. > > I suppose that an EAL --flag could work too, it would mean that EAL would > depend on this library. The --vdev trick keeps the library standalone. > > I don't have a strong opinion either way. :) > This was done already for specific EAL configuration items such as vfio intr_mode or PCI uio configuration. Of course this is ugly, but the --telemetry parameter can exist without compiling the lib. You can add a warning if the TELEMETRY Mconfig item is not set to mitigate. The main issue is that you need to add getters because you cannot declare an external *struct internal_config* reference. I agree this is awkward, and this is exactly the reason we need a way for libraries to register options in the EAL, but this is not yet done. The virtual device solution however is a crutch used to emulate this absent framework. This will complicate developping the proper solution and its adoption once done. I would not be clear then to the dev that they can translate the telemetry shim parameter to the new framework, without having to rework the whole infrastructure of the lib (and this is without talking about reworking the build system to remove the telemetry driver). Even having to add a new driver subsection only for telemetry is awkward. So we might certainly wait for second or third opinions, but I am firmly convinced it would be easier in order to maintain the project (both from EAL and systems standpoint and library standpoint) without the vdev trick. > > > I only see afterward the selftest being triggered via kvargs. I haven't > > yet looked at the testing done, but if it is only unit test, the "test" > > app would be better suited. If it is integration testing to verify the > > behavior of the library with other PMDs, you probably need specific > > context, thus selftest being insufficient on its own and useless for > > other users. > > Correct, self tests are triggered by kvargs. This same model is used > in eg: eventdev PMDs to run selftests, where the tests are pretty complex > and specific to the device under test. > > Again, I don't have a strong opinion but I don't see any issue with it > being included in the vdev / telemetry library. We could write a shim > test that the "official" test binary runs the telemetry tests if that is > your concern? > > Okay, I have no strong opinion about this (actually I prefer having the test code close to the code-under-test), but eventdev can spawn device objects to drive the test and provide configuration. It would be more complicated using the same logic with a pure library, without the vdev. > > > Control threads are used to get CPU cycles for telemetry, which > > > are configured in this patch also. > > > > > > Signed-off-by: Ciara Power > > > Signed-off-by: Brian Archbold > > > > Regards, > > -- > > Gaëtan Rivet > > 6WIND > > Thanks for review, and there's a lightning talk at Userspace so please > do provide input there too :) -Harry -- Gaëtan Rivet 6WIND