When trace is enabled (T flag is set) and trace_dst field is set
to value greater than 0 (0 is reserved for trace events) then
debug logs will be written to trace instance pointed by trace_dst
value, for example when trace_dst value is 2 then debug logs will
be written to <debugfs>/tracing/instances/dyndbg_inst_2 instance.
Given trace instance will not be initialized until debug logs are
requested to be written to it and afer init will persist until
reboot.

Signed-off-by: Łukasz Bartosik <lb@semihalf.com>
---
 lib/Kconfig.debug   |  1 +
 lib/dynamic_debug.c | 79 ++++++++++++++++++++++++++++++++++++++++++---
 2 files changed, 76 insertions(+), 4 deletions(-)

diff --git a/lib/Kconfig.debug b/lib/Kconfig.debug
index fa307f93fa2e..9617e92c046d 100644
--- a/lib/Kconfig.debug
+++ b/lib/Kconfig.debug
@@ -181,6 +181,7 @@ config DYNAMIC_DEBUG_CORE
 	bool "Enable core function of dynamic debug support"
 	depends on PRINTK
 	depends on (DEBUG_FS || PROC_FS)
+	depends on TRACING
 	help
 	  Enable core functional support of dynamic debug. It is useful
 	  when you want to tie dynamic debug to your kernel modules with
diff --git a/lib/dynamic_debug.c b/lib/dynamic_debug.c
index c5cd28e74a02..541d9d522b3b 100644
--- a/lib/dynamic_debug.c
+++ b/lib/dynamic_debug.c
@@ -36,6 +36,7 @@
 #include <linux/sched.h>
 #include <linux/device.h>
 #include <linux/netdevice.h>
+#include <linux/trace.h>
 
 #define CREATE_TRACE_POINTS
 #include <trace/events/dyndbg.h>
@@ -81,6 +82,18 @@ module_param(verbose, int, 0644);
 MODULE_PARM_DESC(verbose, " dynamic_debug/control processing "
 		 "( 0 = off (default), 1 = module add/rm, 2 = >control summary, 3 = parsing, 4 = per-site changes)");
 
+/*
+ * When trace is enabled (T flag is set) and trace_dst field is set
+ * to value greater than 0 (0 is reserved for trace events) then
+ * debug logs will be written to trace instance pointed by trace_dst
+ * value, for example when trace_dst value is 2 then debug logs will
+ * be written to <debugfs>/tracing/instances/dyndbg_inst_2 instance.
+ * Given trace instance will not be initialized until debug logs are
+ * requested to be written to it and afer init will persist until
+ * reboot.
+ */
+static struct trace_array *trace_arr[TRACE_DST_MAX];
+
 static inline struct dd_ctrl *get_ctrl(struct _ddebug *desc)
 {
 	return &desc->ctrl;
@@ -255,6 +268,45 @@ static struct ddebug_class_map *ddebug_find_valid_class(struct ddebug_table cons
 	return NULL;
 }
 
+static int handle_trace_dst(struct dd_ctrl *ctrl)
+{
+#define TRACE_INST_NAME_LEN 16
+	char instance_name[TRACE_INST_NAME_LEN];
+	struct trace_array *arr;
+	int ret = -EINVAL;
+
+	/* check if trace (T flag) is enabled */
+	if (!(ctrl->flags & _DPRINTK_FLAGS_TRACE))
+		return 0;
+
+	/* check if trace destination are trace events */
+	if (!ctrl->trace_dst)
+		return 0;
+
+	/* check if trace instance is already set up */
+	if (trace_arr[ctrl->trace_dst])
+		return 0;
+
+	snprintf(instance_name, TRACE_INST_NAME_LEN,
+		 "dyndbg_inst_%u", ctrl->trace_dst);
+	arr = trace_array_get_by_name(instance_name);
+	if (!arr)
+		goto err;
+
+	ret = trace_array_init_printk(arr);
+	if (ret)
+		goto err_init;
+
+	trace_arr[ctrl->trace_dst] = arr;
+	return 0;
+
+err_init:
+	trace_array_put(arr);
+	trace_array_destroy(arr);
+err:
+	return ret;
+}
+
 #define __outvar /* filled by callee */
 /*
  * Search the tables for _ddebug's which match the given `query' and
@@ -338,6 +390,9 @@ static int ddebug_change(const struct ddebug_query *query,
 			nctrl.trace_dst = modifiers->trace_dst;
 			if (!memcmp(&nctrl, get_ctrl(dp), sizeof(nctrl)))
 				continue;
+
+			if (handle_trace_dst(&nctrl))
+				continue;
 #ifdef CONFIG_JUMP_LABEL
 			if (get_flags(dp) & _DPRINTK_FLAGS_ENABLED) {
 				if (!(nctrl.flags & _DPRINTK_FLAGS_ENABLED))
@@ -977,8 +1032,8 @@ static DEFINE_PER_CPU(struct ddebug_trace_bufs, ddebug_trace_bufs);
 static DEFINE_PER_CPU(int, ddebug_trace_reserve);
 
 __printf(3, 0)
-static void ddebug_trace(struct _ddebug *desc, const struct device *dev,
-			 const char *fmt, va_list args)
+static void ddebug_trace_event(struct _ddebug *desc, const struct device *dev,
+			       const char *fmt, va_list args)
 {
 	struct ddebug_trace_buf *buf;
 	int bufidx;
@@ -1010,6 +1065,15 @@ static void ddebug_trace(struct _ddebug *desc, const struct device *dev,
 	preempt_enable_notrace();
 }
 
+__printf(2, 0)
+static void ddebug_trace_instance(struct _ddebug *desc, const char *fmt,
+				  va_list *args)
+{
+	struct va_format vaf = { .fmt = fmt, .va = args};
+
+	trace_array_printk(trace_arr[get_trace_dst(desc)], _THIS_IP_, "%pV", &vaf);
+}
+
 __printf(2, 3)
 static void ddebug_printk(struct _ddebug *desc, const char *fmt, ...)
 {
@@ -1022,7 +1086,11 @@ static void ddebug_printk(struct _ddebug *desc, const char *fmt, ...)
 		 * All callers include the KERN_DEBUG prefix to keep the
 		 * vprintk case simple; strip it out for tracing.
 		 */
-		ddebug_trace(desc, NULL, fmt + strlen(KERN_DEBUG), args);
+		if (get_trace_dst(desc))
+			ddebug_trace_instance(desc, fmt, &args);
+		else
+			ddebug_trace_event(desc, NULL,
+					   fmt + strlen(KERN_DEBUG), args);
 		va_end(args);
 	}
 
@@ -1044,7 +1112,10 @@ static void ddebug_dev_printk(struct _ddebug *desc, const struct device *dev,
 		va_list args;
 
 		va_start(args, fmt);
-		ddebug_trace(desc, dev, fmt, args);
+		if (get_trace_dst(desc))
+			ddebug_trace_instance(desc, fmt, &args);
+		else
+			ddebug_trace_event(desc, dev, fmt, args);
 		va_end(args);
 	}
 
-- 
2.42.0.869.gea05f2083d-goog

On Fri, Nov 3, 2023 at 7:10 AM Łukasz Bartosik <lb@semihalf.com> wrote:
>
> When trace is enabled (T flag is set) and trace_dst field is set
> to value greater than 0 (0 is reserved for trace events) then
> debug logs will be written to trace instance pointed by trace_dst
> value, for example when trace_dst value is 2 then debug logs will
> be written to <debugfs>/tracing/instances/dyndbg_inst_2 instance.
> Given trace instance will not be initialized until debug logs are
> requested to be written to it and afer init will persist until
> reboot.
>

