All of lore.kernel.org
 help / color / mirror / Atom feed
* [ patch 00/21 ] support multiple, pending ddebugs at kernel-boot
@ 2011-07-11 15:21 Jim Cromie
  2011-07-11 18:30 ` Bart Van Assche
  2011-07-12 10:01 ` Bart Van Assche
  0 siblings, 2 replies; 7+ messages in thread
From: Jim Cromie @ 2011-07-11 15:21 UTC (permalink / raw)
  To: jbaron; +Cc: linux-kernel, bvanassche, joe, gregkh, gnb

now with subject line.

On Mon, Jul 11, 2011 at 1:46 AM, Jim Cromie <jim.cromie@gmail.com> wrote:
>
> This patchset extends dynamic-debug facility to allow
> use of pr_debug() within a loadable module's module_init()
> function.  Query/rules can be given on the boot-line,
> and are saved to a pending list if they cannot be applied
> immediately.  Later, when the module is being loaded, the
> pending list is scanned, and matching rules are applied.
> Thus pr_debug() calls in the module's initialization function
> are active when it is invoked.
>
>
> Changes since rev1:
>
> - rebased on top of Jasons & Joes patchset
> - fixed accidental unescape removal, noted by Bart
> - trim src-path patch checks for matching prefix before trimming
>  should now work for out-of-tree modules.
> - undid verbose newline-strip in exec-queries - Bart
> - verbose param 644, not 744 - Bart, Greg
> - whitespace - Bart
> - added 'a' flag - Jason, Bart
> - drop pending_max - Bart
>
> New Stuff, Bigger items:
>
> - lock around all list-work, pending-ct
>
> In response to Bart's locking-bug observation, I hoisted locks up to
> callers, so theyd protect all list, pending-ct manipulation.  This
> means longer hold-times, but less locking/unlocking.  Left as separate
> patch for now, partly cuz having pr_info's under lock gave me some
> heartburn.  That said, lockdep didnt complain.
>
> - 'a' pending query modification, removal
>
> Ive extended flags spec to have <match-flags>* <op> <new-flags>*
> IE, matching /[pmflta]*[+-=][pmflta]*/
>
> match-flags (optional) allows a query/rule to be more selective, which
> increases the utility of otherwise unconstrained rules.  So the
> following query matches all call-sites that are already enabled,
> adding the TID flag.
>
> $> echo " p+t " > <dbgfs>/dynamic_debug/control
>
> Further, it allows modification or deletion of currently pending
> queries:
>
> $> echo "module foo +ap" > <debugfs>/dynamic_debug/control
> $> echo "module foo +apt" > <debugfs>/dynamic_debug/control
> $> echo "module foo ap=" > <debugfs>/dynamic_debug/control
>
> 1st command adds a pending query on module foo
> 2nd command modifies it by adding a TID flag
> 3rd command deletes the pending query by setting flags to 0
>
> Note that 2,3 have exact match on the query-spec, the match-flags in 3
> specify flags required to match against the pending query; the 't'
> flag added in 2 is not required, but allowed.
>
> TODO:
>
> 1. when pending-queries is not empty, theres a hang during shutdown.
> If I delete all pending queries, shutdown works cleanly.  Ive added
> code to clear the pending-list into ddebug_remove_all_tables(), but
> its apparently not executed.  Please advise how to invoke
> ddebug_remove_all_tables() at shutdown, and whether to do it
> separately.
>
> 2. $> cat <dbfs>/dynamic_debug/pending
> This would display currently pending queries, simplifying their deletion.
>
> 3. Decide whether to keep pending on list after they apply.
> Given that user added 'a' flag, and can now delete them, its
> reasonable to leave them on pending list.  Bart's point, TBD.
>
> Notes:
>
> 1. There is a subtle asymmetry in the 'a' flag, example 1 doesnt care
> that 'p' is still enabled for the query, once 'a' is turned off, we'd
> remove it from the pending list, and same holds for any unconstrained
> flags, like t.  However ISTM this is what the user would/should expect.
>
> 2. echo " +p " > /dbg/dynamic_debug/control
>
> The above felt a little radical, but it isnt really; it works on
> mainline.  Therefore one part of the Doc is slightly misleading (last
> sentence):
>
>  The match-spec's are used to choose a subset of the known dprintk()
>  callsites to which to apply the flags-spec.  Think of them as a query
>  with implicit ANDs between each pair.  Note that an empty list of
>  match-specs is possible, but is not very useful because it will not
>  match any debug statement callsites.
>
> 3. this runs 2 separate writes.
>
> printf "module nsc_gpio +p\n module pc8736x_gpio +p\npc8736x_gpio +p\n" \
>       > /dbg/dynamic_debug/control
>
> This form of command is seen by kernel as 2 separate writes, so the
> form is not usable in boot-line in mainline; it breaks the debug
> facility (see patch 6).  Patch 3 allows multiple commands, separated
> by ';', and does work on boot-line.
>
> thanks
> Jim Cromie
>
> [PATCH 01/21] dynamic_debug: add pending flag 'a' to make pending queries explicit
> [PATCH 02/21] dynamic_debug: allow changing of dynamic_debug verbosity any time
> [PATCH 03/21] dynamic_debug: process multiple commands on a line
> [PATCH 04/21] dynamic_debug: warn when >1 of each type of match-spec is given
> [PATCH 05/21] dynamic_debug: pr_err() call should not depend upon verbosity
> [PATCH 06/21] dynamic_debug: dont kill entire facility on error parsing ddebug_query
> [PATCH 07/21] dynamic_debug: return int from ddebug_change
> [PATCH 08/21] dynamic_debug: factor show_ddebug_query out of ddebug_parse_query
> [PATCH 09/21] dynamic_debug: save_pending() saves non-matching queries for later.
> [PATCH 10/21] dynamic_debug: call apply_pending_queries from ddebug_add_module
> [PATCH 11/21] dynamic_debug: refactor query_matches_callsite out of ddebug_change
> [PATCH 12/21] dynamic_debug: document use of pending queries at boot-time
> [PATCH 13/21] dynamic_debug: require 'a' flag to explicitly mark pending queries
> [PATCH 14/21] dynamic_debug: hoist locking in ddebug_change to callers
> [PATCH 15/21] dynamic_debug: describe_flags with '=[ptmfl]*'
> [PATCH 16/21] dynamic_debug: define several levels of verbosity
> [PATCH 17/21] dynamic_debug: non-optimization - remove != NULL
> [PATCH 18/21] dynamic_debug: trim source-path prefix from dynamic_debug/control
> [PATCH 19/21] dynamic_debug: add flags filtering to flags spec handling
> [PATCH 20/21] dynamic_debug: clear pending_queries list in remove_all_tables
> [PATCH 21/21] dynamic_debug: delete pending queries
>

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [ patch 00/21 ] support multiple, pending ddebugs at kernel-boot
  2011-07-11 15:21 [ patch 00/21 ] support multiple, pending ddebugs at kernel-boot Jim Cromie
