On 11/25, Andi Kleen wrote: > > > This is subjective, but personally I disagree. Contrary, imho it > > is good that tracehook hides the (simple) details. I do not understand > > why the reader of, say, do_fork() should see the contents of > > tracehook_report_clone_complete(). This will complicate the understanding. > > Someone who has to debug or review fork needs to know what's going on. > > Yes they can find out by going through inlines, but that > just costs more time and distracts from the actual problem. > > > Those people who want to understand/change fork() do not care about > > utrace/ptrace usually. > > > > And please note that it is much, much easier to change this code > > when it lives in tracehooks.h instead of sched.c/signal.c/etc. > > The problem is that when you have to trace this code when something > goes wrong the extra layer just holds you up. For debugging usually > abstraction is a bad idea. > > My experience is also that in general such extra "abstraction layers" > are frowned upon in Linux kernel code style. For example when new > vendor drivers are submitted for hardware like NICs etc, > they frequently tend to have all kinds of "abstraction layers". > Typically the first step to linuxify them is to get rid of those. > > This makes the code more readable, shorter, better to debug and read.
OK, let's try to remove these helpers. Let's take a random one, tracehook_report_exec(). The current code in search_binary_handler: if (retval >= 0) { if (depth == 0) tracehook_report_exec(fmt, bprm, regs); put_binfmt(fmt); allow_write_access(bprm->file); if (bprm->file) fput(bprm->file); bprm->file = NULL; current->did_exec = 1; proc_exec_connector(current); return retval; } becomes: if (retval >= 0) { if (depth == 0) { if (unlikely(task_utrace_flags(current) & UTRACE_EVENT(EXEC))) utrace_report_exec(fmt, bprm, regs); if (!ptrace_event(PT_TRACE_EXEC, PTRACE_EVENT_EXEC, 0) && unlikely(task_ptrace(current) & PT_PTRACED)) send_sig(SIGTRAP, current, 0); } put_binfmt(fmt); allow_write_access(bprm->file); if (bprm->file) fput(bprm->file); bprm->file = NULL; current->did_exec = 1; proc_exec_connector(current); return retval; } Cleanup? I don't think so. OK, when CONFIG_UTRACE goes away, we can kill a lot of old code, and in tracehooks too. So the code above becomes if (retval >= 0) { if (depth == 0) { if (unlikely(task_utrace_flags(current) & UTRACE_EVENT(EXEC))) utrace_report_exec(fmt, bprm, regs); } put_binfmt(fmt); allow_write_access(bprm->file); if (bprm->file) fput(bprm->file); bprm->file = NULL; current->did_exec = 1; proc_exec_connector(current); return retval; } Much better. But in this case please note that most of tracehooks just do: if (unlikely(task_utrace_flags(current) & SOME_EVENT)) utrace_report_some_event(); I really don't understand why we shouldn't have (trivial!) helpers for this. (As for naming - personally I do not care at all ;) You can argue that some tracehooks (say, exit_notify() path) can be simplified. Yes, we are going to do this. And again, when CONFIG_UTRACE goes away, we can just kill some tracehooks. Say, most of them in do_fork() path. > Because the inlines do not add anything to functionality and actually > hide what the code does, that is obfuscation. This applies to any function. As for tracehooks, they mostly hide "if (task_utrace_flags(current))" check and nothing more. > For you it might be obvious > because you've been hacking that code for quite some time, but for > someone who is not in your position that's different. Yes, this is true. Let me repeat, I know that this is subjective and I am biased. Oleg.