restating 00 comments -

you can get rid of integer destination ids by adding a new command: open/close

$> echo  \
 open kms-instance \;\
 class DRM_UT_KMS -T:kms-instance  # preset-dests-disable-sites \;\
> /proc/dynamic_debug/control

echo "class DRM_UT_KMS +T # enable sites, with the preselected data" >
/proc/dynamic_debug/control

open - assign name to id, reserve it, hide it from user
+T:valid-name # fail command if name wasnt opened

and +T  w/o dest means use existing setting, not just 0 (unless thats
the existing setting)

> Signed-off-by: Łukasz Bartosik <lb@semihalf.com>
> ---
>  lib/Kconfig.debug   |  1 +
>  lib/dynamic_debug.c | 79 ++++++++++++++++++++++++++++++++++++++++++---
>  2 files changed, 76 insertions(+), 4 deletions(-)
>
> diff --git a/lib/Kconfig.debug b/lib/Kconfig.debug
> index fa307f93fa2e..9617e92c046d 100644
> --- a/lib/Kconfig.debug
> +++ b/lib/Kconfig.debug
> @@ -181,6 +181,7 @@ config DYNAMIC_DEBUG_CORE
>         bool "Enable core function of dynamic debug support"
>         depends on PRINTK
>         depends on (DEBUG_FS || PROC_FS)
> +       depends on TRACING
>         help
>           Enable core functional support of dynamic debug. It is useful
>           when you want to tie dynamic debug to your kernel modules with
> diff --git a/lib/dynamic_debug.c b/lib/dynamic_debug.c
> index c5cd28e74a02..541d9d522b3b 100644
> --- a/lib/dynamic_debug.c
> +++ b/lib/dynamic_debug.c
> @@ -36,6 +36,7 @@
>  #include <linux/sched.h>
>  #include <linux/device.h>
>  #include <linux/netdevice.h>
> +#include <linux/trace.h>
>
>  #define CREATE_TRACE_POINTS
>  #include <trace/events/dyndbg.h>
> @@ -81,6 +82,18 @@ module_param(verbose, int, 0644);
>  MODULE_PARM_DESC(verbose, " dynamic_debug/control processing "
>                  "( 0 = off (default), 1 = module add/rm, 2 = >control summary, 3 = parsing, 4 = per-site changes)");
>
> +/*
> + * When trace is enabled (T flag is set) and trace_dst field is set
> + * to value greater than 0 (0 is reserved for trace events) then
> + * debug logs will be written to trace instance pointed by trace_dst
> + * value, for example when trace_dst value is 2 then debug logs will
> + * be written to <debugfs>/tracing/instances/dyndbg_inst_2 instance.
> + * Given trace instance will not be initialized until debug logs are
> + * requested to be written to it and afer init will persist until
> + * reboot.
> + */
> +static struct trace_array *trace_arr[TRACE_DST_MAX];
> +
>  static inline struct dd_ctrl *get_ctrl(struct _ddebug *desc)
>  {
>         return &desc->ctrl;
> @@ -255,6 +268,45 @@ static struct ddebug_class_map *ddebug_find_valid_class(struct ddebug_table cons
>         return NULL;
>  }
>
> +static int handle_trace_dst(struct dd_ctrl *ctrl)
> +{
> +#define TRACE_INST_NAME_LEN 16
> +       char instance_name[TRACE_INST_NAME_LEN];
> +       struct trace_array *arr;
> +       int ret = -EINVAL;
> +
> +       /* check if trace (T flag) is enabled */
> +       if (!(ctrl->flags & _DPRINTK_FLAGS_TRACE))
> +               return 0;
> +
> +       /* check if trace destination are trace events */
> +       if (!ctrl->trace_dst)
> +               return 0;
> +
> +       /* check if trace instance is already set up */
> +       if (trace_arr[ctrl->trace_dst])
> +               return 0;
> +
> +       snprintf(instance_name, TRACE_INST_NAME_LEN,
> +                "dyndbg_inst_%u", ctrl->trace_dst);
> +       arr = trace_array_get_by_name(instance_name);
> +       if (!arr)
> +               goto err;
> +
> +       ret = trace_array_init_printk(arr);
> +       if (ret)
> +               goto err_init;
> +
> +       trace_arr[ctrl->trace_dst] = arr;
> +       return 0;
> +
> +err_init:
> +       trace_array_put(arr);
> +       trace_array_destroy(arr);
> +err:
> +       return ret;
> +}
> +
>  #define __outvar /* filled by callee */
>  /*
>   * Search the tables for _ddebug's which match the given `query' and
> @@ -338,6 +390,9 @@ static int ddebug_change(const struct ddebug_query *query,
>                         nctrl.trace_dst = modifiers->trace_dst;
>                         if (!memcmp(&nctrl, get_ctrl(dp), sizeof(nctrl)))
>                                 continue;
> +
> +                       if (handle_trace_dst(&nctrl))
> +                               continue;
>  #ifdef CONFIG_JUMP_LABEL
>                         if (get_flags(dp) & _DPRINTK_FLAGS_ENABLED) {
>                                 if (!(nctrl.flags & _DPRINTK_FLAGS_ENABLED))
> @@ -977,8 +1032,8 @@ static DEFINE_PER_CPU(struct ddebug_trace_bufs, ddebug_trace_bufs);
>  static DEFINE_PER_CPU(int, ddebug_trace_reserve);
>
>  __printf(3, 0)
> -static void ddebug_trace(struct _ddebug *desc, const struct device *dev,
> -                        const char *fmt, va_list args)
> +static void ddebug_trace_event(struct _ddebug *desc, const struct device *dev,
> +                              const char *fmt, va_list args)
>  {
>         struct ddebug_trace_buf *buf;
>         int bufidx;
> @@ -1010,6 +1065,15 @@ static void ddebug_trace(struct _ddebug *desc, const struct device *dev,
>         preempt_enable_notrace();
>  }
>
> +__printf(2, 0)
> +static void ddebug_trace_instance(struct _ddebug *desc, const char *fmt,
> +                                 va_list *args)
> +{
> +       struct va_format vaf = { .fmt = fmt, .va = args};
> +
> +       trace_array_printk(trace_arr[get_trace_dst(desc)], _THIS_IP_, "%pV", &vaf);
> +}
> +
>  __printf(2, 3)
>  static void ddebug_printk(struct _ddebug *desc, const char *fmt, ...)
>  {
> @@ -1022,7 +1086,11 @@ static void ddebug_printk(struct _ddebug *desc, const char *fmt, ...)
>                  * All callers include the KERN_DEBUG prefix to keep the
>                  * vprintk case simple; strip it out for tracing.
>                  */
> -               ddebug_trace(desc, NULL, fmt + strlen(KERN_DEBUG), args);
> +               if (get_trace_dst(desc))
> +                       ddebug_trace_instance(desc, fmt, &args);
> +               else
> +                       ddebug_trace_event(desc, NULL,
> +                                          fmt + strlen(KERN_DEBUG), args);
>                 va_end(args);
>         }
>
> @@ -1044,7 +1112,10 @@ static void ddebug_dev_printk(struct _ddebug *desc, const struct device *dev,
>                 va_list args;
>
>                 va_start(args, fmt);
> -               ddebug_trace(desc, dev, fmt, args);
> +               if (get_trace_dst(desc))
> +                       ddebug_trace_instance(desc, fmt, &args);
> +               else
> +                       ddebug_trace_event(desc, dev, fmt, args);
>                 va_end(args);
>         }
>
> --
> 2.42.0.869.gea05f2083d-goog
>

