| Message ID | 20131025210820.GA7589@redhat.com (mailing list archive) |
|---|---|
| State | Superseded, archived |
| Delegated to: | Mike Snitzer |
| Headers | show |
On Fri, Oct 25 2013 at 5:08pm -0400, Mike Snitzer <snitzer@redhat.com> wrote: > On Fri, Oct 25 2013 at 4:13pm -0400, > Alasdair G Kergon <agk@redhat.com> wrote: > > > On Thu, Oct 24, 2013 at 02:30:34PM -0400, Mike Snitzer wrote: > > > From: Morgan Mears <morgan.mears@netapp.com> > > > > > This commit includes a non-terminal policy (aka "shim") called trc that > > > may be stacked ontop of a terminal policy (e.g. mq). > > > The second shim, trc, adds function-call-level tracing to the policy > > > stack. By default, an INFO-level log message including function name > > > and parameter(s) will be generated whenever most of the cache policy > > > interface functions are called. An interface to increase or decrease > > > the verbosity of the trace output is also provided. > > > > Firstly, why not call it 'trace' in full, rather than abbreviating it to 3 > > consonants? > > We can easily rename. > > > > +++ b/drivers/md/dm-cache-policy-trc.c > > > > > +#define DM_TRC_OUT(lev, p, f, arg...) \ > > > + do { \ > > > + if (to_trc_policy(p)->trace_level >= lev) \ > > > + DMINFO("%s: " f, __func__, ## arg); \ > > > + } while (0) > > > > OK for private debugging, but I can't pretend to be very keen on this > > one going upstream in this form. Might this not need to support high > > volumes of messages sometimes? Were other upstream mechanisms > > considered? (E.g. see Documentation/trace). > > Think this would take care of it (not full-blown tracepoints like > blktrace but certainly more performant than standard printk): I folded this trace_printk change in and renamed from trc to trace, see: https://git.kernel.org/cgit/linux/kernel/git/device-mapper/linux-dm.git/commit/?h=for-next&id=f6f5db50495b16ff23a0febf38f9ee9d964b12dd -- dm-devel mailing list dm-devel@redhat.com https://www.redhat.com/mailman/listinfo/dm-devel
On Fri, 2013-10-25 at 18:44 -0400, Mike Snitzer wrote: > On Fri, Oct 25 2013 at 5:08pm -0400, > Mike Snitzer <snitzer@redhat.com> wrote: > > > On Fri, Oct 25 2013 at 4:13pm -0400, > > Alasdair G Kergon <agk@redhat.com> wrote: > > > > > On Thu, Oct 24, 2013 at 02:30:34PM -0400, Mike Snitzer wrote: > > > > From: Morgan Mears <morgan.mears@netapp.com> > > > > > > > This commit includes a non-terminal policy (aka "shim") called trc that > > > > may be stacked ontop of a terminal policy (e.g. mq). > > > > The second shim, trc, adds function-call-level tracing to the policy > > > > stack. By default, an INFO-level log message including function name > > > > and parameter(s) will be generated whenever most of the cache policy > > > > interface functions are called. An interface to increase or decrease > > > > the verbosity of the trace output is also provided. > > > > > > Firstly, why not call it 'trace' in full, rather than abbreviating it to 3 > > > consonants? > > > > We can easily rename. > > > > > > +++ b/drivers/md/dm-cache-policy-trc.c > > > > > > > +#define DM_TRC_OUT(lev, p, f, arg...) \ > > > > + do { \ > > > > + if (to_trc_policy(p)->trace_level >= lev) \ > > > > + DMINFO("%s: " f, __func__, ## arg); \ > > > > + } while (0) > > > > > > OK for private debugging, but I can't pretend to be very keen on this > > > one going upstream in this form. Might this not need to support high > > > volumes of messages sometimes? Were other upstream mechanisms > > > considered? (E.g. see Documentation/trace). > > > > Think this would take care of it (not full-blown tracepoints like > > blktrace but certainly more performant than standard printk): What's wrong with full blown tracepoints? > > I folded this trace_printk change in and renamed from trc to trace, see: > https://git.kernel.org/cgit/linux/kernel/git/device-mapper/linux-dm.git/commit/?h=for-next&id=f6f5db50495b16ff23a0febf38f9ee9d964b12dd > Please please please DO NOT USE TRACE_PRINTK! It's a debugging tool, and should never be used in the kernel proper. It was meant for users to use it for debugging and then strip it out. Now what you could do is to set up tracepoints to record the information you want, either in the function itself, or add the wrapper function too as a separate policy. Not sure if that would be any help or not. But unless this is something that is considered "debug use only" and never put into a production kernel, do not use trace_printk(). Note, tracepoints will also give you the ability to pick and choose the traces instead of doing it by trace_level. Thanks, -- Steve -- dm-devel mailing list dm-devel@redhat.com https://www.redhat.com/mailman/listinfo/dm-devel
On Fri, Nov 01 2013 at 5:39pm -0400, Steven Rostedt <srostedt@redhat.com> wrote: > > > > > > Think this would take care of it (not full-blown tracepoints like > > > blktrace but certainly more performant than standard printk): > > What's wrong with full blown tracepoints? Nothing. Just wasn't as quick to switch to while keeping the intent of the original implementation. > > I folded this trace_printk change in and renamed from trc to trace, see: > > https://git.kernel.org/cgit/linux/kernel/git/device-mapper/linux-dm.git/commit/?h=for-next&id=f6f5db50495b16ff23a0febf38f9ee9d964b12dd > > > > Please please please DO NOT USE TRACE_PRINTK! Sure, I'll have a look at using proper tracepoints. Worst case, in the near-term, we drop this type of patch for v3.13 considering how close we are to merge. > It's a debugging tool, and should never be used in the kernel proper. It > was meant for users to use it for debugging and then strip it out. > > Now what you could do is to set up tracepoints to record the information > you want, either in the function itself, or add the wrapper function too > as a separate policy. Not sure if that would be any help or not. > > But unless this is something that is considered "debug use only" and > never put into a production kernel, do not use trace_printk(). It really is debug only, but I cannot promise nobody will ever add a trace layer in production -- especially if it is as easy as using "trace+mq" vs "mq" when specifying the policy name. > Note, tracepoints will also give you the ability to pick and choose the > traces instead of doing it by trace_level. Yeap, I'm aware. Thanks for your feedback Steve. -- dm-devel mailing list dm-devel@redhat.com https://www.redhat.com/mailman/listinfo/dm-devel
Index: linux/drivers/md/dm-cache-policy-trc.c =================================================================== --- linux.orig/drivers/md/dm-cache-policy-trc.c +++ linux/drivers/md/dm-cache-policy-trc.c @@ -27,7 +27,7 @@ #define DM_TRC_OUT(lev, p, f, arg...) \ do { \ if (to_trc_policy(p)->trace_level >= lev) \ - DMINFO("%s: " f, __func__, ## arg); \ + DMTRACE("%s: " f, __func__, ## arg); \ } while (0) enum dm_trace_lev_e { Index: linux/include/linux/device-mapper.h =================================================================== --- linux.orig/include/linux/device-mapper.h +++ linux/include/linux/device-mapper.h @@ -520,6 +520,9 @@ extern struct ratelimit_state dm_ratelim "\n", ## arg); \ } while (0) +#define DMTRACE(f, arg...) \ + trace_printk(DM_NAME ": " DM_MSG_PREFIX ": " f "\n", ## arg) + #ifdef CONFIG_DM_DEBUG # define DMDEBUG(f, arg...) \ printk(KERN_DEBUG DM_NAME ": " DM_MSG_PREFIX " DEBUG: " f "\n", ## arg)