Re: [RFC] kill format from marker_set_probe

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.
- 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.
- 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.


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

Mathieu


> > 
> > My point is about keeping the MARK maintenance effort in the
> > instrumented code and the probe module as low as possible. Currently you
> > either have to define some common format constant in a header file
> > (while still having to touch all MARKs if you change a parameter...),
> > or you have to keep the MARKs and the probe installation code manually
> > in sync - both not optimal. The idea of markers should be to keep the
> > instrumentation as local as possible, and not to force the user to
> > scatter the information. My current feeling is that this should be
> > addressed before going mainline with the markers.
> > 
> > Then, when looking at LTTng, wouldn't such a change remove the need for
> > the format field in ltt_probe_data? If there remains a need for that
> > string, one may rather consider to add some query service to the
> > marker API.
> 
> Now about LTTng again: I still think it doesn't need that extra check in
> a probe module. There is already validation of trace file against XML
> file later on. An LTT probe module may just generate the CRC, but based
> on the format string that is obtained via some should-be-added service:
> 
> struct __mark_marker_data *marker_query(const char *name,
> 					int instance_no);
> 
> Once time permits again, I could cook some patches (end of next week).
> 
> > 
> > Ok, that were just a few thoughts I wanted to save before putting LTTng
> > for Xenomai aside again. :)
> > 
> > Jan
> > 
> > 
> > Index: linux-2.6.20/kernel/module.c
> > ===================================================================
> > --- linux-2.6.20.orig/kernel/module.c
> > +++ linux-2.6.20/kernel/module.c
> > @@ -340,16 +340,18 @@ static int _marker_set_probe_range(int f
> > 
> >         for (iter = begin; iter < end; iter++) {
> >                 if (strcmp(name, iter->mdata->name) == 0) {
> > -                       if (format
> > -                               && strcmp(format, iter->mdata->format) != 0) {
> > -                               printk(KERN_NOTICE
> > -                                       "Format mismatch for probe %s "
> > -                                       "(%s), marker (%s)\n",
> > -                                       name,
> > -                                       format,
> > -                                       iter->mdata->format);
> > -                               continue;
> > -                       }
> > +                       if (format) {
> > +                               if strcmp(format, iter->mdata->format) != 0) {
> > +                                       printk(KERN_NOTICE
> > +                                               "Format mismatch for probe "
> > +                                               "%s (%s), marker (%s)\n",
> > +                                               name,
> > +                                               format,
> > +                                               iter->mdata->format);
> > +                                       continue;
> > +                               }
> > +                       } else
> > +                               format = iter->mdata->format;
> >                         if (flags & _MF_LOCKDEP
> >                                 && !(iter->mdata->flags & _MF_LOCKDEP)) {
> >                                         printk(KERN_NOTICE
> > 
> > 
> > PS: Doing something like
> > 	if (strcmp(name, iter->mdata->name) != 0)
> > 		continue;
> > in the loop above could save you one indention level. :)
> > 
> 
> Jan
> 



-- 
Mathieu Desnoyers
Computer Engineering Ph.D. Student, Ecole Polytechnique de Montreal
OpenPGP key fingerprint: 8CD5 52C3 8E3C 4140 715F  BA06 3F25 A8FE 3BAE 9A68

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

-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.6 (GNU/Linux)

iD8DBQFGPe1mPyWo/juummgRAlL1AJ4+ovLnBTQgJXvzvSAMFAGp7ip2mQCeK0Rp
mNQeHL51rebRNgKDgIo+JEo=
=S/7y
-----END PGP SIGNATURE-----