@ 2011-07-11 18:30 ` Bart Van Assche
  2011-07-11 19:06   ` Jim Cromie
  2011-07-12 10:01 ` Bart Van Assche
  1 sibling, 1 reply; 7+ messages in thread
From: Bart Van Assche @ 2011-07-11 18:30 UTC (permalink / raw)
  To: Jim Cromie; +Cc: jbaron, linux-kernel, joe, gregkh, gnb

On Mon, Jul 11, 2011 at 5:21 PM, Jim Cromie <jim.cromie@gmail.com> wrote:
> On Mon, Jul 11, 2011 at 1:46 AM, Jim Cromie <jim.cromie@gmail.com> wrote:
>>
>> This patchset extends dynamic-debug facility to allow
>> use of pr_debug() within a loadable module's module_init()
>> function.  Query/rules can be given on the boot-line,
>> and are saved to a pending list if they cannot be applied
>> immediately.  Later, when the module is being loaded, the
>> pending list is scanned, and matching rules are applied.
>> Thus pr_debug() calls in the module's initialization function
>> are active when it is invoked.
>>
>>
>> Changes since rev1:
>>
>> - rebased on top of Jasons & Joes patchset
>> - fixed accidental unescape removal, noted by Bart
>> - trim src-path patch checks for matching prefix before trimming
>>  should now work for out-of-tree modules.
>> - undid verbose newline-strip in exec-queries - Bart
>> - verbose param 644, not 744 - Bart, Greg
>> - whitespace - Bart
>> - added 'a' flag - Jason, Bart
>> - drop pending_max - Bart

As far as I can see with v2 of this patch set a query gets on the
pending list if either +a has been specified or no matches were found
the first time a query is run. Are both mechanisms necessary ? If not,
I propose to leave out the second. That will not only allow to
simplify the code somewhat but will also reduce confusion for
dynamic_printk users.

Bart.

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [ patch 00/21 ] support multiple, pending ddebugs at kernel-boot
  2011-07-11 18:30 ` Bart Van Assche
