The tapset now includes several embedded-c functions, which require much more thorough testing (pass-5, deliberately erroneous inputs) than is currently done. The embedded-c should be stress-tested or removed.
Created attachment 4660 [details] proposed patch Here's a proposed patch that gets rid of a good bit of the embedded-C. I'd appreciate comments on it since it is a bit large and invasive. I'm also still in the process of testing it further.
Created attachment 4661 [details] full version of proc_mem.stp Here's the full version of the new proc_mem.stp, for a bit easier reading.
We should probably do the paranoid thing of checking the task flags from _stp_proc_mm() always, or never. So either make it take a task struct pointer (and pass current_task() if not given a pid), or just do the thing for "current" that we do for the _pid() cases: mm = @cast(task_current(), "task_struct", "kernel<linux/sched.h>")->mm; atomic_long_read() is always used inside a condition (CONFIG_NR_CPUS >= CONFIG_SPLIT_PTLOCK_CPUS) and the other branch of the conditional is always the same field, but then read as "non-atomic". The code would be nicer to read if the conditional check was inside the read function, that would just take the address of the field. You can also get away with just returning the ->counter field instead of doing the full atomic read since the result is not really guaranteed anyway. If you keep it as a generic public function (without _ prefixes) then it probably should move to some other tapset and have more documentation. Otherwise, please make it local/private.
(In reply to comment #3) > The code would be nicer to read if > the conditional check was inside the read function, that would just take the > address of the field. You can also get away with just returning the ->counter > field instead of doing the full atomic read since the result is not really > guaranteed anyway. If you keep it as a generic public function (without _ > prefixes) then it probably should move to some other tapset and have more > documentation. Otherwise, please make it local/private. To expand on that. A generic automic_read function could use something like if (@defined(@cast(addr, "atomic_long_t")->counter)) ...
(In reply to comment #3) > We should probably do the paranoid thing of checking the task flags from > _stp_proc_mm() always, or never. So either make it take a task struct pointer > (and pass current_task() if not given a pid), or just do the thing for "current" > that we do for the _pid() cases: > mm = @cast(task_current(), "task_struct", "kernel<linux/sched.h>")->mm; Hmm. I guess we could turn _stp_proc_mm() into something like '_stp_valid_task()' which would be an embedded-C function that would check the task's flags. > atomic_long_read() is always used inside a condition (CONFIG_NR_CPUS >= > CONFIG_SPLIT_PTLOCK_CPUS) and the other branch of the conditional is always the > same field, but then read as "non-atomic". The code would be nicer to read if > the conditional check was inside the read function, that would just take the > address of the field. You can also get away with just returning the ->counter > field instead of doing the full atomic read since the result is not really > guaranteed anyway. If you keep it as a generic public function (without _ > prefixes) then it probably should move to some other tapset and have more > documentation. Otherwise, please make it local/private. atomic_long_read() is meant to be public, used whenever needed. That's why it is in 'atomic_funcs.stp'. Since it was meant to be generic, it can't assume details like the CONFIG_SPLIT_PTLOCK_CPUS split you mentioned.
(In reply to comment #4) > (In reply to comment #3) > To expand on that. A generic automic_read function could use something like if > (@defined(@cast(addr, "atomic_long_t")->counter)) ... An 'atomic_long_t' type is meant to be opaque, only modified by the atomic_* set of functions. Bypassing them to get to the internals isn't a good idea. Different arches do different things here. For instance, on s390 the guts of atomic_read() do a 'barrier()' before returning the value.
(In reply to comment #5) > (In reply to comment #3) > > We should probably do the paranoid thing of checking the task flags from > > _stp_proc_mm() always, or never. So either make it take a task struct pointer > > (and pass current_task() if not given a pid), or just do the thing for "current" > > that we do for the _pid() cases: > > mm = @cast(task_current(), "task_struct", "kernel<linux/sched.h>")->mm; > > Hmm. I guess we could turn _stp_proc_mm() into something like > '_stp_valid_task()' which would be an embedded-C function that would check the > task's flags. Are just always return ->mm directly. When not using current the task and mm struct can "slip away" (since we neither hold the tasklist_lock, nor increase the mm_users). So the test is only paranoid, not a guarantee. > atomic_long_read() is meant to be public, used whenever needed. That's why it > is in 'atomic_funcs.stp'. Ah, it is already in its own tapset, missed that. That is good. > Since it was meant to be generic, it can't assume > details like the CONFIG_SPLIT_PTLOCK_CPUS split you mentioned. But the CONFIG_SPLIT_PTLOCK_CPUS just check how the atomic is represented, the generic function can do that (or doesn't even have to if you write it as embedded C).
(In reply to comment #6) > (In reply to comment #4) > > (In reply to comment #3) > > To expand on that. A generic automic_read function could use something like if > > (@defined(@cast(addr, "atomic_long_t")->counter)) ... > > An 'atomic_long_t' type is meant to be opaque, only modified by the atomic_* set > of functions. Bypassing them to get to the internals isn't a good idea. > Different arches do different things here. For instance, on s390 the guts of > atomic_read() do a 'barrier()' before returning the value. Depends on whether or not you want to get rid of the extra embedded C function and whether or not you really need it to be an atomic read or not. You could just read the value directly and not care about reading in the middle of an update. Since in this code we cannot/aren't getting locks on the task/mm datastructures anyway the values read could be off for other reasons already.
(In reply to comment #7) > > Since it was meant to be generic, it can't assume > > details like the CONFIG_SPLIT_PTLOCK_CPUS split you mentioned. > > But the CONFIG_SPLIT_PTLOCK_CPUS just check how the atomic is represented, the > generic function can do that (or doesn't even have to if you write it as > embedded C). Actually this is not correct (it is getting late here, sorry). The check is indeed for whether it is an atomic_long_t or a plain long. Although that might often be how things are defined (either it is an atomic or a non-atomic of the same basic type) making that generic in an atomic_read() function is impossible (since you only have the address then and not the actual type).
Created attachment 4662 [details] revised patch Here's an updated version of the patch.
Created attachment 4663 [details] full version of proc_mem.stp, v2 Full version for easier reading.
Why is _stp_valid_task() embedded-c in the v2 patch?
(In reply to comment #12) > Why is _stp_valid_task() embedded-c in the v2 patch? It could be rewritten in scrip language, but I'd have to add 3 new embedded-C functions to get at the values of _STP_PF_KTHREAD, PF_EXITING, and PF_STARTING. It seemed like a wash.
Fixed in commit d7c88bf by removing (most of) the embedded-C functions.