Re: [PATCH] LTT for 2.5.38 1/9: Core infrastructure

[Ingo Molnar]

On Sun, 22 Sep 2002, Karim Yaghmour wrote:

> >  - remove the 'event registration' and callback stuff. It just introduces
> >    unnecessery runtime overhead. Use an include file as a registry of
> >    events instead. This will simplify things greatly.
> 
> OK, basically then all the trace points call the trace driver directly.

yes. And in fact i'd suggest to not make it a driver but create a new
kernel/trace.c file - if it's a central mechanism then it should live in a
central place.

> > Why do you need a
> >    table of callbacks registered to an event? Nothing in your patches
> >    actually uses it ...
> 
> True, nothing in the patches actually uses it as this point. This was
> added with the mindset of letting other tools than LTT use the trace
> points already provided by LTT.

okay. The thing is that generic callbacks and data hooks in the task
structure are an invitation for various types of abuses - security and GPL
type abuses. People do get very nervous when seeing such stuff - eg. read
back Christoph Hellwig's comment from a few weeks ago. It's a red flag for
many people. Provide a clean and concentrated set of APIs, no callbacks,
no unnecessery hooks. I can see the technical reasons why you have added
it - it's in theory an extensible interface, but generally we tend to add
such stuff when it's needed - if it's needed at all.

> > Just use one tracing function that copies the
> >    arguments into a per-CPU ringbuffer. It's really just a few lines.
> 
> Sure, the writing of data itself is trivial. The reason you find the
> driver to be rather full is because of its need to do a couple of
> extra operations:
> - Get timestamp and use delta since begining of buffer to reduce
> trace size. (i.e. because of the rate at which traces are filled, it's
> essential to be able to cut down in the data written as much as possible).

yes - but even this one can also be solved by providing 2-3 macros that
each are hardcoded for one specific event length each - this should cover
about 90% of the events. Plus perhaps a more generic entry to handle the
longer/rarer event lengths, and the variable event length stuff.

> - Filter events according to event mask.

yes - this is handled by the event_allowed() function.

> - Copy extra data in case of some events (e.g. filenames). (We're
> working on ways to simplify this).

are you sure you want to copy filenames? File descriptor and inode numbers
ought to be enough.

> - Synchronize with trace daemon to save trace data. (A single per-CPU
> circular buffer may be useful when doing kernel devleopment, but user
> tracing often requires N buffers).
>
> In addition, because this data is available from user-space, you need to
> be able to deal with many buffers. For example, you don't want some
> random user to know everything that's happening on the entire system for
> obvious security reasons. So the tracer will need to be able to have
> per-user and per-process buffers.

in fact i have the feeling that you should not expose any of this to
ordinary users. Performance measurements are to be done by administrator
types - all this stuff has heavy memory allocation impact anyway.

in exactly which cases do you want to have multiple trace buffers? A
single (large enough if needed) buffer should be enough. This i think is
one of the core issues of your design.

	Ingo