Re: [RFC] kill format from marker_set_probe

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.

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

_______________________________________________
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

iD8DBQFGPcJzniDOoMHTA+kRAlniAJ99b4JguRsw1bkoSMTJQJZEohEnTwCaAhlj
mwWC7LsTjslEj4idQ2tEWxg=
=Xxbz
-----END PGP SIGNATURE-----