15:13 <irker972> systemtap: fche systemtap.git:azhang/pr13838 * release-4.3-89-g995846f21 / tapset/floatingpoint.stp: PR13838: drop unneeded formatting #defines from floatingpoint.stp

15:46 <irker972> systemtap: fche systemtap.git:azhang/pr13838 * release-4.3-90-gc37ca85dd / testsuite/buildok/floatingpoint.stp: buildok/floatingpoint.stp: make runnable

16:24 <irker972> systemtap: fche systemtap.git:azhang/pr13838 * release-4.3-91-g047fb1ab6 / doc/SystemTap_Tapset_Reference/Makefile.am doc/SystemTap_Tapset_Reference/Makefile.in doc/SystemTap_Tapset_Reference/tapsets.tmpl java/Makefile.in python/Makefile.in stap-exporter/Makefile.in tapset/floatingpoint.stp: tapset/floatingpoint.stp: add to tapset reference guide

18:16 <kerneltoast> good morning fche. i think i've got to the bottom of some of the issues agentzh and i are facing, and i wanted to ask you a bit about what we've found

18:16 <fche> wow

18:16 <kerneltoast> it only took a few weeks :D

18:17 <kerneltoast> so, i noticed that in the reporting pass everywhere, every engine attached to a process is iterated through, and then the callback for each engine is run via start_callback()

18:17 <kerneltoast> start_callback() takes a pointer to a report struct

18:18 <kerneltoast> everywhere start_callback() is used in a loop on every engine, it is passed a pointer to a single on-stack report struct

18:18 <kerneltoast> a prime example is in utrace_resume()

18:19 <kerneltoast> where we have the following code:

18:19 <kerneltoast> list_for_each_entry(engine, &utrace->attached, entry)

18:19 <kerneltoast> start_callback(utrace, &report, engine, task, 0);

18:19 <kerneltoast> report.action = UTRACE_RESUME;

18:20 <kerneltoast> a single report struct allocated on the stack is used for every engine's callback, but this causes some weird behavior

18:21 <kerneltoast> each callback modifies report->action to indicate what it wants the target to do

18:21 <kerneltoast> so what ends up happening is that each subsequent engine callback overwrites report->action with what it wants

18:22 <kerneltoast> and the engine at the tail of the list gets the final say on what report->action is

18:23 <kerneltoast> the problem is that not all engines agree on what they want the target to do

18:23 <kerneltoast> some engines may want UTRACE_INTERRUPT, others may want UTRACE_STOP

18:24 <fche> (keep going: dude, we should attach this whole observation set as a block comment into stp_utrace.c!!! please)

18:24 <kerneltoast> when an engine is first attached in stap_start_task_finder(), the action it requests by default is UTRACE_STOP

18:25 <kerneltoast> the engine will then request UTRACE_INTERRUPT, but things get hairy when a reporting pass occurs before that

18:25 <kerneltoast> when a brand new engine is attached, and a reporting pass occurs before it can request UTRACE_INTERRUPT, it will end up requesting UTRACE_STOP to the target process during the reporting pass

18:26 <kerneltoast> if this new engine is not at the tail of the attached list, it will just be ignored

18:26 <kerneltoast> but when such an engine is at the tail of the list, its UTRACE_STOP request will go through

