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 BC9E0A00BE; Thu, 16 Jun 2022 11:00:11 +0200 (CEST) Received: from [217.70.189.124] (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id A380B42BCA; Thu, 16 Jun 2022 11:00:11 +0200 (CEST) Received: from smartserver.smartsharesystems.com (smartserver.smartsharesystems.com [77.243.40.215]) by mails.dpdk.org (Postfix) with ESMTP id 2AAB04281E for ; Thu, 16 Jun 2022 11:00:10 +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 0/5] support telemetry dump dev X-MimeOLE: Produced By Microsoft Exchange V6.5 Date: Thu, 16 Jun 2022 11:00:08 +0200 Message-ID: <98CBD80474FA8B44BF855DF32C47DC35D8712F@smartserver.smartshare.dk> In-Reply-To: X-MS-Has-Attach: X-MS-TNEF-Correlator: Thread-Topic: [PATCH 0/5] support telemetry dump dev Thread-Index: AdiBWc1F+tRdaMeZRGGf1R198RJVVwAAK9JA References: <20220615073915.14041-1-fengchengwen@huawei.com> <98CBD80474FA8B44BF855DF32C47DC35D8712A@smartserver.smartshare.dk> From: =?iso-8859-1?Q?Morten_Br=F8rup?= To: "Bruce Richardson" Cc: "Chengwen Feng" , , , , , , , , 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: Bruce Richardson [mailto:bruce.richardson@intel.com] > Sent: Thursday, 16 June 2022 10.19 >=20 > On Wed, Jun 15, 2022 at 10:15:27PM +0200, Morten Br=F8rup wrote: > > > From: Chengwen Feng [mailto:fengchengwen@huawei.com] > > > Sent: Wednesday, 15 June 2022 09.39 > > > > > > This patchset contains five patch which support telemetry dump > > > dmadev/eventdev/rawdev/ethdev. > > > > > > Chengwen Feng (5): > > > usertools: use non-strict when json-loads in telemetry > > > dmadev: support telemetry dump dmadev > > > eventdev: support telemetry dump eventdev > > > rawdev: support telemetry dump rawdev > > > ethdev: support telemetry private dump > > > > > > lib/dmadev/rte_dmadev.c | 39 = +++++++++++++++++++++++++++++++++ > > > lib/ethdev/rte_ethdev.c | 42 > ++++++++++++++++++++++++++++++++++++ > > > lib/eventdev/rte_eventdev.c | 43 > +++++++++++++++++++++++++++++++++++++ > > > lib/rawdev/rte_rawdev.c | 42 > ++++++++++++++++++++++++++++++++++++ > > > usertools/dpdk-telemetry.py | 2 +- > > > 5 files changed, 167 insertions(+), 1 deletion(-) > > > > > > -- > > > 2.33.0 > > > > > > > While hoping for someone to fix the Telemetry library bug regarding > the missing JSON encoding of strings > (https://bugs.dpdk.org/show_bug.cgi?id=3D1037), thus making the patch > regarding usertools/dpdk-telemetry.py superfluous, > > > > Series-Acked-by: Morten Br=F8rup > > >=20 > I disagree with taking this series without fixing the underlying = issue. > I > don't think we should be sending invalid json data so doing proper > escaping of \n\t etc. characters on encode is the best solution. OK. I'll accept upgrading from hope to requirement. :-) >=20 > There is also some merit in adding in support for tagging strings that > don't contain invalid characters, if perf is affected in some cases. You seem to be suggesting that we, in addition to RTE_TEL_STRING_VAL add = a new telemetry value type for "strings not requiring any encoding by = any protocols, JSON, HTML, SNMP, SOAP, InfluxDB, or whatever the future = might bring", for performance reasons? This would require that the telemetry providers call either = rte_tel_data_plain_string() instead of rte_tel_data_string(), depending = on the contents of the string, and damn well make sure they don't call = rte_tel_data_plain_string() with strings containing special characters. Alternatively, the rte_tel_data_string() function would need to scan for = special characters and mark the string type accordingly. I don't think it is a good idea modifying the lower layer of the = telemetry library to support this. I prefer to keep the choice of encoding or simple copying (assuming that = simply copying provides a performance benefit) in the protocol specific = encoding functions, such as rte_tel_json_str(). The performance cost of making the decision (to encode or not) in the = protocol layer, i.e. in rte_tel_json_str(), is not higher than it is in = the lower layer, i.e. in rte_tel_data_string(). And some protocols, e.g. = SNMP, do allow a lot of special characters in strings without encoding. > That > said, in the testing I have done in the past: > * for jobs like getting the nic stats or nic xstats, the vast majority > of > the cycles spent in the telemetry function is doing PCI reads for = the > stats > * for simpler jobs e.g. getting list of ethdevs, the vast majority of > the > time is actually being spent in the kernel socket handling >=20 > Based on what I've seen (and unfortunately I didn't note down the = exact > figures for above), its likely that even if performance of the > interface is > a concern we can probably add in some basic string encoding without > really > affecting things much. Agree. Especially if we keep the implementation simple. E.g. we can = reuse size limit from the unencoded for the encoded string. >=20 > /Bruce