From: "Morten Brørup" <mb@smartsharesystems.com>
To: "Bruce Richardson" <bruce.richardson@intel.com>
Cc: "Chengwen Feng" <fengchengwen@huawei.com>, <thomas@monjalon.net>,
<ferruh.yigit@xilinx.com>, <kevin.laatz@intel.com>,
<andrew.rybchenko@oktetlabs.ru>, <jerinj@marvell.com>,
<sachin.saxena@oss.nxp.com>, <hemant.agrawal@nxp.com>,
<dev@dpdk.org>
Subject: RE: [PATCH 0/5] support telemetry dump dev
Date: Thu, 16 Jun 2022 11:00:08 +0200 [thread overview]
Message-ID: <98CBD80474FA8B44BF855DF32C47DC35D8712F@smartserver.smartshare.dk> (raw)
In-Reply-To: <YqrnhxER1Aw8hH1p@bricha3-MOBL.ger.corp.intel.com>
> From: Bruce Richardson [mailto:bruce.richardson@intel.com]
> Sent: Thursday, 16 June 2022 10.19
>
> On Wed, Jun 15, 2022 at 10:15:27PM +0200, Morten Brørup 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=1037), thus making the patch
> regarding usertools/dpdk-telemetry.py superfluous,
> >
> > Series-Acked-by: Morten Brørup <mb@smartsharesystems.com>
> >
>
> 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. :-)
>
> 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
>
> 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.
>
> /Bruce
next prev parent reply other threads:[~2022-06-16 9:00 UTC|newest]
Thread overview: 39+ messages / expand[flat|nested] mbox.gz Atom feed top
2022-06-15 7:39 Chengwen Feng
2022-06-15 7:39 ` [PATCH 1/5] usertools: use non-strict when json-loads in telemetry Chengwen Feng
2022-06-15 13:54 ` Morten Brørup
2022-06-15 16:54 ` Bruce Richardson
2022-06-15 18:01 ` Morten Brørup
2022-06-15 20:09 ` Morten Brørup
2022-06-15 7:39 ` [PATCH 2/5] dmadev: support telemetry dump dmadev Chengwen Feng
2022-06-15 7:39 ` [PATCH 3/5] eventdev: support telemetry dump eventdev Chengwen Feng
2022-06-15 7:39 ` [PATCH 4/5] rawdev: support telemetry dump rawdev Chengwen Feng
2022-06-15 7:39 ` [PATCH 5/5] ethdev: support telemetry private dump Chengwen Feng
2022-06-15 20:15 ` [PATCH 0/5] support telemetry dump dev Morten Brørup
2022-06-16 8:19 ` Bruce Richardson
2022-06-16 9:00 ` Morten Brørup [this message]
2022-06-17 9:46 ` [PATCH v2 0/5] support telemetry dump Chengwen Feng
2022-06-17 9:46 ` [PATCH v2 1/5] telemetry: escape special char when tel string Chengwen Feng
2022-06-17 11:16 ` Morten Brørup
2022-06-17 11:25 ` Bruce Richardson
2022-06-17 17:05 ` Stephen Hemminger
2022-06-18 3:52 ` fengchengwen
2022-06-18 9:59 ` Morten Brørup
2022-06-22 7:57 ` Power, Ciara
2022-06-22 9:19 ` Bruce Richardson
2022-06-23 16:45 ` Bruce Richardson
2022-06-17 11:27 ` Bruce Richardson
2022-06-17 9:46 ` [PATCH v2 2/5] dmadev: support telemetry dump dmadev Chengwen Feng
2022-06-17 9:46 ` [PATCH v2 3/5] eventdev: support telemetry dump eventdev Chengwen Feng
2022-06-17 9:46 ` [PATCH v2 4/5] rawdev: support telemetry dump rawdev Chengwen Feng
2022-06-17 9:46 ` [PATCH v2 5/5] ethdev: support telemetry private dump Chengwen Feng
2022-09-13 2:44 ` [PATCH v3 0/4] support telemetry dump dev Chengwen Feng
2022-09-13 2:44 ` [PATCH v3 1/4] dmadev: support telemetry dump dmadev Chengwen Feng
2022-09-13 2:44 ` [PATCH v3 2/4] eventdev: support telemetry dump eventdev Chengwen Feng
2022-09-13 2:44 ` [PATCH v3 3/4] rawdev: support telemetry dump rawdev Chengwen Feng
2022-09-13 2:44 ` [PATCH v3 4/4] ethdev: support telemetry private dump Chengwen Feng
2022-09-13 7:13 ` [PATCH v4 0/4] support telemetry dump dev Chengwen Feng
2022-09-13 7:13 ` [PATCH v4 1/4] dmadev: support telemetry dump dmadev Chengwen Feng
2022-09-13 7:13 ` [PATCH v4 2/4] eventdev: support telemetry dump eventdev Chengwen Feng
2022-09-13 7:13 ` [PATCH v4 3/4] rawdev: support telemetry dump rawdev Chengwen Feng
2022-09-13 7:13 ` [PATCH v4 4/4] ethdev: support telemetry private dump Chengwen Feng
2022-10-03 7:28 ` [PATCH v4 0/4] support telemetry dump dev David Marchand
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=98CBD80474FA8B44BF855DF32C47DC35D8712F@smartserver.smartshare.dk \
--to=mb@smartsharesystems.com \
--cc=andrew.rybchenko@oktetlabs.ru \
--cc=bruce.richardson@intel.com \
--cc=dev@dpdk.org \
--cc=fengchengwen@huawei.com \
--cc=ferruh.yigit@xilinx.com \
--cc=hemant.agrawal@nxp.com \
--cc=jerinj@marvell.com \
--cc=kevin.laatz@intel.com \
--cc=sachin.saxena@oss.nxp.com \
--cc=thomas@monjalon.net \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).