* [PATCH] dyndbg: make dyndbg a known cli param @ 2021-09-09 16:17 Andrew Halaney 2021-09-09 21:34 ` Jason Baron 0 siblings, 1 reply; 10+ messages in thread From: Andrew Halaney @ 2021-09-09 16:17 UTC (permalink / raw) To: jbaron; +Cc: linux-kernel, Andrew Halaney Right now dyndbg shows up as an unknown parameter if used on boot: Unknown command line parameters: dyndbg=module params +p ; module sys +p That's because it is unknown, it doesn't sit in the __param section, so the processing done to warn users supplying an unknown parameter doesn't think it is legitimate. Install a dummy handler to register it. This was chosen instead of the approach the (deprecated) ddebug_query param takes, which is to have a real handler that copies the string taking up a KiB memory. Fixes: 86d1919a4fb0 ("init: print out unknown kernel parameters") Signed-off-by: Andrew Halaney <ahalaney@redhat.com> --- This is the last valid param I know of that was getting flagged on boot if used correctly. Please let me know if the alternative approach of actually copying the string is preferred and I'll spin that up instead. Sort of an aside, but what's the policy for removing deprecated cli params? ddebug_query has been deprecated for a very long time now, but I am not sure if removing params like that is considered almost as bad as breaking userspace considering some systems might update their kernels but not the bootloader supplying the param. lib/dynamic_debug.c | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/lib/dynamic_debug.c b/lib/dynamic_debug.c index cb5abb42c16a..84c16309cc63 100644 --- a/lib/dynamic_debug.c +++ b/lib/dynamic_debug.c @@ -761,6 +761,18 @@ static __init int ddebug_setup_query(char *str) __setup("ddebug_query=", ddebug_setup_query); +/* + * Install a noop handler to make dyndbg look like a normal kernel cli param. + * This avoids warnings about dyndbg being an unknown cli param when supplied + * by a user. + */ +static __init int dyndbg_setup(char *str) +{ + return 1; +} + +__setup("dyndbg=", dyndbg_setup); + /* * File_ops->write method for <debugfs>/dynamic_debug/control. Gathers the * command text from userspace, parses and executes it. -- 2.31.1 ^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH] dyndbg: make dyndbg a known cli param 2021-09-09 16:17 [PATCH] dyndbg: make dyndbg a known cli param Andrew Halaney @ 2021-09-09 21:34 ` Jason Baron 2021-09-10 14:00 ` Andrew Halaney [not found] ` <CAJfuBxzX9nEvxC1s-4uRCzLwN0=3gbFT__9vO_coEM5CrpnJng@mail.gmail.com> 0 siblings, 2 replies; 10+ messages in thread From: Jason Baron @ 2021-09-09 21:34 UTC (permalink / raw) To: Andrew Halaney; +Cc: linux-kernel, Jim Cromie On 9/9/21 12:17 PM, Andrew Halaney wrote: > Right now dyndbg shows up as an unknown parameter if used on boot: > > Unknown command line parameters: dyndbg=module params +p ; module sys +p > > That's because it is unknown, it doesn't sit in the __param > section, so the processing done to warn users supplying an unknown > parameter doesn't think it is legitimate. > > Install a dummy handler to register it. This was chosen instead of the > approach the (deprecated) ddebug_query param takes, which is to > have a real handler that copies the string taking up a KiB memory. > > Fixes: 86d1919a4fb0 ("init: print out unknown kernel parameters") > Signed-off-by: Andrew Halaney <ahalaney@redhat.com> > --- > > This is the last valid param I know of that was getting flagged on boot > if used correctly. Please let me know if the alternative approach of > actually copying the string is preferred and I'll spin that up instead. > Hi Andrew, So I think you are referring to the string copying that ddebug_query= does. I don't think that works for 'dyndbg=' b/c its actually looking through the entire command line for stuff like <module_name>.dyndbg="blah". So I think what you prposed below makes sense, we could perhaps add a note as to why it's a noop. As I mentioned it needs to look through the entire string. > Sort of an aside, but what's the policy for removing deprecated cli > params? ddebug_query has been deprecated for a very long time now, but I > am not sure if removing params like that is considered almost as bad as > breaking userspace considering some systems might update their kernels > but not the bootloader supplying the param. I think it's probably ok to remove at this point, especially now that we are going to flag it as unknown, right? So I feel like that change can logically go with this series if you want as a separate patch. Thanks, -Jason > > lib/dynamic_debug.c | 12 ++++++++++++ > 1 file changed, 12 insertions(+) > > diff --git a/lib/dynamic_debug.c b/lib/dynamic_debug.c > index cb5abb42c16a..84c16309cc63 100644 > --- a/lib/dynamic_debug.c > +++ b/lib/dynamic_debug.c > @@ -761,6 +761,18 @@ static __init int ddebug_setup_query(char *str) > > __setup("ddebug_query=", ddebug_setup_query); > > +/* > + * Install a noop handler to make dyndbg look like a normal kernel cli param. > + * This avoids warnings about dyndbg being an unknown cli param when supplied > + * by a user. > + */ > +static __init int dyndbg_setup(char *str) > +{ > + return 1; > +} > + > +__setup("dyndbg=", dyndbg_setup); > + > /* > * File_ops->write method for <debugfs>/dynamic_debug/control. Gathers the > * command text from userspace, parses and executes it. > ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] dyndbg: make dyndbg a known cli param 2021-09-09 21:34 ` Jason Baron @ 2021-09-10 14:00 ` Andrew Halaney [not found] ` <CAJfuBxzX9nEvxC1s-4uRCzLwN0=3gbFT__9vO_coEM5CrpnJng@mail.gmail.com> 1 sibling, 0 replies; 10+ messages in thread From: Andrew Halaney @ 2021-09-10 14:00 UTC (permalink / raw) To: Jason Baron; +Cc: linux-kernel, Jim Cromie On Thu, Sep 09, 2021 at 05:34:51PM -0400, Jason Baron wrote: > > > On 9/9/21 12:17 PM, Andrew Halaney wrote: > > Right now dyndbg shows up as an unknown parameter if used on boot: > > > > Unknown command line parameters: dyndbg=module params +p ; module sys +p > > > > That's because it is unknown, it doesn't sit in the __param > > section, so the processing done to warn users supplying an unknown > > parameter doesn't think it is legitimate. > > > > Install a dummy handler to register it. This was chosen instead of the > > approach the (deprecated) ddebug_query param takes, which is to > > have a real handler that copies the string taking up a KiB memory. > > > > Fixes: 86d1919a4fb0 ("init: print out unknown kernel parameters") > > Signed-off-by: Andrew Halaney <ahalaney@redhat.com> > > --- > > > > This is the last valid param I know of that was getting flagged on boot > > if used correctly. Please let me know if the alternative approach of > > actually copying the string is preferred and I'll spin that up instead. > > > > Hi Andrew, > > So I think you are referring to the string copying that ddebug_query= does. > I don't think that works for 'dyndbg=' b/c its actually looking through > the entire command line for stuff like <module_name>.dyndbg="blah". > > So I think what you prposed below makes sense, we could perhaps add a note > as to why it's a noop. As I mentioned it needs to look through the entire > string. > I think that the ddebug_query= style works fine for dyndbg=, and that things like <module_name>.dyndbg= go through a different path to actually have the work done. When dyndbg= is processed through dynamic_debug_init() all of the <module_name>.dyndbg= style queries don't do anything ultimately. When the module is actually loaded unknown_module_param_cb() checks for the module's dyndbg query and calls ddebug_dyndbg_module_param_cb() directly. It is at this point that I believe the query is really applied. For example, this is what I see in dmesg: [ 0.045553] Kernel command line: BOOT_IMAGE=(hd0,msdos1)/vmlinuz-5.14.0+ root=UUID=9a0e5b84-1190-465f-a587-f2f2a00a8e91 ro rhgb quiet "dyndbg=module params +p ; module sys +p" dynamic_debug.verbose=2 kvm.dyndbg=+pflmt # Initial dynamic_debug processing.. [ 0.114308] dyndbg: 6 debug prints in module main [ 0.114308] dyndbg: 1 debug prints in module initramfs <snip (no kvm debug prints... since it is a module and not loaded)> # Processing normal dyndbg= for builtins, param has matches but sys # does not [ 0.114308] dyndbg: dyndbg="module params +p ; module sys +p" [ 0.114308] dyndbg: query 0: "module params +p " <snip> [ 0.114308] dyndbg: applied: func="" file="" module="params" format="" lineno=0-0 [ 0.114308] dyndbg: query 1: "module sys +p" [ 0.114308] dyndbg: no-match: func="" file="" module="sys" format="" lineno=0-0 [ 0.114308] dyndbg: processed 2 queries, with 4 matches, 0 errs # Trying to process kvm.dyndbg=, but no effect as module isn't loaded [ 0.114308] doing dyndbg params: kvm.dyndbg='+pflmt' [ 0.114308] dyndbg: kvm.dyndbg="+pflmt" <snip> [ 0.114308] dyndbg: processed 1 queries, with 0 matches, 0 errs # kvm module is loaded, kernel/module.c calls ddebug_dyndbg_module_param_cb() # this time it does something [ 3.651764] doing kvm, parsing ARGS: 'dyndbg=+pflmt ' [ 3.651767] doing kvm: dyndbg='+pflmt' [ 3.651768] dyndbg: module: kvm dyndbg="+pflmt" [ 3.651769] dyndbg: query 0: "+pflmt" <snip> [ 3.652414] dyndbg: applied: func="" file="" module="kvm" format="" lineno=0-0 [ 3.652416] dyndbg: processed 1 queries, with 10 matches, 0 errs With that being said, I think ddebug_query= style of copying the dyndbg="string" is all that is really needed for processing builtins, and the module loading method that's already in place shouldn't be affected. Does that change you opinion of the approach (or am I totally misunderstanding)? Processing the whole cli seems unnecessary, but I'm also a (unjustified?) stickler for adding additional memory usage in the patch. > > > Sort of an aside, but what's the policy for removing deprecated cli > > params? ddebug_query has been deprecated for a very long time now, but I > > am not sure if removing params like that is considered almost as bad as > > breaking userspace considering some systems might update their kernels > > but not the bootloader supplying the param. > > I think it's probably ok to remove at this point, especially now that > we are going to flag it as unknown, right? So I feel like that change > can logically go with this series if you want as a separate patch. ddebug_query actually _is_ known (it registers with __setup() so there's no warning like the one I listed in the commit description (although the deprecated message in lib/dynamic_debug.c is present if you actually use it). So I sort of feel like the removal would be separate to this patchset. Let me know when your thoughts when you answer the prior question and I'll react with a v2 accordingly! > > Thanks, > > -Jason > > > > > > lib/dynamic_debug.c | 12 ++++++++++++ > > 1 file changed, 12 insertions(+) > > > > diff --git a/lib/dynamic_debug.c b/lib/dynamic_debug.c > > index cb5abb42c16a..84c16309cc63 100644 > > --- a/lib/dynamic_debug.c > > +++ b/lib/dynamic_debug.c > > @@ -761,6 +761,18 @@ static __init int ddebug_setup_query(char *str) > > > > __setup("ddebug_query=", ddebug_setup_query); > > > > +/* > > + * Install a noop handler to make dyndbg look like a normal kernel cli param. > > + * This avoids warnings about dyndbg being an unknown cli param when supplied > > + * by a user. > > + */ > > +static __init int dyndbg_setup(char *str) > > +{ > > + return 1; > > +} > > + > > +__setup("dyndbg=", dyndbg_setup); > > + > > /* > > * File_ops->write method for <debugfs>/dynamic_debug/control. Gathers the > > * command text from userspace, parses and executes it. > > > ^ permalink raw reply [flat|nested] 10+ messages in thread
[parent not found: <CAJfuBxzX9nEvxC1s-4uRCzLwN0=3gbFT__9vO_coEM5CrpnJng@mail.gmail.com>]
* Re: [PATCH] dyndbg: make dyndbg a known cli param [not found] ` <CAJfuBxzX9nEvxC1s-4uRCzLwN0=3gbFT__9vO_coEM5CrpnJng@mail.gmail.com> @ 2021-09-10 18:24 ` Andrew Halaney 2021-09-10 19:31 ` jim.cromie 0 siblings, 1 reply; 10+ messages in thread From: Andrew Halaney @ 2021-09-10 18:24 UTC (permalink / raw) To: jim.cromie; +Cc: Jason Baron, LKML On Fri, Sep 10, 2021 at 11:14:45AM -0600, jim.cromie@gmail.com wrote: > On Thu, Sep 9, 2021 at 3:34 PM Jason Baron <jbaron@akamai.com> wrote: > > > > > > > On 9/9/21 12:17 PM, Andrew Halaney wrote: > > > Right now dyndbg shows up as an unknown parameter if used on boot: > > > > > > Unknown command line parameters: dyndbg=module params +p ; module > > sys +p > > > > > > That's because it is unknown, it doesn't sit in the __param > > > section, so the processing done to warn users supplying an unknown > > > parameter doesn't think it is legitimate. > > > > > > your usage is incorrect for what youre trying to do in that example > what you need is: > > params.dyndbg=+p sys.dyndbg=+p > > dyndbg is properly unknown as a kernel param, it isnt one. > ( it was called a "fake" module param when added.) > $module.dyndbg is good, since its after the $module. (and the dot) > it also then inherits the "scan bootargs for relevant ones on module load" > behavior > > That example is (slightly altered) from Documentation/admin-guide/dynamic-debug-howto.rst, I can change the example used to be a little less confusing (using the module keyword is confusing, I could use something like func or file instead of what the docs use as an example). Is that what you're after, a better example usage of dyndbg= being whined about in dmesg for the commit message, or am I misunderstanding? I agree that dyndbg right now (both dyndbg= and $module.dyndbg=) are "fake" params, but I'd like to remove that message for dyndbg= as it is misleading. The code for module loading luckily already handles it all properly and doesn't warn on proper usage: static int unknown_module_param_cb(char *param, char *val, const char *modname, void *arg) { struct module *mod = arg; int ret; if (strcmp(param, "async_probe") == 0) { mod->async_probe_requested = true; return 0; } /* Check for magic 'dyndbg' arg */ ret = ddebug_dyndbg_module_param_cb(param, val, modname); if (ret != 0) pr_warn("%s: unknown parameter '%s' ignored\n", modname, param); return 0; } > > > > > > > > Install a dummy handler to register it. This was chosen instead of the > > > approach the (deprecated) ddebug_query param takes, which is to > > > have a real handler that copies the string taking up a KiB memory. > > > > > > Fixes: 86d1919a4fb0 ("init: print out unknown kernel parameters") > > > Signed-off-by: Andrew Halaney <ahalaney@redhat.com> > > > --- > > > > > > This is the last valid param I know of that was getting flagged on boot > > > if used correctly. Please let me know if the alternative approach of > > > actually copying the string is preferred and I'll spin that up instead. > > > > > > > Hi Andrew, > > > > So I think you are referring to the string copying that ddebug_query= does. > > I don't think that works for 'dyndbg=' b/c its actually looking through > > the entire command line for stuff like <module_name>.dyndbg="blah". > > > > So I think what you prposed below makes sense, we could perhaps add a note > > as to why it's a noop. As I mentioned it needs to look through the entire > > string. > > > > > > > Sort of an aside, but what's the policy for removing deprecated cli > > > params? ddebug_query has been deprecated for a very long time now, but I > > > am not sure if removing params like that is considered almost as bad as > > > breaking userspace considering some systems might update their kernels > > > but not the bootloader supplying the param. > > > > I think it's probably ok to remove at this point, especially now that > > we are going to flag it as unknown, right? So I feel like that change > > can logically go with this series if you want as a separate patch. > > > > Thanks, > > > > -Jason > > > > > > > > > > lib/dynamic_debug.c | 12 ++++++++++++ > > > 1 file changed, 12 insertions(+) > > > > > > diff --git a/lib/dynamic_debug.c b/lib/dynamic_debug.c > > > index cb5abb42c16a..84c16309cc63 100644 > > > --- a/lib/dynamic_debug.c > > > +++ b/lib/dynamic_debug.c > > > @@ -761,6 +761,18 @@ static __init int ddebug_setup_query(char *str) > > > > > > __setup("ddebug_query=", ddebug_setup_query); > > > > > > +/* > > > + * Install a noop handler to make dyndbg look like a normal kernel cli > > param. > > > + * This avoids warnings about dyndbg being an unknown cli param when > > supplied > > > + * by a user. > > > + */ > > > +static __init int dyndbg_setup(char *str) > > > +{ > > > + return 1; > > > +} > > > + > > > +__setup("dyndbg=", dyndbg_setup); > > > + > > > /* > > > * File_ops->write method for <debugfs>/dynamic_debug/control. Gathers > > the > > > * command text from userspace, parses and executes it. > > > > > ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] dyndbg: make dyndbg a known cli param 2021-09-10 18:24 ` Andrew Halaney @ 2021-09-10 19:31 ` jim.cromie 2021-09-10 20:16 ` Andrew Halaney 0 siblings, 1 reply; 10+ messages in thread From: jim.cromie @ 2021-09-10 19:31 UTC (permalink / raw) To: Andrew Halaney; +Cc: Jason Baron, LKML On Fri, Sep 10, 2021 at 12:24 PM Andrew Halaney <ahalaney@redhat.com> wrote: > > On Fri, Sep 10, 2021 at 11:14:45AM -0600, jim.cromie@gmail.com wrote: > > On Thu, Sep 9, 2021 at 3:34 PM Jason Baron <jbaron@akamai.com> wrote: > > > > > > > > > > > On 9/9/21 12:17 PM, Andrew Halaney wrote: > > > > Right now dyndbg shows up as an unknown parameter if used on boot: > > > > > > > > Unknown command line parameters: dyndbg=module params +p ; module > > > sys +p > > > > > > > > That's because it is unknown, it doesn't sit in the __param > > > > section, so the processing done to warn users supplying an unknown > > > > parameter doesn't think it is legitimate. > > > > > > > > > > your usage is incorrect for what youre trying to do in that example > > what you need is: > > > > params.dyndbg=+p sys.dyndbg=+p > > > > dyndbg is properly unknown as a kernel param, it isnt one. > > ( it was called a "fake" module param when added.) > > $module.dyndbg is good, since its after the $module. (and the dot) > > it also then inherits the "scan bootargs for relevant ones on module load" > > behavior > > > > > > That example is (slightly altered) from > Documentation/admin-guide/dynamic-debug-howto.rst, oh dear, I see the lines youre referring to. I am surprised. fyi, those lines are: // enable pr_debugs in 2 builtins, #cmt is stripped dyndbg="module params +p #cmt ; module sys +p" is your patchset removing those lines ? if so, ack that part. > I can change the example used to be a little less confusing (using the > module keyword is confusing, I could use something like > func or file instead of what the docs use as an example). yes please, I saw bad usage, thought faulty premise. > Is that what you're after, a better example usage of dyndbg= being > whined about in dmesg for the commit message, or am I misunderstanding? I guess Im inured to it. Heres my regular version with a similar addition. Unknown command line parameters: virtme_link_mods=/home/jimc/projects/lx/wk-next/builds/local-i915m/.virtme_mods/lib/modules/0.0.0 virtme_initmount0=/root virtme_initmount1=/root/sbin virtme_stty_con=rows 27 cols 109 iutf8 virtme_chdir=home/jimc/projects/lx dyndbg=+p most of them do something, just not for the kernel. I dont think this is worth explicitly silencing. rather rip out the misleading doc. > I agree that dyndbg right now (both dyndbg= and $module.dyndbg=) are > "fake" params, but I'd like to remove that message for dyndbg= as it is > misleading. The code for module loading luckily already handles it all > properly and doesn't warn on proper usage: > > static int unknown_module_param_cb(char *param, char *val, const char *modname, > void *arg) > { > struct module *mod = arg; > int ret; > > if (strcmp(param, "async_probe") == 0) { > mod->async_probe_requested = true; > return 0; > } > > /* Check for magic 'dyndbg' arg */ > ret = ddebug_dyndbg_module_param_cb(param, val, modname); > if (ret != 0) > pr_warn("%s: unknown parameter '%s' ignored\n", modname, param); > return 0; > } > > > > > > > > > > > > > > > Install a dummy handler to register it. This was chosen instead of the > > > > approach the (deprecated) ddebug_query param takes, which is to > > > > have a real handler that copies the string taking up a KiB memory. > > > > > > > > Fixes: 86d1919a4fb0 ("init: print out unknown kernel parameters") > > > > Signed-off-by: Andrew Halaney <ahalaney@redhat.com> > > > > --- > > > > > > > > This is the last valid param I know of that was getting flagged on boot > > > > if used correctly. Please let me know if the alternative approach of > > > > actually copying the string is preferred and I'll spin that up instead. > > > > > > > > > > Hi Andrew, > > > > > > So I think you are referring to the string copying that ddebug_query= does. > > > I don't think that works for 'dyndbg=' b/c its actually looking through > > > the entire command line for stuff like <module_name>.dyndbg="blah". > > > > > > So I think what you prposed below makes sense, we could perhaps add a note > > > as to why it's a noop. As I mentioned it needs to look through the entire > > > string. > > > > > > > > > > Sort of an aside, but what's the policy for removing deprecated cli > > > > params? ddebug_query has been deprecated for a very long time now, but I > > > > am not sure if removing params like that is considered almost as bad as > > > > breaking userspace considering some systems might update their kernels > > > > but not the bootloader supplying the param. > > > > > > I think it's probably ok to remove at this point, especially now that > > > we are going to flag it as unknown, right? So I feel like that change > > > can logically go with this series if you want as a separate patch. > > > > > > Thanks, > > > > > > -Jason > > > > > > > > > > > > > > lib/dynamic_debug.c | 12 ++++++++++++ > > > > 1 file changed, 12 insertions(+) > > > > > > > > diff --git a/lib/dynamic_debug.c b/lib/dynamic_debug.c > > > > index cb5abb42c16a..84c16309cc63 100644 > > > > --- a/lib/dynamic_debug.c > > > > +++ b/lib/dynamic_debug.c > > > > @@ -761,6 +761,18 @@ static __init int ddebug_setup_query(char *str) > > > > > > > > __setup("ddebug_query=", ddebug_setup_query); > > > > > > > > +/* > > > > + * Install a noop handler to make dyndbg look like a normal kernel cli > > > param. > > > > + * This avoids warnings about dyndbg being an unknown cli param when > > > supplied > > > > + * by a user. > > > > + */ > > > > +static __init int dyndbg_setup(char *str) > > > > +{ > > > > + return 1; > > > > +} > > > > + > > > > +__setup("dyndbg=", dyndbg_setup); > > > > + > > > > /* > > > > * File_ops->write method for <debugfs>/dynamic_debug/control. Gathers > > > the > > > > * command text from userspace, parses and executes it. > > > > > > > > ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] dyndbg: make dyndbg a known cli param 2021-09-10 19:31 ` jim.cromie @ 2021-09-10 20:16 ` Andrew Halaney 2021-09-10 20:28 ` Andrew Halaney ` (2 more replies) 0 siblings, 3 replies; 10+ messages in thread From: Andrew Halaney @ 2021-09-10 20:16 UTC (permalink / raw) To: jim.cromie; +Cc: Jason Baron, LKML On Fri, Sep 10, 2021 at 01:31:22PM -0600, jim.cromie@gmail.com wrote: > On Fri, Sep 10, 2021 at 12:24 PM Andrew Halaney <ahalaney@redhat.com> wrote: > > > > On Fri, Sep 10, 2021 at 11:14:45AM -0600, jim.cromie@gmail.com wrote: > > > On Thu, Sep 9, 2021 at 3:34 PM Jason Baron <jbaron@akamai.com> wrote: > > > > > > > > > > > > > > > On 9/9/21 12:17 PM, Andrew Halaney wrote: > > > > > Right now dyndbg shows up as an unknown parameter if used on boot: > > > > > > > > > > Unknown command line parameters: dyndbg=module params +p ; module > > > > sys +p > > > > > > > > > > That's because it is unknown, it doesn't sit in the __param > > > > > section, so the processing done to warn users supplying an unknown > > > > > parameter doesn't think it is legitimate. > > > > > > > > > > > > > > your usage is incorrect for what youre trying to do in that example > > > what you need is: > > > > > > params.dyndbg=+p sys.dyndbg=+p > > > > > > dyndbg is properly unknown as a kernel param, it isnt one. > > > ( it was called a "fake" module param when added.) > > > $module.dyndbg is good, since its after the $module. (and the dot) > > > it also then inherits the "scan bootargs for relevant ones on module load" > > > behavior > > > > > > > > > > That example is (slightly altered) from > > Documentation/admin-guide/dynamic-debug-howto.rst, > > oh dear, I see the lines youre referring to. > I am surprised. > fyi, those lines are: > // enable pr_debugs in 2 builtins, #cmt is stripped > dyndbg="module params +p #cmt ; module sys +p" > > is your patchset removing those lines ? > if so, ack that part. > > > I can change the example used to be a little less confusing (using the > > module keyword is confusing, I could use something like > > func or file instead of what the docs use as an example). > > yes please, I saw bad usage, thought faulty premise. > > > Is that what you're after, a better example usage of dyndbg= being > > whined about in dmesg for the commit message, or am I misunderstanding? > > I guess Im inured to it. Heres my regular version with a similar addition. > > Unknown command line parameters: > virtme_link_mods=/home/jimc/projects/lx/wk-next/builds/local-i915m/.virtme_mods/lib/modules/0.0.0 > virtme_initmount0=/root virtme_initmount1=/root/sbin > virtme_stty_con=rows 27 cols 109 iutf8 > virtme_chdir=home/jimc/projects/lx dyndbg=+p > > most of them do something, just not for the kernel. > > I dont think this is worth explicitly silencing. > rather rip out the misleading doc. > Ohhhhh, ok now I think I'm following what you and Jason are saying. dyndbg= parameter does need to process the whole cli, for cases like when $module.dyndbg= is supplied but $module is a builtin. And you are saying that this syntax (although it works) 'dyndbg="module params +p #cmt ; module sys +p"' is not the intended usage. So converting dyndbg= to act like ddebug_query= won't work because $module.dyndbg="+p" should work if $module is builtin or a module (settles my open discussion with Jason). I'm going to hold my ground and try and silence the warning, because I think the kernel parameters interface is well defined enough (kernel params go before the -- i.e. "these are kernel params -- these are for init" With that in mind I'll make a v2 series out of this doing: 1. Clean up the doc to show intended usage on cli, something like (params.dyndbg="+p" sys.dyndbg="+p" dyndbg="file init/main.c +p #cmt ; func pc87360_init_device +p") 2. Remove ddebug_query per Jason's approval 3. Silence the param with this patch here, with a commit message updated explaining why dyndbg= needs to be able to process the whole cli, per Jason Please lemme know if there are any strong objections in the meantime and thanks for the feedback! > > > I agree that dyndbg right now (both dyndbg= and $module.dyndbg=) are > > "fake" params, but I'd like to remove that message for dyndbg= as it is > > misleading. The code for module loading luckily already handles it all > > properly and doesn't warn on proper usage: > > > > static int unknown_module_param_cb(char *param, char *val, const char *modname, > > void *arg) > > { > > struct module *mod = arg; > > int ret; > > > > if (strcmp(param, "async_probe") == 0) { > > mod->async_probe_requested = true; > > return 0; > > } > > > > /* Check for magic 'dyndbg' arg */ > > ret = ddebug_dyndbg_module_param_cb(param, val, modname); > > if (ret != 0) > > pr_warn("%s: unknown parameter '%s' ignored\n", modname, param); > > return 0; > > } > > > > > > > > > > > > > > > > > > > > > > Install a dummy handler to register it. This was chosen instead of the > > > > > approach the (deprecated) ddebug_query param takes, which is to > > > > > have a real handler that copies the string taking up a KiB memory. > > > > > > > > > > Fixes: 86d1919a4fb0 ("init: print out unknown kernel parameters") > > > > > Signed-off-by: Andrew Halaney <ahalaney@redhat.com> > > > > > --- > > > > > > > > > > This is the last valid param I know of that was getting flagged on boot > > > > > if used correctly. Please let me know if the alternative approach of > > > > > actually copying the string is preferred and I'll spin that up instead. > > > > > > > > > > > > > Hi Andrew, > > > > > > > > So I think you are referring to the string copying that ddebug_query= does. > > > > I don't think that works for 'dyndbg=' b/c its actually looking through > > > > the entire command line for stuff like <module_name>.dyndbg="blah". > > > > > > > > So I think what you prposed below makes sense, we could perhaps add a note > > > > as to why it's a noop. As I mentioned it needs to look through the entire > > > > string. > > > > > > > > > > > > > Sort of an aside, but what's the policy for removing deprecated cli > > > > > params? ddebug_query has been deprecated for a very long time now, but I > > > > > am not sure if removing params like that is considered almost as bad as > > > > > breaking userspace considering some systems might update their kernels > > > > > but not the bootloader supplying the param. > > > > > > > > I think it's probably ok to remove at this point, especially now that > > > > we are going to flag it as unknown, right? So I feel like that change > > > > can logically go with this series if you want as a separate patch. > > > > > > > > Thanks, > > > > > > > > -Jason > > > > > > > > > > > > > > > > > > lib/dynamic_debug.c | 12 ++++++++++++ > > > > > 1 file changed, 12 insertions(+) > > > > > > > > > > diff --git a/lib/dynamic_debug.c b/lib/dynamic_debug.c > > > > > index cb5abb42c16a..84c16309cc63 100644 > > > > > --- a/lib/dynamic_debug.c > > > > > +++ b/lib/dynamic_debug.c > > > > > @@ -761,6 +761,18 @@ static __init int ddebug_setup_query(char *str) > > > > > > > > > > __setup("ddebug_query=", ddebug_setup_query); > > > > > > > > > > +/* > > > > > + * Install a noop handler to make dyndbg look like a normal kernel cli > > > > param. > > > > > + * This avoids warnings about dyndbg being an unknown cli param when > > > > supplied > > > > > + * by a user. > > > > > + */ > > > > > +static __init int dyndbg_setup(char *str) > > > > > +{ > > > > > + return 1; > > > > > +} > > > > > + > > > > > +__setup("dyndbg=", dyndbg_setup); > > > > > + > > > > > /* > > > > > * File_ops->write method for <debugfs>/dynamic_debug/control. Gathers > > > > the > > > > > * command text from userspace, parses and executes it. > > > > > > > > > > > > ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] dyndbg: make dyndbg a known cli param 2021-09-10 20:16 ` Andrew Halaney @ 2021-09-10 20:28 ` Andrew Halaney 2021-09-10 22:02 ` jim.cromie 2021-09-11 3:04 ` jim.cromie 2 siblings, 0 replies; 10+ messages in thread From: Andrew Halaney @ 2021-09-10 20:28 UTC (permalink / raw) To: jim.cromie; +Cc: Jason Baron, LKML On Fri, Sep 10, 2021 at 03:16:35PM -0500, Andrew Halaney wrote: > On Fri, Sep 10, 2021 at 01:31:22PM -0600, jim.cromie@gmail.com wrote: > > On Fri, Sep 10, 2021 at 12:24 PM Andrew Halaney <ahalaney@redhat.com> wrote: > > > > > > On Fri, Sep 10, 2021 at 11:14:45AM -0600, jim.cromie@gmail.com wrote: > > > > On Thu, Sep 9, 2021 at 3:34 PM Jason Baron <jbaron@akamai.com> wrote: > > > > > > > > > > > > > > > > > > > On 9/9/21 12:17 PM, Andrew Halaney wrote: > > > > > > Right now dyndbg shows up as an unknown parameter if used on boot: > > > > > > > > > > > > Unknown command line parameters: dyndbg=module params +p ; module > > > > > sys +p > > > > > > > > > > > > That's because it is unknown, it doesn't sit in the __param > > > > > > section, so the processing done to warn users supplying an unknown > > > > > > parameter doesn't think it is legitimate. > > > > > > > > > > > > > > > > > > your usage is incorrect for what youre trying to do in that example > > > > what you need is: > > > > > > > > params.dyndbg=+p sys.dyndbg=+p > > > > > > > > dyndbg is properly unknown as a kernel param, it isnt one. > > > > ( it was called a "fake" module param when added.) > > > > $module.dyndbg is good, since its after the $module. (and the dot) > > > > it also then inherits the "scan bootargs for relevant ones on module load" > > > > behavior > > > > > > > > > > > > > > That example is (slightly altered) from > > > Documentation/admin-guide/dynamic-debug-howto.rst, > > > > oh dear, I see the lines youre referring to. > > I am surprised. > > fyi, those lines are: > > // enable pr_debugs in 2 builtins, #cmt is stripped > > dyndbg="module params +p #cmt ; module sys +p" > > > > is your patchset removing those lines ? > > if so, ack that part. > > > > > I can change the example used to be a little less confusing (using the > > > module keyword is confusing, I could use something like > > > func or file instead of what the docs use as an example). > > > > yes please, I saw bad usage, thought faulty premise. > > > > > Is that what you're after, a better example usage of dyndbg= being > > > whined about in dmesg for the commit message, or am I misunderstanding? > > > > I guess Im inured to it. Heres my regular version with a similar addition. > > > > Unknown command line parameters: > > virtme_link_mods=/home/jimc/projects/lx/wk-next/builds/local-i915m/.virtme_mods/lib/modules/0.0.0 > > virtme_initmount0=/root virtme_initmount1=/root/sbin > > virtme_stty_con=rows 27 cols 109 iutf8 > > virtme_chdir=home/jimc/projects/lx dyndbg=+p > > > > most of them do something, just not for the kernel. > > > > I dont think this is worth explicitly silencing. > > rather rip out the misleading doc. > > > > Ohhhhh, ok now I think I'm following what you and Jason are saying. > > dyndbg= parameter does need to process the whole cli, for cases like > when $module.dyndbg= is supplied but $module is a builtin. And you are > saying that this syntax (although it works) > 'dyndbg="module params +p #cmt ; module sys +p"' is not the intended > usage. > > So converting dyndbg= to act like ddebug_query= won't work because > $module.dyndbg="+p" should work if $module is builtin or a module > (settles my open discussion with Jason). Egh, sorry for double reply here but I think I got too excited with that realization. $module.dyndbg="+p" won't work if $module is a builtin unless dyndbg= is also on the command line independently. So I'm kind of thinking that the current documentation showing 'dyndbg="module params +p #cmt ; module sys +p"' is correct for params/sys as long as they're builtins. So the whole [1] and [3] in the list below don't quite make sense to me and the questions I had to Jason are still open.... dang. > > I'm going to hold my ground and try and silence the warning, because I > think the kernel parameters interface is well defined enough (kernel > params go before the -- i.e. "these are kernel params -- these are for init" > > With that in mind I'll make a v2 series out of this doing: > 1. Clean up the doc to show intended usage on cli, something like > (params.dyndbg="+p" sys.dyndbg="+p" dyndbg="file init/main.c +p #cmt ; func pc87360_init_device +p") > 2. Remove ddebug_query per Jason's approval > 3. Silence the param with this patch here, with a commit message > updated explaining why dyndbg= needs to be able to process the whole > cli, per Jason > > Please lemme know if there are any strong objections in the meantime and > thanks for the feedback! > > > > > > I agree that dyndbg right now (both dyndbg= and $module.dyndbg=) are > > > "fake" params, but I'd like to remove that message for dyndbg= as it is > > > misleading. The code for module loading luckily already handles it all > > > properly and doesn't warn on proper usage: > > > > > > static int unknown_module_param_cb(char *param, char *val, const char *modname, > > > void *arg) > > > { > > > struct module *mod = arg; > > > int ret; > > > > > > if (strcmp(param, "async_probe") == 0) { > > > mod->async_probe_requested = true; > > > return 0; > > > } > > > > > > /* Check for magic 'dyndbg' arg */ > > > ret = ddebug_dyndbg_module_param_cb(param, val, modname); > > > if (ret != 0) > > > pr_warn("%s: unknown parameter '%s' ignored\n", modname, param); > > > return 0; > > > } > > > > > > > > > > > > > > > > > > > > > > > > > > > > > Install a dummy handler to register it. This was chosen instead of the > > > > > > approach the (deprecated) ddebug_query param takes, which is to > > > > > > have a real handler that copies the string taking up a KiB memory. > > > > > > > > > > > > Fixes: 86d1919a4fb0 ("init: print out unknown kernel parameters") > > > > > > Signed-off-by: Andrew Halaney <ahalaney@redhat.com> > > > > > > --- > > > > > > > > > > > > This is the last valid param I know of that was getting flagged on boot > > > > > > if used correctly. Please let me know if the alternative approach of > > > > > > actually copying the string is preferred and I'll spin that up instead. > > > > > > > > > > > > > > > > Hi Andrew, > > > > > > > > > > So I think you are referring to the string copying that ddebug_query= does. > > > > > I don't think that works for 'dyndbg=' b/c its actually looking through > > > > > the entire command line for stuff like <module_name>.dyndbg="blah". > > > > > > > > > > So I think what you prposed below makes sense, we could perhaps add a note > > > > > as to why it's a noop. As I mentioned it needs to look through the entire > > > > > string. > > > > > > > > > > > > > > > > Sort of an aside, but what's the policy for removing deprecated cli > > > > > > params? ddebug_query has been deprecated for a very long time now, but I > > > > > > am not sure if removing params like that is considered almost as bad as > > > > > > breaking userspace considering some systems might update their kernels > > > > > > but not the bootloader supplying the param. > > > > > > > > > > I think it's probably ok to remove at this point, especially now that > > > > > we are going to flag it as unknown, right? So I feel like that change > > > > > can logically go with this series if you want as a separate patch. > > > > > > > > > > Thanks, > > > > > > > > > > -Jason > > > > > > > > > > > > > > > > > > > > > > lib/dynamic_debug.c | 12 ++++++++++++ > > > > > > 1 file changed, 12 insertions(+) > > > > > > > > > > > > diff --git a/lib/dynamic_debug.c b/lib/dynamic_debug.c > > > > > > index cb5abb42c16a..84c16309cc63 100644 > > > > > > --- a/lib/dynamic_debug.c > > > > > > +++ b/lib/dynamic_debug.c > > > > > > @@ -761,6 +761,18 @@ static __init int ddebug_setup_query(char *str) > > > > > > > > > > > > __setup("ddebug_query=", ddebug_setup_query); > > > > > > > > > > > > +/* > > > > > > + * Install a noop handler to make dyndbg look like a normal kernel cli > > > > > param. > > > > > > + * This avoids warnings about dyndbg being an unknown cli param when > > > > > supplied > > > > > > + * by a user. > > > > > > + */ > > > > > > +static __init int dyndbg_setup(char *str) > > > > > > +{ > > > > > > + return 1; > > > > > > +} > > > > > > + > > > > > > +__setup("dyndbg=", dyndbg_setup); > > > > > > + > > > > > > /* > > > > > > * File_ops->write method for <debugfs>/dynamic_debug/control. Gathers > > > > > the > > > > > > * command text from userspace, parses and executes it. > > > > > > > > > > > > > > > > ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] dyndbg: make dyndbg a known cli param 2021-09-10 20:16 ` Andrew Halaney 2021-09-10 20:28 ` Andrew Halaney @ 2021-09-10 22:02 ` jim.cromie 2021-09-11 3:04 ` jim.cromie 2 siblings, 0 replies; 10+ messages in thread From: jim.cromie @ 2021-09-10 22:02 UTC (permalink / raw) To: Andrew Halaney; +Cc: Jason Baron, LKML On Fri, Sep 10, 2021 at 2:16 PM Andrew Halaney <ahalaney@redhat.com> wrote: > > On Fri, Sep 10, 2021 at 01:31:22PM -0600, jim.cromie@gmail.com wrote: > > On Fri, Sep 10, 2021 at 12:24 PM Andrew Halaney <ahalaney@redhat.com> wrote: > > > > > > On Fri, Sep 10, 2021 at 11:14:45AM -0600, jim.cromie@gmail.com wrote: > > > > On Thu, Sep 9, 2021 at 3:34 PM Jason Baron <jbaron@akamai.com> wrote: > > > > > > > > > > > > > > > > > > > On 9/9/21 12:17 PM, Andrew Halaney wrote: > > > > > > Right now dyndbg shows up as an unknown parameter if used on boot: > > > > > > > > > > > > Unknown command line parameters: dyndbg=module params +p ; module > > > > > sys +p > > > > > > > > > > > > That's because it is unknown, it doesn't sit in the __param > > > > > > section, so the processing done to warn users supplying an unknown > > > > > > parameter doesn't think it is legitimate. > > > > > > > > > > > > > > > > > > your usage is incorrect for what youre trying to do in that example > > > > what you need is: > > > > > > > > params.dyndbg=+p sys.dyndbg=+p > > > > > > > > dyndbg is properly unknown as a kernel param, it isnt one. > > > > ( it was called a "fake" module param when added.) > > > > $module.dyndbg is good, since its after the $module. (and the dot) > > > > it also then inherits the "scan bootargs for relevant ones on module load" > > > > behavior > > > > > > > > > > > > > > That example is (slightly altered) from > > > Documentation/admin-guide/dynamic-debug-howto.rst, > > > > oh dear, I see the lines youre referring to. > > I am surprised. > > fyi, those lines are: > > // enable pr_debugs in 2 builtins, #cmt is stripped > > dyndbg="module params +p #cmt ; module sys +p" > > > > is your patchset removing those lines ? > > if so, ack that part. > > > > > I can change the example used to be a little less confusing (using the > > > module keyword is confusing, I could use something like > > > func or file instead of what the docs use as an example). > > > > yes please, I saw bad usage, thought faulty premise. > > > > > Is that what you're after, a better example usage of dyndbg= being > > > whined about in dmesg for the commit message, or am I misunderstanding? > > > > I guess Im inured to it. Heres my regular version with a similar addition. > > > > Unknown command line parameters: > > virtme_link_mods=/home/jimc/projects/lx/wk-next/builds/local-i915m/.virtme_mods/lib/modules/0.0.0 > > virtme_initmount0=/root virtme_initmount1=/root/sbin > > virtme_stty_con=rows 27 cols 109 iutf8 > > virtme_chdir=home/jimc/projects/lx dyndbg=+p > > > > most of them do something, just not for the kernel. > > > > I dont think this is worth explicitly silencing. > > rather rip out the misleading doc. > > > > Ohhhhh, ok now I think I'm following what you and Jason are saying. > > dyndbg= parameter does need to process the whole cli, for cases like > when $module.dyndbg= is supplied but $module is a builtin. And you are > saying that this syntax (although it works) > 'dyndbg="module params +p #cmt ; module sys +p"' is not the intended > usage. > > So converting dyndbg= to act like ddebug_query= won't work because > $module.dyndbg="+p" should work if $module is builtin or a module > (settles my open discussion with Jason). yes, $mod.dyndbg=+p works now whether $mod is builtin or loadable. that must stick. if bare dyndbg="str" works like an alias for ddebug_query= (its amazing what one forgets) then I agree the warning is misleading. ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] dyndbg: make dyndbg a known cli param 2021-09-10 20:16 ` Andrew Halaney 2021-09-10 20:28 ` Andrew Halaney 2021-09-10 22:02 ` jim.cromie @ 2021-09-11 3:04 ` jim.cromie 2021-09-13 20:10 ` Andrew Halaney 2 siblings, 1 reply; 10+ messages in thread From: jim.cromie @ 2021-09-11 3:04 UTC (permalink / raw) To: Andrew Halaney; +Cc: Jason Baron, LKML > > So converting dyndbg= to act like ddebug_query= won't work because > $module.dyndbg="+p" should work if $module is builtin or a module > (settles my open discussion with Jason). > they can both work, because the dot. \w+.\w+ is separate, which is why it works for loadable modules. and Ive just re-verified that these both ( bare/global and after $mod. ) work dyndbg=+pmfl turns on all builtins [ 128.341650] 8250:serial8250_interrupt:140: serial8250_interrupt(4): end [ 128.344996] 8250:serial8250_interrupt:140: serial8250_interrupt(4): end ^C bash-5.1# echo -p > /proc/dynamic_debug/control dyndbg: read 3 bytes from userspace < -p > dyndbg: query 0: <-p> mod:<*> dyndbg: split into words: <-p> dyndbg: op=<-> dyndbg: flags=0x1 dyndbg: *flagsp=0x0 *maskp=0xfffffffe dyndbg: parsed: func=<> file=<> module=<> format=<> lineno=0-0 dyndbg: applied: func=<> file=<> module=<> format=<> lineno=0-0 dyndbg: processed 1 queries, with 3055 matches, 0 errs bash-5.1# cat /proc/cmdline virtme_link_mods=/home/jimc/projects/lx/wk-next/builds/local-i915m/.virtme_mods/lib/modules/0.0.0 virtme_initmount0=/root virtme_initmount1=/root/sbin earlyprintk=serial,ttyS0,115200 console=ttyS0 psmouse.proto=exps "virtme_stty_con=rows 27 cols 109 iutf8" TERM=xterm-256color virtme_chdir=home/jimc/projects/lx rootfstype=9p rootflags=version=9p2000.L,trans=virtio,access=any,msize=200M raid=noautodetect ro nokaslr dynamic_debug.verbose=3 module.dyndbg=+pmf dyndbg=+pmfl init=/bin/sh -- -c "mount -t tmpfs run /run;mkdir -p /run/virtme/guesttools;/bin/mount -n -t 9p -o ro,version=9p2000.L,trans=virtio,access=any,msize=104857600 virtme.guesttools /run/virtme/guesttools;exec /run/virtme/guesttools/virtme-init" bash-5.1# ^C bash-5.1# modprobe i915 dyndbg: 384 debug prints in module drm dyndbg: 211 debug prints in module drm_kms_helper dyndbg: 2 debug prints in module ttm dyndbg: 8 debug prints in module video dyndbg: 1821 debug prints in module i915 ********************************************************** ** NOTICE NOTICE NOTICE NOTICE NOTICE NOTICE NOTICE ** ** ** ** trace_printk() being used. Allocating extra memory. ** ** ** ** This means that this is a DEBUG kernel and it is ** ** unsafe for production use. ** ** ** ** If you see this message and you are not debugging ** ** the kernel, report this immediately to your vendor! ** ** ** ** NOTICE NOTICE NOTICE NOTICE NOTICE NOTICE NOTICE ** ********************************************************** bash-5.1# wc /proc/dynamic_debug/control 5482 47545 613066 /proc/dynamic_debug/control bash-5.1# grep =p /proc/dynamic_debug/control | wc 3 21 244 bash-5.1# QEMU: Terminated bash-5.1# wc /proc/dynamic_debug/control 3056 24089 307832 /proc/dynamic_debug/control bash-5.1# grep =pfml /proc/dynamic_debug/control | wc 0 0 0 bash-5.1# cat /proc/cmdline virtme_link_mods=/home/jimc/projects/lx/wk-next/builds/local-i915m/.virtme_mods/lib/modules/0.0.0 virtme_initmount0=/root virtme_initmount1=/root/sbin earlyprintk=serial,ttyS0,115200 console=ttyS0 psmouse.proto=exps "virtme_stty_con=rows 27 cols 109 iutf8" TERM=xterm-256color virtme_chdir=home/jimc/projects/lx rootfstype=9p rootflags=version=9p2000.L,trans=virtio,access=any,msize=200M raid=noautodetect ro nokaslr dynamic_debug.verbose=3 module.dyndbg=+pmf *.dyndbg=+pmfl init=/bin/sh -- -c "mount -t tmpfs run /run;mkdir -p /run/virtme/guesttools;/bin/mount -n -t 9p -o ro,version=9p2000.L,trans=virtio,access=any,msize=104857600 virtme.guesttools /run/virtme/guesttools;exec /run/virtme/guesttools/virtme-init" bash-5.1# grep =p /proc/dynamic_debug/control | wc 3039 23944 305800 bash-5.1# grep =pmfl /proc/dynamic_debug/control |wc 3039 23944 305800 bash-5.1# echo -p > /proc/dynamic_debug/control dyndbg: read 3 bytes from userspace < -p > dyndbg: query 0: <-p> mod:<*> dyndbg: split into words: <-p> dyndbg: op=<-> dyndbg: flags=0x1 dyndbg: *flagsp=0x0 *maskp=0xfffffffe dyndbg: parsed: func=<> file=<> module=<> format=<> lineno=0-0 dyndbg: applied: func=<> file=<> module=<> format=<> lineno=0-0 dyndbg: processed 1 queries, with 3055 matches, 0 errs bash-5.1# grep =pmfl /proc/dynamic_debug/control |wc 0 0 0 > I'm going to hold my ground and try and silence the warning, because I > think the kernel parameters interface is well defined enough (kernel > params go before the -- i.e. "these are kernel params -- these are for init" > Policy zone: DMA32 Kernel command line: virtme_link_mods=/home/jimc/projects/lx/wk-next/builds/local-i915m/.virtme_mods/lib/modules/0.0.0 virtme_initmount0=/root virtme_initmount1=/root/sbin earlyprintk=serial,ttyS0,115200 console=ttyS0 psmouse.proto=exps "virtme_stty_con=rows 27 cols 109 iutf8" TERM=xterm-256color virtme_chdir=home/jimc/projects/lx rootfstype=9p rootflags=version=9p2000.L,trans=virtio,access=any,msize=200M raid=noautodetect ro nokaslr dynamic_debug.verbose=3 module.dyndbg=+pmf init=/bin/sh -- -c "mount -t tmpfs run /run;mkdir -p /run/virtme/guesttools;/bin/mount -n -t 9p -o ro,version=9p2000.L,trans=virtio,access=any,msize=104857600 virtme.guesttools /run/virtme/guesttools;exec /run/virtme/guesttools/virtme-init" Unknown command line parameters: virtme_link_mods=/home/jimc/projects/lx/wk-next/builds/local-i915m/.virtme_mods/lib/modules/0.0.0 virtme_initmount0=/root virtme_initmount1=/root/sbin virtme_stty_con=rows 27 cols 109 iutf8 virtme_chdir=home/jimc/projects/lx Dentry cache hash table entries: 262144 (order: 9, 2097152 bytes, linear) in the above, I see unknown complaints about items after the -- But I agree the warning is misleading. > With that in mind I'll make a v2 series out of this doing: > 1. Clean up the doc to show intended usage on cli, something like > (params.dyndbg="+p" sys.dyndbg="+p" dyndbg="file init/main.c +p #cmt ; func pc87360_init_device +p") how about 1 builtin, 1 loadable, and use a wildcard somewhere, to hint that theres more than 2 ways to make a "group" > 2. Remove ddebug_query per Jason's approval sorry for not keeping up - is the problem the static storage ? is there a way to fix that for all setup= stuff ? if you could make ddebug_query into an alias for dyndbg it might fix the narrow case. > 3. Silence the param with this patch here, with a commit message > updated explaining why dyndbg= needs to be able to process the whole > cli, per Jason > > Please lemme know if there are any strong objections in the meantime and > thanks for the feedback! > > > > > > I agree that dyndbg right now (both dyndbg= and $module.dyndbg=) are > > > "fake" params, but I'd like to remove that message for dyndbg= as it is > > > misleading. The code for module loading luckily already handles it all > > > properly and doesn't warn on proper usage: s/luckily/serendipitously/ - the way fake just worked was an aha for me. just dont kill the fake-ness while quieting the warning thanks JIm ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] dyndbg: make dyndbg a known cli param 2021-09-11 3:04 ` jim.cromie @ 2021-09-13 20:10 ` Andrew Halaney 0 siblings, 0 replies; 10+ messages in thread From: Andrew Halaney @ 2021-09-13 20:10 UTC (permalink / raw) To: jim.cromie; +Cc: Jason Baron, LKML On Fri, Sep 10, 2021 at 09:04:20PM -0600, jim.cromie@gmail.com wrote: > > > > So converting dyndbg= to act like ddebug_query= won't work because > > $module.dyndbg="+p" should work if $module is builtin or a module > > (settles my open discussion with Jason). > > > > they can both work, because the dot. > > \w+.\w+ is separate, which is why it works for loadable modules. > > and Ive just re-verified that these both ( bare/global and after $mod. ) work > > dyndbg=+pmfl > turns on all builtins Yeah, you are totally right, my head clearly was not working well Friday afternoon. Although I think converting dyndbg= to behave more like ddebug_query wrt copying the string is pointless, since you've shown quite well here that we need to process the whole command line when we start up dynamic debug to catch any $module.dyndbg= usage that is actually builtin currently. So you've settled that for me, I think the noop approach here is best, but will update commit message to reflect reality. > > [ 128.341650] 8250:serial8250_interrupt:140: serial8250_interrupt(4): end > [ 128.344996] 8250:serial8250_interrupt:140: serial8250_interrupt(4): end > ^C > bash-5.1# echo -p > /proc/dynamic_debug/control > dyndbg: read 3 bytes from userspace < > -p > > > dyndbg: query 0: <-p> mod:<*> > dyndbg: split into words: <-p> > dyndbg: op=<-> > dyndbg: flags=0x1 > dyndbg: *flagsp=0x0 *maskp=0xfffffffe > dyndbg: parsed: func=<> file=<> module=<> format=<> lineno=0-0 > dyndbg: applied: func=<> file=<> module=<> format=<> lineno=0-0 > dyndbg: processed 1 queries, with 3055 matches, 0 errs > bash-5.1# cat /proc/cmdline > virtme_link_mods=/home/jimc/projects/lx/wk-next/builds/local-i915m/.virtme_mods/lib/modules/0.0.0 > virtme_initmount0=/root virtme_initmount1=/root/sbin > earlyprintk=serial,ttyS0,115200 console=ttyS0 psmouse.proto=exps > "virtme_stty_con=rows 27 cols 109 iutf8" TERM=xterm-256color > virtme_chdir=home/jimc/projects/lx rootfstype=9p > rootflags=version=9p2000.L,trans=virtio,access=any,msize=200M > raid=noautodetect ro nokaslr dynamic_debug.verbose=3 > module.dyndbg=+pmf dyndbg=+pmfl init=/bin/sh -- -c "mount -t tmpfs run > /run;mkdir -p /run/virtme/guesttools;/bin/mount -n -t 9p -o > ro,version=9p2000.L,trans=virtio,access=any,msize=104857600 > virtme.guesttools /run/virtme/guesttools;exec > /run/virtme/guesttools/virtme-init" > bash-5.1# ^C > bash-5.1# modprobe i915 > dyndbg: 384 debug prints in module drm > dyndbg: 211 debug prints in module drm_kms_helper > dyndbg: 2 debug prints in module ttm > dyndbg: 8 debug prints in module video > dyndbg: 1821 debug prints in module i915 > > ********************************************************** > ** NOTICE NOTICE NOTICE NOTICE NOTICE NOTICE NOTICE ** > ** ** > ** trace_printk() being used. Allocating extra memory. ** > ** ** > ** This means that this is a DEBUG kernel and it is ** > ** unsafe for production use. ** > ** ** > ** If you see this message and you are not debugging ** > ** the kernel, report this immediately to your vendor! ** > ** ** > ** NOTICE NOTICE NOTICE NOTICE NOTICE NOTICE NOTICE ** > ********************************************************** > bash-5.1# wc /proc/dynamic_debug/control > 5482 47545 613066 /proc/dynamic_debug/control > bash-5.1# grep =p /proc/dynamic_debug/control | wc > 3 21 244 > bash-5.1# QEMU: Terminated > > > > bash-5.1# wc /proc/dynamic_debug/control > 3056 24089 307832 /proc/dynamic_debug/control > bash-5.1# grep =pfml /proc/dynamic_debug/control | wc > 0 0 0 > bash-5.1# cat /proc/cmdline > virtme_link_mods=/home/jimc/projects/lx/wk-next/builds/local-i915m/.virtme_mods/lib/modules/0.0.0 > virtme_initmount0=/root virtme_initmount1=/root/sbin > earlyprintk=serial,ttyS0,115200 console=ttyS0 psmouse.proto=exps > "virtme_stty_con=rows 27 cols 109 iutf8" TERM=xterm-256color > virtme_chdir=home/jimc/projects/lx rootfstype=9p > rootflags=version=9p2000.L,trans=virtio,access=any,msize=200M > raid=noautodetect ro nokaslr dynamic_debug.verbose=3 > module.dyndbg=+pmf *.dyndbg=+pmfl init=/bin/sh -- -c "mount -t tmpfs > run /run;mkdir -p /run/virtme/guesttools;/bin/mount -n -t 9p -o > ro,version=9p2000.L,trans=virtio,access=any,msize=104857600 > virtme.guesttools /run/virtme/guesttools;exec > /run/virtme/guesttools/virtme-init" > bash-5.1# grep =p /proc/dynamic_debug/control | wc > 3039 23944 305800 > bash-5.1# grep =pmfl /proc/dynamic_debug/control |wc > 3039 23944 305800 > bash-5.1# echo -p > /proc/dynamic_debug/control > dyndbg: read 3 bytes from userspace < > -p > > > dyndbg: query 0: <-p> mod:<*> > dyndbg: split into words: <-p> > dyndbg: op=<-> > dyndbg: flags=0x1 > dyndbg: *flagsp=0x0 *maskp=0xfffffffe > dyndbg: parsed: func=<> file=<> module=<> format=<> lineno=0-0 > dyndbg: applied: func=<> file=<> module=<> format=<> lineno=0-0 > dyndbg: processed 1 queries, with 3055 matches, 0 errs > bash-5.1# grep =pmfl /proc/dynamic_debug/control |wc > 0 0 0 > > > > > I'm going to hold my ground and try and silence the warning, because I > > think the kernel parameters interface is well defined enough (kernel > > params go before the -- i.e. "these are kernel params -- these are for init" > > > > > Policy zone: DMA32 > Kernel command line: > virtme_link_mods=/home/jimc/projects/lx/wk-next/builds/local-i915m/.virtme_mods/lib/modules/0.0.0 > virtme_initmount0=/root virtme_initmount1=/root/sbin > earlyprintk=serial,ttyS0,115200 console=ttyS0 psmouse.proto=exps > "virtme_stty_con=rows 27 cols 109 iutf8" TERM=xterm-256color > virtme_chdir=home/jimc/projects/lx rootfstype=9p > rootflags=version=9p2000.L,trans=virtio,access=any,msize=200M > raid=noautodetect ro nokaslr dynamic_debug.verbose=3 > module.dyndbg=+pmf init=/bin/sh -- -c "mount -t tmpfs run /run;mkdir > -p /run/virtme/guesttools;/bin/mount -n -t 9p -o > ro,version=9p2000.L,trans=virtio,access=any,msize=104857600 > virtme.guesttools /run/virtme/guesttools;exec > /run/virtme/guesttools/virtme-init" > Unknown command line parameters: > virtme_link_mods=/home/jimc/projects/lx/wk-next/builds/local-i915m/.virtme_mods/lib/modules/0.0.0 > virtme_initmount0=/root virtme_initmount1=/root/sbin > virtme_stty_con=rows 27 cols 109 iutf8 > virtme_chdir=home/jimc/projects/lx > Dentry cache hash table entries: 262144 (order: 9, 2097152 bytes, linear) > > > in the above, I see unknown complaints about items after the -- I'm the doofus who added the warning message output, so I definitely don't want to ignore you claiming there are complaints after the -- because there shouldn't be... but I've re-read that block 3 times now and don't see anything after the -- that's in the unknown list. Can you please highlight it for me? I'd like to figure out why it is showing up if that's happening. > > But I agree the warning is misleading. > > > > > > With that in mind I'll make a v2 series out of this doing: > > 1. Clean up the doc to show intended usage on cli, something like > > (params.dyndbg="+p" sys.dyndbg="+p" dyndbg="file init/main.c +p #cmt ; func pc87360_init_device +p") > > how about 1 builtin, 1 loadable, and use a wildcard somewhere, > to hint that theres more than 2 ways to make a "group" > Sure, I'll illustrate using: builtin (with explicit comment about how this works for when params module is builtin or loadable): params.dyndbg="+p" module: kvm.dyndbg="+p" wildcard/dyndbg: dyndbg="file init/* +p #cmt ; func pc87360_init_device +p" Please let me know of any objections (or wait till v2) on that list.. not sure I hit the nail on the head so throwing that up for "pre-review" I suppose. > > 2. Remove ddebug_query per Jason's approval > > sorry for not keeping up - is the problem the static storage ? > is there a way to fix that for all setup= stuff ? > if you could make ddebug_query into an alias for dyndbg it might fix > the narrow case. > No real problem, I just looked at this code for the first time to tackle the dyndbg= warning and looked into ddebug_query a bit. After looking I realized it has been deprecated for quite a while and thought it would be good to remove it if there's no objections just to clean up house. It does eat some static storage for the string. I'm not sure if there's a great way to solve such a problem systematically, but yes ddebug_query could be made into an alias for dyndbg and function fine still for this case. If there's some objection to removing it entirely I'll do that just to save a little more space. > > > 3. Silence the param with this patch here, with a commit message > > updated explaining why dyndbg= needs to be able to process the whole > > cli, per Jason > > > > Please lemme know if there are any strong objections in the meantime and > > thanks for the feedback! > > > > > > > > > I agree that dyndbg right now (both dyndbg= and $module.dyndbg=) are > > > > "fake" params, but I'd like to remove that message for dyndbg= as it is > > > > misleading. The code for module loading luckily already handles it all > > > > properly and doesn't warn on proper usage: > > s/luckily/serendipitously/ - the way fake just worked was an aha for me. > > just dont kill the fake-ness while quieting the warning > > thanks > JIm > ^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2021-09-13 20:10 UTC | newest] Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2021-09-09 16:17 [PATCH] dyndbg: make dyndbg a known cli param Andrew Halaney 2021-09-09 21:34 ` Jason Baron 2021-09-10 14:00 ` Andrew Halaney [not found] ` <CAJfuBxzX9nEvxC1s-4uRCzLwN0=3gbFT__9vO_coEM5CrpnJng@mail.gmail.com> 2021-09-10 18:24 ` Andrew Halaney 2021-09-10 19:31 ` jim.cromie 2021-09-10 20:16 ` Andrew Halaney 2021-09-10 20:28 ` Andrew Halaney 2021-09-10 22:02 ` jim.cromie 2021-09-11 3:04 ` jim.cromie 2021-09-13 20:10 ` Andrew Halaney
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).