[PATCH v1 09/12] dyndbg: add trace destination field to _ddebug

Łukasz Bartosik posted 12 patches 2 years, 1 month ago
There is a newer version of this series
[PATCH v1 09/12] dyndbg: add trace destination field to _ddebug
Posted by Łukasz Bartosik 2 years, 1 month ago
Add trace destination field (trace_dst) to the _ddebug structure.
The trace destination field is used to determine output of debug
logs when +T is set. Setting trace_dst value to 0 (default) enables
output to prdbg and devdbg trace events. Setting trace_dst value to
a value in range of [1..255] enables output to trace instance.

Signed-off-by: Łukasz Bartosik <lb@semihalf.com>
---
 include/linux/dynamic_debug.h | 13 +++++++++++--
 lib/dynamic_debug.c           | 28 +++++++++++++++++++---------
 2 files changed, 30 insertions(+), 11 deletions(-)

diff --git a/include/linux/dynamic_debug.h b/include/linux/dynamic_debug.h
index 684766289bfc..3084302876b4 100644
--- a/include/linux/dynamic_debug.h
+++ b/include/linux/dynamic_debug.h
@@ -60,9 +60,18 @@ struct _ddebug {
 #else
 #define _DPRINTK_FLAGS_DEFAULT 0
 #endif
-	struct {
+	struct dd_ctrl {
 		unsigned int flags:8;
-		unsigned unused:24;
+	/*
+	 * The trace destination field is used to determine output of debug
+	 * logs when +T is set. Setting trace_dst value to 0 (default) enables
+	 * output to prdbg and devdbg trace events. Setting trace_dst value to
+	 * a value in range of [1..255] enables output to trace instance.
+	 */
+#define TRACE_DST_BITS 8
+		unsigned int trace_dst:TRACE_DST_BITS;
+#define TRACE_DST_MAX	((1 << TRACE_DST_BITS) - 1)
+		unsigned unused:16;
 	} ctrl;
 } __attribute__((aligned(8)));
 
diff --git a/lib/dynamic_debug.c b/lib/dynamic_debug.c
index ca87adf327df..3218ab078a76 100644
--- a/lib/dynamic_debug.c
+++ b/lib/dynamic_debug.c
@@ -80,14 +80,24 @@ 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)");
 
+static inline struct dd_ctrl *get_ctrl(struct _ddebug *desc)
+{
+	return &desc->ctrl;
+}
+
+static inline void set_ctrl(struct _ddebug *desc, struct dd_ctrl *ctrl)
+{
+	desc->ctrl = *ctrl;
+}
+
 static inline unsigned int get_flags(const struct _ddebug *desc)
 {
 	return desc->ctrl.flags;
 }
 
-static inline void set_flags(struct _ddebug *desc, unsigned int val)
+static inline unsigned int get_trace_dst(const struct _ddebug *desc)
 {
-	desc->ctrl.flags = val;
+	return desc->ctrl.trace_dst;
 }
 
 /* Return the path relative to source root */