@ 2011-07-11 19:06   ` Jim Cromie
  2011-07-12  5:53     ` Bart Van Assche
  0 siblings, 1 reply; 7+ messages in thread
From: Jim Cromie @ 2011-07-11 19:06 UTC (permalink / raw)
  To: Bart Van Assche; +Cc: jbaron, linux-kernel, joe, gregkh, gnb

On Mon, Jul 11, 2011 at 12:30 PM, Bart Van Assche <bvanassche@acm.org> wrote:
> On Mon, Jul 11, 2011 at 5:21 PM, Jim Cromie <jim.cromie@gmail.com> wrote:
>> On Mon, Jul 11, 2011 at 1:46 AM, Jim Cromie <jim.cromie@gmail.com> wrote:
>>>
>>> This patchset extends dynamic-debug facility to allow
>>> use of pr_debug() within a loadable module's module_init()
>>> function.  Query/rules can be given on the boot-line,
>>> and are saved to a pending list if they cannot be applied
>>> immediately.  Later, when the module is being loaded, the
>>> pending list is scanned, and matching rules are applied.
>>> Thus pr_debug() calls in the module's initialization function
>>> are active when it is invoked.
>>>
>>>
>>> Changes since rev1:
>>>
>>> - rebased on top of Jasons & Joes patchset
>>> - fixed accidental unescape removal, noted by Bart
>>> - trim src-path patch checks for matching prefix before trimming
>>>  should now work for out-of-tree modules.
>>> - undid verbose newline-strip in exec-queries - Bart
>>> - verbose param 644, not 744 - Bart, Greg
>>> - whitespace - Bart
>>> - added 'a' flag - Jason, Bart
>>> - drop pending_max - Bart
>
> As far as I can see with v2 of this patch set a query gets on the
> pending list if either +a has been specified or no matches were found
> the first time a query is run. Are both mechanisms necessary ? If not,
> I propose to leave out the second. That will not only allow to
> simplify the code somewhat but will also reduce confusion for
> dynamic_printk users.
>

Both conditions are necessary:
- 'a' flag required,
- query not directly applicable.

root@voyage:~# echo 1 > /sys/module/dynamic_debug/parameters/verbose

root@voyage:~# echo "module nosuch +p" > /dbg/dynamic_debug/control
dynamic_debug:ddebug_proc_open: called
dynamic_debug:ddebug_proc_write: read 17 bytes from userspace
dynamic_debug:ddebug_exec_queries: query 0: "module nosuch +p
"
dynamic_debug:ddebug_tokenize: split into words: "module" "nosuch" "+p"
dynamic_debug:ddebug_parse_query: parsed q->function="(null)"
q->filename="(null)" q->module="nosuch" q->format="(null)"
q->lineno=0-0
dynamic_debug:ddebug_parse_flags:  flags_filter=0x0
 op='+'
 flags=0x1
dynamic_debug:ddebug_parse_flags: *flagsp=0x1 *maskp=0xffffffff
dynamic_debug:ddebug_exec_query: no match on: q->function="(null)"
q->filename="(null)" q->module="nosuch" q->format="(null)"
q->lineno=0-0
dynamic_debug:ddebug_exec_query: nfound 0 on q->function="(null)"
q->filename="(null)" q->module="nosuch" q->format="(null)"
q->lineno=0-0
dynamic_debug:ddebug_exec_queries: processed 1 queries, with 0 errs

root@voyage:~# cat /sys/module/dynamic_debug/parameters/pending_ct
0

root@voyage:~# echo "module nosuch +ap" > /dbg/dynamic_debug/control
dynamic_debug:ddebug_proc_open: called
dynamic_debug:ddebug_proc_write: read 18 bytes from userspace
dynamic_debug:ddebug_exec_queries: query 0: "module nosuch +ap
"
dynamic_debug:ddebug_tokenize: split into words: "module" "nosuch" "+ap"
dynamic_debug:ddebug_parse_query: parsed q->function="(null)"
q->filename="(null)" q->module="nosuch" q->format="(null)"
q->lineno=0-0
dynamic_debug:ddebug_parse_flags:  flags_filter=0x0
 op='+'
 flags=0x21
dynamic_debug:ddebug_parse_flags: *flagsp=0x21 *maskp=0xffffffff
dynamic_debug:ddebug_save_pending: add to pending:
q->function="(null)" q->filename="(null)" q->module="nosuch"
q->format="(null)" q->lineno=0-0
dynamic_debug:ddebug_save_pending: query saved as pending 1
dynamic_debug:ddebug_exec_query: nfound 0 on q->function="(null)"
q->filename="(null)" q->module="nosuch" q->format="(null)"
q->lineno=0-0
dynamic_debug:ddebug_exec_queries: processed 1 queries, with 0 errs

