diff mbox series

trace: take live traces via telemetry

Message ID 20221013074928.3062458-1-david.marchand@redhat.com (mailing list archive)
State Superseded, archived
Delegated to: Thomas Monjalon
Headers
Series trace: take live traces via telemetry |

Checks

Context Check Description
ci/checkpatch success coding style OK
ci/Intel-compilation success Compilation OK
ci/iol-intel-Functional success Functional Testing PASS
ci/intel-Testing success Testing PASS
ci/iol-mellanox-Performance success Performance Testing PASS
ci/iol-aarch64-unit-testing success Testing PASS
ci/iol-aarch64-compile-testing success Testing PASS
ci/iol-x86_64-unit-testing success Testing PASS
ci/iol-x86_64-compile-testing success Testing PASS
ci/github-robot: build success github build: passed
ci/iol-intel-Performance success Performance Testing PASS

Commit Message

David Marchand Oct. 13, 2022, 7:49 a.m. UTC 
  Register telemetry commands to list and configure trace points and later
save traces for a running DPDK application.

Note: trace point names contain a '.', so the list of valid characters
used in telemetry commands and dictionary keys has been extended.

Example with testpmd running with two net/null ports (startup command
from devtools/test-null.sh):

--> /trace/list,lib.ethdev.*
{"/trace/list": {"lib.ethdev.configure": "Disabled",
    "lib.ethdev.rxq.setup": "Disabled",
    "lib.ethdev.txq.setup": "Disabled",
    "lib.ethdev.start": "Disabled",
    "lib.ethdev.stop": "Disabled",
    "lib.ethdev.close": "Disabled",
    "lib.ethdev.rx.burst": "Disabled",
    "lib.ethdev.tx.burst": "Disabled"}}

--> /trace/enable,lib.ethdev.st*
{"/trace/enable": {"Count": 2}}
--> /trace/enable,lib.ethdev.st*
{"/trace/enable": {"Count": 0}}

--> /trace/list,lib.ethdev.*
{"/trace/list": {"lib.ethdev.configure": "Disabled",
    "lib.ethdev.rxq.setup": "Disabled",
    "lib.ethdev.txq.setup": "Disabled",
    "lib.ethdev.start": "Enabled",
    "lib.ethdev.stop": "Enabled",
    "lib.ethdev.close": "Disabled",
    "lib.ethdev.rx.burst": "Disabled",
    "lib.ethdev.tx.burst": "Disabled"}}

testpmd> stop
...
testpmd> port stop all
...
testpmd> port start all
...
testpmd> start
...

--> /trace/save
{"/trace/save": {"Status": "OK",
    "Path": ".../dpdk-traces/rte-2022-10-12-AM-10-51-48"}}

$ babeltrace .../dpdk-traces/rte-2022-10-12-AM-10-51-48
[10:51:36.229878723] (+?.?????????) lib.ethdev.stop:
    { cpu_id = 0x0, name = "dpdk-testpmd" }, { port_id = 0x0, ret = 0 }
[10:51:36.229880251] (+0.000001528) lib.ethdev.stop:
    { cpu_id = 0x0, name = "dpdk-testpmd" }, { port_id = 0x1, ret = 0 }
[10:51:40.449359774] (+4.219479523) lib.ethdev.start:
    { cpu_id = 0x0, name = "dpdk-testpmd" }, { port_id = 0x0 }
[10:51:40.449377877] (+0.000018103) lib.ethdev.start:
    { cpu_id = 0x0, name = "dpdk-testpmd" }, { port_id = 0x1 }

--> /trace/disable,*
{"/trace/disable": {"Count": 2}}

Signed-off-by: David Marchand <david.marchand@redhat.com>
---
For runtime testing, please use this patch in addition with series 25183.
Depends-on: series-25183 ("Trace subsystem fixes")

---
 lib/eal/common/eal_common_trace.c | 78 +++++++++++++++++++++++++++++++
 lib/telemetry/telemetry_data.c    |  1 +
 2 files changed, 79 insertions(+)

Comments

Jerin Jacob Oct. 13, 2022, 2:09 p.m. UTC | #1 
On Thu, Oct 13, 2022 at 1:20 PM David Marchand
<david.marchand@redhat.com> wrote:
>

