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. (!)
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. (!!)
- 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.
- In a similar vein, it creates Yet Another Debugfs ABI, instead of
trying to extend existing instrumentation.
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.
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 <[email protected]>
Thanks,
Ingo
On Thu, 2013-10-24 at 09:58 +0200, Ingo Molnar 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 feel I'm partially to blame. Jovi has sent us several emails to look
at his tree and I told him I would when I get time. What I should have
done was told him to break up the changes and send them out as a patch
series.
>
> 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.
Again, this may be partially our fault. We should have told Jovi to send
out the patches and a pointer to a git tree is not acceptable. Then we
could have had the necessary discussions required for this.
But I agree, this should not be just dumped into the staging tree until
the patches themselves have been posted and reviewed.
I'll have to NAK it too.
-- Steve
On Thu, Oct 24, 2013 at 9:46 AM, Steven Rostedt <[email protected]> wrote:
>
> But I agree, this should not be just dumped into the staging tree until
> the patches themselves have been posted and reviewed.
Btw, it's not just the commit history. The actual file layout is
terminally horrible too. The actual LWN article made it look like ktap
was just a user-space tool, and I was thinking that it was like
tools/pert/, just in staging.
But looking at the tree, it looks like parts of it is a kernel module,
and parts of it is the user space thing, and it's totally impossible
to see which is which, it's just all mixed up in the same directory
structure.
Maybe I misunderstood, but that was my reaction from a very quick look.
Linus
On Thu, Oct 24, 2013 at 04:46:00AM -0400, Steven Rostedt wrote:
> On Thu, 2013-10-24 at 09:58 +0200, Ingo Molnar 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 feel I'm partially to blame. Jovi has sent us several emails to look
> at his tree and I told him I would when I get time. What I should have
> done was told him to break up the changes and send them out as a patch
> series.
>
>
> >
> > 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.
>
> Again, this may be partially our fault. We should have told Jovi to send
> out the patches and a pointer to a git tree is not acceptable. Then we
> could have had the necessary discussions required for this.
>
> But I agree, this should not be just dumped into the staging tree until
> the patches themselves have been posted and reviewed.
>
> I'll have to NAK it too.
Ok, I've just talked to Ingo in person about this, and I will revert the
ktap code, and work with Jovi to get this merged "properly" for 3.14 or
so. I do want to audit/change the user/kernel interface in ktap as I'm
not sold on the current one, so after I get that done, I'll work to get
a set of patches created to merge this to the "real" part of the kernel.
Jovi, sorry about this, but the good news is that everyone seems to
agree that this is the way to do this properly, so the end result will
be good for everyone involved. Again, great job with creating ktap, it
seems that it fills a real need that people are happy to see
implemented.
thanks,
greg k-h
On Thu, Oct 24, 2013 at 10:42:37AM +0100, Linus Torvalds wrote:
> On Thu, Oct 24, 2013 at 9:46 AM, Steven Rostedt <[email protected]> wrote:
> >
> > But I agree, this should not be just dumped into the staging tree until
> > the patches themselves have been posted and reviewed.
>
> Btw, it's not just the commit history. The actual file layout is
> terminally horrible too. The actual LWN article made it look like ktap
> was just a user-space tool, and I was thinking that it was like
> tools/pert/, just in staging.
>
> But looking at the tree, it looks like parts of it is a kernel module,
> and parts of it is the user space thing, and it's totally impossible
> to see which is which, it's just all mixed up in the same directory
> structure.
>
> Maybe I misunderstood, but that was my reaction from a very quick look.
No, you are correct, it is a mix and mess, and will be fixed up.
greg k-h
On Thu, Oct 24, 2013 at 4:46 PM, Steven Rostedt <[email protected]> wrote:
> On Thu, 2013-10-24 at 09:58 +0200, Ingo Molnar 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 feel I'm partially to blame. Jovi has sent us several emails to look
> at his tree and I told him I would when I get time. What I should have
> done was told him to break up the changes and send them out as a patch
> series.
>
Steven, it's my fault, I'm really sorry for this.
When Greg kindly tried to help merge this into staging tree, I should
talk to you guys firstly, at that time I thought drivers/staging would
be a proper place to cleanup code or integrate with other subsystem,
I'm wrong on this, entirely my fault.
>
>>
>> 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.
>
> Again, this may be partially our fault. We should have told Jovi to send
> out the patches and a pointer to a git tree is not acceptable. Then we
> could have had the necessary discussions required for this.
>
> But I agree, this should not be just dumped into the staging tree until
> the patches themselves have been posted and reviewed.
>
> I'll have to NAK it too.
>
Accept, also with Ingo's.
I admit there have many places need to cleanup in ktap code, and there
also have a long todo list, I will finish it before start review process.
Again, really sorry for this, please forgive me this mistake.
Jovi.
On Thu, 2013-10-24 at 20:11 +0800, Jovi Zhangwei wrote:
> I admit there have many places need to cleanup in ktap code, and there
> also have a long todo list, I will finish it before start review process.
>
> Again, really sorry for this, please forgive me this mistake.
Don't sweat it ;-)
You have lots of nice ideas and we are looking forward to your work.
-- Steve
Hi Ingo,
On Thu, Oct 24, 2013 at 3:58 PM, Ingo Molnar <[email protected]> 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 <[email protected]>
>
Accept, really sorry about this mistake action, entirely my fault.
Jovi
On Thu, Oct 24, 2013 at 5:49 PM, Greg Kroah-Hartman
<[email protected]> wrote:
> On Thu, Oct 24, 2013 at 04:46:00AM -0400, Steven Rostedt wrote:
>> On Thu, 2013-10-24 at 09:58 +0200, Ingo Molnar 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 feel I'm partially to blame. Jovi has sent us several emails to look
>> at his tree and I told him I would when I get time. What I should have
>> done was told him to break up the changes and send them out as a patch
>> series.
>>
>>
>> >
>> > 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.
>>
>> Again, this may be partially our fault. We should have told Jovi to send
>> out the patches and a pointer to a git tree is not acceptable. Then we
>> could have had the necessary discussions required for this.
>>
>> But I agree, this should not be just dumped into the staging tree until
>> the patches themselves have been posted and reviewed.
>>
>> I'll have to NAK it too.
>
> Ok, I've just talked to Ingo in person about this, and I will revert the
> ktap code, and work with Jovi to get this merged "properly" for 3.14 or
> so. I do want to audit/change the user/kernel interface in ktap as I'm
> not sold on the current one, so after I get that done, I'll work to get
> a set of patches created to merge this to the "real" part of the kernel.
>
> Jovi, sorry about this, but the good news is that everyone seems to
> agree that this is the way to do this properly, so the end result will
> be good for everyone involved. Again, great job with creating ktap, it
> seems that it fills a real need that people are happy to see
> implemented.
>
Greg, I'm the person need to say sorry, my fault, you helped me a lot.
I will cleanup the code before sending patch series to review.
Jovi
Em Thu, Oct 24, 2013 at 08:11:36PM +0800, Jovi Zhangwei escreveu:
> I admit there have many places need to cleanup in ktap code, and there
> also have a long todo list, I will finish it before start review process.
>
> Again, really sorry for this, please forgive me this mistake.
Just go eroding it, try to figure out things that could be useful on its
own, find the right place in the tree, send a flow of small self
contained patches that comes with tooling that uses whatever new kernel
feature that is added.
I think that even if you just add a 'perf test' entry that shows how the
new feature should be used by tooling, exercising it and checking its
results, so that every time people try 'perf test' it gets re verified,
that would be ok when no other tools/ living source code exercises it.
- Arnaldo
On Fri, Oct 25, 2013 at 3:46 AM, Arnaldo Carvalho de Melo
<[email protected]> wrote:
> Em Thu, Oct 24, 2013 at 08:11:36PM +0800, Jovi Zhangwei escreveu:
>> I admit there have many places need to cleanup in ktap code, and there
>> also have a long todo list, I will finish it before start review process.
>>
>> Again, really sorry for this, please forgive me this mistake.
>
> Just go eroding it, try to figure out things that could be useful on its
> own, find the right place in the tree, send a flow of small self
> contained patches that comes with tooling that uses whatever new kernel
> feature that is added.
>
> I think that even if you just add a 'perf test' entry that shows how the
> new feature should be used by tooling, exercising it and checking its
> results, so that every time people try 'perf test' it gets re verified,
> that would be ok when no other tools/ living source code exercises it.
>
Nice, I will take some time to look at it.
Thank you.
Jovi
* Linus Torvalds <[email protected]> wrote:
> On Thu, Oct 24, 2013 at 9:46 AM, Steven Rostedt <[email protected]> wrote:
> >
> > But I agree, this should not be just dumped into the staging
> > tree until the patches themselves have been posted and reviewed.
>
> Btw, it's not just the commit history. The actual file layout is
> terminally horrible too. The actual LWN article made it look like
> ktap was just a user-space tool, and I was thinking that it was
> like tools/pert/, just in staging.
>
> But looking at the tree, it looks like parts of it is a kernel
> module, and parts of it is the user space thing, and it's totally
> impossible to see which is which, it's just all mixed up in the
> same directory structure.
>
> Maybe I misunderstood, but that was my reaction from a very quick
> look.
Yes, that's what I saw as well. I'd be less worried about it all if
it was tooling alone, but this is actually mostly kernel side code,
which was not apparent at all to me either, from the structure of
it.
Thanks,
Ingo
* Jovi Zhangwei <[email protected]> wrote:
> Hi Ingo,
>
> On Thu, Oct 24, 2013 at 3:58 PM, Ingo Molnar <[email protected]> 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.
Thanks!
> > 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.
Okay, cool! I've also Cc:-ed Masami, who was also very receptive in
person of the idea to merge this kind of scripting into perf probe.
(or perf ktap, whichever structure makes most sense from the end
user POV.)
> > Nacked-by: Ingo Molnar <[email protected]>
>
> Accept, really sorry about this mistake action, entirely my fault.
Don't worry about it - your plans look very promising IMO.
Thanks,
Ingo
On Thu, Oct 24, 2013 at 9:58 AM, Ingo Molnar <[email protected]> wrote:
> - 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.
I really hope we don't end up wit 'perf ktap'. As a user, I really don't want
to know what the underlying mechanism is nor learn the command line
idiosyncrasies, I just want to 'perf trace'.
Pekka
* Pekka Enberg <[email protected]> wrote:
> On Thu, Oct 24, 2013 at 9:58 AM, Ingo Molnar <[email protected]> wrote:
> > - 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.
>
> I really hope we don't end up wit 'perf ktap'. As a user, I really
> don't want to know what the underlying mechanism is nor learn the
> command line idiosyncrasies, I just want to 'perf trace'.
Agreed.
Thanks,
Ingo
On Fri, Oct 25, 2013 at 6:15 PM, Ingo Molnar <[email protected]> wrote:
>
> * Jovi Zhangwei <[email protected]> wrote:
>
>> > - 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.
>
> Okay, cool! I've also Cc:-ed Masami, who was also very receptive in
> person of the idea to merge this kind of scripting into perf probe.
>
> (or perf ktap, whichever structure makes most sense from the end
> user POV.)
>
Thanks. An addition question I want to discuss in here is the ktap code
structure layout in first patch series, this don't need to dig out any ktap
design detail, so we can make agreement on this point, and ease for me
to prepare patch series.
Do I need to prepare patchset target on staging tree or "real" part of kernel?
If target on driver/staging/ktap, then kernel code and userspace code still
need to locate at same directory, that many people may don't like it.
Target on "real" part kernel?
- include/trace/ktap (header file common used by interpreter and
userspace compiler)
- kernel/trace/ktap (interpreter code, ktapvm, pure kernel module)
- tools/perf/ktap?(userspace compiler code)
As I also agree integrating ktap and perf together, two subsystem can share
many codes, so it's better putting ktap userspace into perf directory.
Whatever the name calling of perf and ktap integrating, ktap still would be
a single directory, and could be compile into a standalone binary ktap.
How about this sounds?
Thanks.
Jovi
* Jovi Zhangwei <[email protected]> wrote:
> Thanks. An addition question I want to discuss in here is the ktap
> code structure layout in first patch series, this don't need to
> dig out any ktap design detail, so we can make agreement on this
> point, and ease for me to prepare patch series.
>
> Do I need to prepare patchset target on staging tree or "real"
> part of kernel? [...]
I'd suggest adding it to the core, i.e. kernel/tracing/ and
kernel/trace/trace_events_filter.c in particular which includes the
current filter script interpreter.
(Please also make sure that the Lua copyright notices get carried
over properly.)
> [...] If target on driver/staging/ktap, then kernel code and
> userspace code still need to locate at same directory, that many
> people may don't like it.
>
> Target on "real" part kernel? - include/trace/ktap (header file
> common used by interpreter and userspace compiler) -
> kernel/trace/ktap (interpreter code, ktapvm, pure kernel module) -
> tools/perf/ktap?(userspace compiler code)
> As I also agree integrating ktap and perf together, two
> subsystem can share many codes, so it's better putting ktap
> userspace into perf directory.
Once there's a more split-out submission it will be easier to see
what belongs where. I agree with Pekka that for the user the UI
should be integrated and obvious.
I'd also like there to be a natural 'extract the script'
functionality from an installed tap script. This gives more
flexibiliy and improves security as well: no hidden, binary-only
crap, every script installed on a running system should be
extractable in source form, should be reviewable and modifiable.
Thanks,
Ingo
(2013/10/25 19:15), Ingo Molnar wrote:
>
> * Jovi Zhangwei <[email protected]> wrote:
>
>> Hi Ingo,
>>
>> On Thu, Oct 24, 2013 at 3:58 PM, Ingo Molnar <[email protected]> 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.
>
> Thanks!
>
>>> 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.
Hmm, IMHO, the current simple filter itself is not needed to be merged
at least ftrace side, since the ktap filter requires userspace compiler
on the other hand ftrace does it directly by debugfs.
Perhaps, after the bytecode generator and JIT compiler is introduced,
we can pass filter rules to the generator via debugfs.
>>> - 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.
>
> Okay, cool! I've also Cc:-ed Masami, who was also very receptive in
> person of the idea to merge this kind of scripting into perf probe.
Yeah, that is what I recommend him. If ktap uses directly perf
tools, it will simplify its design (compared with systemtap...)
Thank you,
--
Masami HIRAMATSU
IT Management Research Dept. Linux Technology Center
Hitachi, Ltd., Yokohama Research Laboratory
E-mail: [email protected]
(2013/10/26 17:59), Ingo Molnar wrote:
>
> * Jovi Zhangwei <[email protected]> wrote:
>
>> Thanks. An addition question I want to discuss in here is the ktap
>> code structure layout in first patch series, this don't need to
>> dig out any ktap design detail, so we can make agreement on this
>> point, and ease for me to prepare patch series.
>>
>> Do I need to prepare patchset target on staging tree or "real"
>> part of kernel? [...]
>
> I'd suggest adding it to the core, i.e. kernel/tracing/ and
> kernel/trace/trace_events_filter.c in particular which includes the
> current filter script interpreter.
It means we'll need to put Lua compiler in the kernel...
I just recommend to put the ktap *on* the ftrace or perf. Not directly
integrate it. Bytecode interpreter is good, limited fomula parser is also
good, but IMHO, integrating complete lua compiler into the kernel looks
crazy.
I think it is just enough to include lua compiler as a tool in the kernel.
> (Please also make sure that the Lua copyright notices get carried
> over properly.)
>
>> [...] If target on driver/staging/ktap, then kernel code and
>> userspace code still need to locate at same directory, that many
>> people may don't like it.
>>
>> Target on "real" part kernel? - include/trace/ktap (header file
>> common used by interpreter and userspace compiler) -
>> kernel/trace/ktap (interpreter code, ktapvm, pure kernel module) -
>> tools/perf/ktap?(userspace compiler code)
>> As I also agree integrating ktap and perf together, two
>> subsystem can share many codes, so it's better putting ktap
>> userspace into perf directory.
>
> Once there's a more split-out submission it will be easier to see
> what belongs where. I agree with Pekka that for the user the UI
> should be integrated and obvious.
But, what about perf script ? :)
ktap is for online scripting and perf-script is for offline scripting,
so both are worth to have, I think.
> I'd also like there to be a natural 'extract the script'
> functionality from an installed tap script. This gives more
> flexibiliy and improves security as well: no hidden, binary-only
> crap, every script installed on a running system should be
> extractable in source form, should be reviewable and modifiable.
>
Would you mean the bytecode should be decodable? or loaded with
source code in the kernel?
Thank you,
--
Masami HIRAMATSU
IT Management Research Dept. Linux Technology Center
Hitachi, Ltd., Yokohama Research Laboratory
E-mail: [email protected]
(2013/10/26 17:59), Ingo Molnar wrote:
>
> * Jovi Zhangwei <[email protected]> wrote:
>
>> Thanks. An addition question I want to discuss in here is the ktap
>> code structure layout in first patch series, this don't need to
>> dig out any ktap design detail, so we can make agreement on this
>> point, and ease for me to prepare patch series.
>>
>> Do I need to prepare patchset target on staging tree or "real"
>> part of kernel? [...]
>
> I'd suggest adding it to the core, i.e. kernel/tracing/ and
> kernel/trace/trace_events_filter.c in particular which includes the
> current filter script interpreter.
It means we'll need to put Lua compiler in the kernel...
I just recommend to put the ktap *on* the ftrace or perf. Not directly
integrate it. Bytecode interpreter is good, limited fomula parser is also
good, but IMHO, integrating complete lua compiler into the kernel looks
crazy.
I think it is just enough to include lua compiler as a tool in the kernel.
> (Please also make sure that the Lua copyright notices get carried
> over properly.)
>
>> [...] If target on driver/staging/ktap, then kernel code and
>> userspace code still need to locate at same directory, that many
>> people may don't like it.
>>
>> Target on "real" part kernel? - include/trace/ktap (header file
>> common used by interpreter and userspace compiler) -
>> kernel/trace/ktap (interpreter code, ktapvm, pure kernel module) -
>> tools/perf/ktap?(userspace compiler code)
>> As I also agree integrating ktap and perf together, two
>> subsystem can share many codes, so it's better putting ktap
>> userspace into perf directory.
>
> Once there's a more split-out submission it will be easier to see
> what belongs where. I agree with Pekka that for the user the UI
> should be integrated and obvious.
But, what about perf script ? :)
ktap is for online scripting and perf-script is for offline scripting,
so both are worth to have, I think.
> I'd also like there to be a natural 'extract the script'
> functionality from an installed tap script. This gives more
> flexibiliy and improves security as well: no hidden, binary-only
> crap, every script installed on a running system should be
> extractable in source form, should be reviewable and modifiable.
>
Would you mean the bytecode should be decodable? or loaded with
source code in the kernel?
Thank you,
--
Masami HIRAMATSU
IT Management Research Dept. Linux Technology Center
Hitachi, Ltd., Yokohama Research Laboratory
E-mail: [email protected]
* Masami Hiramatsu <[email protected]> wrote:
> (2013/10/26 17:59), Ingo Molnar wrote:
> >
> > * Jovi Zhangwei <[email protected]> wrote:
> >
> >> Thanks. An addition question I want to discuss in here is the ktap
> >> code structure layout in first patch series, this don't need to
> >> dig out any ktap design detail, so we can make agreement on this
> >> point, and ease for me to prepare patch series.
> >>
> >> Do I need to prepare patchset target on staging tree or "real"
> >> part of kernel? [...]
> >
> > I'd suggest adding it to the core, i.e. kernel/tracing/ and
> > kernel/trace/trace_events_filter.c in particular which includes the
> > current filter script interpreter.
>
> It means we'll need to put Lua compiler in the kernel...
How big is a reasonable implementation right now?
> Would you mean the bytecode should be decodable? or loaded with
> source code in the kernel?
Loaded with the source code into the kernel - like OpenGL shader
source code is loaded. (except that there's no bin-only exception.)
These are small scripts most of whom are (much) smaller than a
single page.
Thanks,
Ingo
On Mon, Oct 28, 2013 at 8:16 PM, Masami Hiramatsu
<[email protected]> wrote:
> (2013/10/26 17:59), Ingo Molnar wrote:
>>
>> * Jovi Zhangwei <[email protected]> wrote:
>>
>>> Thanks. An addition question I want to discuss in here is the ktap
>>> code structure layout in first patch series, this don't need to
>>> dig out any ktap design detail, so we can make agreement on this
>>> point, and ease for me to prepare patch series.
>>>
>>> Do I need to prepare patchset target on staging tree or "real"
>>> part of kernel? [...]
>>
>> I'd suggest adding it to the core, i.e. kernel/tracing/ and
>> kernel/trace/trace_events_filter.c in particular which includes the
>> current filter script interpreter.
>
> It means we'll need to put Lua compiler in the kernel...
> I just recommend to put the ktap *on* the ftrace or perf. Not directly
> integrate it. Bytecode interpreter is good, limited fomula parser is also
> good, but IMHO, integrating complete lua compiler into the kernel looks
> crazy.
> I think it is just enough to include lua compiler as a tool in the kernel.
>
Agree, putting lua language compiler into kernel is a crazy idea, and I
cannot find the good reason for this, IMO.
If you knows lua, they combine compiler and interpreter together, but
when I decided to start ktap, I decoupled the compiler and interpreter,
some reasons behind that decision:
- focus on interpreter(ktapvm)
we should optimize interpreter as much as we can, but if we also put
compiler into kernel, we need to open another eye on compiler, to be
careful on performance/overhead of compiler design, too crazy to
consider those issues, and not needed.
- debugging info (like vmline, CTF, dwarf)
debugging info should handle in userspace, not kernel.
- less kernel problems (aka, safety)
- future language syntax change
easy to change the compiler in userspace, also it's have a possibility
one kernel interpreter back end for different compiler.
ktap is defined as"lightweight", mainly for ktapvm. if putting compiler
into kernel, then it's not "lightweight" anymore.
Thanks.
Jovi