18:26 <fche> (ISTR the kernel utrace had some sort of algebra system to combine the various engines' UTRACE_* judgements)

18:26 <kerneltoast> and the target process will enter utrace_stop(), upon which it will never exit until it receives a SIGKILL

18:26 <fche> (maybe the stp_utrace emulation doesn't do that part the same way as the original)

18:27 <kerneltoast> so there are three issues here i think

18:27 <kerneltoast> 1. the last engine in the attached list decides what action the target process will take, because report->action gets overwritten

18:28 <kerneltoast> 2. the default action for an attached engine is UTRACE_STOP. i don't think we want the target process to stop when an engine gets attached to it

18:29 <kerneltoast> 3. the UTRACE_STOP state currently has no natural way of being exited. I think UTRACE_STOP as a whole is an incomplete feature, and i'm not sure what needs it

18:30 <fche> UTRACE_INTERRUPT instead?

18:30 <kerneltoast> either that, or we have the engine do nothing in the reporting passes until it actually requests something

19:11 <fche> kerneltoast,

19:11 <fche> er we just kind of stopped there

19:11 <kerneltoast> i'm not sure what to do

19:11 <fche> aha

19:11 <kerneltoast> there are a lot of different ways to address these

19:11 <kerneltoast> and i have no idea which would be preferred :)

19:11 <fche> aha

19:12 <fche> re. 2, yeah I'm pretty sure a default of UTRACE_STOP is counterproductive

19:12 <fche> lemme check ancient rhel6 original-utrace code

19:13 <kerneltoast> UTRACE_STOP is also used in __stp_utrace_attach_match_filename()

19:13 <kerneltoast> not sure why

19:13 <kerneltoast> i also tried going through the git history to find incentives for these things, but all the old commits are just big code drops

19:14 <fche> https://web.elastic.org/~fche/frob-utrace/DocBook/ <<< by the way:

19:14 <fche> this is from the original utrace (well, 10 years old code :)

19:14 <fche> but it was well documented

19:15 <kerneltoast> oh wow

19:15 <fche> the stp_utrace.c code was a reimplementation of the same thing roughly

19:15 <fche> and it's possible that some edge cases weren't thought through (maybe not in the old code either)

19:15 <fche> but at least there's more background info & another reference code base

19:16 <kerneltoast> yeah this sheds light on the intent behind a lot of things

19:16 <fche> nearby one can find various generations of the original utrace implementation too, patches etc.

19:17 <fche> https://web.elastic.org/~fche/frob-utrace/2.6.37/utrace.patch <<< e.g.

19:17 <kerneltoast> from that original patch: "An attached engine does nothing by default."

19:18 <kerneltoast> so we should ignore newly attached engines in the reporting passes, until they request something, at least to follow the original intention

19:19 <kerneltoast> that can just be achieved with another bit to the flags field of the engine

19:19 <fche> they may not be able to request something unless they are reported to, not sure

19:19 <kerneltoast> ah

19:20 <fche> anyway

19:20 <fche> it may be worth reading through that for more background info before deciding on a suggestion

19:21 <kerneltoast> yeah. any idea where to find the old utrace flag algebra you mentioned?

19:21 <fche> been trying to find that with a quick glance, lemme try harder :)

19:21 <fche> it might be as simple as ... "the lowest number enum-value wins"

19:22 <kerneltoast> if (action < report->action)

19:22 <kerneltoast> report->action = action;

19:22 <kerneltoast> that's in finish_callback_report()

19:23 <fche> hm and in original utrace.patch:

19:23 <fche> +static void finish_report(struct task_struct *task, struct utrace *utrace,

19:23 <fche> + struct utrace_report *report, bool will_not_stop)

19:23 <fche> + enum utrace_resume_action resume = report->action;

19:23 <fche> +{

19:23 <fche> +

19:23 <fche> + if (resume == UTRACE_STOP)

19:23 <fche> + resume = will_not_stop ? UTRACE_REPORT : UTRACE_RESUME;

19:23 <fche> +

19:23 <fche> + if (resume < utrace->resume) {

19:23 <fche> + spin_lock(&utrace->lock);

19:23 <fche> + utrace->resume = resume;

19:25 <kerneltoast> ok so that behavior is implemented correctly. UTRACE_STOP wins as expected

19:25 <fche> yeah but it shouldn't persist

19:25 <fche> maybe some engine is not getting called again as it should be to release the stop?

19:26 <kerneltoast> it persists partially because UTRACE_RESUME gets filtered out

19:26 <kerneltoast> in __stp_utrace_attach():

19:27 <kerneltoast> if (action != UTRACE_RESUME) {

19:27 <kerneltoast> rc = utrace_control(tsk, engine, action);

19:27 <kerneltoast> i've shown you this before iirc

19:27 <kerneltoast> removing that UTRACE_RESUME filter got the stopped processes to resume again, but not always

19:30 <kerneltoast> so from what i can tell, UTRACE_STOP is missing a lot of stuff

19:31 <kerneltoast> and it's only used as the default request when an engine is attached

19:31 <kerneltoast> perhaps it should be removed?

19:32 <kerneltoast> the goal of it seems to be to stop the target process in order to safely inspect it. but with the task work api, that's not an issue

19:32 <kerneltoast> since we are always working from within the context of the target process