I would suggest to change the subject as "trace: enable trace
operations via telemetry" or so

> Register telemetry commands to list and configure trace points and later
> save traces for a running DPDK application.
>
> Note: trace point names contain a '.', so the list of valid characters
> used in telemetry commands and dictionary keys has been extended.
>
> Example with testpmd running with two net/null ports (startup command
> from devtools/test-null.sh):
>
> --> /trace/disable,*
> {"/trace/disable": {"Count": 2}}
>
> Signed-off-by: David Marchand <david.marchand@redhat.com>

> +}
> +
> +static int
> +trace_telemetry_list(const char *cmd __rte_unused,
> +       const char *params, struct rte_tel_data *d)
> +{
> +       struct trace_point *tp;
> +
> +       rte_tel_data_start_dict(d);
> +       STAILQ_FOREACH(tp, &tp_list, next) {
> +               if (params != NULL && fnmatch(params, tp->name, 0) != 0)
> +                       continue;
> +
> +               rte_tel_data_add_dict_string(d, tp->name,
> +                       rte_trace_point_is_enabled(tp->handle) ?  "Enabled" : "Disabled");

Could be changed to "Ena" and "Dis" or similar to reduce traffic on wire.

Also, it may be good to add a few text in
doc/guides/prog_guide/trace_lib.rst to tell this feature.

Acked-by: Jerin Jacob <jerinj@marvell.com>
Bruce Richardson Oct. 18, 2022, 1:14 p.m. UTC | #2 
On Thu, Oct 13, 2022 at 09:49:28AM +0200, David Marchand wrote:
> Register telemetry commands to list and configure trace points and later
> save traces for a running DPDK application.
> 
> Note: trace point names contain a '.', so the list of valid characters
> used in telemetry commands and dictionary keys has been extended.
> 
> Example with testpmd running with two net/null ports (startup command
> from devtools/test-null.sh):
> 
> --> /trace/list,lib.ethdev.*
> {"/trace/list": {"lib.ethdev.configure": "Disabled",
>     "lib.ethdev.rxq.setup": "Disabled",
>     "lib.ethdev.txq.setup": "Disabled",
>     "lib.ethdev.start": "Disabled",
>     "lib.ethdev.stop": "Disabled",
>     "lib.ethdev.close": "Disabled",
>     "lib.ethdev.rx.burst": "Disabled",
>     "lib.ethdev.tx.burst": "Disabled"}}
> 
> --> /trace/enable,lib.ethdev.st*
> {"/trace/enable": {"Count": 2}}
> --> /trace/enable,lib.ethdev.st*
> {"/trace/enable": {"Count": 0}}
> 
> --> /trace/list,lib.ethdev.*
> {"/trace/list": {"lib.ethdev.configure": "Disabled",
>     "lib.ethdev.rxq.setup": "Disabled",
>     "lib.ethdev.txq.setup": "Disabled",
>     "lib.ethdev.start": "Enabled",
>     "lib.ethdev.stop": "Enabled",
>     "lib.ethdev.close": "Disabled",
>     "lib.ethdev.rx.burst": "Disabled",
>     "lib.ethdev.tx.burst": "Disabled"}}
> 
> testpmd> stop
> ...
> testpmd> port stop all
> ...
> testpmd> port start all
> ...
> testpmd> start
> ...
> 
> --> /trace/save
> {"/trace/save": {"Status": "OK",
>     "Path": ".../dpdk-traces/rte-2022-10-12-AM-10-51-48"}}
> 
> $ babeltrace .../dpdk-traces/rte-2022-10-12-AM-10-51-48
> [10:51:36.229878723] (+?.?????????) lib.ethdev.stop:
>     { cpu_id = 0x0, name = "dpdk-testpmd" }, { port_id = 0x0, ret = 0 }
> [10:51:36.229880251] (+0.000001528) lib.ethdev.stop:
>     { cpu_id = 0x0, name = "dpdk-testpmd" }, { port_id = 0x1, ret = 0 }
> [10:51:40.449359774] (+4.219479523) lib.ethdev.start:
>     { cpu_id = 0x0, name = "dpdk-testpmd" }, { port_id = 0x0 }
> [10:51:40.449377877] (+0.000018103) lib.ethdev.start:
>     { cpu_id = 0x0, name = "dpdk-testpmd" }, { port_id = 0x1 }
> 
> --> /trace/disable,*
> {"/trace/disable": {"Count": 2}}
> 
> Signed-off-by: David Marchand <david.marchand@redhat.com>
> ---
> For runtime testing, please use this patch in addition with series 25183.
> Depends-on: series-25183 ("Trace subsystem fixes")
> 
> ---
>  lib/eal/common/eal_common_trace.c | 78 +++++++++++++++++++++++++++++++
>  lib/telemetry/telemetry_data.c    |  1 +
>  2 files changed, 79 insertions(+)
> 
> diff --git a/lib/eal/common/eal_common_trace.c b/lib/eal/common/eal_common_trace.c
> index f9b187d15f..9a54987b42 100644
> --- a/lib/eal/common/eal_common_trace.c
> +++ b/lib/eal/common/eal_common_trace.c
> @@ -12,6 +12,7 @@
>  #include <rte_lcore.h>
>  #include <rte_per_lcore.h>
>  #include <rte_string_fns.h>
> +#include <rte_telemetry.h>
>  
>  #include "eal_trace.h"
>  
> @@ -530,3 +531,80 @@ __rte_trace_point_register(rte_trace_point_t *handle, const char *name,
>  
>  	return -rte_errno;
>  }
> +
> +static int
> +trace_telemetry_enable(const char *cmd __rte_unused,
> +	const char *params, struct rte_tel_data *d)
> +{
> +	unsigned int count;
> +
> +	if (params == NULL || strlen(params) == 0)
> +		return -1;
> +	rte_tel_data_start_dict(d);
> +	count = __atomic_load_n(&trace.status, __ATOMIC_RELAXED);
> +	rte_trace_pattern(params, true);
> +	rte_tel_data_add_dict_int(d, "Count",
> +		__atomic_load_n(&trace.status, __ATOMIC_RELAXED) - count);
> +	return 0;
> +
> +}
> +
> +static int
> +trace_telemetry_disable(const char *cmd __rte_unused,
> +	const char *params, struct rte_tel_data *d)
> +{
> +	unsigned int count;
> +
> +	if (params == NULL || strlen(params) == 0)
> +		return -1;
> +	rte_tel_data_start_dict(d);
> +	count = __atomic_load_n(&trace.status, __ATOMIC_RELAXED);
> +	rte_trace_pattern(params, false);
> +	rte_tel_data_add_dict_int(d, "Count",
> +		count - __atomic_load_n(&trace.status, __ATOMIC_RELAXED));
> +	return 0;
> +
> +}
> +
> +static int
> +trace_telemetry_list(const char *cmd __rte_unused,
> +	const char *params, struct rte_tel_data *d)
> +{
> +	struct trace_point *tp;
> +
> +	rte_tel_data_start_dict(d);
> +	STAILQ_FOREACH(tp, &tp_list, next) {
> +		if (params != NULL && fnmatch(params, tp->name, 0) != 0)
> +			continue;
> +
> +		rte_tel_data_add_dict_string(d, tp->name,
> +			rte_trace_point_is_enabled(tp->handle) ?  "Enabled" : "Disabled");
> +	}
> +
> +	return 0;
> +}
> +
> +static int
> +trace_telemetry_save(const char *cmd __rte_unused,
> +	const char *params __rte_unused, struct rte_tel_data *d)
> +{
> +	struct trace *trace = trace_obj_get();
> +
> +	rte_tel_data_start_dict(d);
> +	rte_tel_data_add_dict_string(d, "Status", rte_trace_save() == 0 ? "OK" : "KO");
> +	rte_tel_data_add_dict_string(d, "Path", trace->dir);
> +
> +	return 0;
> +}
> +
> +RTE_INIT(trace_telemetry)
> +{
> +	rte_telemetry_register_cmd("/trace/enable", trace_telemetry_enable,
> +		"Enable trace points matching the provided pattern. Parameters: string pattern.");
> +	rte_telemetry_register_cmd("/trace/disable", trace_telemetry_disable,
> +		"Disable trace points matching the provided pattern. Parameters: string pattern.");
> +	rte_telemetry_register_cmd("/trace/list", trace_telemetry_list,
> +		"List trace points. Parameters: string pattern.");
> +	rte_telemetry_register_cmd("/trace/save", trace_telemetry_save,
> +		"Save current traces. Takes no parameter.");
> +}
> diff --git a/lib/telemetry/telemetry_data.c b/lib/telemetry/telemetry_data.c
> index 34366ecee3..5b319c18fb 100644
> --- a/lib/telemetry/telemetry_data.c
> +++ b/lib/telemetry/telemetry_data.c
> @@ -106,6 +106,7 @@ valid_name(const char *name)
>  			['a' ... 'z'] = 1,
>  			['_'] = 1,
>  			['/'] = 1,
> +			['.'] = 1,
>  	};
>  	while (*name != '\0') {
>  		if ((size_t)*name >= RTE_DIM(allowed) || allowed[(int)*name] == 0)

I don't see an issue with allowing "." characters in dictionary names, so
for this part:

Acked-by: Bruce Richardson <bruce.richardson@intel.com>
Morten Brørup Oct. 18, 2022, 2:33 p.m. UTC | #3 
> From: David Marchand [mailto:david.marchand@redhat.com]
> Sent: Thursday, 13 October 2022 09.49
> 
> Register telemetry commands to list and configure trace points and
> later
> save traces for a running DPDK application.
> 
> Note: trace point names contain a '.', so the list of valid characters
> used in telemetry commands and dictionary keys has been extended.

Regarding '.' in telemetry commands and dictionary keys, I agree with Bruce to allow it.

> 
> Example with testpmd running with two net/null ports (startup command
> from devtools/test-null.sh):
> 
> --> /trace/list,lib.ethdev.*
> {"/trace/list": {"lib.ethdev.configure": "Disabled",
>     "lib.ethdev.rxq.setup": "Disabled",
>     "lib.ethdev.txq.setup": "Disabled",
>     "lib.ethdev.start": "Disabled",
>     "lib.ethdev.stop": "Disabled",
>     "lib.ethdev.close": "Disabled",
>     "lib.ethdev.rx.burst": "Disabled",
>     "lib.ethdev.tx.burst": "Disabled"}}

Jerin commented that "Disabled"/"Enabled" are a bit verbose, and suggested shortening them.

It seems to me that these values are Boolean, and should be true or false (not surrounded by quotation marks), instead of some string representing a Boolean value. Note: This would require expanding the telemetry library with a Boolean type.

Alternatively, use integer values 0 or 1.

If we want to represent Boolean values as strings, I vote for "TRUE" and "FALSE", using all upper case to indicate that they are magic strings - and also to help avoid confusion with the JSON true/false Boolean values, which are all lower case.

> 
> --> /trace/enable,lib.ethdev.st*
> {"/trace/enable": {"Count": 2}}
> --> /trace/enable,lib.ethdev.st*
> {"/trace/enable": {"Count": 0}}
> 
> --> /trace/list,lib.ethdev.*
> {"/trace/list": {"lib.ethdev.configure": "Disabled",
>     "lib.ethdev.rxq.setup": "Disabled",
>     "lib.ethdev.txq.setup": "Disabled",
>     "lib.ethdev.start": "Enabled",
>     "lib.ethdev.stop": "Enabled",
>     "lib.ethdev.close": "Disabled",
>     "lib.ethdev.rx.burst": "Disabled",
>     "lib.ethdev.tx.burst": "Disabled"}}
> 
> testpmd> stop
> ...
> testpmd> port stop all
> ...
> testpmd> port start all
> ...
> testpmd> start
> ...
> 
> --> /trace/save
> {"/trace/save": {"Status": "OK",
>     "Path": ".../dpdk-traces/rte-2022-10-12-AM-10-51-48"}}
> 
> $ babeltrace .../dpdk-traces/rte-2022-10-12-AM-10-51-48
> [10:51:36.229878723] (+?.?????????) lib.ethdev.stop:
>     { cpu_id = 0x0, name = "dpdk-testpmd" }, { port_id = 0x0, ret = 0 }
> [10:51:36.229880251] (+0.000001528) lib.ethdev.stop:
>     { cpu_id = 0x0, name = "dpdk-testpmd" }, { port_id = 0x1, ret = 0 }
> [10:51:40.449359774] (+4.219479523) lib.ethdev.start:
>     { cpu_id = 0x0, name = "dpdk-testpmd" }, { port_id = 0x0 }
> [10:51:40.449377877] (+0.000018103) lib.ethdev.start:
>     { cpu_id = 0x0, name = "dpdk-testpmd" }, { port_id = 0x1 }
> 
> --> /trace/disable,*
> {"/trace/disable": {"Count": 2}}
> 
> Signed-off-by: David Marchand <david.marchand@redhat.com>
> ---
Bruce Richardson Oct. 18, 2022, 4:20 p.m. UTC | #4 
On Tue, Oct 18, 2022 at 04:33:49PM +0200, Morten Brørup wrote:
> > From: David Marchand [mailto:david.marchand@redhat.com]
> > Sent: Thursday, 13 October 2022 09.49
> > 
> > Register telemetry commands to list and configure trace points and
> > later
> > save traces for a running DPDK application.
> > 
> > Note: trace point names contain a '.', so the list of valid characters
> > used in telemetry commands and dictionary keys has been extended.
> 
> Regarding '.' in telemetry commands and dictionary keys, I agree with Bruce to allow it.
> 
> > 
> > Example with testpmd running with two net/null ports (startup command
> > from devtools/test-null.sh):
> > 
> > --> /trace/list,lib.ethdev.*
> > {"/trace/list": {"lib.ethdev.configure": "Disabled",
> >     "lib.ethdev.rxq.setup": "Disabled",
> >     "lib.ethdev.txq.setup": "Disabled",
> >     "lib.ethdev.start": "Disabled",
> >     "lib.ethdev.stop": "Disabled",
> >     "lib.ethdev.close": "Disabled",
> >     "lib.ethdev.rx.burst": "Disabled",
> >     "lib.ethdev.tx.burst": "Disabled"}}
> 
> Jerin commented that "Disabled"/"Enabled" are a bit verbose, and suggested shortening them.
> 
> It seems to me that these values are Boolean, and should be true or false (not surrounded by quotation marks), instead of some string representing a Boolean value. Note: This would require expanding the telemetry library with a Boolean type.
> 
> Alternatively, use integer values 0 or 1.
> 
> If we want to represent Boolean values as strings, I vote for "TRUE" and "FALSE", using all upper case to indicate that they are magic strings - and also to help avoid confusion with the JSON true/false Boolean values, which are all lower case.

+1 for integer values (at least in the short term). I don't think we want
these as strings.

/Bruce
David Marchand Oct. 19, 2022, 7:38 a.m. UTC | #5 
Hello Morten, Bruce,

On Tue, Oct 18, 2022 at 4:34 PM Morten Brørup <mb@smartsharesystems.com> wrote:
> > --> /trace/list,lib.ethdev.*
> > {"/trace/list": {"lib.ethdev.configure": "Disabled",
> >     "lib.ethdev.rxq.setup": "Disabled",
> >     "lib.ethdev.txq.setup": "Disabled",
> >     "lib.ethdev.start": "Disabled",
> >     "lib.ethdev.stop": "Disabled",
> >     "lib.ethdev.close": "Disabled",
> >     "lib.ethdev.rx.burst": "Disabled",
> >     "lib.ethdev.tx.burst": "Disabled"}}
>
> Jerin commented that "Disabled"/"Enabled" are a bit verbose, and suggested shortening them.

I forgot to handle this part (EINTR...), thanks for reminding.


> It seems to me that these values are Boolean, and should be true or false (not surrounded by quotation marks), instead of some string representing a Boolean value. Note: This would require expanding the telemetry library with a Boolean type.

Indeed.

>
> Alternatively, use integer values 0 or 1.
>
> If we want to represent Boolean values as strings, I vote for "TRUE" and "FALSE", using all upper case to indicate that they are magic strings - and also to help avoid confusion with the JSON true/false Boolean values, which are all lower case.

Introducing those strings is confusing, especially if we later
introduce the boolean type.
I had a quick try and adding the boolean type is relatively easy (I
just posted a patch, see
https://patchwork.dpdk.org/project/dpdk/patch/20221019073702.3948624-1-david.marchand@redhat.com/).

I would either go with adding this new type (though we are past rc1,
this addition is self contained and low risk), or use simple integers.
Morten Brørup Oct. 19, 2022, 8:21 a.m. UTC | #6 
> From: David Marchand [mailto:david.marchand@redhat.com]
> Sent: Wednesday, 19 October 2022 09.39
> 
> Hello Morten, Bruce,
> 
> On Tue, Oct 18, 2022 at 4:34 PM Morten Brørup
> <mb@smartsharesystems.com> wrote:
> > > --> /trace/list,lib.ethdev.*
> > > {"/trace/list": {"lib.ethdev.configure": "Disabled",
> > >     "lib.ethdev.rxq.setup": "Disabled",
> > >     "lib.ethdev.txq.setup": "Disabled",
> > >     "lib.ethdev.start": "Disabled",
> > >     "lib.ethdev.stop": "Disabled",
> > >     "lib.ethdev.close": "Disabled",
> > >     "lib.ethdev.rx.burst": "Disabled",
> > >     "lib.ethdev.tx.burst": "Disabled"}}
> >
> > Jerin commented that "Disabled"/"Enabled" are a bit verbose, and
> suggested shortening them.
> 
> I forgot to handle this part (EINTR...), thanks for reminding.
> 
> 
> > It seems to me that these values are Boolean, and should be true or
> false (not surrounded by quotation marks), instead of some string
> representing a Boolean value. Note: This would require expanding the
> telemetry library with a Boolean type.
> 
> Indeed.
> 
> >
> > Alternatively, use integer values 0 or 1.
> >
> > If we want to represent Boolean values as strings, I vote for "TRUE"
> and "FALSE", using all upper case to indicate that they are magic
> strings - and also to help avoid confusion with the JSON true/false
> Boolean values, which are all lower case.
> 
> Introducing those strings is confusing, especially if we later
> introduce the boolean type.
> I had a quick try and adding the boolean type is relatively easy (I
> just posted a patch, see
> https://patchwork.dpdk.org/project/dpdk/patch/20221019073702.3948624-1-
> david.marchand@redhat.com/).
> 
> I would either go with adding this new type (though we are past rc1,
> this addition is self contained and low risk), or use simple integers.

+1 to adding this new type.

It might be wider ranging, though:

Are other existing telemetry data in fact Boolean, but currently using some other type, and should switch to using the new Boolean type too?

> 
> 
> --
> David Marchand
>
David Marchand Oct. 19, 2022, 8:28 a.m. UTC | #7 
On Wed, Oct 19, 2022 at 10:21 AM Morten Brørup <mb@smartsharesystems.com> wrote:
> > > It seems to me that these values are Boolean, and should be true or
> > false (not surrounded by quotation marks), instead of some string
> > representing a Boolean value. Note: This would require expanding the
> > telemetry library with a Boolean type.
> >
> > Indeed.
> >
> > >
> > > Alternatively, use integer values 0 or 1.
> > >
> > > If we want to represent Boolean values as strings, I vote for "TRUE"
> > and "FALSE", using all upper case to indicate that they are magic
> > strings - and also to help avoid confusion with the JSON true/false
> > Boolean values, which are all lower case.
> >
> > Introducing those strings is confusing, especially if we later
> > introduce the boolean type.
> > I had a quick try and adding the boolean type is relatively easy (I
> > just posted a patch, see
> > https://patchwork.dpdk.org/project/dpdk/patch/20221019073702.3948624-1-
> > david.marchand@redhat.com/).
> >
> > I would either go with adding this new type (though we are past rc1,
> > this addition is self contained and low risk), or use simple integers.
>
> +1 to adding this new type.
>
> It might be wider ranging, though:
>
> Are other existing telemetry data in fact Boolean, but currently using some other type, and should switch to using the new Boolean type too?

I wondered about the same :-) but I did not check, for now.
Bruce Richardson Oct. 19, 2022, 10:53 a.m. UTC | #8 
On Tue, Oct 18, 2022 at 02:14:37PM +0100, Bruce Richardson wrote:
> On Thu, Oct 13, 2022 at 09:49:28AM +0200, David Marchand wrote:
> > Register telemetry commands to list and configure trace points and later
> > save traces for a running DPDK application.
> > 
> > Note: trace point names contain a '.', so the list of valid characters
> > used in telemetry commands and dictionary keys has been extended.
> > 
<snip>

> > diff --git a/lib/telemetry/telemetry_data.c b/lib/telemetry/telemetry_data.c
> > index 34366ecee3..5b319c18fb 100644
> > --- a/lib/telemetry/telemetry_data.c
> > +++ b/lib/telemetry/telemetry_data.c
> > @@ -106,6 +106,7 @@ valid_name(const char *name)
> >  			['a' ... 'z'] = 1,
> >  			['_'] = 1,
> >  			['/'] = 1,
> > +			['.'] = 1,
> >  	};
> >  	while (*name != '\0') {
> >  		if ((size_t)*name >= RTE_DIM(allowed) || allowed[(int)*name] == 0)
> 
> I don't see an issue with allowing "." characters in dictionary names, so
> for this part:
> 
> Acked-by: Bruce Richardson <bruce.richardson@intel.com>

By way of additional minor follow-up:
* I think the addition of "." to the list of allowed characters should be a
  separate patch, rather than just added as part of this larger patch. If
  doing a separate commit to add boolean type, this could be a change in that
  set.
* There are a couple of API doxygen comments for the dictionary functions,
  rte_tel_add_dict_*, where the allowed characters in the name are called
  out. You probably should add "." to those comments too.

Regards,
/Bruce
David Marchand Oct. 19, 2022, 1:46 p.m. UTC | #9 
On Wed, Oct 19, 2022 at 12:53 PM Bruce Richardson
<bruce.richardson@intel.com> wrote:
>
> On Tue, Oct 18, 2022 at 02:14:37PM +0100, Bruce Richardson wrote:
> > On Thu, Oct 13, 2022 at 09:49:28AM +0200, David Marchand wrote:
> > > Register telemetry commands to list and configure trace points and later
> > > save traces for a running DPDK application.
> > >
> > > Note: trace point names contain a '.', so the list of valid characters
> > > used in telemetry commands and dictionary keys has been extended.
> > >
> <snip>
>
> > > diff --git a/lib/telemetry/telemetry_data.c b/lib/telemetry/telemetry_data.c
> > > index 34366ecee3..5b319c18fb 100644
> > > --- a/lib/telemetry/telemetry_data.c
> > > +++ b/lib/telemetry/telemetry_data.c
> > > @@ -106,6 +106,7 @@ valid_name(const char *name)
> > >                     ['a' ... 'z'] = 1,
> > >                     ['_'] = 1,
> > >                     ['/'] = 1,
> > > +                   ['.'] = 1,
> > >     };
> > >     while (*name != '\0') {
> > >             if ((size_t)*name >= RTE_DIM(allowed) || allowed[(int)*name] == 0)
> >
> > I don't see an issue with allowing "." characters in dictionary names, so
> > for this part:
> >
> > Acked-by: Bruce Richardson <bruce.richardson@intel.com>
>
> By way of additional minor follow-up:
> * I think the addition of "." to the list of allowed characters should be a
>   separate patch, rather than just added as part of this larger patch. If
>   doing a separate commit to add boolean type, this could be a change in that
>   set.
> * There are a couple of API doxygen comments for the dictionary functions,
>   rte_tel_add_dict_*, where the allowed characters in the name are called
>   out. You probably should add "." to those comments too.

Oh indeed, so ok let's go with a separate patch.

Previously, I thought the telemetry additions for traces were fine to
go with the fixes series.
But seeing how it evolved, I'll merge the traces fixes (who got acked)
now and respin a separate series for the rest.
diff mbox series

Patch

diff --git a/lib/eal/common/eal_common_trace.c b/lib/eal/common/eal_common_trace.c
index f9b187d15f..9a54987b42 100644
--- a/lib/eal/common/eal_common_trace.c
+++ b/lib/eal/common/eal_common_trace.c
@@ -12,6 +12,7 @@ 
 #include <rte_lcore.h>
 #include <rte_per_lcore.h>
 #include <rte_string_fns.h>
+#include <rte_telemetry.h>
 
 #include "eal_trace.h"
 
@@ -530,3 +531,80 @@  __rte_trace_point_register(rte_trace_point_t *handle, const char *name,
 
 	return -rte_errno;
 }
+
+static int
+trace_telemetry_enable(const char *cmd __rte_unused,
+	const char *params, struct rte_tel_data *d)
+{
+	unsigned int count;
+
+	if (params == NULL || strlen(params) == 0)
+		return -1;
+	rte_tel_data_start_dict(d);
+	count = __atomic_load_n(&trace.status, __ATOMIC_RELAXED);
+	rte_trace_pattern(params, true);
+	rte_tel_data_add_dict_int(d, "Count",
+		__atomic_load_n(&trace.status, __ATOMIC_RELAXED) - count);
+	return 0;
+
+}
+
+static int
+trace_telemetry_disable(const char *cmd __rte_unused,
+	const char *params, struct rte_tel_data *d)
+{
+	unsigned int count;
+
+	if (params == NULL || strlen(params) == 0)
+		return -1;
+	rte_tel_data_start_dict(d);
+	count = __atomic_load_n(&trace.status, __ATOMIC_RELAXED);
+	rte_trace_pattern(params, false);
+	rte_tel_data_add_dict_int(d, "Count",
+		count - __atomic_load_n(&trace.status, __ATOMIC_RELAXED));
+	return 0;
+
+}
+
+static int
+trace_telemetry_list(const char *cmd __rte_unused,
+	const char *params, struct rte_tel_data *d)
+{
+	struct trace_point *tp;
+
+	rte_tel_data_start_dict(d);
+	STAILQ_FOREACH(tp, &tp_list, next) {
+		if (params != NULL && fnmatch(params, tp->name, 0) != 0)
+			continue;
+
+		rte_tel_data_add_dict_string(d, tp->name,
+			rte_trace_point_is_enabled(tp->handle) ?  "Enabled" : "Disabled");
+	}
+
+	return 0;
+}
+
+static int
+trace_telemetry_save(const char *cmd __rte_unused,
+	const char *params __rte_unused, struct rte_tel_data *d)
+{
+	struct trace *trace = trace_obj_get();
+
+	rte_tel_data_start_dict(d);
+	rte_tel_data_add_dict_string(d, "Status", rte_trace_save() == 0 ? "OK" : "KO");
+	rte_tel_data_add_dict_string(d, "Path", trace->dir);
+
+	return 0;
+}
+
+RTE_INIT(trace_telemetry)
+{
+	rte_telemetry_register_cmd("/trace/enable", trace_telemetry_enable,
+		"Enable trace points matching the provided pattern. Parameters: string pattern.");
+	rte_telemetry_register_cmd("/trace/disable", trace_telemetry_disable,
+		"Disable trace points matching the provided pattern. Parameters: string pattern.");
+	rte_telemetry_register_cmd("/trace/list", trace_telemetry_list,
+		"List trace points. Parameters: string pattern.");
+	rte_telemetry_register_cmd("/trace/save", trace_telemetry_save,
+		"Save current traces. Takes no parameter.");
+}
diff --git a/lib/telemetry/telemetry_data.c b/lib/telemetry/telemetry_data.c
index 34366ecee3..5b319c18fb 100644
--- a/lib/telemetry/telemetry_data.c
+++ b/lib/telemetry/telemetry_data.c
@@ -106,6 +106,7 @@  valid_name(const char *name)
 			['a' ... 'z'] = 1,
 			['_'] = 1,
 			['/'] = 1,
+			['.'] = 1,
 	};
 	while (*name != '\0') {
 		if ((size_t)*name >= RTE_DIM(allowed) || allowed[(int)*name] == 0)