Re: [RFC] kill format from marker_set_probe

Hi Mathieu,

Mathieu Desnoyers wrote:
> Hi Jan,
> 
> Thanks for digging into this : this area effectively need some work. See
> below,
> 
> * Jan Kiszka ([email protected]) wrote:
>> Jan Kiszka wrote:
>>> Hello Mathieu,
>>>
>>> what is the idea behind asking the caller of marker_set_probe for the
>>> marker's format string? Just consistency checking?
>>>
>>> In case there is only one instance of some marker (quite common I
>>> think), this adds no value IMHO. And if there are more, I would suggest
>>> to check the format of the first instance against the others instead.
>>> See draft below.
>> OK, reflecting this again, I think I got the generic point about the
>> interface: It is intended to check instrumentation against probe module
>> in case both live in separate projects AND there is no other way of
>> validation. The check is already optional (format may be NULL), so I
>> would only suggest to add my patch in order to perform inter-marker
>> checks on marker_set_probe in any case.
>>
> 
> As you have seen, the format string is an optional argument to
> marker_set_probe. The only reason why it is still there is that I have
> no mechanism to verify that the XML event description and the probe
> module are in sync. At least, by putting the format strings in the probe
> module and checking that they match the kernel markers, I can have an
> ordered list of format strings and make the eyeball check easier. This
> is temporary and have to change.
> 
> Prior to the markers, I generated the tracing code from the XML
> description : I was then sure that the code was in sync with the events
> in the trace because of the following points :
> - gcc verified the data types passed as parameter to the tracing
>   functions.
> - The tracing functions (in header files) were generated by genevent and
>   were given a checksum
> - When the facilities (set of tracing functions) were loaded, the
>   checksum was saved in a facility load event.
> - The viewer, LTTV, checked that the facility XML matched the facility
>   load checksum.
> 
> Now, the state is the following :
> - The kernel programmer can describe a marker with a format string.
>   There is no type verification between the arguments and the format
>   string, because I extend the standard format strings. Could be fixed
>   by extending the sparse checker.

Yes, I saw this. The other issue with plain markers is the fact that
format strings can vary between markers of the same name without any
complaint during build. That's why I suggested to check this property
unconditionally when a set of markers is attached to a probe function.
But maybe this is once teachable to sparse or whatever as well.

> - A probe can be connected to the marker. It can either dynamically
>   parse the string to serialize the information (see
>   ltt/ltt-serialize.c) or "expect" the arguments in a specific order and
>   unroll them. In the second case, it is important that the probe module
>   specify the expected format string somewhere.

Yep.

> - The XML description must be kept in sync by hand with the probe
>   module. If someone has suggestion on how to describe the same
>   information found in the XML in C structure, and later on export them,
>   I am open to ideas.

Rough idea for a semi-automated generation: What about some script that
scans the whole C code for markers, extracts names and arguments with
their types, and then builds a new XML file or checks an existing one
for consistency.

There will still be some unavoidable XML editing parts: the descriptions
and special types. But that script (or interactive tool) could at least
mark missing information in the XML file and/or prompt the user to
update them.

Later on, that tool could just as well generate the required probe
module for ltt, hmm, basically making the format string a non-issue.
Should work at least for standard probe modules, not for those requiring
special serialisation. Or one passes facility descriptions down to a
generic probe module that is able to register basic facilities (i.e.
with standard serialisation) and to attach markers dynamically.

> 
> I think this fundamental issue must be addressed first before we remove
> the little type verification we currently have.

But we should already try to reduce the scenarios where it is required
to pass. As the translation tool sketched above may take quite some
time to develop, I would prefer to simplify the manual workflow _now_,
i.e. overcome the format stuff for ltt probe modules. That would create
a typical workflow like this:

 1. Spread some marker in your kernel code.
 2. Write an ltt probe module that stuffs a set of markers into a
    facility, but that only has to know their names.
 3. Write the XML description
[4. Adapt/extend lttv when special visualisation is desired. Sigh, that
    bothers me right now - yet another construction site...]

And the update cycle:
 1. Change some markers formats. You may forget to do this in all places
    on first run.
 2. Adapt the XML file (maybe also forgetting some updates), but don't
    touch the probe module unless you add/remove/rename markers.
[3. Adapt lttv. Sigh again, I hope this doesn't happen too often.]
 4. Load the probe module and watch it complain about inconsistent
    marker formats. Fix this.
 5. Take a trace and try to load it with lttv, watch it complain about
    inconsistencies between trace file and XML description (Is this
    already working reliably? This is what the CRC is for? There is also
    an argument size check, right?). Finally fix the XML file.

I think we (Xenomai) could live with keeping markers and XML file(s) in
sync manually, because we could maintain both things together. But for
mainline, the automated parsing/checking of markers might be more
appropriate (unless you manage to push the XML files as well - but I
guess instrumentation patches will already be a challenge).

Jan

_______________________________________________
Ltt-dev mailing list
[email protected]
http://listserv.shafik.org/mailman/listinfo/ltt-dev

-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.5 (GNU/Linux)
Comment: Using GnuPG with SUSE - http://enigmail.mozdev.org

iD8DBQFGPmSGniDOoMHTA+kRAoe+AJsH4oGVYH+Js4iTgDdbMNy+spFzOgCfSaas
Y7RB8fcpJi61tG/7OAI8gzA=
=bR8L
-----END PGP SIGNATURE-----