root@voyage:~# cat /sys/module/dynamic_debug/parameters/pending_ct
1

> Bart.
>

thanks
~jimc

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [ patch 00/21 ] support multiple, pending ddebugs at kernel-boot
  2011-07-11 19:06   ` Jim Cromie
@ 2011-07-12  5:53     ` Bart Van Assche
  2011-07-12  7:39       ` Jim Cromie
  0 siblings, 1 reply; 7+ messages in thread
From: Bart Van Assche @ 2011-07-12  5:53 UTC (permalink / raw)
  To: Jim Cromie; +Cc: jbaron, linux-kernel, joe, gregkh, gnb

On Mon, Jul 11, 2011 at 9:06 PM, Jim Cromie <jim.cromie@gmail.com> wrote:
> On Mon, Jul 11, 2011 at 12:30 PM, Bart Van Assche <bvanassche@acm.org> wrote:
>> As far as I can see with v2 of this patch set a query gets on the
>> pending list if either +a has been specified or no matches were found
>> the first time a query is run. Are both mechanisms necessary ? If not,
>> I propose to leave out the second. That will not only allow to
>> simplify the code somewhat but will also reduce confusion for
>> dynamic_printk users.
>>
>
> Both conditions are necessary:
> - 'a' flag required,
> - query not directly applicable.
>
> root@voyage:~# echo "module nosuch +p" > /dbg/dynamic_debug/control
> root@voyage:~# cat /sys/module/dynamic_debug/parameters/pending_ct
> 0
>
> root@voyage:~# echo "module nosuch +ap" > /dbg/dynamic_debug/control
> root@voyage:~# cat /sys/module/dynamic_debug/parameters/pending_ct
> 1

The above example contradicts the following code added in patch 09/21:

 	/* actually go and implement the change */
 	nfound = ddebug_change(&query, flags, mask);
+
+	pr_info("nfound %d on %s\n", nfound, show_ddebug_query(&query));
+	if (!nfound)
+		return ddebug_save_pending(&query, flags, mask);
+
 	return 0;

Bart.

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [ patch 00/21 ] support multiple, pending ddebugs at kernel-boot
  2011-07-12  5:53     ` Bart Van Assche
@ 2011-07-12  7:39       ` Jim Cromie
  2011-07-12  8:16         ` Bart Van Assche
  0 siblings, 1 reply; 7+ messages in thread
From: Jim Cromie @ 2011-07-12  7:39 UTC (permalink / raw)
  To: Bart Van Assche; +Cc: jbaron, linux-kernel, joe, gregkh, gnb

On Mon, Jul 11, 2011 at 11:53 PM, Bart Van Assche <bvanassche@acm.org> wrote:
> On Mon, Jul 11, 2011 at 9:06 PM, Jim Cromie <jim.cromie@gmail.com> wrote:
>> On Mon, Jul 11, 2011 at 12:30 PM, Bart Van Assche <bvanassche@acm.org> wrote:
>>> As far as I can see with v2 of this patch set a query gets on the
>>> pending list if either +a has been specified or no matches were found
>>> the first time a query is run. Are both mechanisms necessary ? If not,
>>> I propose to leave out the second. That will not only allow to
>>> simplify the code somewhat but will also reduce confusion for
>>> dynamic_printk users.
>>>
>>
>> Both conditions are necessary:
>> - 'a' flag required,
>> - query not directly applicable.
>>
>> root@voyage:~# echo "module nosuch +p" > /dbg/dynamic_debug/control
>> root@voyage:~# cat /sys/module/dynamic_debug/parameters/pending_ct
>> 0
>>
>> root@voyage:~# echo "module nosuch +ap" > /dbg/dynamic_debug/control
>> root@voyage:~# cat /sys/module/dynamic_debug/parameters/pending_ct
>> 1
>
> The above example contradicts the following code added in patch 09/21:
>
>        /* actually go and implement the change */
>        nfound = ddebug_change(&query, flags, mask);
> +
> +       pr_info("nfound %d on %s\n", nfound, show_ddebug_query(&query));
> +       if (!nfound)
> +               return ddebug_save_pending(&query, flags, mask);
> +
>        return 0;
>
> Bart.
>

In patch 13, that becomes

+	if (!nfound) {
+		if (flags & _DPRINTK_FLAGS_APPEND)
+			return ddebug_save_pending(&query, flags, mask);
+		else
+			pr_warn("no match on: %s\n",
+				show_ddebug_query(&query));
+	}
 	return 0;
 }