sob., 4 lis 2023 o 22:49 <jim.cromie@gmail.com> napisał(a):
>
> On Fri, Nov 3, 2023 at 7:10 AM Łukasz Bartosik <lb@semihalf.com> wrote:
> >
> > When trace is enabled (T flag is set) and trace_dst field is set
> > to value greater than 0 (0 is reserved for trace events) then
> > debug logs will be written to trace instance pointed by trace_dst
> > value, for example when trace_dst value is 2 then debug logs will
> > be written to <debugfs>/tracing/instances/dyndbg_inst_2 instance.
> > Given trace instance will not be initialized until debug logs are
> > requested to be written to it and afer init will persist until
> > reboot.
> >
>
> restating 00 comments -
>
> you can get rid of integer destination ids by adding a new command: open/close
>
> $> echo  \
>  open kms-instance \;\
>  class DRM_UT_KMS -T:kms-instance  # preset-dests-disable-sites \;\
> > /proc/dynamic_debug/control
>

Instead of using above command to preset destination we could preset
destination with open command. I mean last successful
open would preset destination ? What do you think ?

>
> and +T  w/o dest means use existing setting, not just 0 (unless thats
> the existing setting)
>

Sounds good.




> > Signed-off-by: Łukasz Bartosik <lb@semihalf.com>
> > ---
> >  lib/Kconfig.debug   |  1 +
> >  lib/dynamic_debug.c | 79 ++++++++++++++++++++++++++++++++++++++++++---
> >  2 files changed, 76 insertions(+), 4 deletions(-)
> >
> > diff --git a/lib/Kconfig.debug b/lib/Kconfig.debug
> > index fa307f93fa2e..9617e92c046d 100644
> > --- a/lib/Kconfig.debug
> > +++ b/lib/Kconfig.debug
> > @@ -181,6 +181,7 @@ config DYNAMIC_DEBUG_CORE
> >         bool "Enable core function of dynamic debug support"
> >         depends on PRINTK
> >         depends on (DEBUG_FS || PROC_FS)
> > +       depends on TRACING
> >         help
> >           Enable core functional support of dynamic debug. It is useful
> >           when you want to tie dynamic debug to your kernel modules with
> > diff --git a/lib/dynamic_debug.c b/lib/dynamic_debug.c
> > index c5cd28e74a02..541d9d522b3b 100644
> > --- a/lib/dynamic_debug.c
> > +++ b/lib/dynamic_debug.c
> > @@ -36,6 +36,7 @@
> >  #include <linux/sched.h>
> >  #include <linux/device.h>
> >  #include <linux/netdevice.h>
> > +#include <linux/trace.h>
> >
> >  #define CREATE_TRACE_POINTS
> >  #include <trace/events/dyndbg.h>
> > @@ -81,6 +82,18 @@ module_param(verbose, int, 0644);
> >  MODULE_PARM_DESC(verbose, " dynamic_debug/control processing "
> >                  "( 0 = off (default), 1 = module add/rm, 2 = >control summary, 3 = parsing, 4 = per-site changes)");
> >
> > +/*
> > + * When trace is enabled (T flag is set) and trace_dst field is set
> > + * to value greater than 0 (0 is reserved for trace events) then
> > + * debug logs will be written to trace instance pointed by trace_dst
> > + * value, for example when trace_dst value is 2 then debug logs will
> > + * be written to <debugfs>/tracing/instances/dyndbg_inst_2 instance.
> > + * Given trace instance will not be initialized until debug logs are
> > + * requested to be written to it and afer init will persist until
> > + * reboot.
> > + */
> > +static struct trace_array *trace_arr[TRACE_DST_MAX];
> > +
> >  static inline struct dd_ctrl *get_ctrl(struct _ddebug *desc)
> >  {
> >         return &desc->ctrl;
> > @@ -255,6 +268,45 @@ static struct ddebug_class_map *ddebug_find_valid_class(struct ddebug_table cons
> >         return NULL;
> >  }
> >
> > +static int handle_trace_dst(struct dd_ctrl *ctrl)
> > +{
> > +#define TRACE_INST_NAME_LEN 16
> > +       char instance_name[TRACE_INST_NAME_LEN];
> > +       struct trace_array *arr;
> > +       int ret = -EINVAL;
> > +
> > +       /* check if trace (T flag) is enabled */
> > +       if (!(ctrl->flags & _DPRINTK_FLAGS_TRACE))
> > +               return 0;
> > +
> > +       /* check if trace destination are trace events */
> > +       if (!ctrl->trace_dst)
> > +               return 0;
> > +
> > +       /* check if trace instance is already set up */
> > +       if (trace_arr[ctrl->trace_dst])
> > +               return 0;
> > +
> > +       snprintf(instance_name, TRACE_INST_NAME_LEN,
> > +                "dyndbg_inst_%u", ctrl->trace_dst);
> > +       arr = trace_array_get_by_name(instance_name);
> > +       if (!arr)
> > +               goto err;
> > +
> > +       ret = trace_array_init_printk(arr);
> > +       if (ret)
> > +               goto err_init;
> > +
> > +       trace_arr[ctrl->trace_dst] = arr;
> > +       return 0;
> > +
> > +err_init:
> > +       trace_array_put(arr);
> > +       trace_array_destroy(arr);
> > +err:
> > +       return ret;
> > +}
> > +
> >  #define __outvar /* filled by callee */
> >  /*
> >   * Search the tables for _ddebug's which match the given `query' and
> > @@ -338,6 +390,9 @@ static int ddebug_change(const struct ddebug_query *query,
> >                         nctrl.trace_dst = modifiers->trace_dst;
> >                         if (!memcmp(&nctrl, get_ctrl(dp), sizeof(nctrl)))
> >                                 continue;
> > +
> > +                       if (handle_trace_dst(&nctrl))
> > +                               continue;
> >  #ifdef CONFIG_JUMP_LABEL
> >                         if (get_flags(dp) & _DPRINTK_FLAGS_ENABLED) {
> >                                 if (!(nctrl.flags & _DPRINTK_FLAGS_ENABLED))
> > @@ -977,8 +1032,8 @@ static DEFINE_PER_CPU(struct ddebug_trace_bufs, ddebug_trace_bufs);
> >  static DEFINE_PER_CPU(int, ddebug_trace_reserve);
> >
> >  __printf(3, 0)
> > -static void ddebug_trace(struct _ddebug *desc, const struct device *dev,
> > -                        const char *fmt, va_list args)
> > +static void ddebug_trace_event(struct _ddebug *desc, const struct device *dev,
> > +                              const char *fmt, va_list args)
> >  {
> >         struct ddebug_trace_buf *buf;
> >         int bufidx;
> > @@ -1010,6 +1065,15 @@ static void ddebug_trace(struct _ddebug *desc, const struct device *dev,
> >         preempt_enable_notrace();
> >  }
> >
> > +__printf(2, 0)
> > +static void ddebug_trace_instance(struct _ddebug *desc, const char *fmt,
> > +                                 va_list *args)
> > +{
> > +       struct va_format vaf = { .fmt = fmt, .va = args};
> > +
> > +       trace_array_printk(trace_arr[get_trace_dst(desc)], _THIS_IP_, "%pV", &vaf);
> > +}
> > +
> >  __printf(2, 3)
> >  static void ddebug_printk(struct _ddebug *desc, const char *fmt, ...)
> >  {
> > @@ -1022,7 +1086,11 @@ static void ddebug_printk(struct _ddebug *desc, const char *fmt, ...)
> >                  * All callers include the KERN_DEBUG prefix to keep the
> >                  * vprintk case simple; strip it out for tracing.
> >                  */
> > -               ddebug_trace(desc, NULL, fmt + strlen(KERN_DEBUG), args);
> > +               if (get_trace_dst(desc))
> > +                       ddebug_trace_instance(desc, fmt, &args);
> > +               else
> > +                       ddebug_trace_event(desc, NULL,
> > +                                          fmt + strlen(KERN_DEBUG), args);
> >                 va_end(args);
> >         }
> >
> > @@ -1044,7 +1112,10 @@ static void ddebug_dev_printk(struct _ddebug *desc, const struct device *dev,
> >                 va_list args;
> >
> >                 va_start(args, fmt);
> > -               ddebug_trace(desc, dev, fmt, args);
> > +               if (get_trace_dst(desc))
> > +                       ddebug_trace_instance(desc, fmt, &args);
> > +               else
> > +                       ddebug_trace_event(desc, dev, fmt, args);
> >                 va_end(args);
> >         }
> >
> > --
> > 2.42.0.869.gea05f2083d-goog
> >

