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 D266442DC2; Mon, 3 Jul 2023 15:44:42 +0200 (CEST) Received: from mails.dpdk.org (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id 6154540EF0; Mon, 3 Jul 2023 15:44:42 +0200 (CEST) Received: from dkmailrelay1.smartsharesystems.com (smartserver.smartsharesystems.com [77.243.40.215]) by mails.dpdk.org (Postfix) with ESMTP id 94C8240ED5 for ; Mon, 3 Jul 2023 15:44:40 +0200 (CEST) Received: from smartserver.smartsharesystems.com (smartserver.smartsharesys.local [192.168.4.10]) by dkmailrelay1.smartsharesystems.com (Postfix) with ESMTP id 8474520644; Mon, 3 Jul 2023 15:44:38 +0200 (CEST) Content-class: urn:content-classes:message MIME-Version: 1.0 Content-Type: text/plain; charset="iso-8859-1" Content-Transfer-Encoding: quoted-printable Subject: RE: [PATCH v5 2/2] ethdev: support xstats reset telemetry command Date: Mon, 3 Jul 2023 15:44:36 +0200 Message-ID: <98CBD80474FA8B44BF855DF32C47DC35D87A5C@smartserver.smartshare.dk> X-MimeOLE: Produced By Microsoft Exchange V6.5 In-Reply-To: <18761354.fAMKPKieAE@thomas> X-MS-Has-Attach: X-MS-TNEF-Correlator: Thread-Topic: [PATCH v5 2/2] ethdev: support xstats reset telemetry command Thread-Index: AdmtftNIsMwEK+QGS3CqHONs7p8pmgANDrrw References: <20221219090723.29356-1-fengchengwen@huawei.com> <12802674.ZYm5mLc6kN@thomas> <18761354.fAMKPKieAE@thomas> From: =?iso-8859-1?Q?Morten_Br=F8rup?= To: "Thomas Monjalon" , "Dongdong Liu" , "fengchengwen" Cc: "Bruce Richardson" , , "Ferruh Yigit" , , , , 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 > From: Thomas Monjalon [mailto:thomas@monjalon.net] > Sent: Monday, 3 July 2023 09.20 >=20 > 03/07/2023 05:58, fengchengwen: > > > > On 2023/2/20 21:05, Thomas Monjalon wrote: > > > 17/02/2023 10:44, fengchengwen: > > >> On 2023/2/16 20:54, Bruce Richardson wrote: > > >>> On Thu, Feb 16, 2023 at 08:42:34PM +0800, fengchengwen wrote: > > >>>> On 2023/2/16 20:06, Ferruh Yigit wrote: > > >>>>> On 2/16/2023 11:53 AM, fengchengwen wrote: > > >>>>>> On 2023/2/15 11:19, Dongdong Liu wrote: > > >>>>>>> Hi Chengwen > > >>>>>>> > > >>>>>>> On 2023/2/9 10:32, Chengwen Feng wrote: > > >>>>>>>> The xstats reset is useful for debugging, so add it to the > ethdev > > >>>>>>>> telemetry command lists. > > >>>>>>>> > > >>>>>>>> Signed-off-by: Chengwen Feng > > >>>>>>> This patch looks good, so > > >>>>>>> Reviewed-by: Dongdong Liu > > >>>>>>> > > >>>>>>> A minior question > > >>>>>>> Do we need to support stats reset ? > > >>>>>> > > >>>>>> Stats is contained by xstats, and future direction I think is > xstats. > > >>>>>> So I think we don't need support stats reset. > > >>>>>> > > >>>>> > > >>>>> I have similar question with Dongdong, readonly values are = safe > for > > >>>>> telemetry, but modifying data can be more tricky since we = don't > have > > >>>>> locking in ethdev APIs, this can cause concurrency issues. > > >>>> > > >>>> Yes, it indeed has concurrency issues. > > >>>> > > >>>>> > > >>>>> Overall do we want telemetry go that way and become something > that > > >>>>> alters ethdev data/config? > > >>>> > > >>>> There are at least two part of data: config and status. > > >>>> For stats (which belong status data) could help for debugging, = I > think it's acceptable. > > >>>> > > >>>> As for concurrency issues. People should know what to do and = when > to do, just like > > >>>> the don't invoke config API (e.g. dev_configure/dev_start/...) > concurrency. > > >>>> > > >>> While this is probably ok for now, I think in next release we > should look > > >>> to add some sort of support for locking for destructive ops in a > future > > >>> release. For example, we could: > > >>> > > >>> 1. Add support for marking a callback as "destructive" and only > allow it to > > >>> be called if only one connection is present or > > >>> > > >>> 2. Make it possible for callbacks to query the number of > connections so > > >>> that the callback itself is non-destructive in more than one > connection is > > >>> open. > > >>> > > >>> [Both of these will require locking support so that new > connections aren't > > >>> openned when the callback is in-flight!] > > >> > > >> Except telemetry, the application may have other console could > execute DPDK API. > > >> So I think trying to keep it simple, it's up to the user to = invoke. > > > > > > No, the user should not be responsible for concurrency issues. > > > We can ask the app developper to take care, > > > but not to the user who has no control on what happens in the app. > > > > > > On a more general note, I feel the expansion of telemetry is not > controlled enough. > > > I would like to stop on adding more telemetry until we have a = clear > guideline > > > about what is telemetry for and how to use it. > > > > Hi Thomas, > > > > Should this be discussed on TB? >=20 > What would be your question exactly? A general comment about telemetry: If an application exposes telemetry through an end user facing API, e.g. = http(s) REST, it would be nice if non-read-only telemetry paths are easy = to identify by following some DPDK standard convention, so the = application does not need to manually maintain an allow-list of = read-only paths. Bruce's documentation about trace/log/telemetry/dump might also need to = be updated regarding non-read-only telemetry actions.