On Mon, Sep 22, 2008 at 02:52:09PM -0400, Mathieu Desnoyers wrote:
> > On Sat, 20 Sep 2008 09:55:48 -0400
> > Mathieu Desnoyers <[email protected]> wrote:
> >
> > > How about :
> > >
> > > trace_mark(ftrace_evname, "size %lu binary %pW",
> > > sizeof(mystruct), mystruct);
> > > or
> > > trace_mark(sched_wakeup, "target_pid %ld", task->pid);
> > >
> > > Note the namespacing with buffers being "ftrace" and "sched" here.
> > >
> > > That would encapsulate the whole
> > > - Event ID registration
> > > - Event type registration
> > > - Sending data out
> > > - Enabling the event source directly at the source
> > >
> > > We can then export the markers through a debugfs file and let userland
> > > enable them one by one and possibly connect systemtap filters on them
> > > (one table of registered filters, one table for the markers, a command
> > > file to connect/disconnect filters to/from markers).
> >
> > I would like to ask for the following from the start: have a field for
> > a longer description of the marker that describes it's usage and
> > context. Getting this there from the start is critical, because only
> > when adding the marker point do people still really remember why/what
> > (and having to type a good description also helps them to realize if
> > this is the right point or not). This can then be exposed to the user
> > so he has a standing chance of knowing what the marker is about.
> >
> > It also has a standing chance of being updated when the code changes
> > this way
> >
>
> I agree, and I think it might be required in both markers and
> tracepoints.
>
> Given that tracepoints are declared in a global header
> (DECLARE_TRACE()), I would add this kind of description here. Tracepoint
> uses within the kernel code (statements like :
> trace_sched_switch(prev, next);
> added to the scheduler) would therefore be tied to the description
> without having to contain it in the core kernel code.
>
> Markers, on the other hand, could become the "event description"
> interface which is exported to userspace. Considering that, I guess it's
> as important to let a precise description follow the markers.
>
> Mathieu
>
>
hi,
Tracepoints and markers seem to both have their place, with tracepoints
being integral to kernel users, and markers being important for
userspace. However, it seems to me like there is overlap in the
code and an extra level of indirection when markers are layered on
tracespoints. could they be merged a bit more?
What if we extended DEFINE_TRACE() to also create a
'set_marker(marker_cb)' function where 'marker_cb' has the function signature:
marker_cb(<tracepoint prototype>, *marker_probe_func);
We then also create 'register_marker_##name' function in DEFINE_TRACE(),
which allows one to regiser marker callbacks in the usual way.
Then 'marker_cb' function is then called in '__DO_TRACE' if anybody has
registered a marker (which can set the tracepoint.state appropriately).
The 'marker_cb' function then marshalls its arguemnts and passes them
through to the marker functions that were registered.
I think in this way we can simplify the tracepoints and markers by
combining them to a large extent.
thanks,
-Jason
* Jason Baron ([email protected]) wrote:
> On Mon, Sep 22, 2008 at 02:52:09PM -0400, Mathieu Desnoyers wrote:
> > > On Sat, 20 Sep 2008 09:55:48 -0400
> > > Mathieu Desnoyers <[email protected]> wrote:
> > >
> > > > How about :
> > > >
> > > > trace_mark(ftrace_evname, "size %lu binary %pW",
> > > > sizeof(mystruct), mystruct);
> > > > or
> > > > trace_mark(sched_wakeup, "target_pid %ld", task->pid);
> > > >
> > > > Note the namespacing with buffers being "ftrace" and "sched" here.
> > > >
> > > > That would encapsulate the whole
> > > > - Event ID registration
> > > > - Event type registration
> > > > - Sending data out
> > > > - Enabling the event source directly at the source
> > > >
> > > > We can then export the markers through a debugfs file and let userland
> > > > enable them one by one and possibly connect systemtap filters on them
> > > > (one table of registered filters, one table for the markers, a command
> > > > file to connect/disconnect filters to/from markers).
> > >
> > > I would like to ask for the following from the start: have a field for
> > > a longer description of the marker that describes it's usage and
> > > context. Getting this there from the start is critical, because only
> > > when adding the marker point do people still really remember why/what
> > > (and having to type a good description also helps them to realize if
> > > this is the right point or not). This can then be exposed to the user
> > > so he has a standing chance of knowing what the marker is about.
> > >
> > > It also has a standing chance of being updated when the code changes
> > > this way
> > >
> >
> > I agree, and I think it might be required in both markers and
> > tracepoints.
> >
> > Given that tracepoints are declared in a global header
> > (DECLARE_TRACE()), I would add this kind of description here. Tracepoint
> > uses within the kernel code (statements like :
> > trace_sched_switch(prev, next);
> > added to the scheduler) would therefore be tied to the description
> > without having to contain it in the core kernel code.
> >
> > Markers, on the other hand, could become the "event description"
> > interface which is exported to userspace. Considering that, I guess it's
> > as important to let a precise description follow the markers.
> >
> > Mathieu
> >
> >
>
> hi,
>
> Tracepoints and markers seem to both have their place, with tracepoints
> being integral to kernel users, and markers being important for
> userspace. However, it seems to me like there is overlap in the
> code and an extra level of indirection when markers are layered on
> tracespoints. could they be merged a bit more?
>
> What if we extended DEFINE_TRACE() to also create a
> 'set_marker(marker_cb)' function where 'marker_cb' has the function signature:
>
> marker_cb(<tracepoint prototype>, *marker_probe_func);
>
> We then also create 'register_marker_##name' function in DEFINE_TRACE(),
> which allows one to regiser marker callbacks in the usual way.
>
> Then 'marker_cb' function is then called in '__DO_TRACE' if anybody has
> registered a marker (which can set the tracepoint.state appropriately).
>
> The 'marker_cb' function then marshalls its arguemnts and passes them
> through to the marker functions that were registered.
>
> I think in this way we can simplify the tracepoints and markers by
> combining them to a large extent.
>
> thanks,
>
> -Jason
>
I think what you propose here is already in y LTTng tree in a different
form. It's a patch to markers to allow declaring a marker which enables
an associated tracepoint when enabled. This way, we can have a marker
(exposed to userspace) connecting itself automatically to a tracepoint
when enabled.
It's here :
http://git.kernel.org/?p=linux/kernel/git/compudj/linux-2.6-lttng.git;a=commitdiff;h=d52ea7c48f47a1179aee01636d515cfea4ff6ede;hp=0a7b5c02209f3582ed1369ec818a1b389bd45a09
Note that locking depends on the psrwlock patch so we can have nested
module list readers. Otherwise locking becomes _really_ messy. :-(
Mathieu
>
>
>
--
Mathieu Desnoyers
OpenPGP key fingerprint: 8CD5 52C3 8E3C 4140 715F BA06 3F25 A8FE 3BAE 9A68
On Fri, Oct 03, 2008 at 12:11:54PM -0400, Mathieu Desnoyers wrote:
> > > > > How about :
> > > > >
> > > > > trace_mark(ftrace_evname, "size %lu binary %pW",
> > > > > sizeof(mystruct), mystruct);
> > > > > or
> > > > > trace_mark(sched_wakeup, "target_pid %ld", task->pid);
> > > > >
> > > > > Note the namespacing with buffers being "ftrace" and "sched" here.
> > > > >
> > > > > That would encapsulate the whole
> > > > > - Event ID registration
> > > > > - Event type registration
> > > > > - Sending data out
> > > > > - Enabling the event source directly at the source
> > > > >
> > > > > We can then export the markers through a debugfs file and let userland
> > > > > enable them one by one and possibly connect systemtap filters on them
> > > > > (one table of registered filters, one table for the markers, a command
> > > > > file to connect/disconnect filters to/from markers).
> > > >
> > > > I would like to ask for the following from the start: have a field for
> > > > a longer description of the marker that describes it's usage and
> > > > context. Getting this there from the start is critical, because only
> > > > when adding the marker point do people still really remember why/what
> > > > (and having to type a good description also helps them to realize if
> > > > this is the right point or not). This can then be exposed to the user
> > > > so he has a standing chance of knowing what the marker is about.
> > > >
> > > > It also has a standing chance of being updated when the code changes
> > > > this way
> > > >
> > >
> > > I agree, and I think it might be required in both markers and
> > > tracepoints.
> > >
> > > Given that tracepoints are declared in a global header
> > > (DECLARE_TRACE()), I would add this kind of description here. Tracepoint
> > > uses within the kernel code (statements like :
> > > trace_sched_switch(prev, next);
> > > added to the scheduler) would therefore be tied to the description
> > > without having to contain it in the core kernel code.
> > >
> > > Markers, on the other hand, could become the "event description"
> > > interface which is exported to userspace. Considering that, I guess it's
> > > as important to let a precise description follow the markers.
> > >
> > > Mathieu
> > >
> > >
> >
> > hi,
> >
> > Tracepoints and markers seem to both have their place, with tracepoints
> > being integral to kernel users, and markers being important for
> > userspace. However, it seems to me like there is overlap in the
> > code and an extra level of indirection when markers are layered on
> > tracespoints. could they be merged a bit more?
> >
> > What if we extended DEFINE_TRACE() to also create a
> > 'set_marker(marker_cb)' function where 'marker_cb' has the function signature:
> >
> > marker_cb(<tracepoint prototype>, *marker_probe_func);
> >
> > We then also create 'register_marker_##name' function in DEFINE_TRACE(),
> > which allows one to regiser marker callbacks in the usual way.
> >
> > Then 'marker_cb' function is then called in '__DO_TRACE' if anybody has
> > registered a marker (which can set the tracepoint.state appropriately).
> >
> > The 'marker_cb' function then marshalls its arguemnts and passes them
> > through to the marker functions that were registered.
> >
> > I think in this way we can simplify the tracepoints and markers by
> > combining them to a large extent.
> >
> > thanks,
> >
> > -Jason
> >
>
> I think what you propose here is already in y LTTng tree in a different
> form. It's a patch to markers to allow declaring a marker which enables
> an associated tracepoint when enabled. This way, we can have a marker
> (exposed to userspace) connecting itself automatically to a tracepoint
> when enabled.
>
> It's here :
> http://git.kernel.org/?p=linux/kernel/git/compudj/linux-2.6-lttng.git;a=commitdiff;h=d52ea7c48f47a1179aee01636d515cfea4ff6ede;hp=0a7b5c02209f3582ed1369ec818a1b389bd45a09
>
> Note that locking depends on the psrwlock patch so we can have nested
> module list readers. Otherwise locking becomes _really_ messy. :-(
>
> Mathieu
>
That patch simplifies using markers with tracepoints and couples
markers and tracepoints much more closely. But I was proposing to make
the coupling tighter...
Couldn't 'marker_probe_register()' register the marker directly with
the tracepoint callsite? Have DEFINE_TRACE() take an additional argument
which references a marker callback funtion. That function would look
like (very loose C code):
marker_blah_callback(TPPROTO(arg1, arg2), marker_probe_func *probe,
private_data)
{
probe(private_data, "%arg1 %arg2", arg1->a, arg2->b);
}
The 'marker_blah_callback()' would be invoked from within DO_TRACE() for
each marker that has been registered with the associated tracepoint, in
a similar way to how we iterate over the tracepoint callbacks, we can
iterate over the registered markers and pass them to the
'marker_blah_callback()' function.
By associating the marker_blah_callback() in DEFINE_TRACE(), we only
need to look in one file to understand what is associated with a
particular tracepoint. I think marker.c and tracepoint.c could also be
consolidated at that point.
thanks,
-Jason
* Jason Baron ([email protected]) wrote:
> On Fri, Oct 03, 2008 at 12:11:54PM -0400, Mathieu Desnoyers wrote:
> > > > > > How about :
> > > > > >
> > > > > > trace_mark(ftrace_evname, "size %lu binary %pW",
> > > > > > sizeof(mystruct), mystruct);
> > > > > > or
> > > > > > trace_mark(sched_wakeup, "target_pid %ld", task->pid);
> > > > > >
> > > > > > Note the namespacing with buffers being "ftrace" and "sched" here.
> > > > > >
> > > > > > That would encapsulate the whole
> > > > > > - Event ID registration
> > > > > > - Event type registration
> > > > > > - Sending data out
> > > > > > - Enabling the event source directly at the source
> > > > > >
> > > > > > We can then export the markers through a debugfs file and let userland
> > > > > > enable them one by one and possibly connect systemtap filters on them
> > > > > > (one table of registered filters, one table for the markers, a command
> > > > > > file to connect/disconnect filters to/from markers).
> > > > >
> > > > > I would like to ask for the following from the start: have a field for
> > > > > a longer description of the marker that describes it's usage and
> > > > > context. Getting this there from the start is critical, because only
> > > > > when adding the marker point do people still really remember why/what
> > > > > (and having to type a good description also helps them to realize if
> > > > > this is the right point or not). This can then be exposed to the user
> > > > > so he has a standing chance of knowing what the marker is about.
> > > > >
> > > > > It also has a standing chance of being updated when the code changes
> > > > > this way
> > > > >
> > > >
> > > > I agree, and I think it might be required in both markers and
> > > > tracepoints.
> > > >
> > > > Given that tracepoints are declared in a global header
> > > > (DECLARE_TRACE()), I would add this kind of description here. Tracepoint
> > > > uses within the kernel code (statements like :
> > > > trace_sched_switch(prev, next);
> > > > added to the scheduler) would therefore be tied to the description
> > > > without having to contain it in the core kernel code.
> > > >
> > > > Markers, on the other hand, could become the "event description"
> > > > interface which is exported to userspace. Considering that, I guess it's
> > > > as important to let a precise description follow the markers.
> > > >
> > > > Mathieu
> > > >
> > > >
> > >
> > > hi,
> > >
> > > Tracepoints and markers seem to both have their place, with tracepoints
> > > being integral to kernel users, and markers being important for
> > > userspace. However, it seems to me like there is overlap in the
> > > code and an extra level of indirection when markers are layered on
> > > tracespoints. could they be merged a bit more?
> > >
> > > What if we extended DEFINE_TRACE() to also create a
> > > 'set_marker(marker_cb)' function where 'marker_cb' has the function signature:
> > >
> > > marker_cb(<tracepoint prototype>, *marker_probe_func);
> > >
> > > We then also create 'register_marker_##name' function in DEFINE_TRACE(),
> > > which allows one to regiser marker callbacks in the usual way.
> > >
> > > Then 'marker_cb' function is then called in '__DO_TRACE' if anybody has
> > > registered a marker (which can set the tracepoint.state appropriately).
> > >
> > > The 'marker_cb' function then marshalls its arguemnts and passes them
> > > through to the marker functions that were registered.
> > >
> > > I think in this way we can simplify the tracepoints and markers by
> > > combining them to a large extent.
> > >
> > > thanks,
> > >
> > > -Jason
> > >
> >
> > I think what you propose here is already in y LTTng tree in a different
> > form. It's a patch to markers to allow declaring a marker which enables
> > an associated tracepoint when enabled. This way, we can have a marker
> > (exposed to userspace) connecting itself automatically to a tracepoint
> > when enabled.
> >
> > It's here :
> > http://git.kernel.org/?p=linux/kernel/git/compudj/linux-2.6-lttng.git;a=commitdiff;h=d52ea7c48f47a1179aee01636d515cfea4ff6ede;hp=0a7b5c02209f3582ed1369ec818a1b389bd45a09
> >
> > Note that locking depends on the psrwlock patch so we can have nested
> > module list readers. Otherwise locking becomes _really_ messy. :-(
> >
> > Mathieu
> >
>
> That patch simplifies using markers with tracepoints and couples
> markers and tracepoints much more closely. But I was proposing to make
> the coupling tighter...
>
> Couldn't 'marker_probe_register()' register the marker directly with
> the tracepoint callsite? Have DEFINE_TRACE() take an additional argument
> which references a marker callback funtion. That function would look
> like (very loose C code):
>
> marker_blah_callback(TPPROTO(arg1, arg2), marker_probe_func *probe,
I don't want the tracepoints to be coupled with markers (which are a
userspace API). The other way around is fine : letting a marker
automatically enable a tracepoint makes sense, but the opposite would
tie the in-kernel API (tracepoint) to the external marker
representation, and I would like to avoid that.
And how do you plan to deal with :
TPPROTO(arg1, arg2) == void ?
C won't let you define stuff like :
blah(void, marker_probe_func *probe, void *private_data)
The devil is in the details.... ;)
Mathieu
> private_data)
> {
> probe(private_data, "%arg1 %arg2", arg1->a, arg2->b);
> }
>
> The 'marker_blah_callback()' would be invoked from within DO_TRACE() for
> each marker that has been registered with the associated tracepoint, in
> a similar way to how we iterate over the tracepoint callbacks, we can
> iterate over the registered markers and pass them to the
> 'marker_blah_callback()' function.
>
> By associating the marker_blah_callback() in DEFINE_TRACE(), we only
> need to look in one file to understand what is associated with a
> particular tracepoint. I think marker.c and tracepoint.c could also be
> consolidated at that point.
>
> thanks,
>
> -Jason
>
>
>
>
>
>
>
--
Mathieu Desnoyers
OpenPGP key fingerprint: 8CD5 52C3 8E3C 4140 715F BA06 3F25 A8FE 3BAE 9A68
On Fri, Oct 03, 2008 at 03:10:26PM -0400, Mathieu Desnoyers wrote:
> * Jason Baron ([email protected]) wrote:
> > On Fri, Oct 03, 2008 at 12:11:54PM -0400, Mathieu Desnoyers wrote:
> > > > > > > How about :
> > > > > > >
> > > > > > > trace_mark(ftrace_evname, "size %lu binary %pW",
> > > > > > > sizeof(mystruct), mystruct);
> > > > > > > or
> > > > > > > trace_mark(sched_wakeup, "target_pid %ld", task->pid);
> > > > > > >
> > > > > > > Note the namespacing with buffers being "ftrace" and "sched" here.
> > > > > > >
> > > > > > > That would encapsulate the whole
> > > > > > > - Event ID registration
> > > > > > > - Event type registration
> > > > > > > - Sending data out
> > > > > > > - Enabling the event source directly at the source
> > > > > > >
> > > > > > > We can then export the markers through a debugfs file and let userland
> > > > > > > enable them one by one and possibly connect systemtap filters on them
> > > > > > > (one table of registered filters, one table for the markers, a command
> > > > > > > file to connect/disconnect filters to/from markers).
> > > > > >
> > > > > > I would like to ask for the following from the start: have a field for
> > > > > > a longer description of the marker that describes it's usage and
> > > > > > context. Getting this there from the start is critical, because only
> > > > > > when adding the marker point do people still really remember why/what
> > > > > > (and having to type a good description also helps them to realize if
> > > > > > this is the right point or not). This can then be exposed to the user
> > > > > > so he has a standing chance of knowing what the marker is about.
> > > > > >
> > > > > > It also has a standing chance of being updated when the code changes
> > > > > > this way
> > > > > >
> > > > >
> > > > > I agree, and I think it might be required in both markers and
> > > > > tracepoints.
> > > > >
> > > > > Given that tracepoints are declared in a global header
> > > > > (DECLARE_TRACE()), I would add this kind of description here. Tracepoint
> > > > > uses within the kernel code (statements like :
> > > > > trace_sched_switch(prev, next);
> > > > > added to the scheduler) would therefore be tied to the description
> > > > > without having to contain it in the core kernel code.
> > > > >
> > > > > Markers, on the other hand, could become the "event description"
> > > > > interface which is exported to userspace. Considering that, I guess it's
> > > > > as important to let a precise description follow the markers.
> > > > >
> > > > > Mathieu
> > > > >
> > > > >
> > > >
> > > > hi,
> > > >
> > > > Tracepoints and markers seem to both have their place, with tracepoints
> > > > being integral to kernel users, and markers being important for
> > > > userspace. However, it seems to me like there is overlap in the
> > > > code and an extra level of indirection when markers are layered on
> > > > tracespoints. could they be merged a bit more?
> > > >
> > > > What if we extended DEFINE_TRACE() to also create a
> > > > 'set_marker(marker_cb)' function where 'marker_cb' has the function signature:
> > > >
> > > > marker_cb(<tracepoint prototype>, *marker_probe_func);
> > > >
> > > > We then also create 'register_marker_##name' function in DEFINE_TRACE(),
> > > > which allows one to regiser marker callbacks in the usual way.
> > > >
> > > > Then 'marker_cb' function is then called in '__DO_TRACE' if anybody has
> > > > registered a marker (which can set the tracepoint.state appropriately).
> > > >
> > > > The 'marker_cb' function then marshalls its arguemnts and passes them
> > > > through to the marker functions that were registered.
> > > >
> > > > I think in this way we can simplify the tracepoints and markers by
> > > > combining them to a large extent.
> > > >
> > > > thanks,
> > > >
> > > > -Jason
> > > >
> > >
> > > I think what you propose here is already in y LTTng tree in a different
> > > form. It's a patch to markers to allow declaring a marker which enables
> > > an associated tracepoint when enabled. This way, we can have a marker
> > > (exposed to userspace) connecting itself automatically to a tracepoint
> > > when enabled.
> > >
> > > It's here :
> > > http://git.kernel.org/?p=linux/kernel/git/compudj/linux-2.6-lttng.git;a=commitdiff;h=d52ea7c48f47a1179aee01636d515cfea4ff6ede;hp=0a7b5c02209f3582ed1369ec818a1b389bd45a09
> > >
> > > Note that locking depends on the psrwlock patch so we can have nested
> > > module list readers. Otherwise locking becomes _really_ messy. :-(
> > >
> > > Mathieu
> > >
> >
> > That patch simplifies using markers with tracepoints and couples
> > markers and tracepoints much more closely. But I was proposing to make
> > the coupling tighter...
> >
> > Couldn't 'marker_probe_register()' register the marker directly with
> > the tracepoint callsite? Have DEFINE_TRACE() take an additional argument
> > which references a marker callback funtion. That function would look
> > like (very loose C code):
> >
> > marker_blah_callback(TPPROTO(arg1, arg2), marker_probe_func *probe,
>
> I don't want the tracepoints to be coupled with markers (which are a
> userspace API). The other way around is fine : letting a marker
> automatically enable a tracepoint makes sense, but the opposite would
> tie the in-kernel API (tracepoint) to the external marker
> representation, and I would like to avoid that.
>
The interface to markers is still marker_probe_register() and
marker_probe_unregister(). I don't see how that changes with this
proposal?
> And how do you plan to deal with :
>
> TPPROTO(arg1, arg2) == void ?
>
> C won't let you define stuff like :
>
> blah(void, marker_probe_func *probe, void *private_data)
>
it'd be simple enough to pass the the noargs requirement down as an
extra argument to DO_TRACE(), and then invoke the callback with no arguments.
thanks,
-Jason
* Jason Baron ([email protected]) wrote:
> On Fri, Oct 03, 2008 at 03:10:26PM -0400, Mathieu Desnoyers wrote:
> > * Jason Baron ([email protected]) wrote:
> > > On Fri, Oct 03, 2008 at 12:11:54PM -0400, Mathieu Desnoyers wrote:
> > > > > > > > How about :
> > > > > > > >
> > > > > > > > trace_mark(ftrace_evname, "size %lu binary %pW",
> > > > > > > > sizeof(mystruct), mystruct);
> > > > > > > > or
> > > > > > > > trace_mark(sched_wakeup, "target_pid %ld", task->pid);
> > > > > > > >
> > > > > > > > Note the namespacing with buffers being "ftrace" and "sched" here.
> > > > > > > >
> > > > > > > > That would encapsulate the whole
> > > > > > > > - Event ID registration
> > > > > > > > - Event type registration
> > > > > > > > - Sending data out
> > > > > > > > - Enabling the event source directly at the source
> > > > > > > >
> > > > > > > > We can then export the markers through a debugfs file and let userland
> > > > > > > > enable them one by one and possibly connect systemtap filters on them
> > > > > > > > (one table of registered filters, one table for the markers, a command
> > > > > > > > file to connect/disconnect filters to/from markers).
> > > > > > >
> > > > > > > I would like to ask for the following from the start: have a field for
> > > > > > > a longer description of the marker that describes it's usage and
> > > > > > > context. Getting this there from the start is critical, because only
> > > > > > > when adding the marker point do people still really remember why/what
> > > > > > > (and having to type a good description also helps them to realize if
> > > > > > > this is the right point or not). This can then be exposed to the user
> > > > > > > so he has a standing chance of knowing what the marker is about.
> > > > > > >
> > > > > > > It also has a standing chance of being updated when the code changes
> > > > > > > this way
> > > > > > >
> > > > > >
> > > > > > I agree, and I think it might be required in both markers and
> > > > > > tracepoints.
> > > > > >
> > > > > > Given that tracepoints are declared in a global header
> > > > > > (DECLARE_TRACE()), I would add this kind of description here. Tracepoint
> > > > > > uses within the kernel code (statements like :
> > > > > > trace_sched_switch(prev, next);
> > > > > > added to the scheduler) would therefore be tied to the description
> > > > > > without having to contain it in the core kernel code.
> > > > > >
> > > > > > Markers, on the other hand, could become the "event description"
> > > > > > interface which is exported to userspace. Considering that, I guess it's
> > > > > > as important to let a precise description follow the markers.
> > > > > >
> > > > > > Mathieu
> > > > > >
> > > > > >
> > > > >
> > > > > hi,
> > > > >
> > > > > Tracepoints and markers seem to both have their place, with tracepoints
> > > > > being integral to kernel users, and markers being important for
> > > > > userspace. However, it seems to me like there is overlap in the
> > > > > code and an extra level of indirection when markers are layered on
> > > > > tracespoints. could they be merged a bit more?
> > > > >
> > > > > What if we extended DEFINE_TRACE() to also create a
> > > > > 'set_marker(marker_cb)' function where 'marker_cb' has the function signature:
> > > > >
> > > > > marker_cb(<tracepoint prototype>, *marker_probe_func);
> > > > >
> > > > > We then also create 'register_marker_##name' function in DEFINE_TRACE(),
> > > > > which allows one to regiser marker callbacks in the usual way.
> > > > >
> > > > > Then 'marker_cb' function is then called in '__DO_TRACE' if anybody has
> > > > > registered a marker (which can set the tracepoint.state appropriately).
> > > > >
> > > > > The 'marker_cb' function then marshalls its arguemnts and passes them
> > > > > through to the marker functions that were registered.
> > > > >
> > > > > I think in this way we can simplify the tracepoints and markers by
> > > > > combining them to a large extent.
> > > > >
> > > > > thanks,
> > > > >
> > > > > -Jason
> > > > >
> > > >
> > > > I think what you propose here is already in y LTTng tree in a different
> > > > form. It's a patch to markers to allow declaring a marker which enables
> > > > an associated tracepoint when enabled. This way, we can have a marker
> > > > (exposed to userspace) connecting itself automatically to a tracepoint
> > > > when enabled.
> > > >
> > > > It's here :
> > > > http://git.kernel.org/?p=linux/kernel/git/compudj/linux-2.6-lttng.git;a=commitdiff;h=d52ea7c48f47a1179aee01636d515cfea4ff6ede;hp=0a7b5c02209f3582ed1369ec818a1b389bd45a09
> > > >
> > > > Note that locking depends on the psrwlock patch so we can have nested
> > > > module list readers. Otherwise locking becomes _really_ messy. :-(
> > > >
> > > > Mathieu
> > > >
> > >
> > > That patch simplifies using markers with tracepoints and couples
> > > markers and tracepoints much more closely. But I was proposing to make
> > > the coupling tighter...
> > >
> > > Couldn't 'marker_probe_register()' register the marker directly with
> > > the tracepoint callsite? Have DEFINE_TRACE() take an additional argument
> > > which references a marker callback funtion. That function would look
> > > like (very loose C code):
> > >
> > > marker_blah_callback(TPPROTO(arg1, arg2), marker_probe_func *probe,
> >
> > I don't want the tracepoints to be coupled with markers (which are a
> > userspace API). The other way around is fine : letting a marker
> > automatically enable a tracepoint makes sense, but the opposite would
> > tie the in-kernel API (tracepoint) to the external marker
> > representation, and I would like to avoid that.
> >
>
> The interface to markers is still marker_probe_register() and
> marker_probe_unregister(). I don't see how that changes with this
> proposal?
>
"Have DEFINE_TRACE() take an additional argument which references a
marker callback funtion." -> it would tie the tracepoint definition to a
marker. Or am I misunderstanding something ?
Mathieu
>
> > And how do you plan to deal with :
> >
> > TPPROTO(arg1, arg2) == void ?
> >
> > C won't let you define stuff like :
> >
> > blah(void, marker_probe_func *probe, void *private_data)
> >
>
> it'd be simple enough to pass the the noargs requirement down as an
> extra argument to DO_TRACE(), and then invoke the callback with no arguments.
>
> thanks,
>
> -Jason
>
>
--
Mathieu Desnoyers
OpenPGP key fingerprint: 8CD5 52C3 8E3C 4140 715F BA06 3F25 A8FE 3BAE 9A68
On Fri, Oct 03, 2008 at 03:56:40PM -0400, Mathieu Desnoyers wrote:
> * Jason Baron ([email protected]) wrote:
> > On Fri, Oct 03, 2008 at 03:10:26PM -0400, Mathieu Desnoyers wrote:
> > > * Jason Baron ([email protected]) wrote:
> > > > On Fri, Oct 03, 2008 at 12:11:54PM -0400, Mathieu Desnoyers wrote:
> > > > > > > > > How about :
> > > > > > > > >
> > > > > > > > > trace_mark(ftrace_evname, "size %lu binary %pW",
> > > > > > > > > sizeof(mystruct), mystruct);
> > > > > > > > > or
> > > > > > > > > trace_mark(sched_wakeup, "target_pid %ld", task->pid);
> > > > > > > > >
> > > > > > > > > Note the namespacing with buffers being "ftrace" and "sched" here.
> > > > > > > > >
> > > > > > > > > That would encapsulate the whole
> > > > > > > > > - Event ID registration
> > > > > > > > > - Event type registration
> > > > > > > > > - Sending data out
> > > > > > > > > - Enabling the event source directly at the source
> > > > > > > > >
> > > > > > > > > We can then export the markers through a debugfs file and let userland
> > > > > > > > > enable them one by one and possibly connect systemtap filters on them
> > > > > > > > > (one table of registered filters, one table for the markers, a command
> > > > > > > > > file to connect/disconnect filters to/from markers).
> > > > > > > >
> > > > > > > > I would like to ask for the following from the start: have a field for
> > > > > > > > a longer description of the marker that describes it's usage and
> > > > > > > > context. Getting this there from the start is critical, because only
> > > > > > > > when adding the marker point do people still really remember why/what
> > > > > > > > (and having to type a good description also helps them to realize if
> > > > > > > > this is the right point or not). This can then be exposed to the user
> > > > > > > > so he has a standing chance of knowing what the marker is about.
> > > > > > > >
> > > > > > > > It also has a standing chance of being updated when the code changes
> > > > > > > > this way
> > > > > > > >
> > > > > > >
> > > > > > > I agree, and I think it might be required in both markers and
> > > > > > > tracepoints.
> > > > > > >
> > > > > > > Given that tracepoints are declared in a global header
> > > > > > > (DECLARE_TRACE()), I would add this kind of description here. Tracepoint
> > > > > > > uses within the kernel code (statements like :
> > > > > > > trace_sched_switch(prev, next);
> > > > > > > added to the scheduler) would therefore be tied to the description
> > > > > > > without having to contain it in the core kernel code.
> > > > > > >
> > > > > > > Markers, on the other hand, could become the "event description"
> > > > > > > interface which is exported to userspace. Considering that, I guess it's
> > > > > > > as important to let a precise description follow the markers.
> > > > > > >
> > > > > > > Mathieu
> > > > > > >
> > > > > > >
> > > > > >
> > > > > > hi,
> > > > > >
> > > > > > Tracepoints and markers seem to both have their place, with tracepoints
> > > > > > being integral to kernel users, and markers being important for
> > > > > > userspace. However, it seems to me like there is overlap in the
> > > > > > code and an extra level of indirection when markers are layered on
> > > > > > tracespoints. could they be merged a bit more?
> > > > > >
> > > > > > What if we extended DEFINE_TRACE() to also create a
> > > > > > 'set_marker(marker_cb)' function where 'marker_cb' has the function signature:
> > > > > >
> > > > > > marker_cb(<tracepoint prototype>, *marker_probe_func);
> > > > > >
> > > > > > We then also create 'register_marker_##name' function in DEFINE_TRACE(),
> > > > > > which allows one to regiser marker callbacks in the usual way.
> > > > > >
> > > > > > Then 'marker_cb' function is then called in '__DO_TRACE' if anybody has
> > > > > > registered a marker (which can set the tracepoint.state appropriately).
> > > > > >
> > > > > > The 'marker_cb' function then marshalls its arguemnts and passes them
> > > > > > through to the marker functions that were registered.
> > > > > >
> > > > > > I think in this way we can simplify the tracepoints and markers by
> > > > > > combining them to a large extent.
> > > > > >
> > > > > > thanks,
> > > > > >
> > > > > > -Jason
> > > > > >
> > > > >
> > > > > I think what you propose here is already in y LTTng tree in a different
> > > > > form. It's a patch to markers to allow declaring a marker which enables
> > > > > an associated tracepoint when enabled. This way, we can have a marker
> > > > > (exposed to userspace) connecting itself automatically to a tracepoint
> > > > > when enabled.
> > > > >
> > > > > It's here :
> > > > > http://git.kernel.org/?p=linux/kernel/git/compudj/linux-2.6-lttng.git;a=commitdiff;h=d52ea7c48f47a1179aee01636d515cfea4ff6ede;hp=0a7b5c02209f3582ed1369ec818a1b389bd45a09
> > > > >
> > > > > Note that locking depends on the psrwlock patch so we can have nested
> > > > > module list readers. Otherwise locking becomes _really_ messy. :-(
> > > > >
> > > > > Mathieu
> > > > >
> > > >
> > > > That patch simplifies using markers with tracepoints and couples
> > > > markers and tracepoints much more closely. But I was proposing to make
> > > > the coupling tighter...
> > > >
> > > > Couldn't 'marker_probe_register()' register the marker directly with
> > > > the tracepoint callsite? Have DEFINE_TRACE() take an additional argument
> > > > which references a marker callback funtion. That function would look
> > > > like (very loose C code):
> > > >
> > > > marker_blah_callback(TPPROTO(arg1, arg2), marker_probe_func *probe,
> > >
> > > I don't want the tracepoints to be coupled with markers (which are a
> > > userspace API). The other way around is fine : letting a marker
> > > automatically enable a tracepoint makes sense, but the opposite would
> > > tie the in-kernel API (tracepoint) to the external marker
> > > representation, and I would like to avoid that.
> > >
> >
> > The interface to markers is still marker_probe_register() and
> > marker_probe_unregister(). I don't see how that changes with this
> > proposal?
> >
>
> "Have DEFINE_TRACE() take an additional argument which references a
> marker callback funtion." -> it would tie the tracepoint definition to a
> marker. Or am I misunderstanding something ?
>
Not sure. Maybe the confusion is that I am really talking about two
callbacks here. First, there is a tracepoint->marker callback which is
the 'marker_blah_callback()' that I mentioned above, and is the one
which is referenced in DEFINE_TRACE(). There is also the marker->userspace
callback which is registered via something similar to marker_probe_register(),
only it is registered directly with the tracepoint.
I think this potentially better address's Arjan's concern b/c it ties
the 'tracepoint->marker' callback directly to the tracepoint. And this
'tracepoint->marker' callback function in essense documents the marker
interface for a tracepoint. And this proposal documents the interfaces
(both tracepoints and markers) all in one place.
If I'm not clear, I can prototype it if you think that would help?
thanks,
-Jason
Hi -
On Fri, Oct 03, 2008 at 03:10:26PM -0400, Mathieu Desnoyers wrote:
> [...]
> I don't want the tracepoints to be coupled with markers (which are a
> userspace API). [...]
I'm glad the discussion seems to be slowly turning toward the event
layering issues, but ... "markers being a userspace API"? That seems
to be grossly misleading terminology. It's a kernel API like anything
else being discussed here. What's different about it
(vs. tracepoints) is that it could be one of the first clients of the
unified ring buffer widget that concretely addresses the issue of
encoding abstract traceworthy events into serialized data.
(Of course, merging the lower level aspects of the marker/tracepoint
implementations is all well and good.)
- FChE