On Fri, Nov 10, 2023 at 7:53 AM Łukasz Bartosik <lb@semihalf.com> wrote:
>
> sob., 4 lis 2023 o 22:49 <jim.cromie@gmail.com> napisał(a):
> >
> > On Fri, Nov 3, 2023 at 7:10 AM Łukasz Bartosik <lb@semihalf.com> wrote:
> > >
> > > When trace is enabled (T flag is set) and trace_dst field is set
> > > to value greater than 0 (0 is reserved for trace events) then
> > > debug logs will be written to trace instance pointed by trace_dst
> > > value, for example when trace_dst value is 2 then debug logs will
> > > be written to <debugfs>/tracing/instances/dyndbg_inst_2 instance.
> > > Given trace instance will not be initialized until debug logs are
> > > requested to be written to it and afer init will persist until
> > > reboot.
> > >
> >
> > restating 00 comments -
> >
> > you can get rid of integer destination ids by adding a new command: open/close
> >
> > $> echo  \
> >  open kms-instance \;\
> >  class DRM_UT_KMS -T:kms-instance  # preset-dests-disable-sites \;\
> > > /proc/dynamic_debug/control
> >
>
> Instead of using above command to preset destination we could preset
> destination with open command. I mean last successful
> open would preset destination ? What do you think ?
>

I dont think it works - if open maps to a dest-number, (or implicit as
TOP-of-stack)
then you just have +T<dest-number>  (or +T <implicit tos>)
rather than +T:dest-name
and you still have to keep track of what dest-numbers were already used.
(or every new dest needs an explicit OPEN before it)

and how do you then get back to default instance ?
open 0 ?
close <previous-handle> ?


by using names, all opens can be at the top,
(and thus document in 1 block all the named-instances)
and any named dest that hasnt been opened is an error
(not just reusing previous OPEN)


> >
> > and +T  w/o dest means use existing setting, not just 0 (unless thats
> > the existing setting)
> >
>
> Sounds good.
>

:-)

pt., 10 lis 2023 o 21:03 <jim.cromie@gmail.com> napisał(a):
>
> On Fri, Nov 10, 2023 at 7:53 AM Łukasz Bartosik <lb@semihalf.com> wrote:
> >
> > sob., 4 lis 2023 o 22:49 <jim.cromie@gmail.com> napisał(a):
> > >
> > > On Fri, Nov 3, 2023 at 7:10 AM Łukasz Bartosik <lb@semihalf.com> wrote:
> > > >
> > > > When trace is enabled (T flag is set) and trace_dst field is set
> > > > to value greater than 0 (0 is reserved for trace events) then
> > > > debug logs will be written to trace instance pointed by trace_dst
> > > > value, for example when trace_dst value is 2 then debug logs will
> > > > be written to <debugfs>/tracing/instances/dyndbg_inst_2 instance.
> > > > Given trace instance will not be initialized until debug logs are
> > > > requested to be written to it and afer init will persist until
> > > > reboot.
> > > >
> > >
> > > restating 00 comments -
> > >
> > > you can get rid of integer destination ids by adding a new command: open/close
> > >
> > > $> echo  \
> > >  open kms-instance \;\
> > >  class DRM_UT_KMS -T:kms-instance  # preset-dests-disable-sites \;\
> > > > /proc/dynamic_debug/control
> > >
> >
> > Instead of using above command to preset destination we could preset
> > destination with open command. I mean last successful
> > open would preset destination ? What do you think ?
> >
>
> I dont think it works - if open maps to a dest-number, (or implicit as
> TOP-of-stack)
> then you just have +T<dest-number>  (or +T <implicit tos>)
> rather than +T:dest-name
> and you still have to keep track of what dest-numbers were already used.
> (or every new dest needs an explicit OPEN before it)
>
> and how do you then get back to default instance ?
> open 0 ?
> close <previous-handle> ?
>
>
> by using names, all opens can be at the top,
> (and thus document in 1 block all the named-instances)
> and any named dest that hasnt been opened is an error
> (not just reusing previous OPEN)
>

Sorry, I should have been more specific with my proposal. Let me use
an example to clarify it:
open usb -> create trace instance "usb" and make it default
echo module usbcore +T > /proc/dynamic_debug/control --> write usbcore
debug logs to trace instance named usb
open tbt --> create trace instance "tbt" and make it default
echo module aaa +T:usb > /proc/dynamic_debug/control --> write aaa
debug logs to trace instance named usb, instance usb has to be used
explicitly

                         because now tbt is default trace instance
echo module bbb +T > /proc/dynamic_debug/control --> write bbb debug
logs to trace instance named tbt
close tbt --> close tbt trace instance, I omit this step but in order
for an instance to be successful closed it must not be used by any
callsite, after
                    closing tbt instance the usb becomes default instance

I agree that your method of setting default trace instance is more flexible:
class DRM_UT_KMS -T:kms-instance  # preset-dests-disable-sites

Maybe we can combine both to set default trace destination ?

Also I think we need a reserved keyword for writing debug logs to
trace events - maybe "event" keyword ?


>
> > >
> > > and +T  w/o dest means use existing setting, not just 0 (unless thats
> > > the existing setting)
> > >
> >
> > Sounds good.
> >
>
> :-)

On Sun, Nov 12, 2023 at 9:32 AM Łukasz Bartosik <lb@semihalf.com> wrote:
>
> pt., 10 lis 2023 o 21:03 <jim.cromie@gmail.com> napisał(a):
> >
> > On Fri, Nov 10, 2023 at 7:53 AM Łukasz Bartosik <lb@semihalf.com> wrote:
> > >
> > > sob., 4 lis 2023 o 22:49 <jim.cromie@gmail.com> napisał(a):
> > > >
> > > > On Fri, Nov 3, 2023 at 7:10 AM Łukasz Bartosik <lb@semihalf.com> wrote:
> > > > >
> > > > > When trace is enabled (T flag is set) and trace_dst field is set
> > > > > to value greater than 0 (0 is reserved for trace events) then
> > > > > debug logs will be written to trace instance pointed by trace_dst
> > > > > value, for example when trace_dst value is 2 then debug logs will
> > > > > be written to <debugfs>/tracing/instances/dyndbg_inst_2 instance.
> > > > > Given trace instance will not be initialized until debug logs are
> > > > > requested to be written to it and afer init will persist until
> > > > > reboot.
> > > > >
> > > >
> > > > restating 00 comments -
> > > >
> > > > you can get rid of integer destination ids by adding a new command: open/close
> > > >
> > > > $> echo  \
> > > >  open kms-instance \;\
> > > >  class DRM_UT_KMS -T:kms-instance  # preset-dests-disable-sites \;\
> > > > > /proc/dynamic_debug/control
> > > >
> > >
> > > Instead of using above command to preset destination we could preset
> > > destination with open command. I mean last successful
> > > open would preset destination ? What do you think ?
> > >
> >
> > I dont think it works - if open maps to a dest-number, (or implicit as
> > TOP-of-stack)
> > then you just have +T<dest-number>  (or +T <implicit tos>)
> > rather than +T:dest-name
> > and you still have to keep track of what dest-numbers were already used.
> > (or every new dest needs an explicit OPEN before it)
> >
> > and how do you then get back to default instance ?
> > open 0 ?
> > close <previous-handle> ?
> >
> >
> > by using names, all opens can be at the top,
> > (and thus document in 1 block all the named-instances)
> > and any named dest that hasnt been opened is an error
> > (not just reusing previous OPEN)
> >
>
> Sorry, I should have been more specific with my proposal. Let me use
> an example to clarify it:
> open usb    # -> create trace instance "usb" and make it default
> echo module usbcore +T > /proc/dynamic_debug/control ## --> write usbcore
> ## debug logs to trace instance named usb
> open tbt --> create trace instance "tbt" and make it default
> echo module aaa +T:usb > /proc/dynamic_debug/control --> write aaa
> debug logs to trace instance named usb, instance usb has to be used
> explicitly
>
>                          because now tbt is default trace instance