@@ -190,8 +200,8 @@ static int ddebug_change(const struct ddebug_query *query,
 {
 	int i;
 	struct ddebug_table *dt;
-	unsigned int newflags;
 	unsigned int nfound = 0;
+	struct dd_ctrl nctrl = {0};
 	struct flagsbuf fbuf, nbuf;
 	struct ddebug_class_map *map = NULL;
 	int __outvar valid_class;
@@ -257,14 +267,14 @@ static int ddebug_change(const struct ddebug_query *query,
 
 			nfound++;
 
-			newflags = (get_flags(dp) & modifiers->mask) | modifiers->flags;
-			if (newflags == get_flags(dp))
+			nctrl.flags = (get_flags(dp) & modifiers->mask) | modifiers->flags;
+			if (!memcmp(&nctrl, get_ctrl(dp), sizeof(struct dd_ctrl)))
 				continue;
 #ifdef CONFIG_JUMP_LABEL
 			if (get_flags(dp) & _DPRINTK_FLAGS_ENABLED) {
-				if (!(newflags & _DPRINTK_FLAGS_ENABLED))
+				if (!(nctrl.flags & _DPRINTK_FLAGS_ENABLED))
 					static_branch_disable(&dp->key.dd_key_true);
-			} else if (newflags & _DPRINTK_FLAGS_ENABLED) {
+			} else if (nctrl.flags & _DPRINTK_FLAGS_ENABLED) {
 				static_branch_enable(&dp->key.dd_key_true);
 			}
 #endif
@@ -272,8 +282,8 @@ static int ddebug_change(const struct ddebug_query *query,
 				  trim_prefix(dp->filename), dp->lineno,
 				  dt->mod_name, dp->function,
 				  ddebug_describe_flags(get_flags(dp), &fbuf),
-				  ddebug_describe_flags(newflags, &nbuf));
-			set_flags(dp, newflags);
+				  ddebug_describe_flags(nctrl.flags, &nbuf));
+			set_ctrl(dp, &nctrl);
 		}
 	}
 	mutex_unlock(&ddebug_lock);
-- 
2.42.0.869.gea05f2083d-goog

Re: [PATCH v1 09/12] dyndbg: add trace destination field to _ddebug
Posted by jim.cromie@gmail.com 2 years, 1 month ago
On Fri, Nov 3, 2023 at 7:10 AM Łukasz Bartosik <lb@semihalf.com> wrote:
>
> Add trace destination field (trace_dst) to the _ddebug structure.
> The trace destination field is used to determine output of debug
> logs when +T is set. Setting trace_dst value to 0 (default) enables
> output to prdbg and devdbg trace events. Setting trace_dst value to
> a value in range of [1..255] enables output to trace instance.


should we do some expectation setting here ?
255 is something of a promise to more than tom,dick,harry.
16-64 is more suggestive of a limited resource,
might encourage more judicious use.

will look further later
Re: [PATCH v1 09/12] dyndbg: add trace destination field to _ddebug
Posted by Łukasz Bartosik 2 years, 1 month ago
sob., 4 lis 2023 o 02:39 <jim.cromie@gmail.com> napisał(a):
>
> On Fri, Nov 3, 2023 at 7:10 AM Łukasz Bartosik <lb@semihalf.com> wrote:
> >
> > Add trace destination field (trace_dst) to the _ddebug structure.
> > The trace destination field is used to determine output of debug
> > logs when +T is set. Setting trace_dst value to 0 (default) enables
> > output to prdbg and devdbg trace events. Setting trace_dst value to
> > a value in range of [1..255] enables output to trace instance.
>
>
> should we do some expectation setting here ?
> 255 is something of a promise to more than tom,dick,harry.
> 16-64 is more suggestive of a limited resource,
> might encourage more judicious use.
>

How about making it configurable in kernel Kconfig with default value
set to 16 or 32 ?

> will look further later
Re: [PATCH v1 09/12] dyndbg: add trace destination field to _ddebug
Posted by jim.cromie@gmail.com 2 years, 1 month ago
On Fri, Nov 10, 2023 at 7:52 AM Łukasz Bartosik <lb@semihalf.com> wrote:
>
> sob., 4 lis 2023 o 02:39 <jim.cromie@gmail.com> napisał(a):
> >
> > On Fri, Nov 3, 2023 at 7:10 AM Łukasz Bartosik <lb@semihalf.com> wrote:
> > >
> > > Add trace destination field (trace_dst) to the _ddebug structure.
> > > The trace destination field is used to determine output of debug
> > > logs when +T is set. Setting trace_dst value to 0 (default) enables
> > > output to prdbg and devdbg trace events. Setting trace_dst value to
> > > a value in range of [1..255] enables output to trace instance.
> >
> >
> > should we do some expectation setting here ?
> > 255 is something of a promise to more than tom,dick,harry.
> > 16-64 is more suggestive of a limited resource,
> > might encourage more judicious use.
> >
>
> How about making it configurable in kernel Kconfig with default value
> set to 16 or 32 ?
>

