This is the mail archive of the
systemtap@sourceware.org
mailing list for the systemtap project.
Accessing target variables in return probes (was Re: src ChangeLog tapsets.cxx)
- From: David Smith <dsmith at redhat dot com>
- To: "Stone, Joshua I" <joshua dot i dot stone at intel dot com>
- Cc: Systemtap List <systemtap at sources dot redhat dot com>
- Date: Tue, 21 Nov 2006 13:55:37 -0600
- Subject: Accessing target variables in return probes (was Re: src ChangeLog tapsets.cxx)
- References: <C56DB814FAA30B418C75310AC4BB279DF731A9@scsmsx413.amr.corp.intel.com>
Stone, Joshua I wrote:
On Monday, November 20, 2006 2:45 PM, dsmith@sourceware.org wrote:
Log message:
2006-11-20 David Smith <dsmith@redhat.com>
* tapsets.cxx
(dwarf_var_expanding_copy_visitor::visit_target_symbol): BZ
1382. Target variables can be accessed in return probes. A new
function entry probe is generated that saves the target
variables
so that they can be accesssed in the return probe.
This looks very cool! It will be nice to have complicated functionality
like this that "just works" for the end-user.
I don't know if this is still a work in progress, so I apologize if I'm
critiquing prematurely. If nothing else, this is just me jotting down
the thoughts in my head.
Well, I thought it was done, but after reading your email I realize it
isn't...
Here's a few things that I see from inspection:
* The tvar_ctr needs to be per-thread, else you get a race condition.
Consider the simple case where two threads run the same function,
interleaving counter access. (i.e., T1 enters, T2 enters, T1 returns,
T2 returns). The threads will end up reading each other's data!
Sigh. I can't believe I missed this since I sat down and wrote out a
similar situation. Actually the threads will end up getting zeros (the
default value). Since the cached parameter map uses 'tid()' as an
index, the following will happen:
tvar_cached_data[t1, 0] = $tvar (thread 1 entry)
tvar_cached_data[t2, 1] = $tvar (thread 2 entry)
x = tvar_cached_data[t1, 1] (thread 1 exit - no data at [t1, 1])
x = tvar_cached_data[t2, 0] (thread 2 exit - no data at [t2, 0])
To do per-thread counters, I think I'll need another map to store the
counter value for that thread. So, accessing cached parameter would
look something like:
tvar_cached_data[tid(), tvar_ctr[tid()]]
Does that look like it would work? Got any better ideas?
* The counter gets decremented multiple times if someone accesses a
target variable within a loop body.
Gack. I had tested the same parameter getting accessed multiple times,
but not one parameter getting accessed in a loop.
See follow-up thought below.
* The data is still hanging around in the maps after the probe is done,
and the maps are limited in the number of entries. In manually written
code I would 'delete' from the map after reading it, but I can see how
injecting a delete might be challenging. It might be easier if the map
runtime offered a read-and-delete call.
Because of the above problem of the counter getting decremented multiple
times, I'm going to have to figure out how to inject code (that gets
called once!) to decrement the counter. I might as well add a "delete"
there too.
* As a later optimization, consider merging duplicate target-var
references into a single temp call. For example, 'probe
syscall.open.return { if ($flags) print($flags) }' right now duplicates
the effort for each $flags reference.
Currently the code to get the actual parameter value gets merged to a
single function. You are correct that the parameter caching map calls
get duplicated.
This actually might be considered a requirement, since if you have two
different return probes for the same function that access the same
cached parameter, we want them to be separate (so the counters are
handled correctly).
* For the previous three, it would probably be easier if you can inject
the cleanup code at the end of the return probe. Do the map read at the
user's reference site(s), and then do a delete and ctr-- at the end.
(Beware code that uses 'next' to skip out early...)
I'm thinking that it might be possible to inject the code at the
*beginning* of the function, with the help of a temporary variable.
Something like this:
probe kernel.function("foo").return
{
{ /* start of injected code */
tvar_cache_tmp = tvar_cached_data[tid(), tvar_ctr[tid()]]
delete tvar_cached_data[tid(), tvar_ctr[tid()]--]
/* end of injected code */
}
... rest of original probe ...
}
then use 'tvar_cache_tmp' everywhere the target variable was accessed.
Josh, do you (or anyone else) know if it would be a good idea to delete
the counter map value when it reaches 0?
* A potential bug: due to the way string targets are read, only the
string pointer is being saved for the return probe. The
'kernel/user_string' call happens in the return probe. In most cases
this is probably ok, but there might be some functions where that string
is manipulated during the course of execution. The cleanest way I can
think to solve this is to extend the language to automatically treat a
target like a string -- probably using '@mystring' like we do with
positional parameters. Behind the scenes we can look at the pointer
range to figure whether to use kernel_string or user_string.
Hmm, I wonder if we could use a function for that and force users to use
it everywhere. It would be safer and easier.
Thanks for letting me brain-dump...
No, thank you for pointing out the numerous flaws in this before it
started getting used.
--
David Smith
dsmith@redhat.com
Red Hat
http://www.redhat.com
256.217.0141 (direct)
256.837.0057 (fax)