that feels too magical/ action at a distance.

ISTM it also muddles what the "default" is:

my-default:
what each callsite's current/preset dest is/was
the only way to set it is with explicit [-+]T:outstream

your-default:
whatever was last opened. whether it was 2 or 50 lines above,
or set weeks ago, the last time somebody opened an instance.

and as more instances are created
(potentially by different users?
after all there are separate instances,
and presumably separate interests),
the default gets less predictable.


> echo module bbb +T > /proc/dynamic_debug/control --> write bbb debug
> logs to trace instance named tbt
> close tbt --> close tbt trace instance, I omit this step but in order
> for an instance to be successful closed it must not be used by any
> callsite, after
>                     closing tbt instance the usb becomes default instance

so after 'close tbt',  the previous 'open usb' is now top-of-stack ?

how does that affect all existing callsite-users of tbt ?
do they continue to use the trace-instance theyve been writing to ?
If not, then reverting to the global instance seems much more predictable.

Or are you proposing that the close fails because the instance is still in use ?
this seems least surprising,
and more robust in the face of the next 'open foo'
which could otherwize reuse the dst_id mapped to tbt


>
> I agree that your method of setting default trace instance is more flexible:
> class DRM_UT_KMS -T:kms-instance  # preset-dests-disable-sites
>
> Maybe we can combine both to set default trace destination ?
>
> Also I think we need a reserved keyword for writing debug logs to
> trace events - maybe "event" keyword ?

do you mean "event" as a selector, like module, function, class, etc ?
if so, what are the values ?
any event under  /sys/kernel/debug/tracing/events/ ?

how does this get used ?

>
>
> >
> > > >
> > > > and +T  w/o dest means use existing setting, not just 0 (unless thats
> > > > the existing setting)
> > > >
> > >
> > > Sounds good.
> > >
> >
> > :-)

pon., 13 lis 2023 o 19:59 <jim.cromie@gmail.com> napisał(a):
>
> On Sun, Nov 12, 2023 at 9:32 AM Łukasz Bartosik <lb@semihalf.com> wrote:
> >
> > pt., 10 lis 2023 o 21:03 <jim.cromie@gmail.com> napisał(a):
> > >
> > > On Fri, Nov 10, 2023 at 7:53 AM Łukasz Bartosik <lb@semihalf.com> wrote:
> > > >
> > > > sob., 4 lis 2023 o 22:49 <jim.cromie@gmail.com> napisał(a):
> > > > >
> > > > > On Fri, Nov 3, 2023 at 7:10 AM Łukasz Bartosik <lb@semihalf.com> wrote:
> > > > > >
> > > > > > When trace is enabled (T flag is set) and trace_dst field is set
> > > > > > to value greater than 0 (0 is reserved for trace events) then
> > > > > > debug logs will be written to trace instance pointed by trace_dst
> > > > > > value, for example when trace_dst value is 2 then debug logs will
> > > > > > be written to <debugfs>/tracing/instances/dyndbg_inst_2 instance.
> > > > > > Given trace instance will not be initialized until debug logs are
> > > > > > requested to be written to it and afer init will persist until
> > > > > > reboot.
> > > > > >
> > > > >
> > > > > restating 00 comments -
> > > > >
> > > > > you can get rid of integer destination ids by adding a new command: open/close
> > > > >
> > > > > $> echo  \
> > > > >  open kms-instance \;\
> > > > >  class DRM_UT_KMS -T:kms-instance  # preset-dests-disable-sites \;\
> > > > > > /proc/dynamic_debug/control
> > > > >
> > > >
> > > > Instead of using above command to preset destination we could preset
> > > > destination with open command. I mean last successful
> > > > open would preset destination ? What do you think ?
> > > >
> > >
> > > I dont think it works - if open maps to a dest-number, (or implicit as
> > > TOP-of-stack)
> > > then you just have +T<dest-number>  (or +T <implicit tos>)
> > > rather than +T:dest-name
> > > and you still have to keep track of what dest-numbers were already used.
> > > (or every new dest needs an explicit OPEN before it)
> > >
> > > and how do you then get back to default instance ?
> > > open 0 ?
> > > close <previous-handle> ?
> > >
> > >
> > > by using names, all opens can be at the top,
> > > (and thus document in 1 block all the named-instances)
> > > and any named dest that hasnt been opened is an error
> > > (not just reusing previous OPEN)
> > >
> >
> > Sorry, I should have been more specific with my proposal. Let me use
> > an example to clarify it:
> > open usb    # -> create trace instance "usb" and make it default
> > echo module usbcore +T > /proc/dynamic_debug/control ## --> write usbcore
> > ## debug logs to trace instance named usb
> > open tbt --> create trace instance "tbt" and make it default
> > echo module aaa +T:usb > /proc/dynamic_debug/control --> write aaa
> > debug logs to trace instance named usb, instance usb has to be used
> > explicitly
> >
> >                          because now tbt is default trace instance
>
> that feels too magical/ action at a distance.
>
> ISTM it also muddles what the "default" is:
>
> my-default:
> what each callsite's current/preset dest is/was
> the only way to set it is with explicit [-+]T:outstream
>

I see your point, I will follow your suggestion.

> your-default:
> whatever was last opened. whether it was 2 or 50 lines above,
> or set weeks ago, the last time somebody opened an instance.
>
> and as more instances are created
> (potentially by different users?
> after all there are separate instances,
> and presumably separate interests),
> the default gets less predictable.
>
>
> > echo module bbb +T > /proc/dynamic_debug/control --> write bbb debug
> > logs to trace instance named tbt
> > close tbt --> close tbt trace instance, I omit this step but in order
> > for an instance to be successful closed it must not be used by any
> > callsite, after
> >                     closing tbt instance the usb becomes default instance
>
> so after 'close tbt',  the previous 'open usb' is now top-of-stack ?
>
> how does that affect all existing callsite-users of tbt ?
> do they continue to use the trace-instance theyve been writing to ?
> If not, then reverting to the global instance seems much more predictable.
>
> Or are you proposing that the close fails because the instance is still in use ?
> this seems least surprising,
> and more robust in the face of the next 'open foo'
> which could otherwize reuse the dst_id mapped to tbt
>
>
> >
> > I agree that your method of setting default trace instance is more flexible:
> > class DRM_UT_KMS -T:kms-instance  # preset-dests-disable-sites
> >
> > Maybe we can combine both to set default trace destination ?
> >
> > Also I think we need a reserved keyword for writing debug logs to
> > trace events - maybe "event" keyword ?
>
> do you mean "event" as a selector, like module, function, class, etc ?
> if so, what are the values ?
> any event under  /sys/kernel/debug/tracing/events/ ?
>
> how does this get used ?
>