its altered once more, when I add filter-flags,
but that doesnt really change my assertion.

 	nfound = ddebug_change(&query, flags, mask, filter);
 	if (!nfound) {
-		if (flags & _DPRINTK_FLAGS_APPEND)
+		if (flags & _DPRINTK_FLAGS_APPEND ||
+			filter & _DPRINTK_FLAGS_APPEND)
 			rc = ddebug_save_pending(&query, flags, mask);

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [ patch 00/21 ] support multiple, pending ddebugs at kernel-boot
  2011-07-12  7:39       ` Jim Cromie
@ 2011-07-12  8:16         ` Bart Van Assche
  0 siblings, 0 replies; 7+ messages in thread
From: Bart Van Assche @ 2011-07-12  8:16 UTC (permalink / raw)
  To: Jim Cromie; +Cc: jbaron, linux-kernel, joe, gregkh, gnb

On Tue, Jul 12, 2011 at 9:39 AM, Jim Cromie <jim.cromie@gmail.com> wrote:
> On Mon, Jul 11, 2011 at 11:53 PM, Bart Van Assche <bvanassche@acm.org> wrote:
>> The above example contradicts the following code added in patch 09/21:
>>
>>        /* actually go and implement the change */
>>        nfound = ddebug_change(&query, flags, mask);
>> +
>> +       pr_info("nfound %d on %s\n", nfound, show_ddebug_query(&query));
>> +       if (!nfound)
>> +               return ddebug_save_pending(&query, flags, mask);
>> +
>>        return 0;
>
> In patch 13, that becomes
>
> +       if (!nfound) {
> +               if (flags & _DPRINTK_FLAGS_APPEND)
> +                       return ddebug_save_pending(&query, flags, mask);
> +               else
> +                       pr_warn("no match on: %s\n",
> +                               show_ddebug_query(&query));
> +       }
>        return 0;
>  }
>
>
> its altered once more, when I add filter-flags,
> but that doesn’t really change my assertion.
>
>        nfound = ddebug_change(&query, flags, mask, filter);
>        if (!nfound) {
> -               if (flags & _DPRINTK_FLAGS_APPEND)
> +               if (flags & _DPRINTK_FLAGS_APPEND ||
> +                       filter & _DPRINTK_FLAGS_APPEND)
>                        rc = ddebug_save_pending(&query, flags, mask);
>

Sorry, but I'm not sure that addressing review comments by adding
follow-up patches that modify previous patches is the proper way to
present a patch series. IMHO this patch series would be a lot easier
to review if patches that modify previous patches in the same patch
series were avoided. If you are not yet familiar with patch squashing
via git rebase --interactive, I think it's a good idea to learn more
about it first.

Also, what are the implications for bisectability if review comments
about bugs are addressed by follow-up patches instead of modifying the
patches on which the comments apply ?

Thanks,

Bart.

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [ patch 00/21 ] support multiple, pending ddebugs at kernel-boot
  2011-07-11 15:21 [ patch 00/21 ] support multiple, pending ddebugs at kernel-boot Jim Cromie
  2011-07-11 18:30 ` Bart Van Assche
@ 2011-07-12 10:01 ` Bart Van Assche
  1 sibling, 0 replies; 7+ messages in thread
From: Bart Van Assche @ 2011-07-12 10:01 UTC (permalink / raw)
  To: Jim Cromie; +Cc: jbaron, linux-kernel, joe, gregkh, gnb

On Mon, Jul 11, 2011 at 5:21 PM, Jim Cromie <jim.cromie@gmail.com> wrote:
>> TODO:
>> [ ... ]
>> 3. Decide whether to keep pending on list after they apply.
>> Given that user added 'a' flag, and can now delete them, its
>> reasonable to leave them on pending list.  Bart's point, TBD.

It's good that users can not only add items to the pending list but
that they can remove items from the pending list too. Does v2 of this
patch set provide a way to query the pending list ? If not, should
such a facility be provided ?

Bart.

^ permalink raw reply	[flat|nested] 7+ messages in thread

end of thread, other threads:[~2011-07-12 10:01 UTC | newest]

Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2011-07-11 15:21 [ patch 00/21 ] support multiple, pending ddebugs at kernel-boot Jim Cromie
2011-07-11 18:30 ` Bart Van Assche
2011-07-11 19:06   ` Jim Cromie
2011-07-12  5:53     ` Bart Van Assche
2011-07-12  7:39       ` Jim Cromie
2011-07-12  8:16         ` Bart Van Assche
2011-07-12 10:01 ` Bart Van Assche

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.