ktap inclusion in drivers/staging/?

ktap inclusion in drivers/staging/?

[Ingo Molnar]

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 <mingo@kernel.org>

Thanks,

	Ingo

Re: ktap inclusion in drivers/staging/?

[Steven Rostedt]

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

Re: ktap inclusion in drivers/staging/?

[Linus Torvalds]

On Thu, Oct 24, 2013 at 9:46 AM, Steven Rostedt <rostedt@goodmis.org> 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

Re: ktap inclusion in drivers/staging/?

[Greg Kroah-Hartman]

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

Re: ktap inclusion in drivers/staging/?

[Greg Kroah-Hartman]

On Thu, Oct 24, 2013 at 10:42:37AM +0100, Linus Torvalds wrote:
> On Thu, Oct 24, 2013 at 9:46 AM, Steven Rostedt <rostedt@goodmis.org> 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

Re: ktap inclusion in drivers/staging/?

[Jovi Zhangwei]

On Thu, Oct 24, 2013 at 4:46 PM, Steven Rostedt <rostedt@goodmis.org> 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.

Re: ktap inclusion in drivers/staging/?

[Steven Rostedt]

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

Re: ktap inclusion in drivers/staging/?

[Jovi Zhangwei]

Hi Ingo,

On Thu, Oct 24, 2013 at 3:58 PM, Ingo Molnar <mingo@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 <mingo@kernel.org>
>
Accept, really sorry about this mistake action, entirely my fault.

Jovi

Re: ktap inclusion in drivers/staging/?

[Jovi Zhangwei]

On Thu, Oct 24, 2013 at 5:49 PM, Greg Kroah-Hartman
<gregkh@linuxfoundation.org> 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

Re: ktap inclusion in drivers/staging/?

[Arnaldo Carvalho de Melo]

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

Re: ktap inclusion in drivers/staging/?

[Jovi Zhangwei]

On Fri, Oct 25, 2013 at 3:46 AM, Arnaldo Carvalho de Melo
<acme@ghostprotocols.net> 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

Re: ktap inclusion in drivers/staging/?

[Ingo Molnar]

* Linus Torvalds <torvalds@linux-foundation.org> wrote:

> On Thu, Oct 24, 2013 at 9:46 AM, Steven Rostedt <rostedt@goodmis.org> 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

Re: ktap inclusion in drivers/staging/?

[Ingo Molnar]

* Jovi Zhangwei <jovi.zhangwei@gmail.com> wrote:

> Hi Ingo,
> 
> On Thu, Oct 24, 2013 at 3:58 PM, Ingo Molnar <mingo@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.

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 <mingo@kernel.org>
>
> Accept, really sorry about this mistake action, entirely my fault.

Don't worry about it - your plans look very promising IMO.

Thanks,

	Ingo

Re: ktap inclusion in drivers/staging/?

[Pekka Enberg]

On Thu, Oct 24, 2013 at 9:58 AM, Ingo Molnar <mingo@kernel.org> 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

Re: ktap inclusion in drivers/staging/?

[Ingo Molnar]

* Pekka Enberg <penberg@kernel.org> wrote:

> On Thu, Oct 24, 2013 at 9:58 AM, Ingo Molnar <mingo@kernel.org> 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

Re: ktap inclusion in drivers/staging/?

[Jovi Zhangwei]

On Fri, Oct 25, 2013 at 6:15 PM, Ingo Molnar <mingo@kernel.org> wrote:
>
> * Jovi Zhangwei <jovi.zhangwei@gmail.com> 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

Re: ktap inclusion in drivers/staging/?

[Ingo Molnar]

* Jovi Zhangwei <jovi.zhangwei@gmail.com> 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

Re: Re: ktap inclusion in drivers/staging/?

[Masami Hiramatsu]

(2013/10/25 19:15), Ingo Molnar wrote:
> 
> * Jovi Zhangwei <jovi.zhangwei@gmail.com> wrote:
> 
>> Hi Ingo,
>>
>> On Thu, Oct 24, 2013 at 3:58 PM, Ingo Molnar <mingo@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.
> 
> 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: masami.hiramatsu.pt@hitachi.com

Re: Re: ktap inclusion in drivers/staging/?

[Masami Hiramatsu]

(2013/10/26 17:59), Ingo Molnar wrote:
> 
> * Jovi Zhangwei <jovi.zhangwei@gmail.com> 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: masami.hiramatsu.pt@hitachi.com

Re: Re: ktap inclusion in drivers/staging/?

[Masami Hiramatsu]

(2013/10/26 17:59), Ingo Molnar wrote:
> 
> * Jovi Zhangwei <jovi.zhangwei@gmail.com> 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: masami.hiramatsu.pt@hitachi.com

Re: Re: ktap inclusion in drivers/staging/?

[Ingo Molnar]

* Masami Hiramatsu <masami.hiramatsu.pt@hitachi.com> wrote:

> (2013/10/26 17:59), Ingo Molnar wrote:
> > 
> > * Jovi Zhangwei <jovi.zhangwei@gmail.com> 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

Re: Re: ktap inclusion in drivers/staging/?

[Jovi Zhangwei]

On Mon, Oct 28, 2013 at 8:16 PM, Masami Hiramatsu
<masami.hiramatsu.pt@hitachi.com> wrote:
> (2013/10/26 17:59), Ingo Molnar wrote:
>>
>> * Jovi Zhangwei <jovi.zhangwei@gmail.com> 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