I meant that we need to reserve name/keyword to enable writing debug
logs to trace events (prdbg and devdbg), for example
echo module usbcore +T:event > /proc/dynamic_debug/control

Or do you anticipate other way to do it ?

> >
> >
> > >
> > > > >
> > > > > and +T  w/o dest means use existing setting, not just 0 (unless thats
> > > > > the existing setting)
> > > > >
> > > >
> > > > Sounds good.
> > > >
> > >
> > > :-)

On Mon, Nov 13, 2023 at 4:44 PM Łukasz Bartosik <lb@semihalf.com> wrote:
>
> pon., 13 lis 2023 o 19:59 <jim.cromie@gmail.com> napisał(a):
> >
> > On Sun, Nov 12, 2023 at 9:32 AM Łukasz Bartosik <lb@semihalf.com> wrote:
> > >
> > > pt., 10 lis 2023 o 21:03 <jim.cromie@gmail.com> napisał(a):
> > > >
> > > > On Fri, Nov 10, 2023 at 7:53 AM Łukasz Bartosik <lb@semihalf.com> wrote:
> > > > >
> > > > > sob., 4 lis 2023 o 22:49 <jim.cromie@gmail.com> napisał(a):
> > > > > >
> > > > > > On Fri, Nov 3, 2023 at 7:10 AM Łukasz Bartosik <lb@semihalf.com> wrote:
> > > > > > >
> > > > > > > When trace is enabled (T flag is set) and trace_dst field is set
> > > > > > > to value greater than 0 (0 is reserved for trace events) then
> > > > > > > debug logs will be written to trace instance pointed by trace_dst
> > > > > > > value, for example when trace_dst value is 2 then debug logs will
> > > > > > > be written to <debugfs>/tracing/instances/dyndbg_inst_2 instance.
> > > > > > > Given trace instance will not be initialized until debug logs are
> > > > > > > requested to be written to it and afer init will persist until
> > > > > > > reboot.
> > > > > > >
> > > > > >
> > > > > > restating 00 comments -
> > > > > >
> > > > > > you can get rid of integer destination ids by adding a new command: open/close
> > > > > >
> > > > > > $> echo  \
> > > > > >  open kms-instance \;\
> > > > > >  class DRM_UT_KMS -T:kms-instance  # preset-dests-disable-sites \;\
> > > > > > > /proc/dynamic_debug/control
> > > > > >
> > > > >
> > > > > Instead of using above command to preset destination we could preset
> > > > > destination with open command. I mean last successful
> > > > > open would preset destination ? What do you think ?
> > > > >
> > > >
> > > > I dont think it works - if open maps to a dest-number, (or implicit as
> > > > TOP-of-stack)
> > > > then you just have +T<dest-number>  (or +T <implicit tos>)
> > > > rather than +T:dest-name
> > > > and you still have to keep track of what dest-numbers were already used.
> > > > (or every new dest needs an explicit OPEN before it)
> > > >
> > > > and how do you then get back to default instance ?
> > > > open 0 ?
> > > > close <previous-handle> ?
> > > >
> > > >
> > > > by using names, all opens can be at the top,
> > > > (and thus document in 1 block all the named-instances)
> > > > and any named dest that hasnt been opened is an error
> > > > (not just reusing previous OPEN)
> > > >
> > >
> > > Sorry, I should have been more specific with my proposal. Let me use
> > > an example to clarify it:
> > > open usb    # -> create trace instance "usb" and make it default
> > > echo module usbcore +T > /proc/dynamic_debug/control ## --> write usbcore
> > > ## debug logs to trace instance named usb
> > > open tbt --> create trace instance "tbt" and make it default
> > > echo module aaa +T:usb > /proc/dynamic_debug/control --> write aaa
> > > debug logs to trace instance named usb, instance usb has to be used
> > > explicitly
> > >
> > >                          because now tbt is default trace instance
> >
> > that feels too magical/ action at a distance.
> >
> > ISTM it also muddles what the "default" is:
> >
> > my-default:
> > what each callsite's current/preset dest is/was
> > the only way to set it is with explicit [-+]T:outstream
> >
>
> I see your point, I will follow your suggestion.
>
> > your-default:
> > whatever was last opened. whether it was 2 or 50 lines above,
> > or set weeks ago, the last time somebody opened an instance.
> >
> > and as more instances are created
> > (potentially by different users?
> > after all there are separate instances,
> > and presumably separate interests),
> > the default gets less predictable.
> >
> >
> > > echo module bbb +T > /proc/dynamic_debug/control --> write bbb debug
> > > logs to trace instance named tbt
> > > close tbt --> close tbt trace instance, I omit this step but in order
> > > for an instance to be successful closed it must not be used by any
> > > callsite, after
> > >                     closing tbt instance the usb becomes default instance
> >
> > so after 'close tbt',  the previous 'open usb' is now top-of-stack ?
> >
> > how does that affect all existing callsite-users of tbt ?
> > do they continue to use the trace-instance theyve been writing to ?
> > If not, then reverting to the global instance seems much more predictable.
> >
> > Or are you proposing that the close fails because the instance is still in use ?
> > this seems least surprising,
> > and more robust in the face of the next 'open foo'
> > which could otherwize reuse the dst_id mapped to tbt
> >
> >
> > >
> > > I agree that your method of setting default trace instance is more flexible:
> > > class DRM_UT_KMS -T:kms-instance  # preset-dests-disable-sites
> > >
> > > Maybe we can combine both to set default trace destination ?
> > >
> > > Also I think we need a reserved keyword for writing debug logs to
> > > trace events - maybe "event" keyword ?
> >
> > do you mean "event" as a selector, like module, function, class, etc ?
> > if so, what are the values ?
> > any event under  /sys/kernel/debug/tracing/events/ ?
> >
> > how does this get used ?
> >
>
> I meant that we need to reserve name/keyword to enable writing debug
> logs to trace events (prdbg and devdbg), for example
> echo module usbcore +T:event > /proc/dynamic_debug/control
>
> Or do you anticipate other way to do it ?

way back, when I had even fewer clues,
I sent patches to call trace-printk when +T was set.
Steve didnt like it, I think cuz it could flood the tracebuf.

Thats why I added the prdbg and devdbg event-types,
so that they could be disabled easily using /sys/kernel/debug/tracing/
putting them squarely under trace-control.

Note that this puts 2 off-switches in series,
both tracefs and >control can disable all the pr_debug traffic,
tracefs by event-type, and >control at individual callsite level.

echo 1 > /sys/kernel/debug/tracing/dyndbg/enable
echo 1 > /sys/kernel/debug/tracing/dyndbg/prdbg/enable
echo 1 > /sys/kernel/debug/tracing/dyndbg/devdbg/enable

I briefly thought about linking the 2 off-switches,
but punted cuz I thought it complicated things,
(how exactly would they get coupled?)
and I didnt want to distract from larger goals

Does that address your question ?

On a related point, I also added drm_dbg and drm_devdbg.
Those are issued from __drm_dbg & __drm_dev_dbg
 respectively when CONFIG_DRM_USE_DYNAMIC_DEBUG=n.

Im not sure theyre more useful than confusing yet.

>
> > >
> > >
> > > >
> > > > > >
> > > > > > and +T  w/o dest means use existing setting, not just 0 (unless thats
> > > > > > the existing setting)
> > > > > >
> > > > >
> > > > > Sounds good.
> > > > >
> > > >
> > > > :-)

