[PATCH v3 01/21] dyndbg-docs: eschew file /full/path query in docs

[PATCH v3 01/21] dyndbg-docs: eschew file /full/path query in docs

[Jim Cromie]

Regarding:
commit 2b6783191da7 ("dynamic_debug: add trim_prefix() to provide source-root relative paths")
commit a73619a845d5 ("kbuild: use -fmacro-prefix-map to make __FILE__ a relative path")

2nd commit broke dynamic-debug's "file $fullpath" query form, but
nobody noticed because 1st commit had trimmed prefixes from
control-file output, so the click-copy-pasting of fullpaths into new
queries had ceased; that query form became unused.

Removing the function is cleanest, but it could be useful in
old-compiler corner cases, where __FILE__ still has /full/path,
and it safely does nothing otherwize.

So instead, quietly deprecate "file /full/path" query form, by
removing all /full/paths examples in the docs.  I skipped adding a
back-compat note.

Signed-off-by: Jim Cromie <jim.cromie@gmail.com>
---
 .../admin-guide/dynamic-debug-howto.rst       | 19 +++++++++----------
 1 file changed, 9 insertions(+), 10 deletions(-)

diff --git a/Documentation/admin-guide/dynamic-debug-howto.rst b/Documentation/admin-guide/dynamic-debug-howto.rst
index 1012bd9305e9..57108f64afc8 100644
--- a/Documentation/admin-guide/dynamic-debug-howto.rst
+++ b/Documentation/admin-guide/dynamic-debug-howto.rst
@@ -70,10 +70,10 @@ statements via::
 
   nullarbor:~ # cat <debugfs>/dynamic_debug/control
   # filename:lineno [module]function flags format
-  /usr/src/packages/BUILD/sgi-enhancednfs-1.4/default/net/sunrpc/svc_rdma.c:323 [svcxprt_rdma]svc_rdma_cleanup =_ "SVCRDMA Module Removed, deregister RPC RDMA transport\012"
-  /usr/src/packages/BUILD/sgi-enhancednfs-1.4/default/net/sunrpc/svc_rdma.c:341 [svcxprt_rdma]svc_rdma_init =_ "\011max_inline       : %d\012"
-  /usr/src/packages/BUILD/sgi-enhancednfs-1.4/default/net/sunrpc/svc_rdma.c:340 [svcxprt_rdma]svc_rdma_init =_ "\011sq_depth         : %d\012"
-  /usr/src/packages/BUILD/sgi-enhancednfs-1.4/default/net/sunrpc/svc_rdma.c:338 [svcxprt_rdma]svc_rdma_init =_ "\011max_requests     : %d\012"
+  net/sunrpc/svc_rdma.c:323 [svcxprt_rdma]svc_rdma_cleanup =_ "SVCRDMA Module Removed, deregister RPC RDMA transport\012"
+  net/sunrpc/svc_rdma.c:341 [svcxprt_rdma]svc_rdma_init =_ "\011max_inline       : %d\012"
+  net/sunrpc/svc_rdma.c:340 [svcxprt_rdma]svc_rdma_init =_ "\011sq_depth         : %d\012"
+  net/sunrpc/svc_rdma.c:338 [svcxprt_rdma]svc_rdma_init =_ "\011max_requests     : %d\012"
   ...
 
 
@@ -93,7 +93,7 @@ the debug statement callsites with any non-default flags::
 
   nullarbor:~ # awk '$3 != "=_"' <debugfs>/dynamic_debug/control
   # filename:lineno [module]function flags format
-  /usr/src/packages/BUILD/sgi-enhancednfs-1.4/default/net/sunrpc/svcsock.c:1603 [sunrpc]svc_send p "svc_process: st_sendto returned %d\012"
+  net/sunrpc/svcsock.c:1603 [sunrpc]svc_send p "svc_process: st_sendto returned %d\012"
 
 Command Language Reference
 ==========================
@@ -166,13 +166,12 @@ func
 	func svc_tcp_accept
 
 file
-    The given string is compared against either the full pathname, the
-    src-root relative pathname, or the basename of the source file of
-    each callsite.  Examples::
+    The given string is compared against either the src-root relative
+    pathname, or the basename of the source file of each callsite.
+    Examples::
 
 	file svcsock.c
-	file kernel/freezer.c
-	file /usr/src/packages/BUILD/sgi-enhancednfs-1.4/default/net/sunrpc/svcsock.c
+	file kernel/freezer.c	# ie column 1 of control file
 
 module
     The given string is compared against the module name
-- 
2.26.2

[PATCH v3 02/21] dyndbg-docs: initialization is done early, not arch

[Jim Cromie]

since cf964976484 in 2012, initialization is done with early_initcall,
update the Docs, which still say arch_initcall.

Signed-off-by: Jim Cromie <jim.cromie@gmail.com>
---
 Documentation/admin-guide/dynamic-debug-howto.rst | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/Documentation/admin-guide/dynamic-debug-howto.rst b/Documentation/admin-guide/dynamic-debug-howto.rst
index 57108f64afc8..1423af580bed 100644
--- a/Documentation/admin-guide/dynamic-debug-howto.rst
+++ b/Documentation/admin-guide/dynamic-debug-howto.rst
@@ -250,8 +250,8 @@ the syntax described above, but must not exceed 1023 characters.  Your
 bootloader may impose lower limits.
 
 These ``dyndbg`` params are processed just after the ddebug tables are
-processed, as part of the arch_initcall.  Thus you can enable debug
-messages in all code run after this arch_initcall via this boot
+processed, as part of the early_initcall.  Thus you can enable debug
+messages in all code run after this early_initcall via this boot
 parameter.
 
 On an x86 system for example ACPI enablement is a subsys_initcall and::
-- 
2.26.2

[PATCH v3 13/21] dyndbg: accept 'file foo.c:func1' and 'file foo.c:10-100'

[Jim Cromie]

Accept these additional query forms:

   echo "file $filestr +_" > control

       path/to/file.c:100	# as from control, column 1
       path/to/file.c:1-100	# or any legal line-range
       path/to/file.c:func_A	# as from an editor/browser
       path/to/file.c:drm_\*	# wildcards still work
       path/to/file.c:*_foo	# lead wildcard too

1st 2 examples are treated as line-ranges, 3,4 are treated as func's

Doc these changes, and sprinkle in a few extra wild-card examples and
trailing # explanation texts.

Signed-off-by: Jim Cromie <jim.cromie@gmail.com>
---
 .../admin-guide/dynamic-debug-howto.rst       |  5 +++++
 lib/dynamic_debug.c                           | 20 ++++++++++++++++++-
 2 files changed, 24 insertions(+), 1 deletion(-)

diff --git a/Documentation/admin-guide/dynamic-debug-howto.rst b/Documentation/admin-guide/dynamic-debug-howto.rst
index 1423af580bed..6c04aea8f4cd 100644
--- a/Documentation/admin-guide/dynamic-debug-howto.rst
+++ b/Documentation/admin-guide/dynamic-debug-howto.rst
@@ -164,6 +164,7 @@ func
     of each callsite.  Example::
 
 	func svc_tcp_accept
+	func *recv*		# in rfcomm, bluetooth, ping, tcp
 
 file
     The given string is compared against either the src-root relative
@@ -172,6 +173,9 @@ file
 
 	file svcsock.c
 	file kernel/freezer.c	# ie column 1 of control file
