Hi Ingo, On Thu, Oct 24, 2013 at 3:58 PM, Ingo Molnar <mi...@kernel.org> wrote: > > Greg, > > I was surprised to see 'ktap' appear in the staging tree silently, > via these commits that are visible in today's staging-next: > > 2c856b9e3e06 staging: ktap: remove unused <asm/syscall.h> header file > 687b63a3bfd5 staging: ktap: update email name in MAINTAINERS > c63a164271f8 staging: ktap: add to the kernel tree > > ktap is pretty fresh instrumentation code, announced on lkml a > couple of months ago, and so far I haven't seen much technical > discussion of integrating ktap upstream, mostly I suspect because > not a _single_ patch was sent to linux-kernel for review. (!) > I accept Greg revert this in staging-next tree, It's entirely my fault, sorry.
> An announcement of a Git tree was made (which Git tree is not very > structured), and some very minimal discussion ensued, but no actual > patches were sent with an intent to merge, no technical arguments > were made in favor of merging and nothing conclusive was achieved. > > A couple of very quick (and incomplete) technical objections: > > - The Git commits in staging an absolutely unstructured, > unreviewable mess, due to a single commit adding 16 KLOCs (!) of > code: > > 80 files changed, 16376 insertions(+) > > (I looked at the ktap Git tree as well, it's not much better.) > > - Most of the kernel code comes with near zero explanations in the > code itself. I looked at the kernel code in > drivers/staging/ktap/interpreter/. I have not found a _single_ > substantial in-code comment about design details and > implementational considerations. (!!) > I will add more comments for it, also will draft a design detail in doc/ directory. > - From the little I've been able to decode I get the impression > that the design should be much more integrated into the rest of > instrumentation: the in-kernel Lua bytecode interpreter looks > interesting, it could be an intelligent upgrade (or even outright > replacement) for the current 'filter' interpreter concept we have > for tracepoints - instead of putting a parallel interpreter > implementation into the kernel. > > - In a similar fashion, it would be nice to see it integrated with > 'perf probe' or 'perf ktap', so that users can create probes from > a single place, with coherent syntax and integrated analysis > capabilities. I.e. there's no reason to not make this a > relatively pain-less yet very useful transition. > Yes, I also mentioned this in my RFC email post before, that's the reason why I use perf-like interface in ktap as much as I can, like perf-tracepoints and perf-probe, also ktap can reuse perf debuginfo handling code in future, we are on the same page at this technical point. > - In a similar vein, it creates Yet Another Debugfs ABI, instead of > trying to extend existing instrumentation. > Yes. I tried to create file in /sys/kernel/debug/tracing/ktap or use perf syscall, that's looks more reasonable, but need some patches for kernel, so independent debugfs interface was chosen in "initial stage". > Despite my criticism, I'm actually a big proponent of safe kernel > probing concepts and this code does have many of the qualities that > I always wanted the tracepoint filter code to have in the long run. > Thanks, I'm glad to hear more and more people says ktap is useful, of course the code is still need to improve. > So it does look potentially useful, but _please_ don't merge ktap > via the staging tree yet, until the code becomes reviewable, until > it gets a proper review and until the instrumentation guys (I tried > to Cc: folks who might be interested in it) ack it. > > Kernel instrumentation code should follow established procedures to > gain Acks from interested kernel maintainers. > > Just because we've made it easy to create instrumentation callbacks > and made it possible to hide it all in a separate driver doesn't > mean the whole thing should explode into zillions of disjunct, > incoherent approaches. It's not like kernel instrumentation is an > under-maintained subsystem! > > So until it's all cleared up: > > Nacked-by: Ingo Molnar <mi...@kernel.org> > Accept, really sorry about this mistake action, entirely my fault. Jovi -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