wt., 14 lis 2023 o 02:08 <jim.cromie@gmail.com> napisał(a):
>
> On Mon, Nov 13, 2023 at 4:44 PM Łukasz Bartosik <lb@semihalf.com> wrote:
> >
> > pon., 13 lis 2023 o 19:59 <jim.cromie@gmail.com> napisał(a):
> > >
> > > On Sun, Nov 12, 2023 at 9:32 AM Łukasz Bartosik <lb@semihalf.com> wrote:
> > > >
> > > > pt., 10 lis 2023 o 21:03 <jim.cromie@gmail.com> napisał(a):
> > > > >
> > > > > On Fri, Nov 10, 2023 at 7:53 AM Łukasz Bartosik <lb@semihalf.com> wrote:
> > > > > >
> > > > > > sob., 4 lis 2023 o 22:49 <jim.cromie@gmail.com> napisał(a):
> > > > > > >
> > > > > > > On Fri, Nov 3, 2023 at 7:10 AM Łukasz Bartosik <lb@semihalf.com> wrote:
> > > > > > > >
> > > > > > > > When trace is enabled (T flag is set) and trace_dst field is set
> > > > > > > > to value greater than 0 (0 is reserved for trace events) then
> > > > > > > > debug logs will be written to trace instance pointed by trace_dst
> > > > > > > > value, for example when trace_dst value is 2 then debug logs will
> > > > > > > > be written to <debugfs>/tracing/instances/dyndbg_inst_2 instance.
> > > > > > > > Given trace instance will not be initialized until debug logs are
> > > > > > > > requested to be written to it and afer init will persist until
> > > > > > > > reboot.
> > > > > > > >
> > > > > > >
> > > > > > > restating 00 comments -
> > > > > > >
> > > > > > > you can get rid of integer destination ids by adding a new command: open/close
> > > > > > >
> > > > > > > $> echo  \
> > > > > > >  open kms-instance \;\
> > > > > > >  class DRM_UT_KMS -T:kms-instance  # preset-dests-disable-sites \;\
> > > > > > > > /proc/dynamic_debug/control
> > > > > > >
> > > > > >
> > > > > > Instead of using above command to preset destination we could preset
> > > > > > destination with open command. I mean last successful
> > > > > > open would preset destination ? What do you think ?
> > > > > >
> > > > >
> > > > > I dont think it works - if open maps to a dest-number, (or implicit as
> > > > > TOP-of-stack)
> > > > > then you just have +T<dest-number>  (or +T <implicit tos>)
> > > > > rather than +T:dest-name
> > > > > and you still have to keep track of what dest-numbers were already used.
> > > > > (or every new dest needs an explicit OPEN before it)
> > > > >
> > > > > and how do you then get back to default instance ?
> > > > > open 0 ?
> > > > > close <previous-handle> ?
> > > > >
> > > > >
> > > > > by using names, all opens can be at the top,
> > > > > (and thus document in 1 block all the named-instances)
> > > > > and any named dest that hasnt been opened is an error
> > > > > (not just reusing previous OPEN)
> > > > >
> > > >
> > > > Sorry, I should have been more specific with my proposal. Let me use
> > > > an example to clarify it:
> > > > open usb    # -> create trace instance "usb" and make it default
> > > > echo module usbcore +T > /proc/dynamic_debug/control ## --> write usbcore
> > > > ## debug logs to trace instance named usb
> > > > open tbt --> create trace instance "tbt" and make it default
> > > > echo module aaa +T:usb > /proc/dynamic_debug/control --> write aaa
> > > > debug logs to trace instance named usb, instance usb has to be used
> > > > explicitly
> > > >
> > > >                          because now tbt is default trace instance
> > >
> > > that feels too magical/ action at a distance.
> > >
> > > ISTM it also muddles what the "default" is:
> > >
> > > my-default:
> > > what each callsite's current/preset dest is/was
> > > the only way to set it is with explicit [-+]T:outstream
> > >
> >
> > I see your point, I will follow your suggestion.
> >
> > > your-default:
> > > whatever was last opened. whether it was 2 or 50 lines above,
> > > or set weeks ago, the last time somebody opened an instance.
> > >
> > > and as more instances are created
> > > (potentially by different users?
> > > after all there are separate instances,
> > > and presumably separate interests),
> > > the default gets less predictable.
> > >
> > >
> > > > echo module bbb +T > /proc/dynamic_debug/control --> write bbb debug
> > > > logs to trace instance named tbt
> > > > close tbt --> close tbt trace instance, I omit this step but in order
> > > > for an instance to be successful closed it must not be used by any
> > > > callsite, after
> > > >                     closing tbt instance the usb becomes default instance
> > >
> > > so after 'close tbt',  the previous 'open usb' is now top-of-stack ?
> > >
> > > how does that affect all existing callsite-users of tbt ?
> > > do they continue to use the trace-instance theyve been writing to ?
> > > If not, then reverting to the global instance seems much more predictable.
> > >
> > > Or are you proposing that the close fails because the instance is still in use ?
> > > this seems least surprising,
> > > and more robust in the face of the next 'open foo'
> > > which could otherwize reuse the dst_id mapped to tbt
> > >
> > >
> > > >
> > > > I agree that your method of setting default trace instance is more flexible:
> > > > class DRM_UT_KMS -T:kms-instance  # preset-dests-disable-sites
> > > >
> > > > Maybe we can combine both to set default trace destination ?
> > > >
> > > > Also I think we need a reserved keyword for writing debug logs to
> > > > trace events - maybe "event" keyword ?
> > >
> > > do you mean "event" as a selector, like module, function, class, etc ?
> > > if so, what are the values ?
> > > any event under  /sys/kernel/debug/tracing/events/ ?
> > >
> > > how does this get used ?
> > >
> >
> > I meant that we need to reserve name/keyword to enable writing debug
> > logs to trace events (prdbg and devdbg), for example
> > echo module usbcore +T:event > /proc/dynamic_debug/control
> >
> > Or do you anticipate other way to do it ?
>
> way back, when I had even fewer clues,
> I sent patches to call trace-printk when +T was set.
> Steve didnt like it, I think cuz it could flood the tracebuf.
>
> Thats why I added the prdbg and devdbg event-types,
> so that they could be disabled easily using /sys/kernel/debug/tracing/
> putting them squarely under trace-control.
>
> Note that this puts 2 off-switches in series,
> both tracefs and >control can disable all the pr_debug traffic,
> tracefs by event-type, and >control at individual callsite level.
>
> echo 1 > /sys/kernel/debug/tracing/dyndbg/enable
> echo 1 > /sys/kernel/debug/tracing/dyndbg/prdbg/enable
> echo 1 > /sys/kernel/debug/tracing/dyndbg/devdbg/enable
>
> I briefly thought about linking the 2 off-switches,
> but punted cuz I thought it complicated things,
> (how exactly would they get coupled?)
> and I didnt want to distract from larger goals
>
> Does that address your question ?
>

Jim,

Thanks but it doesn't answer my question.

How do you plan to enable output to tracefs event at a callsite level ?

In my original proposal it was enabled by setting trace destination to
0. Since we are moving to names instead of numbers now I guess we need
to reserve a name for it not to clash with trace instance names
provided by users. That's why I proposed to reserve name "event" for
that purpose and be used as +T:event.

Or did I miss answer to that in our long discussion :> ?

Thanks,
Lukasz