+	file drivers/usb/*	# all callsites under it
+	file inode.c:start_*	# parse :tail as a func (above)
+	file inode.c:1-100	# parse :tail as a line-range (above)
 
 module
     The given string is compared against the module name
@@ -181,6 +185,7 @@ module
 
 	module sunrpc
 	module nfsd
+	module drm*	# both drm, drm_kms_helper
 
 format
     The given string is searched for in the dynamic debug format
diff --git a/lib/dynamic_debug.c b/lib/dynamic_debug.c
index ae6e523fdecd..7eb963b1bd11 100644
--- a/lib/dynamic_debug.c
+++ b/lib/dynamic_debug.c
@@ -321,6 +321,8 @@ static int parse_linerange(struct ddebug_query *query, const char *first)
 	} else {
 		query->last_lineno = query->first_lineno;
 	}
+	vpr_info("parsed line %d-%d\n", query->first_lineno,
+		 query->last_lineno);
 	return 0;
 }
 
@@ -357,6 +359,7 @@ static int ddebug_parse_query(char *words[], int nwords,
 {
 	unsigned int i;
 	int rc = 0;
+	char *fline;
 
 	/* check we have an even number of words */
 	if (nwords % 2 != 0) {
@@ -372,7 +375,22 @@ static int ddebug_parse_query(char *words[], int nwords,
 		if (!strcmp(words[i], "func")) {
 			rc = check_set(&query->function, words[i+1], "func");
 		} else if (!strcmp(words[i], "file")) {
-			rc = check_set(&query->filename, words[i+1], "file");
+			if (check_set(&query->filename, words[i+1], "file"))
+				return -EINVAL;
+
+			/* tail :$info is function or line-range */
+			fline = strchr(query->filename, ':');
+			if (!fline)
+				break;
+			*fline++ = '\0';
+			if (isalpha(*fline) || *fline == '*' || *fline == '?') {
+				/* take as function name */
+				if (check_set(&query->function, fline, "func"))
+					return -EINVAL;
+			} else {
+				if (parse_linerange(query, fline))
+					return -EINVAL;
+			}
 		} else if (!strcmp(words[i], "module")) {
 			rc = check_set(&query->module, words[i+1], "module");
 		} else if (!strcmp(words[i], "format")) {
-- 
2.26.2

[PATCH v3 14/21] dyndbg: accept query terms like file=bar and module=foo

[Jim Cromie]

Current code expects "keyword" "arg" as 2 space separated words.
Change to also accept "keyword=arg" form as well, and drop !(nwords%2)
requirement.

Then in rest of function, use new keyword, arg variables instead of
word[i], word[i+1]

Signed-off-by: Jim Cromie <jim.cromie@gmail.com>
---
 .../admin-guide/dynamic-debug-howto.rst       |  1 +
 lib/dynamic_debug.c                           | 51 ++++++++++++-------
 2 files changed, 33 insertions(+), 19 deletions(-)

diff --git a/Documentation/admin-guide/dynamic-debug-howto.rst b/Documentation/admin-guide/dynamic-debug-howto.rst
index 6c04aea8f4cd..e5a8def45f3f 100644
--- a/Documentation/admin-guide/dynamic-debug-howto.rst
+++ b/Documentation/admin-guide/dynamic-debug-howto.rst
@@ -156,6 +156,7 @@ against.  Possible keywords are:::
   ``line-range`` cannot contain space, e.g.
   "1-30" is valid range but "1 - 30" is not.
 
+  ``module=foo`` combined keyword=value form is interchangably accepted
 
 The meanings of each keyword are:
 
diff --git a/lib/dynamic_debug.c b/lib/dynamic_debug.c
index 7eb963b1bd11..e1dd96178f18 100644
--- a/lib/dynamic_debug.c
+++ b/lib/dynamic_debug.c
@@ -351,6 +351,8 @@ static int check_set(const char **dest, char *src, char *name)
  * line <lineno>
  * line <first-lineno>-<last-lineno> // where either may be empty
  *
+ * Also accept combined keyword=value and keyword:value forms
+ *
  * Only 1 of each type is allowed.
  * Returns 0 on success, <0 on error.
  */
@@ -360,22 +362,33 @@ static int ddebug_parse_query(char *words[], int nwords,
 	unsigned int i;
 	int rc = 0;
 	char *fline;
-
-	/* check we have an even number of words */
-	if (nwords % 2 != 0) {
-		pr_err("expecting pairs of match-spec <value>\n");
-		return -EINVAL;
-	}
+	char *keyword, *arg;
 
 	if (modname)
 		/* support $modname.dyndbg=<multiple queries> */
 		query->module = modname;
 
-	for (i = 0; i < nwords; i += 2) {
-		if (!strcmp(words[i], "func")) {
-			rc = check_set(&query->function, words[i+1], "func");
-		} else if (!strcmp(words[i], "file")) {
-			if (check_set(&query->filename, words[i+1], "file"))
+	for (i = 0; i < nwords; i++) {
+		/* accept keyword=arg */
+		vpr_info("%d w:%s\n", i, words[i]);
+
+		keyword = words[i];
+		if ((arg = strchr(keyword, '='))) {
+			*arg++ = '\0';
+		} else {
+			i++; /* next word is arg */
+			if (!(i < nwords)) {
+				pr_err("missing arg to keyword:%s\n", keyword);
+				return -EINVAL;
+			}
+			arg = words[i];
+		}
+		vpr_info("%d key:%s arg:%s\n", i, keyword, arg);
+
+		if (!strcmp(keyword, "func")) {
+			rc = check_set(&query->function, arg, "func");
+		} else if (!strcmp(keyword, "file")) {
+			if (check_set(&query->filename, arg, "file"))
 				return -EINVAL;
 
 			/* tail :$info is function or line-range */
@@ -391,18 +404,18 @@ static int ddebug_parse_query(char *words[], int nwords,
 				if (parse_linerange(query, fline))
 					return -EINVAL;
 			}
-		} else if (!strcmp(words[i], "module")) {
-			rc = check_set(&query->module, words[i+1], "module");
-		} else if (!strcmp(words[i], "format")) {
-			string_unescape_inplace(words[i+1], UNESCAPE_SPACE |
+		} else if (!strcmp(keyword, "module")) {
+			rc = check_set(&query->module, arg, "module");
+		} else if (!strcmp(keyword, "format")) {
+			string_unescape_inplace(arg, UNESCAPE_SPACE |
 							    UNESCAPE_OCTAL |
 							    UNESCAPE_SPECIAL);
-			rc = check_set(&query->format, words[i+1], "format");
-		} else if (!strcmp(words[i], "line")) {
-			if (parse_linerange(query, words[i+1]))
+			rc = check_set(&query->format, arg, "format");
+		} else if (!strcmp(keyword, "line")) {
+			if (parse_linerange(query, arg))
 				return -EINVAL;
 		} else {
-			pr_err("unknown keyword \"%s\"\n", words[i]);
+			pr_err("unknown keyword \"%s\"\n", keyword);
 			return -EINVAL;
 		}
 		if (rc)
-- 
2.26.2

Re: [PATCH v3 14/21] dyndbg: accept query terms like file=bar and module=foo

[jim.cromie]

oops.  got 3 copies of 14/21, this is the good one.  with module=foo
AND file=bar

On Wed, Jun 17, 2020 at 10:26 AM Jim Cromie <jim.cromie@gmail.com> wrote:
>
> Current code expects "keyword" "arg" as 2 space separated words.
> Change to also accept "keyword=arg" form as well, and drop !(nwords%2)
> requirement.
>
> Then in rest of function, use new keyword, arg variables instead of
> word[i], word[i+1]
>
> Signed-off-by: Jim Cromie <jim.cromie@gmail.com>
> ---
>  .../admin-guide/dynamic-debug-howto.rst       |  1 +
>  lib/dynamic_debug.c                           | 51 ++++++++++++-------
>  2 files changed, 33 insertions(+), 19 deletions(-)
>
> diff --git a/Documentation/admin-guide/dynamic-debug-howto.rst b/Documentation/admin-guide/dynamic-debug-howto.rst
> index 6c04aea8f4cd..e5a8def45f3f 100644
> --- a/Documentation/admin-guide/dynamic-debug-howto.rst
> +++ b/Documentation/admin-guide/dynamic-debug-howto.rst
> @@ -156,6 +156,7 @@ against.  Possible keywords are:::
>    ``line-range`` cannot contain space, e.g.
>    "1-30" is valid range but "1 - 30" is not.
>
> +  ``module=foo`` combined keyword=value form is interchangably accepted
>
>  The meanings of each keyword are:
>
> diff --git a/lib/dynamic_debug.c b/lib/dynamic_debug.c
> index 7eb963b1bd11..e1dd96178f18 100644
> --- a/lib/dynamic_debug.c
> +++ b/lib/dynamic_debug.c
> @@ -351,6 +351,8 @@ static int check_set(const char **dest, char *src, char *name)
>   * line <lineno>
>   * line <first-lineno>-<last-lineno> // where either may be empty
>   *
> + * Also accept combined keyword=value and keyword:value forms
> + *
>   * Only 1 of each type is allowed.
>   * Returns 0 on success, <0 on error.
>   */
> @@ -360,22 +362,33 @@ static int ddebug_parse_query(char *words[], int nwords,
>         unsigned int i;
>         int rc = 0;
>         char *fline;
> -
> -       /* check we have an even number of words */
> -       if (nwords % 2 != 0) {
> -               pr_err("expecting pairs of match-spec <value>\n");
> -               return -EINVAL;
> -       }
> +       char *keyword, *arg;
>
>         if (modname)
>                 /* support $modname.dyndbg=<multiple queries> */
>                 query->module = modname;
>
> -       for (i = 0; i < nwords; i += 2) {
> -               if (!strcmp(words[i], "func")) {
> -                       rc = check_set(&query->function, words[i+1], "func");
> -               } else if (!strcmp(words[i], "file")) {
> -                       if (check_set(&query->filename, words[i+1], "file"))
> +       for (i = 0; i < nwords; i++) {
> +               /* accept keyword=arg */
> +               vpr_info("%d w:%s\n", i, words[i]);
> +
> +               keyword = words[i];
> +               if ((arg = strchr(keyword, '='))) {
> +                       *arg++ = '\0';
> +               } else {
> +                       i++; /* next word is arg */
> +                       if (!(i < nwords)) {
> +                               pr_err("missing arg to keyword:%s\n", keyword);
> +                               return -EINVAL;
> +                       }
> +                       arg = words[i];
> +               }
> +               vpr_info("%d key:%s arg:%s\n", i, keyword, arg);
> +
> +               if (!strcmp(keyword, "func")) {
> +                       rc = check_set(&query->function, arg, "func");
> +               } else if (!strcmp(keyword, "file")) {
> +                       if (check_set(&query->filename, arg, "file"))
>                                 return -EINVAL;
>
>                         /* tail :$info is function or line-range */
> @@ -391,18 +404,18 @@ static int ddebug_parse_query(char *words[], int nwords,
>                                 if (parse_linerange(query, fline))
>                                         return -EINVAL;
>                         }
> -               } else if (!strcmp(words[i], "module")) {
> -                       rc = check_set(&query->module, words[i+1], "module");
> -               } else if (!strcmp(words[i], "format")) {
> -                       string_unescape_inplace(words[i+1], UNESCAPE_SPACE |
> +               } else if (!strcmp(keyword, "module")) {
> +                       rc = check_set(&query->module, arg, "module");
> +               } else if (!strcmp(keyword, "format")) {
> +                       string_unescape_inplace(arg, UNESCAPE_SPACE |
>                                                             UNESCAPE_OCTAL |
>                                                             UNESCAPE_SPECIAL);
> -                       rc = check_set(&query->format, words[i+1], "format");
> -               } else if (!strcmp(words[i], "line")) {
> -                       if (parse_linerange(query, words[i+1]))
> +                       rc = check_set(&query->format, arg, "format");
> +               } else if (!strcmp(keyword, "line")) {
> +                       if (parse_linerange(query, arg))
>                                 return -EINVAL;
>                 } else {
> -                       pr_err("unknown keyword \"%s\"\n", words[i]);
> +                       pr_err("unknown keyword \"%s\"\n", keyword);
>                         return -EINVAL;
>                 }
>                 if (rc)
> --
> 2.26.2
>

[PATCH v3 19/21] dyndbg: extend ddebug_parse_flags to accept optional leading filter-flags

[Jim Cromie]

Change ddebug_parse_flags to accept optional filterflags before the
required operator [-+=].  Read the flags into the filter_flags
parameter added in the previous patch.  So this now supplies the
filterflags to ddebug_exec_query.

filterflags work like query terms, they constrain what callsites get
matched before theyre modified.  So like a query, they can be empty.

Filterflags let you read callsite's flagstate, including results of
previous modifications, and require that certain flags are set, before
modifying the callsite further.

So you can build up sets of callsites by marking them with a
particular flagstate, for example 'fmlt', then enable that set in a
batch.

  echo fmlt+p >control

Naturally you can use almost any combo of flags you want for marking,
and can mark several different sets with different patterns.  And then
you can activate them in a bunch:

  echo 'ft+p; mt+p; lt+p;' >control

You just can't prohibit true flags.

Signed-off-by: Jim Cromie <jim.cromie@gmail.com>
---
 .../admin-guide/dynamic-debug-howto.rst       | 18 ++++++++----
 lib/dynamic_debug.c                           | 29 ++++++++++---------
 2 files changed, 28 insertions(+), 19 deletions(-)

diff --git a/Documentation/admin-guide/dynamic-debug-howto.rst b/Documentation/admin-guide/dynamic-debug-howto.rst
index e5a8def45f3f..e1ea0c307fcf 100644
--- a/Documentation/admin-guide/dynamic-debug-howto.rst
+++ b/Documentation/admin-guide/dynamic-debug-howto.rst
@@ -218,13 +218,19 @@ line
 	line -1605          // the 1605 lines from line 1 to line 1605
 	line 1600-          // all lines from line 1600 to the end of the file
 
-The flags specification comprises a change operation followed
-by one or more flag characters.  The change operation is one
-of the characters::
+Flags Specification::
 
-  -    remove the given flags
-  +    add the given flags
-  =    set the flags to the given flags
+  flagspec	::= filterflags? OP modflags
+  filterflags	::= flagset
+  modflags	::= flagset
+  flagset	::= ([pfmltu_] | [PFMLTU_])+	# also cant have pP etc
+  OP		::= [-+=]
+
+OP: modify callsites per following flagset::
+
+  -    remove the following flags
+  +    add the following flags
+  =    set the flags to the following flags
 
 The flags are::
 
diff --git a/lib/dynamic_debug.c b/lib/dynamic_debug.c
index 0fcc688789f4..cf3379b40483 100644
--- a/lib/dynamic_debug.c
+++ b/lib/dynamic_debug.c
@@ -452,33 +452,36 @@ static int ddebug_read_flags(const char *str, struct flag_settings *modifiers)
 }
 
 /*
- * Parse `str' as a flags specification, format [-+=][p]+.
- * Sets up *maskp and *flagsp to be used when changing the
- * flags fields of matched _ddebug's.  Returns 0 on success
- * or <0 on error.
+ * Parse `str' as a flags-spec, ie: [pfmlt_]*[-+=][pfmlt_]+
+ * Fills flagsettings provided.  Returns 0 on success or <0 on error.
  */
 static int ddebug_parse_flags(const char *str,
 			      struct flag_settings *modifiers,
 			      struct flag_settings *filter)
 {
 	int op;
+	char *opp = strpbrk(str, "-+=");
 
-	switch (*str) {
-	case '+':
-	case '-':
-	case '=':
-		op = *str++;
-		break;
-	default:
-		pr_err("bad flag-op %c, at start of %s\n", *str, str);
+	if (!opp) {
+		pr_err("no OP given in %s\n", str);
 		return -EINVAL;
 	}
+	op = *opp;
 	vpr_info("op='%c'\n", op);
 
+	if (opp != str) {
+		/* filterflags precedes OP, grab it */
+		*opp++ = '\0';
+		if (ddebug_read_flags(str, filter))
+			return -EINVAL;
+		str = opp;
+	} else
+		str++;
+
 	if (ddebug_read_flags(str, modifiers))
 		return -EINVAL;
 
-	/* calculate final flags, mask based upon op */
+	/* calculate final mods: flags, mask based upon op */
 	switch (op) {
 	case '=':
 		/* modifiers->flags already set */
-- 
2.26.2

Re: [PATCH v3 19/21] dyndbg: extend ddebug_parse_flags to accept optional leading filter-flags

[Petr Mladek]

On Wed 2020-06-17 10:25:34, Jim Cromie wrote:
> Change ddebug_parse_flags to accept optional filterflags before the
> required operator [-+=].  Read the flags into the filter_flags
> parameter added in the previous patch.  So this now supplies the
> filterflags to ddebug_exec_query.
> 
> filterflags work like query terms, they constrain what callsites get
> matched before theyre modified.  So like a query, they can be empty.
> 
> Filterflags let you read callsite's flagstate, including results of
> previous modifications, and require that certain flags are set, before
> modifying the callsite further.
> 
> So you can build up sets of callsites by marking them with a
> particular flagstate, for example 'fmlt', then enable that set in a
> batch.
> 
>   echo fmlt+p >control
> 
> Naturally you can use almost any combo of flags you want for marking,
> and can mark several different sets with different patterns.  And then
> you can activate them in a bunch:
> 
>   echo 'ft+p; mt+p; lt+p;' >control
> 
> + * Parse `str' as a flags-spec, ie: [pfmlt_]*[-+=][pfmlt_]+

This interface is simply _horrible_ and I do not see a point in this feature!!!

I as a normal dynamic debug user am interested into:

   + enabling/disabling messages from a given module/file/line/function
   + list of available modules/files/lines/functions
   + list of enabled modules/files/lines/functions

I do not understand why I would ever want to do something like:

   + enable messages that print module name and line number
   + disable message that does not print a module name

In fact, IMHO, all the 'flmt' flags were a wrong idea and nobody
really needed them. This information in not needed by other
printk() messages. Why pr_debug() would need them?
They just made the code and interface complicated.

Please, stop all this non-sense!!!

Best Regards,
Petr

Re: [PATCH v3 19/21] dyndbg: extend ddebug_parse_flags to accept optional leading filter-flags

[jim.cromie]

On Thu, Jun 18, 2020 at 6:44 AM Petr Mladek <pmladek@suse.com> wrote:
>
> On Wed 2020-06-17 10:25:34, Jim Cromie wrote:
> > Change ddebug_parse_flags to accept optional filterflags before the
> > required operator [-+=].  Read the flags into the filter_flags
> > parameter added in the previous patch.  So this now supplies the
> > filterflags to ddebug_exec_query.
> >
> > filterflags work like query terms, they constrain what callsites get
> > matched before theyre modified.  So like a query, they can be empty.
> >
> > Filterflags let you read callsite's flagstate, including results of
> > previous modifications, and require that certain flags are set, before
> > modifying the callsite further.
> >
> > So you can build up sets of callsites by marking them with a
> > particular flagstate, for example 'fmlt', then enable that set in a
> > batch.
> >
> >   echo fmlt+p >control
> >
> > Naturally you can use almost any combo of flags you want for marking,
> > and can mark several different sets with different patterns.  And then
> > you can activate them in a bunch:
> >
> >   echo 'ft+p; mt+p; lt+p;' >control
> >
> > + * Parse `str' as a flags-spec, ie: [pfmlt_]*[-+=][pfmlt_]+
>
> This interface is simply _horrible_ and I do not see a point in this feature!!!
>
> I as a normal dynamic debug user am interested into:
>
>    + enabling/disabling messages from a given module/file/line/function
>    + list of available modules/files/lines/functions
>    + list of enabled modules/files/lines/functions
>
> I do not understand why I would ever want to do something like:
>
>    + enable messages that print module name and line number
>    + disable message that does not print a module name

messages dont print them, the flags do, according to USER CHOICE.
a developer who is deeply familiar with the code doesnt need to
see most of it in the logs, average user might need them to comprehend things.

>
> In fact, IMHO, all the 'flmt' flags were a wrong idea and nobody
> really needed them. This information in not needed by other
> printk() messages. Why pr_debug() would need them?
> They just made the code and interface complicated.
>

it looks like they landed fully formed in lib/dynamic_debug.c
probably because that was a unification of several different print
debug systems.

you are free to set them globally:
echo +fmlt >control

or just the ones youre using
echo up+fmlt >control

> Please, stop all this non-sense!!!
>
> Best Regards,
> Petr

Re: [PATCH v3 19/21] dyndbg: extend ddebug_parse_flags to accept optional leading filter-flags

[Petr Mladek]

On Thu 2020-06-18 08:54:58, jim.cromie@gmail.com wrote:
> On Thu, Jun 18, 2020 at 6:44 AM Petr Mladek <pmladek@suse.com> wrote:
> >
> > On Wed 2020-06-17 10:25:34, Jim Cromie wrote:
> > > Change ddebug_parse_flags to accept optional filterflags before the
> > > required operator [-+=].  Read the flags into the filter_flags
> > > parameter added in the previous patch.  So this now supplies the
> > > filterflags to ddebug_exec_query.
> > >
> > > filterflags work like query terms, they constrain what callsites get
> > > matched before theyre modified.  So like a query, they can be empty.
> > >
> > > Filterflags let you read callsite's flagstate, including results of
> > > previous modifications, and require that certain flags are set, before
> > > modifying the callsite further.
> > >
> > > So you can build up sets of callsites by marking them with a
> > > particular flagstate, for example 'fmlt', then enable that set in a
> > > batch.
> > >
> > >   echo fmlt+p >control
> > >
> > > Naturally you can use almost any combo of flags you want for marking,
> > > and can mark several different sets with different patterns.  And then
> > > you can activate them in a bunch:
> > >
> > >   echo 'ft+p; mt+p; lt+p;' >control
> > >
> > > + * Parse `str' as a flags-spec, ie: [pfmlt_]*[-+=][pfmlt_]+
> >
> > This interface is simply _horrible_ and I do not see a point in this feature!!!
> >
> > I as a normal dynamic debug user am interested into:
> >
> >    + enabling/disabling messages from a given module/file/line/function
> >    + list of available modules/files/lines/functions
> >    + list of enabled modules/files/lines/functions
> >
> > I do not understand why I would ever want to do something like:
> >
> >    + enable messages that print module name and line number
> >    + disable message that does not print a module name
> 
> messages dont print them, the flags do, according to USER CHOICE.
> a developer who is deeply familiar with the code doesnt need to
> see most of it in the logs, average user might need them to comprehend things.

Any user, even average, has to deal also with non-debug messages that
do not include this extra information. Why pr_debug() message would
need it?

Message should be useful on its own. The location can be found by
grepping like for any other printk() messages.

Yes, the information might be handy. But all these configuration
choices also make the interface and code complicated. IMHO, it has
been over engineered. And this patch makes it even worse.


Anyway, you answered why the flags are there. But you did not explain
why anyone would need to use a filter based on them. Answer this,
please!!!


> > In fact, IMHO, all the 'flmt' flags were a wrong idea and nobody
> > really needed them. This information in not needed by other
> > printk() messages. Why pr_debug() would need them?
> > They just made the code and interface complicated.
> >
> 
> it looks like they landed fully formed in lib/dynamic_debug.c
> probably because that was a unification of several different print
> debug systems.

No, they were added by the commit 8ba6ebf583f12da32036fc0 ("Dynamic debug:
Add more flags").

There is no explanation why they were added. It probably just looked
like a good idea to the author and nobody complained at that time.

It has been included wihtout any comment, see
https://lore.kernel.org/lkml/201101231717.24175.bvanassche@acm.org/
https://lore.kernel.org/lkml/1300309888-5028-5-git-send-email-gregkh@suse.de/


> you are free to set them globally:
> echo +fmlt >control
> 
> or just the ones youre using
> echo up+fmlt >control

The question is not if I could do so. The question is how many users
do it or need to do so.

Features in this patchset affect the interface with userspace. It
means that they would need to be maintained "forewer". For this,
you need to prove that it is widely useful. Ideally, it should
be outcome of some discussion where people missed this.

I do not see any reasonable usecase for anything like:

   echo 'ft+p; mt+p; lt+p;' >control

Why people would do this, please?

Best Regards,
Petr

[PATCH v3 20/21] dyndbg: add user-flag, negating-flags, and filtering on flags

[Jim Cromie]

1. Add a user-flag [u] which works like the [pfmlt] flags, but has no
effect on callsite behavior; it allows incremental marking of
arbitrary sets of callsites.

2. Add [PFMLTU] flags, which negate their counterparts; P===!p etc.
And in ddebug_read_flags():
   current code does:	[pfmltu_] -> flags
   copy it to:		[PFMLTU_] -> mask

also disallow both of a pair: ie no 'pP', no true & false.

3. Add filtering ops into ddebug_change(), right after all the
callsite-property selections are complete.  These filter on the
callsite's current flagstate before applying modflags.

Why ?

The u-flag & filter flags

The 'u' flag lets the user assemble an arbitary set of callsites.
Then using filter flags, user can activate the 'u' callsite set.

  #> echo 'file foo.c +u; file bar.c +u' > control   # and repeat
  #> echo 'u+p' > control

Of course, you can continue to just activate your set without ever
marking it 1st, but you could trivially add the markup as you go, then
be able to use it as a constraint later, to undo or modify your set.

  #> echo 'file foo.c +up' >control
  .. monitor, debug, finish ..
  #> echo 'u-p' >control

  # then later resume
  #> echo 'u+p' >control

  # disable some cluttering messages, and remove from u-set
  #> echo 'file noisy.c function:jabber_* u-pu' >control

  # for doc, recollection
  grep =pu control > my-favorite-callsites

Note:

Your flagstate after boot is generally not all =_. -DDEBUG will arm
compiled callsites by default, $builtinmod.dyndbg=+p bootargs can
enable them early, and $module.dyndbg=+p bootargs will arm them when
the module is loaded.  But you could manage them with u-flags:

  #> echo '-t' >control		# clear t-flag to use it as 2ndary markup
  #> echo 'p+ut' >control	# mark the boot-enabled set of callsites
  #> echo '-p' >control		# clean your dmesg -w stream

  ... monitor, debug ..
  #> echo 'module of_interest $qterms +pu' >control	# build your set of useful debugs
  #> echo 'module of_interest $qterms UT+pu' >control	# same, but dont alter ut marked set

The user flag isn't strictly needed, but with it you can avoid using
[fmlt] flags for marking, which would alter logging when enabled.

Negating-flags:

Using negating-flags in your filter-flags, you can completely specify
the matching flagstate; not just required flags, but also prohibited
flags.

So if you want to avoid altering your u-set, prohibit the flag with U,
and all marked callsites will be skipped, for whatever change.

    #> echo U-pt >control

At the outer limits, you could use many flag patterns to form separate
subgroups of callsites, then enable those groups by filtering on just
their flagstates, or you could add further constraints on callsite
selection.

Signed-off-by: Jim Cromie <jim.cromie@gmail.com>
---
 .../admin-guide/dynamic-debug-howto.rst       | 31 ++++++++++++++++---
 include/linux/dynamic_debug.h                 |  1 +
 lib/dynamic_debug.c                           | 29 ++++++++++++-----
 3 files changed, 50 insertions(+), 11 deletions(-)

diff --git a/Documentation/admin-guide/dynamic-debug-howto.rst b/Documentation/admin-guide/dynamic-debug-howto.rst
index e1ea0c307fcf..e910865b2edc 100644
--- a/Documentation/admin-guide/dynamic-debug-howto.rst
+++ b/Documentation/admin-guide/dynamic-debug-howto.rst
@@ -239,16 +239,39 @@ The flags are::
   l    Include line number in the printed message
   m    Include module name in the printed message
   t    Include thread ID in messages not generated from interrupt context
+  u    user flag, to mark callsites into a group
   _    No flags are set. (Or'd with others on input)
 
-For ``print_hex_dump_debug()`` and ``print_hex_dump_bytes()``, only ``p`` flag
-have meaning, other flags ignored.
+Additionally, the flag-chars ``[pflmtu]`` have negating flag-chars
+``[PFMLTU]``, which invert the meanings above.  Their use follows.
+
+Using Filters::
+
+Filter-flags specify an optional additional selector on pr_debug
+callsites; with them you can compose an arbitrary set of callsites, by
+iteratively marking them with ``+u``, then enabling them all with
+``u+p``.  You can also use ``fmlt`` flags for this, unless the format
+changes are inconvenient.
+
+Filters can also contain the negating flags, like ``UF``, which select
+only callsites with ``u`` and ``f`` cleared.
+
+Flagsets cannot contain ``pP`` etc, a flag cannot be true and false.
+
+modflags containing upper-case flags is reserved/undefined for now.
+inverted-flags are currently ignored, usage gets trickier if given
+``-pXy``, it should leave x set.
+
+Notes::
+
+For ``print_hex_dump_debug()`` and ``print_hex_dump_bytes()``, only
+``p`` flag has meaning, other flags are ignored.
 
 For display, the flags are preceded by ``=``
 (mnemonic: what the flags are currently equal to).
 
-Note the regexp ``^[-+=][flmpt_]+$`` matches a flags specification.
-To clear all flags at once, use ``=_`` or ``-flmpt``.
+Note the regexp ``^[-+=][flmptu_]+$`` matches a flags specification.
+To clear all flags at once, use ``=_`` or ``-flmptu``.
 
 
 Debug messages during Boot Process
diff --git a/include/linux/dynamic_debug.h b/include/linux/dynamic_debug.h
index aa9ff9e1c0b3..59960a8dd9f9 100644
--- a/include/linux/dynamic_debug.h
+++ b/include/linux/dynamic_debug.h
@@ -32,6 +32,7 @@ struct _ddebug {
 #define _DPRINTK_FLAGS_INCL_FUNCNAME	(1<<2)
 #define _DPRINTK_FLAGS_INCL_LINENO	(1<<3)
 #define _DPRINTK_FLAGS_INCL_TID		(1<<4)
+#define _DPRINTK_FLAGS_USR		(1<<5)
 #if defined DEBUG
 #define _DPRINTK_FLAGS_DEFAULT _DPRINTK_FLAGS_PRINT
 #else
diff --git a/lib/dynamic_debug.c b/lib/dynamic_debug.c
index cf3379b40483..a302a7d8a722 100644
--- a/lib/dynamic_debug.c
+++ b/lib/dynamic_debug.c
@@ -83,13 +83,14 @@ static inline const char *trim_prefix(const char *path)
 	return path + skip;
 }
 
-static struct { unsigned flag:8; char opt_char; } opt_array[] = {
-	{ _DPRINTK_FLAGS_PRINT, 'p' },
-	{ _DPRINTK_FLAGS_INCL_MODNAME, 'm' },
-	{ _DPRINTK_FLAGS_INCL_FUNCNAME, 'f' },
-	{ _DPRINTK_FLAGS_INCL_LINENO, 'l' },
-	{ _DPRINTK_FLAGS_INCL_TID, 't' },
-	{ _DPRINTK_FLAGS_NONE, '_' },
+static struct { unsigned flag:8; char opt_char, not_char; } opt_array[] = {
+	{ _DPRINTK_FLAGS_PRINT,		'p', 'P' },
+	{ _DPRINTK_FLAGS_INCL_MODNAME,	'm', 'M' },
+	{ _DPRINTK_FLAGS_INCL_FUNCNAME,	'f', 'F' },
+	{ _DPRINTK_FLAGS_INCL_LINENO,	'l', 'L' },
+	{ _DPRINTK_FLAGS_INCL_TID,	't', 'T' },
+	{ _DPRINTK_FLAGS_NONE,		'_', '_' },
+	{ _DPRINTK_FLAGS_USR,		'u', 'U' },
 };
 
 struct flagsbuf { char buf[ARRAY_SIZE(opt_array)+1]; };
@@ -194,6 +195,13 @@ static int ddebug_change(const struct ddebug_query *query,
 			    dp->lineno > query->last_lineno)
 				continue;
 
+			/* filter for required flags */
+			if ((dp->flags & filter->flags) != filter->flags)
+				continue;
+			/* filter on prohibited bits */
+			if ((~dp->flags & filter->mask) != filter->mask)
+				continue;
+
 			nfound++;
 
 			newflags = (dp->flags & modifiers->mask) | modifiers->flags;
@@ -440,12 +448,19 @@ static int ddebug_read_flags(const char *str, struct flag_settings *modifiers)
 			if (*str == opt_array[i].opt_char) {
 				modifiers->flags |= opt_array[i].flag;
 				break;
+			} else if (*str == opt_array[i].not_char) {
+				modifiers->mask |= opt_array[i].flag;
+				break;
 			}
 		}
 		if (i < 0) {
 			pr_err("unknown flag '%c'\n", *str);
 			return -EINVAL;
 		}
+		if (modifiers->flags & modifiers->mask) {
+			pr_err("flag '%c' conflicts with previous\n", *str);
+			return -EINVAL;
+		}
 	}
 	vpr_info("flags=0x%x\n", modifiers->flags);
 	return 0;
-- 
2.26.2

Re: [PATCH v3 20/21] dyndbg: add user-flag, negating-flags, and filtering on flags

[Joe Perches]

On Wed, 2020-06-17 at 10:25 -0600, Jim Cromie wrote:
> 1. Add a user-flag [u] which works like the [pfmlt] flags, but has no
> effect on callsite behavior; it allows incremental marking of
> arbitrary sets of callsites.
> 
> 2. Add [PFMLTU] flags, which negate their counterparts; P===!p etc.
> And in ddebug_read_flags():
>    current code does:	[pfmltu_] -> flags
>    copy it to:		[PFMLTU_] -> mask
> 
> also disallow both of a pair: ie no 'pP', no true & false.
> 
> 3. Add filtering ops into ddebug_change(), right after all the
> callsite-property selections are complete.  These filter on the
> callsite's current flagstate before applying modflags.
> 
> Why ?
> 
> The u-flag & filter flags
> 
> The 'u' flag lets the user assemble an arbitary set of callsites.
> Then using filter flags, user can activate the 'u' callsite set.
> 
>   #> echo 'file foo.c +u; file bar.c +u' > control   # and repeat
>   #> echo 'u+p' > control
> 
> Of course, you can continue to just activate your set without ever
> marking it 1st, but you could trivially add the markup as you go, then
> be able to use it as a constraint later, to undo or modify your set.

Does this set selection also allow for selection by
increasing decimal level?

Can sites be enabled for a value less than x?

Re: [PATCH v3 20/21] dyndbg: add user-flag, negating-flags, and filtering on flags

[jim.cromie]

On Wed, Jun 17, 2020 at 4:13 PM Joe Perches <joe@perches.com> wrote:
>
> On Wed, 2020-06-17 at 10:25 -0600, Jim Cromie wrote:
> > 1. Add a user-flag [u] which works like the [pfmlt] flags, but has no
> > effect on callsite behavior; it allows incremental marking of
> > arbitrary sets of callsites.
> >
> > 2. Add [PFMLTU] flags, which negate their counterparts; P===!p etc.
> > And in ddebug_read_flags():
> >    current code does: [pfmltu_] -> flags
> >    copy it to:                [PFMLTU_] -> mask
> >
> > also disallow both of a pair: ie no 'pP', no true & false.
> >
> > 3. Add filtering ops into ddebug_change(), right after all the
> > callsite-property selections are complete.  These filter on the
> > callsite's current flagstate before applying modflags.
> >
> > Why ?
> >
> > The u-flag & filter flags
> >
> > The 'u' flag lets the user assemble an arbitary set of callsites.
> > Then using filter flags, user can activate the 'u' callsite set.
> >
> >   #> echo 'file foo.c +u; file bar.c +u' > control   # and repeat
> >   #> echo 'u+p' > control
> >
> > Of course, you can continue to just activate your set without ever
> > marking it 1st, but you could trivially add the markup as you go, then
> > be able to use it as a constraint later, to undo or modify your set.
>
> Does this set selection also allow for selection by
> increasing decimal level?
>
> Can sites be enabled for a value less than x?
>
>

no, theres no levels added here.
that would be a variation on the WIP pr-class patch in v2, dropped from v3

legacy dyndbg - select on RO callsite info only - file, module, func, line.
    flag state is write only.
filterflags -   additionally select on callsite's flagstate, ie
reading the current flagstate

please look at the export patch 15/21
for how I now think levels, and drm.debug=0x03 can be done.

Re: [PATCH v3 20/21] dyndbg: add user-flag, negating-flags, and filtering on flags

[Petr Mladek]

On Wed 2020-06-17 10:25:35, Jim Cromie wrote:
> 1. Add a user-flag [u] which works like the [pfmlt] flags, but has no
> effect on callsite behavior; it allows incremental marking of
> arbitrary sets of callsites.
> 
> 2. Add [PFMLTU] flags, which negate their counterparts; P===!p etc.
> And in ddebug_read_flags():
>    current code does:	[pfmltu_] -> flags
>    copy it to:		[PFMLTU_] -> mask
> 
> also disallow both of a pair: ie no 'pP', no true & false.
> 
> 3. Add filtering ops into ddebug_change(), right after all the
> callsite-property selections are complete.  These filter on the
> callsite's current flagstate before applying modflags.
> 
> Why ?
> 
> The u-flag & filter flags
> 
> The 'u' flag lets the user assemble an arbitary set of callsites.
> Then using filter flags, user can activate the 'u' callsite set.
> 
>   #> echo 'file foo.c +u; file bar.c +u' > control   # and repeat
>   #> echo 'u+p' > control
> 
> Of course, you can continue to just activate your set without ever
> marking it 1st, but you could trivially add the markup as you go, then
> be able to use it as a constraint later, to undo or modify your set.
> 
>   #> echo 'file foo.c +up' >control
>   .. monitor, debug, finish ..
>   #> echo 'u-p' >control
> 
>   # then later resume
>   #> echo 'u+p' >control
> 
>   # disable some cluttering messages, and remove from u-set
>   #> echo 'file noisy.c function:jabber_* u-pu' >control
> 
>   # for doc, recollection
>   grep =pu control > my-favorite-callsites
> 
> Note:
> 
> Your flagstate after boot is generally not all =_. -DDEBUG will arm
> compiled callsites by default, $builtinmod.dyndbg=+p bootargs can
> enable them early, and $module.dyndbg=+p bootargs will arm them when
> the module is loaded.  But you could manage them with u-flags:
> 
>   #> echo '-t' >control		# clear t-flag to use it as 2ndary markup
>   #> echo 'p+ut' >control	# mark the boot-enabled set of callsites
>   #> echo '-p' >control		# clean your dmesg -w stream
> 
>   ... monitor, debug ..
>   #> echo 'module of_interest $qterms +pu' >control	# build your set of useful debugs
>   #> echo 'module of_interest $qterms UT+pu' >control	# same, but dont alter ut marked set

Does anyone requested this feature, please?

For me, it is really hard to imagine people using these complex and hacky
steps.

Not to say that using t-flag as a markup looks like a real hack.
People either always need the line number in the kernel log or
they do not need it at all.

Let me repeat. Please, stop this non-sense.

Best Regards,
Petr

Re: [PATCH v3 20/21] dyndbg: add user-flag, negating-flags, and filtering on flags

[Petr Mladek]

On Thu 2020-06-18 18:19:12, Petr Mladek wrote:
> On Wed 2020-06-17 10:25:35, Jim Cromie wrote:
> > 1. Add a user-flag [u] which works like the [pfmlt] flags, but has no
> > effect on callsite behavior; it allows incremental marking of
> > arbitrary sets of callsites.
> > 
> > 2. Add [PFMLTU] flags, which negate their counterparts; P===!p etc.
> > And in ddebug_read_flags():
> >    current code does:	[pfmltu_] -> flags
> >    copy it to:		[PFMLTU_] -> mask
> > 
> > also disallow both of a pair: ie no 'pP', no true & false.
> > 
> > 3. Add filtering ops into ddebug_change(), right after all the
> > callsite-property selections are complete.  These filter on the
> > callsite's current flagstate before applying modflags.
> > 
> > Why ?
> > 
> > The u-flag & filter flags
> > 
> > The 'u' flag lets the user assemble an arbitary set of callsites.
> > Then using filter flags, user can activate the 'u' callsite set.
> > 
> >   #> echo 'file foo.c +u; file bar.c +u' > control   # and repeat
> >   #> echo 'u+p' > control
> > 
> > Of course, you can continue to just activate your set without ever
> > marking it 1st, but you could trivially add the markup as you go, then
> > be able to use it as a constraint later, to undo or modify your set.
> > 
> >   #> echo 'file foo.c +up' >control
> >   .. monitor, debug, finish ..
> >   #> echo 'u-p' >control
> > 
> >   # then later resume
> >   #> echo 'u+p' >control
> > 
> >   # disable some cluttering messages, and remove from u-set
> >   #> echo 'file noisy.c function:jabber_* u-pu' >control
> > 
> >   # for doc, recollection
> >   grep =pu control > my-favorite-callsites
> > 
> > Note:
> > 
> > Your flagstate after boot is generally not all =_. -DDEBUG will arm
> > compiled callsites by default, $builtinmod.dyndbg=+p bootargs can
> > enable them early, and $module.dyndbg=+p bootargs will arm them when
> > the module is loaded.  But you could manage them with u-flags:
> > 
> >   #> echo '-t' >control		# clear t-flag to use it as 2ndary markup
> >   #> echo 'p+ut' >control	# mark the boot-enabled set of callsites
> >   #> echo '-p' >control		# clean your dmesg -w stream
> > 
> >   ... monitor, debug ..
> >   #> echo 'module of_interest $qterms +pu' >control	# build your set of useful debugs
> >   #> echo 'module of_interest $qterms UT+pu' >control	# same, but dont alter ut marked set
> 
> Does anyone requested this feature, please?
> 
> For me, it is really hard to imagine people using these complex and hacky
> steps.

I think that all this is motivated by adding support for module
specific groups.

What about storing the group as yet another information for each
message? I mean the same way as we store module name, file, line,
function name.

Then we could add API to define group for a given message:

   pr_debug_group(group_id, fmt, ...);

the interface for the control file might be via new keyword "group".
You could then do something like:

   echo module=drm group=0x3 +p >control

But more importantly you should add functions that might be called
when the drm.debug parameter is changes. I have already mentioned
it is another reply:

    dd_enable_module_group(module_name, group_id);
    dd_disable_module_group(module_name, group_id);


It will _not_ need any new flag or flag filtering.

Best Regards,
Petr

Re: [PATCH v3 20/21] dyndbg: add user-flag, negating-flags, and filtering on flags

[Jason Baron]

On 6/18/20 1:40 PM, Petr Mladek wrote:
> On Thu 2020-06-18 18:19:12, Petr Mladek wrote:
>> On Wed 2020-06-17 10:25:35, Jim Cromie wrote:
>>> 1. Add a user-flag [u] which works like the [pfmlt] flags, but has no
>>> effect on callsite behavior; it allows incremental marking of
>>> arbitrary sets of callsites.
>>>
>>> 2. Add [PFMLTU] flags, which negate their counterparts; P===!p etc.
>>> And in ddebug_read_flags():
>>>    current code does:	[pfmltu_] -> flags
>>>    copy it to:		[PFMLTU_] -> mask
>>>
>>> also disallow both of a pair: ie no 'pP', no true & false.
>>>
>>> 3. Add filtering ops into ddebug_change(), right after all the
>>> callsite-property selections are complete.  These filter on the
>>> callsite's current flagstate before applying modflags.
>>>
>>> Why ?
>>>
>>> The u-flag & filter flags
>>>
>>> The 'u' flag lets the user assemble an arbitary set of callsites.
>>> Then using filter flags, user can activate the 'u' callsite set.
>>>
>>>   #> echo 'file foo.c +u; file bar.c +u' > control   # and repeat
>>>   #> echo 'u+p' > control
>>>
>>> Of course, you can continue to just activate your set without ever
>>> marking it 1st, but you could trivially add the markup as you go, then
>>> be able to use it as a constraint later, to undo or modify your set.
>>>
>>>   #> echo 'file foo.c +up' >control
>>>   .. monitor, debug, finish ..
>>>   #> echo 'u-p' >control
>>>
>>>   # then later resume
>>>   #> echo 'u+p' >control
>>>
>>>   # disable some cluttering messages, and remove from u-set
>>>   #> echo 'file noisy.c function:jabber_* u-pu' >control
>>>
>>>   # for doc, recollection
>>>   grep =pu control > my-favorite-callsites
>>>
>>> Note:
>>>
>>> Your flagstate after boot is generally not all =_. -DDEBUG will arm
>>> compiled callsites by default, $builtinmod.dyndbg=+p bootargs can
>>> enable them early, and $module.dyndbg=+p bootargs will arm them when
>>> the module is loaded.  But you could manage them with u-flags:
>>>
>>>   #> echo '-t' >control		# clear t-flag to use it as 2ndary markup
>>>   #> echo 'p+ut' >control	# mark the boot-enabled set of callsites
>>>   #> echo '-p' >control		# clean your dmesg -w stream
>>>
>>>   ... monitor, debug ..
>>>   #> echo 'module of_interest $qterms +pu' >control	# build your set of useful debugs
>>>   #> echo 'module of_interest $qterms UT+pu' >control	# same, but dont alter ut marked set
>>
>> Does anyone requested this feature, please?
>>
>> For me, it is really hard to imagine people using these complex and hacky
>> steps.
> 
> I think that all this is motivated by adding support for module
> specific groups.
> 
> What about storing the group as yet another information for each
> message? I mean the same way as we store module name, file, line,
> function name.
> 
> Then we could add API to define group for a given message:
> 
>    pr_debug_group(group_id, fmt, ...);
> 
> the interface for the control file might be via new keyword "group".
> You could then do something like:
> 
>    echo module=drm group=0x3 +p >control
> 
> But more importantly you should add functions that might be called
> when the drm.debug parameter is changes. I have already mentioned
> it is another reply:
> 
>     dd_enable_module_group(module_name, group_id);
>     dd_disable_module_group(module_name, group_id);
> 
> 
> It will _not_ need any new flag or flag filtering.
> 
> Best Regards,
> Petr
> 

Yes, I'm wondering as well if people are really going to use the
new flags and filter flags - I mentioned that here:
https://lkml.org/lkml/2020/6/12/732

The grouping stuff is already being used by lots of modules so
that seems useful.

Thanks,

-Jason

Re: [PATCH v3 20/21] dyndbg: add user-flag, negating-flags, and filtering on flags

[jim.cromie]

On Thu, Jun 18, 2020 at 12:17 PM Jason Baron <jbaron@akamai.com> wrote:
>
>
>
> On 6/18/20 1:40 PM, Petr Mladek wrote:
> > On Thu 2020-06-18 18:19:12, Petr Mladek wrote:
> >> On Wed 2020-06-17 10:25:35, Jim Cromie wrote:
> >>> 1. Add a user-flag [u] which works like the [pfmlt] flags, but has no
> >>> effect on callsite behavior; it allows incremental marking of
> >>> arbitrary sets of callsites.
> >>>
> >>> 2. Add [PFMLTU] flags, which negate their counterparts; P===!p etc.
> >>> And in ddebug_read_flags():
> >>>    current code does:       [pfmltu_] -> flags
> >>>    copy it to:              [PFMLTU_] -> mask
> >>>
> >>> also disallow both of a pair: ie no 'pP', no true & false.
> >>>
> >>> 3. Add filtering ops into ddebug_change(), right after all the
> >>> callsite-property selections are complete.  These filter on the
> >>> callsite's current flagstate before applying modflags.
> >>>
> >>> Why ?
> >>>
> >>> The u-flag & filter flags
> >>>
> >>> The 'u' flag lets the user assemble an arbitary set of callsites.
> >>> Then using filter flags, user can activate the 'u' callsite set.
> >>>
> >>>   #> echo 'file foo.c +u; file bar.c +u' > control   # and repeat
> >>>   #> echo 'u+p' > control
> >>>
> >>> Of course, you can continue to just activate your set without ever
> >>> marking it 1st, but you could trivially add the markup as you go, then
> >>> be able to use it as a constraint later, to undo or modify your set.
> >>>
> >>>   #> echo 'file foo.c +up' >control
> >>>   .. monitor, debug, finish ..
> >>>   #> echo 'u-p' >control
> >>>
> >>>   # then later resume
> >>>   #> echo 'u+p' >control
> >>>
> >>>   # disable some cluttering messages, and remove from u-set
> >>>   #> echo 'file noisy.c function:jabber_* u-pu' >control
> >>>
> >>>   # for doc, recollection
> >>>   grep =pu control > my-favorite-callsites
> >>>
> >>> Note:
> >>>
> >>> Your flagstate after boot is generally not all =_. -DDEBUG will arm
> >>> compiled callsites by default, $builtinmod.dyndbg=+p bootargs can
> >>> enable them early, and $module.dyndbg=+p bootargs will arm them when
> >>> the module is loaded.  But you could manage them with u-flags:
> >>>
> >>>   #> echo '-t' >control             # clear t-flag to use it as 2ndary markup
> >>>   #> echo 'p+ut' >control   # mark the boot-enabled set of callsites
> >>>   #> echo '-p' >control             # clean your dmesg -w stream
> >>>
> >>>   ... monitor, debug ..
> >>>   #> echo 'module of_interest $qterms +pu' >control # build your set of useful debugs
> >>>   #> echo 'module of_interest $qterms UT+pu' >control       # same, but dont alter ut marked set
> >>
> >> Does anyone requested this feature, please?
> >>
> >> For me, it is really hard to imagine people using these complex and hacky
> >> steps.
> >
> > I think that all this is motivated by adding support for module
> > specific groups.
> >
> > What about storing the group as yet another information for each
> > message? I mean the same way as we store module name, file, line,
> > function name.
> >
> > Then we could add API to define group for a given message:
> >
> >    pr_debug_group(group_id, fmt, ...);
> >
> > the interface for the control file might be via new keyword "group".
> > You could then do something like:
> >
> >    echo module=drm group=0x3 +p >control
> >
> > But more importantly you should add functions that might be called
> > when the drm.debug parameter is changes. I have already mentioned
> > it is another reply:
> >
> >     dd_enable_module_group(module_name, group_id);
> >     dd_disable_module_group(module_name, group_id);
> >
> >
> > It will _not_ need any new flag or flag filtering.
> >
> > Best Regards,
> > Petr
> >
>
> Yes, I'm wondering as well if people are really going to use the
> new flags and filter flags - I mentioned that here:
> https://lkml.org/lkml/2020/6/12/732
>

yes, I saw, and replied there.
but since that was v1, and we're on v3, we should refresh.

the central use-case is above, 1-liner version summarized here:

1- enable sites as you chase a problem with +up
2- examine them with grep =pu
3- change the set to suit, either by adding or subtracting callsites.
4- continue debugging, and changing callsites to suit
5- grep =pu control > ~/debugging-session-task1-callsites
6- echo up-p >control   # disable for now, leave u-set for later
7- do other stuff
8 echo uP+p >control # reactivate useful debug-state and resume


> The grouping stuff is already being used by lots of modules so
> that seems useful.

I now dont see the need.

given N debug callsites, any group can be defined by <N queries,
probably a lot less
if module authors can use ddebug_exec_queries(), cuz its exported, (15/21)
then they can act (+p or -p) on those sets defined by <N queries.

and now any callsite can be in any number of groups, not just one.
It would be prudent to evaluate such groupings case by case,
because the intersecting callsites are subject to "last manipulator wins"
but its unnecessary to insist that all sets are disjoint.
Unlike pr_debug_n, however its spelled.

>
> Thanks,
>
> -Jason

Re: [PATCH v3 20/21] dyndbg: add user-flag, negating-flags, and filtering on flags

[Jason Baron]

On 6/18/20 3:11 PM, jim.cromie@gmail.com wrote:
> On Thu, Jun 18, 2020 at 12:17 PM Jason Baron <jbaron@akamai.com> wrote:
>>
>>
>>
>> On 6/18/20 1:40 PM, Petr Mladek wrote:
>>> On Thu 2020-06-18 18:19:12, Petr Mladek wrote:
>>>> On Wed 2020-06-17 10:25:35, Jim Cromie wrote:
>>>>> 1. Add a user-flag [u] which works like the [pfmlt] flags, but has no
>>>>> effect on callsite behavior; it allows incremental marking of
>>>>> arbitrary sets of callsites.
>>>>>
>>>>> 2. Add [PFMLTU] flags, which negate their counterparts; P===!p etc.
>>>>> And in ddebug_read_flags():
>>>>>    current code does:       [pfmltu_] -> flags
>>>>>    copy it to:              [PFMLTU_] -> mask
>>>>>
>>>>> also disallow both of a pair: ie no 'pP', no true & false.
>>>>>
>>>>> 3. Add filtering ops into ddebug_change(), right after all the
>>>>> callsite-property selections are complete.  These filter on the
>>>>> callsite's current flagstate before applying modflags.
>>>>>
>>>>> Why ?
>>>>>
>>>>> The u-flag & filter flags
>>>>>
>>>>> The 'u' flag lets the user assemble an arbitary set of callsites.
>>>>> Then using filter flags, user can activate the 'u' callsite set.
>>>>>
>>>>>   #> echo 'file foo.c +u; file bar.c +u' > control   # and repeat
>>>>>   #> echo 'u+p' > control
>>>>>
>>>>> Of course, you can continue to just activate your set without ever
>>>>> marking it 1st, but you could trivially add the markup as you go, then
>>>>> be able to use it as a constraint later, to undo or modify your set.
>>>>>
>>>>>   #> echo 'file foo.c +up' >control
>>>>>   .. monitor, debug, finish ..
>>>>>   #> echo 'u-p' >control
>>>>>
>>>>>   # then later resume
>>>>>   #> echo 'u+p' >control
>>>>>
>>>>>   # disable some cluttering messages, and remove from u-set
>>>>>   #> echo 'file noisy.c function:jabber_* u-pu' >control
>>>>>
>>>>>   # for doc, recollection
>>>>>   grep =pu control > my-favorite-callsites
>>>>>
>>>>> Note:
>>>>>
>>>>> Your flagstate after boot is generally not all =_. -DDEBUG will arm
>>>>> compiled callsites by default, $builtinmod.dyndbg=+p bootargs can
>>>>> enable them early, and $module.dyndbg=+p bootargs will arm them when
>>>>> the module is loaded.  But you could manage them with u-flags:
>>>>>
>>>>>   #> echo '-t' >control             # clear t-flag to use it as 2ndary markup
>>>>>   #> echo 'p+ut' >control   # mark the boot-enabled set of callsites
>>>>>   #> echo '-p' >control             # clean your dmesg -w stream
>>>>>
>>>>>   ... monitor, debug ..
>>>>>   #> echo 'module of_interest $qterms +pu' >control # build your set of useful debugs
>>>>>   #> echo 'module of_interest $qterms UT+pu' >control       # same, but dont alter ut marked set
>>>>
>>>> Does anyone requested this feature, please?
>>>>
>>>> For me, it is really hard to imagine people using these complex and hacky
>>>> steps.
>>>
>>> I think that all this is motivated by adding support for module
>>> specific groups.
>>>
>>> What about storing the group as yet another information for each
>>> message? I mean the same way as we store module name, file, line,
>>> function name.
>>>
>>> Then we could add API to define group for a given message:
>>>
>>>    pr_debug_group(group_id, fmt, ...);
>>>
>>> the interface for the control file might be via new keyword "group".
>>> You could then do something like:
>>>
>>>    echo module=drm group=0x3 +p >control
>>>
>>> But more importantly you should add functions that might be called
>>> when the drm.debug parameter is changes. I have already mentioned
>>> it is another reply:
>>>
>>>     dd_enable_module_group(module_name, group_id);
>>>     dd_disable_module_group(module_name, group_id);
>>>
>>>
>>> It will _not_ need any new flag or flag filtering.
>>>
>>> Best Regards,
>>> Petr
>>>
>>
>> Yes, I'm wondering as well if people are really going to use the
>> new flags and filter flags - I mentioned that here:
>> https://urldefense.proofpoint.com/v2/url?u=https-3A__lkml.org_lkml_2020_6_12_732&d=DwIBaQ&c=96ZbZZcaMF4w0F4jpN6LZg&r=1fLh1mlLqbfetnnGsbwXfpwmGlG4m83mXgtV4vZ1B1A&m=vltk6sSzPDQIqO4gGkJeDY6jcEarG4xTztab2EHtPFY&s=6x1EHNoRxebA99Tu-C2i0s5dmdzyEF9bXVcv_cYoM_I&e= 
>>
> 
> yes, I saw, and replied there.
> but since that was v1, and we're on v3, we should refresh.
> 
> the central use-case is above, 1-liner version summarized here:
> 
> 1- enable sites as you chase a problem with +up
> 2- examine them with grep =pu
> 3- change the set to suit, either by adding or subtracting callsites.
> 4- continue debugging, and changing callsites to suit
> 5- grep =pu control > ~/debugging-session-task1-callsites
> 6- echo up-p >control   # disable for now, leave u-set for later
> 7- do other stuff
> 8 echo uP+p >control # reactivate useful debug-state and resume
> 
> 
>> The grouping stuff is already being used by lots of modules so
>> that seems useful.
> 
> I now dont see the need.
> 
> given N debug callsites, any group can be defined by <N queries,
> probably a lot less
> if module authors can use ddebug_exec_queries(), cuz its exported, (15/21)
> then they can act (+p or -p) on those sets defined by <N queries.
> 
> and now any callsite can be in any number of groups, not just one.
> It would be prudent to evaluate such groupings case by case,
> because the intersecting callsites are subject to "last manipulator wins"
> but its unnecessary to insist that all sets are disjoint.
> Unlike pr_debug_n, however its spelled.
> 

hmm - so I think you are saying there is then no need to change the
calling functions themselves - its still 'pr_debug()'. You could even
use the 'format' qualifier for example to implement your groups that
way.

For example:

pr_debug("failure type1: blah");
pr_debug("failure type2: blah blah");

and then do: ddebug_exec_queries("format type1 +p", module);

I would be curious to see what Stanimir thinks of this proposal
and whether it would work for his venus driver, which is what
prompted this module group discussion.

Thanks,

-Jason

Re: [PATCH v3 20/21] dyndbg: add user-flag, negating-flags, and filtering on flags

[jim.cromie]

On Thu, Jun 18, 2020 at 1:40 PM Jason Baron <jbaron@akamai.com> wrote:
>
>
>
> On 6/18/20 3:11 PM, jim.cromie@gmail.com wrote:
> > On Thu, Jun 18, 2020 at 12:17 PM Jason Baron <jbaron@akamai.com> wrote:
> >>

> >
> >> The grouping stuff is already being used by lots of modules so
> >> that seems useful.
> >
> > I now dont see the need.
> >
> > given N debug callsites, any group can be defined by <N queries,
> > probably a lot less
> > if module authors can use ddebug_exec_queries(), cuz its exported, (15/21)
> > then they can act (+p or -p) on those sets defined by <N queries.
> >
> > and now any callsite can be in any number of groups, not just one.
> > It would be prudent to evaluate such groupings case by case,
> > because the intersecting callsites are subject to "last manipulator wins"
> > but its unnecessary to insist that all sets are disjoint.
> > Unlike pr_debug_n, however its spelled.
> >
>
> hmm - so I think you are saying there is then no need to change the
> calling functions themselves - its still 'pr_debug()'. You could even
> use the 'format' qualifier for example to implement your groups that
> way.
>
> For example:
>
> pr_debug("failure type1: blah");
> pr_debug("failure type2: blah blah");
>
> and then do: ddebug_exec_queries("format type1 +p", module);

Exactly

and using format, which always have user relevant info,
and often some severity indication (forex warn info err)
are a workable classification scheme already in use at least informally

So Id expect that this classification can often be done in 1 query.
define the set of callsites in 1 query-string, add +p or -p to it, and
manipulate away.

Amplifying,
this is the only user interface of consequence in dyndbg.
/sys/.../verbose doesnt count

Letting module authors use it is the full-featured way,
everything else is crap (movie reference)
and would require far more maintenance

>
> I would be curious to see what Stanimir thinks of this proposal
> and whether it would work for his venus driver, which is what
> prompted this module group discussion.
>

Indeed.
Id also like to hear from drm folks

./drm/amd/display/include/logger_types.h:#define DC_LOG_SURFACE(...)
pr_debug("[SURFACE]:"__VA_ARGS__)
./drm/amd/display/include/logger_types.h:#define
DC_LOG_HW_LINK_TRAINING(...)
pr_debug("[HW_LINK_TRAINING]:"__VA_ARGS__)
./drm/amd/display/include/logger_types.h:#define DC_LOG_HW_AUDIO(...)
pr_debug("[HW_AUDIO]:"__VA_ARGS__)
./drm/amd/display/include/logger_types.h:#define DC_LOG_SCALER(...)
pr_debug("[SCALER]:"__VA_ARGS__)
./drm/amd/display/include/logger_types.h:#define DC_LOG_BIOS(...)
pr_debug("[BIOS]:"__VA_ARGS__)
./drm/amd/display/include/logger_types.h:#define
DC_LOG_BANDWIDTH_CALCS(...) pr_debug("[BANDWIDTH_CALCS]:"__VA_ARGS__)
./drm/amd/display/include/logger_types.h:#define DC_LOG_DML(...)
pr_debug("[DML]:"__VA_ARGS__)
./drm/amd/display/include/logger_types.h:#define DC_LOG_IF_TRACE(...)
pr_debug("[IF_TRACE]:"__VA_ARGS__)
./drm/amd/display/include/logger_types.h:#define DC_LOG_GAMMA(...)
pr_debug("[GAMMA]:"__VA_ARGS__)
./drm/amd/display/include/logger_types.h:#define DC_LOG_ALL_GAMMA(...)
pr_debug("[GAMMA]:"__VA_ARGS__)
./drm/amd/display/include/logger_types.h:#define
DC_LOG_ALL_TF_CHANNELS(...) pr_debug("[GAMMA]:"__VA_ARGS__)

those defines suggest that they are already doing this with existing formats
with the export,
they can implement this group control using dyndbg with little effort.
including the tie-in to the __debug var if thats useful

and of course, user can add or subtract from that set ad-hoc.

> Thanks,
>
> -Jason

thanks
jimc

Re: [PATCH v3 20/21] dyndbg: add user-flag, negating-flags, and filtering on flags

[Stanimir Varbanov]

Hi Jason, Jim,

On 6/18/20 10:40 PM, Jason Baron wrote:
> 
> 
> On 6/18/20 3:11 PM, jim.cromie@gmail.com wrote:
>> On Thu, Jun 18, 2020 at 12:17 PM Jason Baron <jbaron@akamai.com> wrote:
>>>
>>>
>>>
>>> On 6/18/20 1:40 PM, Petr Mladek wrote:
>>>> On Thu 2020-06-18 18:19:12, Petr Mladek wrote:
>>>>> On Wed 2020-06-17 10:25:35, Jim Cromie wrote:
>>>>>> 1. Add a user-flag [u] which works like the [pfmlt] flags, but has no
>>>>>> effect on callsite behavior; it allows incremental marking of
>>>>>> arbitrary sets of callsites.
>>>>>>
>>>>>> 2. Add [PFMLTU] flags, which negate their counterparts; P===!p etc.
>>>>>> And in ddebug_read_flags():
>>>>>>    current code does:       [pfmltu_] -> flags
>>>>>>    copy it to:              [PFMLTU_] -> mask
>>>>>>
>>>>>> also disallow both of a pair: ie no 'pP', no true & false.
>>>>>>
>>>>>> 3. Add filtering ops into ddebug_change(), right after all the
>>>>>> callsite-property selections are complete.  These filter on the
>>>>>> callsite's current flagstate before applying modflags.
>>>>>>
>>>>>> Why ?
>>>>>>
>>>>>> The u-flag & filter flags
>>>>>>
>>>>>> The 'u' flag lets the user assemble an arbitary set of callsites.
>>>>>> Then using filter flags, user can activate the 'u' callsite set.
>>>>>>
>>>>>>   #> echo 'file foo.c +u; file bar.c +u' > control   # and repeat
>>>>>>   #> echo 'u+p' > control
>>>>>>
>>>>>> Of course, you can continue to just activate your set without ever
>>>>>> marking it 1st, but you could trivially add the markup as you go, then
>>>>>> be able to use it as a constraint later, to undo or modify your set.
>>>>>>
>>>>>>   #> echo 'file foo.c +up' >control
>>>>>>   .. monitor, debug, finish ..
>>>>>>   #> echo 'u-p' >control
>>>>>>
>>>>>>   # then later resume
>>>>>>   #> echo 'u+p' >control
>>>>>>
>>>>>>   # disable some cluttering messages, and remove from u-set
>>>>>>   #> echo 'file noisy.c function:jabber_* u-pu' >control
>>>>>>
>>>>>>   # for doc, recollection
>>>>>>   grep =pu control > my-favorite-callsites
>>>>>>
>>>>>> Note:
>>>>>>
>>>>>> Your flagstate after boot is generally not all =_. -DDEBUG will arm
>>>>>> compiled callsites by default, $builtinmod.dyndbg=+p bootargs can
>>>>>> enable them early, and $module.dyndbg=+p bootargs will arm them when
>>>>>> the module is loaded.  But you could manage them with u-flags:
>>>>>>
>>>>>>   #> echo '-t' >control             # clear t-flag to use it as 2ndary markup
>>>>>>   #> echo 'p+ut' >control   # mark the boot-enabled set of callsites
>>>>>>   #> echo '-p' >control             # clean your dmesg -w stream
>>>>>>
>>>>>>   ... monitor, debug ..
>>>>>>   #> echo 'module of_interest $qterms +pu' >control # build your set of useful debugs
>>>>>>   #> echo 'module of_interest $qterms UT+pu' >control       # same, but dont alter ut marked set
>>>>>
>>>>> Does anyone requested this feature, please?
>>>>>
>>>>> For me, it is really hard to imagine people using these complex and hacky
>>>>> steps.
>>>>
>>>> I think that all this is motivated by adding support for module
>>>> specific groups.
>>>>
>>>> What about storing the group as yet another information for each
>>>> message? I mean the same way as we store module name, file, line,
>>>> function name.
>>>>
>>>> Then we could add API to define group for a given message:
>>>>
>>>>    pr_debug_group(group_id, fmt, ...);
>>>>
>>>> the interface for the control file might be via new keyword "group".
>>>> You could then do something like:
>>>>
>>>>    echo module=drm group=0x3 +p >control
>>>>
>>>> But more importantly you should add functions that might be called
>>>> when the drm.debug parameter is changes. I have already mentioned
>>>> it is another reply:
>>>>
>>>>     dd_enable_module_group(module_name, group_id);
>>>>     dd_disable_module_group(module_name, group_id);
>>>>
>>>>
>>>> It will _not_ need any new flag or flag filtering.
>>>>
>>>> Best Regards,
>>>> Petr
>>>>
>>>
>>> Yes, I'm wondering as well if people are really going to use the
>>> new flags and filter flags - I mentioned that here:
>>> https://urldefense.proofpoint.com/v2/url?u=https-3A__lkml.org_lkml_2020_6_12_732&d=DwIBaQ&c=96ZbZZcaMF4w0F4jpN6LZg&r=1fLh1mlLqbfetnnGsbwXfpwmGlG4m83mXgtV4vZ1B1A&m=vltk6sSzPDQIqO4gGkJeDY6jcEarG4xTztab2EHtPFY&s=6x1EHNoRxebA99Tu-C2i0s5dmdzyEF9bXVcv_cYoM_I&e= 
>>>
>>
>> yes, I saw, and replied there.
>> but since that was v1, and we're on v3, we should refresh.
>>
>> the central use-case is above, 1-liner version summarized here:
>>
>> 1- enable sites as you chase a problem with +up
>> 2- examine them with grep =pu
>> 3- change the set to suit, either by adding or subtracting callsites.
>> 4- continue debugging, and changing callsites to suit
>> 5- grep =pu control > ~/debugging-session-task1-callsites
>> 6- echo up-p >control   # disable for now, leave u-set for later
>> 7- do other stuff
>> 8 echo uP+p >control # reactivate useful debug-state and resume
>>
>>
>>> The grouping stuff is already being used by lots of modules so
>>> that seems useful.
>>
>> I now dont see the need.
>>
>> given N debug callsites, any group can be defined by <N queries,
>> probably a lot less
>> if module authors can use ddebug_exec_queries(), cuz its exported, (15/21)
>> then they can act (+p or -p) on those sets defined by <N queries.
>>
>> and now any callsite can be in any number of groups, not just one.
>> It would be prudent to evaluate such groupings case by case,
>> because the intersecting callsites are subject to "last manipulator wins"
>> but its unnecessary to insist that all sets are disjoint.
>> Unlike pr_debug_n, however its spelled.
>>
> 
> hmm - so I think you are saying there is then no need to change the
> calling functions themselves - its still 'pr_debug()'. You could even
> use the 'format' qualifier for example to implement your groups that
> way.
> 
> For example:
> 
> pr_debug("failure type1: blah");
> pr_debug("failure type2: blah blah");
> 
> and then do: ddebug_exec_queries("format type1 +p", module);
> 
> I would be curious to see what Stanimir thinks of this proposal
> and whether it would work for his venus driver, which is what
> prompted this module group discussion.

Hmm, we spin in a circle :)

Infact this was my first way of implementing the groups in Venus driver,
you can see it at [1].

 +#define VDBGL(fmt, args...)	pr_debug("VENUSL: " fmt, ##args)
 +#define VDBGM(fmt, args...)	pr_debug("VENUSM: " fmt, ##args)
 +#define VDBGH(fmt, args...)	pr_debug("VENUSH: " fmt, ##args)
 +#define VDBGFW(fmt, args...)	pr_debug("VENUSFW: " fmt, ##args)


[1] https://lkml.org/lkml/2020/5/21/668

-- 
regards,
Stan

Re: [PATCH v3 20/21] dyndbg: add user-flag, negating-flags, and filtering on flags

[jim.cromie]

On Thu, Jun 18, 2020 at 4:34 PM Stanimir Varbanov
<stanimir.varbanov@linaro.org> wrote:
>
> Hi Jason, Jim,
>



> > I would be curious to see what Stanimir thinks of this proposal
> > and whether it would work for his venus driver, which is what
> > prompted this module group discussion.
>
> Hmm, we spin in a circle :)
>
> Infact this was my first way of implementing the groups in Venus driver,
> you can see it at [1].
>
>  +#define VDBGL(fmt, args...)   pr_debug("VENUSL: " fmt, ##args)
>  +#define VDBGM(fmt, args...)   pr_debug("VENUSM: " fmt, ##args)
>  +#define VDBGH(fmt, args...)   pr_debug("VENUSH: " fmt, ##args)
>  +#define VDBGFW(fmt, args...)  pr_debug("VENUSFW: " fmt, ##args)
>

I recall :-)

I think Greg K-Hs   distaste for those defines was for using them,
as it tosses the utility of grep pr_debug

pr_debug("VENUSM:"
is barely longer than
VDBGM

with ddebug_exec_queries, you can leverage the existing format.

>
> [1] https://lkml.org/lkml/2020/5/21/668
>
> --
> regards,
> Stan

thanks
Jim

Re: [PATCH v3 20/21] dyndbg: add user-flag, negating-flags, and filtering on flags

[Jason Baron]

On 6/18/20 6:48 PM, jim.cromie@gmail.com wrote:
> On Thu, Jun 18, 2020 at 4:34 PM Stanimir Varbanov
> <stanimir.varbanov@linaro.org> wrote:
>>
>> Hi Jason, Jim,
>>
> 
> 
> 
>>> I would be curious to see what Stanimir thinks of this proposal
>>> and whether it would work for his venus driver, which is what
>>> prompted this module group discussion.
>>
>> Hmm, we spin in a circle :)
>>
>> Infact this was my first way of implementing the groups in Venus driver,
>> you can see it at [1].
>>
>>  +#define VDBGL(fmt, args...)   pr_debug("VENUSL: " fmt, ##args)
>>  +#define VDBGM(fmt, args...)   pr_debug("VENUSM: " fmt, ##args)
>>  +#define VDBGH(fmt, args...)   pr_debug("VENUSH: " fmt, ##args)
>>  +#define VDBGFW(fmt, args...)  pr_debug("VENUSFW: " fmt, ##args)
>>
> 
> I recall :-)
> 
> I think Greg K-Hs   distaste for those defines was for using them,
> as it tosses the utility of grep pr_debug
> 
> pr_debug("VENUSM:"
> is barely longer than
> VDBGM
> 
> with ddebug_exec_queries, you can leverage the existing format.
> 
>>

Ok, yes, I like this approach because its simple (just exports
ddebug_exec_queries()), and it seems to be quite flexible. Module
authors can 'tag' their queries any way they want.

We could provide some structure, if desired, something like:

#define DYNAMIC_DEBUG_LOW "-V "
#define DYNAMIC_DEBUG_MED "-VV "
#define DYNAMIC_DEBUG_HIGH "-VVV "
#define DYNAMIC_DEBUG_REALLY_HIGH "-VVVV "

And then these could be added to the pr_debug() so:

#define VDBGL(fmt, args...)   pr_debug("VENUSL: "  DYNAMIC_DEBUG_LOW fmt, ##args)
or
#define VDBGL(fmt, args...)   pr_debug(DYNAMIC_DEBUG_LOW fmt, ##args)
or just:
pr_debug(DYNAMIC_DEBUG_LOW "ERROR HERE: %d", err)

Thanks,

-Jason


>> [1] https://urldefense.proofpoint.com/v2/url?u=https-3A__lkml.org_lkml_2020_5_21_668&d=DwIBaQ&c=96ZbZZcaMF4w0F4jpN6LZg&r=1fLh1mlLqbfetnnGsbwXfpwmGlG4m83mXgtV4vZ1B1A&m=29UzIGELhVL1znJsgyjGDKGIEdSkSlCsmAh0jpbWHVQ&s=_szr6DQOsbdQ-oYCR9-fs4b-XG_fotTiObUfG3z6UtY&e= 
>>
>> --
>> regards,
>> Stan
> 
> thanks
> Jim
> 

Re: [PATCH v3 20/21] dyndbg: add user-flag, negating-flags, and filtering on flags

[Petr Mladek]

On Thu 2020-06-18 13:11:05, jim.cromie@gmail.com wrote:
> On Thu, Jun 18, 2020 at 12:17 PM Jason Baron <jbaron@akamai.com> wrote:
> > Yes, I'm wondering as well if people are really going to use the
> > new flags and filter flags - I mentioned that here:
> > https://lkml.org/lkml/2020/6/12/732
> 
> yes, I saw, and replied there.

No, the repply only explains how the interface might be used. There is
no prove that people would actually use it.

> but since that was v1, and we're on v3, we should refresh.
> 
> the central use-case is above, 1-liner version summarized here:
> 
> 1- enable sites as you chase a problem with +up
> 2- examine them with grep =pu
> 3- change the set to suit, either by adding or subtracting callsites.
> 4- continue debugging, and changing callsites to suit
> 5- grep =pu control > ~/debugging-session-task1-callsites
> 6- echo up-p >control   # disable for now, leave u-set for later
> 7- do other stuff
> 8 echo uP+p >control # reactivate useful debug-state and resume

In short, this feature allows repeatedly enable/disable some
slowly growing maze of debug messages. Who need this, please? !!!

If I am debugging then I add/remove debug messages. But I never
enable/disable all of them repeatedly.

Also this is far from the original problem. It was about debugging
a single driver (venus, drm). In this case, people need something
easy to use. The following is the easy way:

   drm.debug = area_of_interest
   venus.debug = level_of_interest

   echo module=drm group=area_of_interest +p >control  [*]
   echo module=venus group=level_of_interes +p >control

Anyway, why filtering and 'u' flag would be necessary to debug these drivers?
Is anyone going to use it?

I would really like to hear the motivation for these features.
Has anyone asked for them?
Or are these just some "interesting" ideas from some brainstorming?


[*] Well, I wonder if the dyndbg interface would even be useful for drm
    because it it is actually split into many modules. So it might require
    creating/maintaining several filters.


Best Regards,
Petr

PS: This is probably my last mail in this thread. It goes in cycle.
    You repeatedly explain how many possibilities the new features
    allow. I repeatedly doubt that they are worth it.

    I also proposed another solution for the original problem (venus)
    but it has never been commented.

    I just hope that these features will not get merged without
    a clear interest in them.

Re: [PATCH v3 20/21] dyndbg: add user-flag, negating-flags, and filtering on flags

[Petr Mladek]

On Fri 2020-06-19 09:45:55, Petr Mladek wrote:
> On Thu 2020-06-18 13:11:05, jim.cromie@gmail.com wrote:
> > On Thu, Jun 18, 2020 at 12:17 PM Jason Baron <jbaron@akamai.com> wrote:
> > > Yes, I'm wondering as well if people are really going to use the
> > > new flags and filter flags - I mentioned that here:
> > > https://lkml.org/lkml/2020/6/12/732
> > 
> > yes, I saw, and replied there.
> 
> No, the repply only explains how the interface might be used. There is
> no prove that people would actually use it.
> 
> > but since that was v1, and we're on v3, we should refresh.
> > 
> > the central use-case is above, 1-liner version summarized here:
> > 
> > 1- enable sites as you chase a problem with +up
> > 2- examine them with grep =pu
> > 3- change the set to suit, either by adding or subtracting callsites.
> > 4- continue debugging, and changing callsites to suit
> > 5- grep =pu control > ~/debugging-session-task1-callsites
> > 6- echo up-p >control   # disable for now, leave u-set for later
> > 7- do other stuff
> > 8 echo uP+p >control # reactivate useful debug-state and resume
> 
> In short, this feature allows repeatedly enable/disable some
> slowly growing maze of debug messages. Who need this, please? !!!
> 
> If I am debugging then I add/remove debug messages. But I never
> enable/disable all of them repeatedly.

Not to say that I usually need to reboot when I reproduce the problem
and before I could try it again. So all dyndbg flags gets lost
between two tests anyway.

Best Regards,
Petr

Re: [PATCH v3 20/21] dyndbg: add user-flag, negating-flags, and filtering on flags

[Greg KH]

On Fri, Jun 19, 2020 at 10:10:24AM +0200, Petr Mladek wrote:
> On Fri 2020-06-19 09:45:55, Petr Mladek wrote:
> > On Thu 2020-06-18 13:11:05, jim.cromie@gmail.com wrote:
> > > On Thu, Jun 18, 2020 at 12:17 PM Jason Baron <jbaron@akamai.com> wrote:
> > > > Yes, I'm wondering as well if people are really going to use the
> > > > new flags and filter flags - I mentioned that here:
> > > > https://lkml.org/lkml/2020/6/12/732
> > > 
> > > yes, I saw, and replied there.
> > 
> > No, the repply only explains how the interface might be used. There is
> > no prove that people would actually use it.
> > 
> > > but since that was v1, and we're on v3, we should refresh.
> > > 
> > > the central use-case is above, 1-liner version summarized here:
> > > 
> > > 1- enable sites as you chase a problem with +up
> > > 2- examine them with grep =pu
> > > 3- change the set to suit, either by adding or subtracting callsites.
> > > 4- continue debugging, and changing callsites to suit
> > > 5- grep =pu control > ~/debugging-session-task1-callsites
> > > 6- echo up-p >control   # disable for now, leave u-set for later
> > > 7- do other stuff
> > > 8 echo uP+p >control # reactivate useful debug-state and resume
> > 
> > In short, this feature allows repeatedly enable/disable some
> > slowly growing maze of debug messages. Who need this, please? !!!
> > 
> > If I am debugging then I add/remove debug messages. But I never
> > enable/disable all of them repeatedly.
> 
> Not to say that I usually need to reboot when I reproduce the problem
> and before I could try it again. So all dyndbg flags gets lost
> between two tests anyway.

I agree, this feels way too complex for no good reason.  Users only need
a specific set of "run this command to enable messages and send us the
logs" instructions.  Nothing complex like this at all.

thanks,

greg k-h

Re: [PATCH v3 20/21] dyndbg: add user-flag, negating-flags, and filtering on flags

[jim.cromie]

On Thu, Jun 18, 2020 at 10:19 AM Petr Mladek <pmladek@suse.com> wrote:
>
> On Wed 2020-06-17 10:25:35, Jim Cromie wrote:


OK.
Please tell me how this chunk of prose fails to explain a use case for
the u-flag
we can differ on how useful it looks.

if u-flag is useful, then filtering on flags is also needed,
to use the flag to enable/disable an arbitrary set of callsites

all the other "flag abuse" you disliked in last patch is avoidable,
unless 2 people are chasing 2 separate problems,
and need to keep their sets distinct

> > Why ?
> >
> > The u-flag & filter flags
> >
> > The 'u' flag lets the user assemble an arbitary set of callsites.
> > Then using filter flags, user can activate the 'u' callsite set.
> >
> >   #> echo 'file foo.c +u; file bar.c +u' > control   # and repeat
> >   #> echo 'u+p' > control
> >
> > Of course, you can continue to just activate your set without ever
> > marking it 1st, but you could trivially add the markup as you go, then
> > be able to use it as a constraint later, to undo or modify your set.
> >
> >   #> echo 'file foo.c +up' >control
> >   .. monitor, debug, finish ..
> >   #> echo 'u-p' >control
> >
> >   # then later resume
> >   #> echo 'u+p' >control
> >
> >   # disable some cluttering messages, and remove from u-set
> >   #> echo 'file noisy.c function:jabber_* u-pu' >control
> >
> >   # for doc, recollection
> >   grep =pu control > my-favorite-callsites
> >
> > Note:
> >
> > Your flagstate after boot is generally not all =_. -DDEBUG will arm
> > compiled callsites by default, $builtinmod.dyndbg=+p bootargs can
> > enable them early, and $module.dyndbg=+p bootargs will arm them when
> > the module is loaded.  But you could manage them with u-flags:
> >
> >   #> echo '-t' >control               # clear t-flag to use it as 2ndary markup
> >   #> echo 'p+ut' >control     # mark the boot-enabled set of callsites
> >   #> echo '-p' >control               # clean your dmesg -w stream
> >
> >   ... monitor, debug ..
> >   #> echo 'module of_interest $qterms +pu' >control   # build your set of useful debugs
> >   #> echo 'module of_interest $qterms UT+pu' >control # same, but dont alter ut marked set
>
> Does anyone requested this feature, please?
>
> For me, it is really hard to imagine people using these complex and hacky
> steps.
>
> Not to say that using t-flag as a markup looks like a real hack.
> People either always need the line number in the kernel log or
> they do not need it at all.
>
> Let me repeat. Please, stop this non-sense.
>
> Best Regards,
> Petr

[PATCH v3 21/21] dyndbg: allow negating flag-chars in modifier flags

[Jim Cromie]

This allows a user to set some flags and clear others at the same
time.  This will make it easier to use the flags for creation of
transient sets, by making it easier to change the markings on those
sets by one flag, or all flags.

Consider the 3 operators [+-=]
 = resets entire flag-set, no memory of before
 - clears flags only, but preserves otherwise
 + sets flags only, but preserves otherwise

It would be nice to be able to set or clear all bits, or any subset,
while preserving untouched bits.  Using -/+ and negating flags
together let you do so.

    echo -ft > control		# in all callsites, clear 2 bits, preserve others
    echo f-ft > control		# if f-bit is true, clear 2 bits, preserve others

using non-empty query terms gives another dimension of selectivity

    echo $qtrms -ft > control	# for a callsite subset, clear 2 bits, preserve others
    echo $qtrms f-ft > control	# for a callsite subset, if f-bit is true, clear 2 bits, preserve others

Signed-off-by: Jim Cromie <jim.cromie@gmail.com>
---
 Documentation/admin-guide/dynamic-debug-howto.rst | 10 ++++++----
 lib/dynamic_debug.c                               |  6 ++++--
 2 files changed, 10 insertions(+), 6 deletions(-)

diff --git a/Documentation/admin-guide/dynamic-debug-howto.rst b/Documentation/admin-guide/dynamic-debug-howto.rst
index e910865b2edc..4539793e39bf 100644
--- a/Documentation/admin-guide/dynamic-debug-howto.rst
+++ b/Documentation/admin-guide/dynamic-debug-howto.rst
@@ -258,9 +258,11 @@ only callsites with ``u`` and ``f`` cleared.
 
 Flagsets cannot contain ``pP`` etc, a flag cannot be true and false.
 
-modflags containing upper-case flags is reserved/undefined for now.
-inverted-flags are currently ignored, usage gets trickier if given
-``-pXy``, it should leave x set.
+modflags may contain upper-case flags also, using these lets you
+invert the flag setting implied by the OP; '-pU' means disable
+printing, and mark that callsite with the user-flag to create a group,
+for optional further manipulation.  Generally, '+p' and '-p' is your
+main choice, and use of negating flags in modflags is rare.
 
 Notes::
 
@@ -270,7 +272,7 @@ For ``print_hex_dump_debug()`` and ``print_hex_dump_bytes()``, only
 For display, the flags are preceded by ``=``
 (mnemonic: what the flags are currently equal to).
 
-Note the regexp ``^[-+=][flmptu_]+$`` matches a flags specification.
+Note the regexp ``/^[-+=][flmptu_]+$/i`` matches a flags specification.
 To clear all flags at once, use ``=_`` or ``-flmptu``.
 
 
diff --git a/lib/dynamic_debug.c b/lib/dynamic_debug.c
index a302a7d8a722..c539bdd7fbe3 100644
--- a/lib/dynamic_debug.c
+++ b/lib/dynamic_debug.c
@@ -498,16 +498,18 @@ static int ddebug_parse_flags(const char *str,
 
 	/* calculate final mods: flags, mask based upon op */
 	switch (op) {
+		unsigned int tmp;
 	case '=':
 		/* modifiers->flags already set */
 		modifiers->mask = 0;
 		break;
 	case '+':
-		modifiers->mask = ~0U;
+		modifiers->mask = ~modifiers->mask;
 		break;
 	case '-':
+		tmp = modifiers->mask;
 		modifiers->mask = ~modifiers->flags;
-		modifiers->flags = 0;
+		modifiers->flags = tmp;
 		break;
 	}
 	vpr_info("mods:flags=0x%x,mask=0x%x filter:flags=0x%x,mask=0x%x\n",
-- 
2.26.2