given the general dislike of extra knobs, it's not the battle I would choose.
ISTM we could start small, add bits later (if needed)
maybe 16 is too parsimonious; esp if DRM wants multiple instances per
driver (device?)



> > will look further later
Re: [PATCH v1 09/12] dyndbg: add trace destination field to _ddebug
Posted by Łukasz Bartosik 2 years, 1 month ago
pt., 10 lis 2023 o 20:37 <jim.cromie@gmail.com> napisał(a):
>
> On Fri, Nov 10, 2023 at 7:52 AM Łukasz Bartosik <lb@semihalf.com> wrote:
> >
> > sob., 4 lis 2023 o 02:39 <jim.cromie@gmail.com> napisał(a):
> > >
> > > On Fri, Nov 3, 2023 at 7:10 AM Łukasz Bartosik <lb@semihalf.com> wrote:
> > > >
> > > > Add trace destination field (trace_dst) to the _ddebug structure.
> > > > The trace destination field is used to determine output of debug
> > > > logs when +T is set. Setting trace_dst value to 0 (default) enables
> > > > output to prdbg and devdbg trace events. Setting trace_dst value to
> > > > a value in range of [1..255] enables output to trace instance.
> > >
> > >
> > > should we do some expectation setting here ?
> > > 255 is something of a promise to more than tom,dick,harry.
> > > 16-64 is more suggestive of a limited resource,
> > > might encourage more judicious use.
> > >
> >
> > How about making it configurable in kernel Kconfig with default value
> > set to 16 or 32 ?
> >
>
> given the general dislike of extra knobs, it's not the battle I would choose.
> ISTM we could start small, add bits later (if needed)
> maybe 16 is too parsimonious; esp if DRM wants multiple instances per
> driver (device?)
>

If we don't want the extra config knob then I would opt for 64.

>
>
> > > will look further later
Re: [PATCH v1 09/12] dyndbg: add trace destination field to _ddebug
Posted by jim.cromie@gmail.com 2 years, 1 month ago
On Sun, Nov 12, 2023 at 9:29 AM Łukasz Bartosik <lb@semihalf.com> wrote:
>
> pt., 10 lis 2023 o 20:37 <jim.cromie@gmail.com> napisał(a):
> >
> > On Fri, Nov 10, 2023 at 7:52 AM Łukasz Bartosik <lb@semihalf.com> wrote:
> > >
> > > sob., 4 lis 2023 o 02:39 <jim.cromie@gmail.com> napisał(a):
> > > >
> > > > On Fri, Nov 3, 2023 at 7:10 AM Łukasz Bartosik <lb@semihalf.com> wrote:
> > > > >
> > > > > Add trace destination field (trace_dst) to the _ddebug structure.
> > > > > The trace destination field is used to determine output of debug
> > > > > logs when +T is set. Setting trace_dst value to 0 (default) enables
> > > > > output to prdbg and devdbg trace events. Setting trace_dst value to
> > > > > a value in range of [1..255] enables output to trace instance.
> > > >
> > > >
> > > > should we do some expectation setting here ?
> > > > 255 is something of a promise to more than tom,dick,harry.
> > > > 16-64 is more suggestive of a limited resource,
> > > > might encourage more judicious use.
> > > >
> > >
> > > How about making it configurable in kernel Kconfig with default value
> > > set to 16 or 32 ?
> > >
> >
> > given the general dislike of extra knobs, it's not the battle I would choose.
> > ISTM we could start small, add bits later (if needed)
> > maybe 16 is too parsimonious; esp if DRM wants multiple instances per
> > driver (device?)
> >
>
> If we don't want the extra config knob then I would opt for 64.

I think I could live with that.
And I dont foresee other uses for spare bits atm.