> On a related point, I also added drm_dbg and drm_devdbg.
> Those are issued from __drm_dbg & __drm_dev_dbg
>  respectively when CONFIG_DRM_USE_DYNAMIC_DEBUG=n.
>
> Im not sure theyre more useful than confusing yet.
>
> >
> > > >
> > > >
> > > > >
> > > > > > >
> > > > > > > and +T  w/o dest means use existing setting, not just 0 (unless thats
> > > > > > > the existing setting)
> > > > > > >
> > > > > >
> > > > > > Sounds good.
> > > > > >
> > > > >
> > > > > :-)

On Tue, Nov 14, 2023 at 12:45 AM Łukasz Bartosik <lb@semihalf.com> wrote:
>
> wt., 14 lis 2023 o 02:08 <jim.cromie@gmail.com> napisał(a):
> >

> > > > > Also I think we need a reserved keyword for writing debug logs to
> > > > > trace events - maybe "event" keyword ?
> > > >
> > > > do you mean "event" as a selector, like module, function, class, etc ?
> > > > if so, what are the values ?
> > > > any event under  /sys/kernel/debug/tracing/events/ ?
> > > >
> > > > how does this get used ?
> > > >
> > >
> > > I meant that we need to reserve name/keyword to enable writing debug
> > > logs to trace events (prdbg and devdbg), for example
> > > echo module usbcore +T:event > /proc/dynamic_debug/control
> > >
> > > Or do you anticipate other way to do it ?
> >
> > way back, when I had even fewer clues,
> > I sent patches to call trace-printk when +T was set.
> > Steve didnt like it, I think cuz it could flood the tracebuf.
> >
> > Thats why I added the prdbg and devdbg event-types,
> > so that they could be disabled easily using /sys/kernel/debug/tracing/
> > putting them squarely under trace-control.
> >
> > Note that this puts 2 off-switches in series,
> > both tracefs and >control can disable all the pr_debug traffic,
> > tracefs by event-type, and >control at individual callsite level.
> >
> > echo 1 > /sys/kernel/debug/tracing/dyndbg/enable
> > echo 1 > /sys/kernel/debug/tracing/dyndbg/prdbg/enable
> > echo 1 > /sys/kernel/debug/tracing/dyndbg/devdbg/enable
> >
> > I briefly thought about linking the 2 off-switches,
> > but punted cuz I thought it complicated things,
> > (how exactly would they get coupled?)
> > and I didnt want to distract from larger goals
> >
> > Does that address your question ?
> >
>
> Jim,
>
> Thanks but it doesn't answer my question.
>
> How do you plan to enable output to tracefs event at a callsite level ?
>
> In my original proposal it was enabled by setting trace destination to
> 0. Since we are moving to names instead of numbers now I guess we need
> to reserve a name for it not to clash with trace instance names
> provided by users. That's why I proposed to reserve name "event" for
> that purpose and be used as +T:event.
>

Ok, I got your point now.

how about we call it "0" ?
it should be an obvious "magical" value,
cuz it doesnt need to be open'd, and cant be close'd

then we can revert to global tracebuf by its "name"
echo module foo +T:0 > /proc/dynamic_debug/control

we probably should also limit the trace-instance-names to ^\w+

> Or did I miss answer to that in our long discussion :> ?

nope :-)

thanks,
Jim

>
> Thanks,
> Lukasz
>
> > On a related point, I also added drm_dbg and drm_devdbg.
> > Those are issued from __drm_dbg & __drm_dev_dbg
> >  respectively when CONFIG_DRM_USE_DYNAMIC_DEBUG=n.
> >
> > Im not sure theyre more useful than confusing yet.
> >
> > >
> > > > >
> > > > >
> > > > > >
> > > > > > > >
> > > > > > > > and +T  w/o dest means use existing setting, not just 0 (unless thats
> > > > > > > > the existing setting)
> > > > > > > >
> > > > > > >
> > > > > > > Sounds good.
> > > > > > >
> > > > > >
> > > > > > :-)

wt., 14 lis 2023 o 16:41 <jim.cromie@gmail.com> napisał(a):
>
> On Tue, Nov 14, 2023 at 12:45 AM Łukasz Bartosik <lb@semihalf.com> wrote:
> >
> > wt., 14 lis 2023 o 02:08 <jim.cromie@gmail.com> napisał(a):
> > >
>
> > > > > > Also I think we need a reserved keyword for writing debug logs to
> > > > > > trace events - maybe "event" keyword ?
> > > > >
> > > > > do you mean "event" as a selector, like module, function, class, etc ?
> > > > > if so, what are the values ?
> > > > > any event under  /sys/kernel/debug/tracing/events/ ?
> > > > >
> > > > > how does this get used ?
> > > > >
> > > >
> > > > I meant that we need to reserve name/keyword to enable writing debug
> > > > logs to trace events (prdbg and devdbg), for example
> > > > echo module usbcore +T:event > /proc/dynamic_debug/control
> > > >
> > > > Or do you anticipate other way to do it ?
> > >
> > > way back, when I had even fewer clues,
> > > I sent patches to call trace-printk when +T was set.
> > > Steve didnt like it, I think cuz it could flood the tracebuf.
> > >
> > > Thats why I added the prdbg and devdbg event-types,
> > > so that they could be disabled easily using /sys/kernel/debug/tracing/
> > > putting them squarely under trace-control.
> > >
> > > Note that this puts 2 off-switches in series,
> > > both tracefs and >control can disable all the pr_debug traffic,
> > > tracefs by event-type, and >control at individual callsite level.
> > >
> > > echo 1 > /sys/kernel/debug/tracing/dyndbg/enable
> > > echo 1 > /sys/kernel/debug/tracing/dyndbg/prdbg/enable
> > > echo 1 > /sys/kernel/debug/tracing/dyndbg/devdbg/enable
> > >
> > > I briefly thought about linking the 2 off-switches,
> > > but punted cuz I thought it complicated things,
> > > (how exactly would they get coupled?)
> > > and I didnt want to distract from larger goals
> > >
> > > Does that address your question ?
> > >
> >
> > Jim,
> >
> > Thanks but it doesn't answer my question.
> >
> > How do you plan to enable output to tracefs event at a callsite level ?
> >
> > In my original proposal it was enabled by setting trace destination to
> > 0. Since we are moving to names instead of numbers now I guess we need
> > to reserve a name for it not to clash with trace instance names
> > provided by users. That's why I proposed to reserve name "event" for
> > that purpose and be used as +T:event.
> >
>
> Ok, I got your point now.
>
> how about we call it "0" ?
> it should be an obvious "magical" value,
> cuz it doesnt need to be open'd, and cant be close'd
>
> then we can revert to global tracebuf by its "name"
> echo module foo +T:0 > /proc/dynamic_debug/control
>

I like it. It resembles surprise emoji :0

> we probably should also limit the trace-instance-names to ^\w+
>

Ack

I think this closes the last open topic to discuss for now.
I will get back with next patchset version to you soon.

> > Or did I miss answer to that in our long discussion :> ?
>
> nope :-)
>
> thanks,
> Jim
>
> >
> > Thanks,
> > Lukasz
> >
> > > On a related point, I also added drm_dbg and drm_devdbg.
> > > Those are issued from __drm_dbg & __drm_dev_dbg
> > >  respectively when CONFIG_DRM_USE_DYNAMIC_DEBUG=n.
> > >
> > > Im not sure theyre more useful than confusing yet.
> > >
> > > >
> > > > > >
> > > > > >
> > > > > > >
> > > > > > > > >
> > > > > > > > > and +T  w/o dest means use existing setting, not just 0 (unless thats
> > > > > > > > > the existing setting)
> > > > > > > > >
> > > > > > > >
> > > > > > > > Sounds good.
> > > > > > > >
> > > > > > >
> > > > > > > :-)