From mboxrd@z Thu Jan 1 00:00:00 1970 From: compudj@krystal.dyndns.org (Mathieu Desnoyers) Date: Thu, 15 Jan 2009 21:09:00 -0500 Subject: [ltt-dev] [PATCH 07/13] implement fmt align In-Reply-To: <496FDB52.6000406@cn.fujitsu.com> References: <496C5A3C.2040206@cn.fujitsu.com> <20090115192245.GG32620@Krystal> <496FDB52.6000406@cn.fujitsu.com> Message-ID: <20090116020900.GA9885@Krystal> * Lai Jiangshan (laijs at cn.fujitsu.com) wrote: > Mathieu Desnoyers wrote: > > * Lai Jiangshan (laijs at cn.fujitsu.com) wrote: > >> we need know the align for accessing the binary data from > >> event.(skip the padding data, calculate offset). > >> > >> It can also be used for write-side. > >> > > > > Yes, but I prefer not, because I like to do a single pass in the format > > string to calculate both the type sizes individually and the largest > > alignment. It makes sense to implement this for the read-side though. > > > > Please see comments below, > > > >> Signed-off-by: Lai Jiangshan > >> --- > >> b/ltt/ltt-serialize.c | 60 +++++++++++++++++++++++++++++++++++++++++++++ > >> include/linux/ltt-tracer.h | 9 ++++++ > >> 2 files changed, 69 insertions(+) > >> diff --git a/include/linux/ltt-tracer.h b/include/linux/ltt-tracer.h > >> index ece6819..b49cac2 100644 > >> --- a/include/linux/ltt-tracer.h > >> +++ b/include/linux/ltt-tracer.h > >> @@ -311,6 +316,9 @@ static inline unsigned int ltt_align(size_t align_drift, size_t size_of_type) > >> size_t alignment = min(sizeof(void *), size_of_type); > >> return (alignment - align_drift) & (alignment - 1); > >> } > >> + > >> +extern unsigned int ltt_fmt_largest_align(size_t align_drift, const char *fmt); > >> + > >> /* Default arch alignment */ > >> #define LTT_ALIGN > >> > >> @@ -327,6 +335,12 @@ static inline unsigned int ltt_align(size_t align_drift, > >> return 0; > >> } > >> > >> +static inline unsigned int ltt_fmt_largest_align(size_t align_drift, > >> + const char *fmt) > >> +{ > >> + return 0; > >> +} > >> + > >> #define LTT_ALIGN __attribute__((packed)) > >> > >> static inline int ltt_get_alignment(void) > >> diff --git a/ltt/ltt-serialize.c b/ltt/ltt-serialize.c > >> index 5c34899..deb9106 100644 > >> --- a/ltt/ltt-serialize.c > >> +++ b/ltt/ltt-serialize.c > >> @@ -531,6 +538,66 @@ > >> } > >> EXPORT_SYMBOL_GPL(ltt_serialize_printf); > >> > >> +#ifdef CONFIG_LTT_ALIGNMENT > >> + > >> +unsigned int ltt_fmt_largest_align(size_t align_drift, const char *fmt) > >> +{ > >> + char trace_size = 0, c_size = 0; > >> + enum ltt_type trace_type = LTT_TYPE_NONE, c_type = LTT_TYPE_NONE; > >> + unsigned long attributes = 0; > >> + int largest_align = 1; > >> + > >> + for (; *fmt ; ++fmt) { > >> + switch (*fmt) { > >> + case '#': > >> + /* tracetypes (#) */ > >> + ++fmt; /* skip first '#' */ > >> + if (*fmt == '#') /* Escaped ## */ > >> + break; > >> + attributes = 0; > >> + fmt = parse_trace_type(fmt, &trace_size, &trace_type, > >> + &attributes); > >> + > >> + largest_align = max_t(int, largest_align, trace_size); > >> + if (largest_align >= ltt_get_alignment()) > >> + goto exit; > >> + break; > >> + case '%': > >> + /* c types (%) */ > >> + ++fmt; /* skip first '%' */ > >> + if (*fmt == '%') /* Escaped %% */ > >> + break; > >> + fmt = parse_c_type(fmt, &c_size, &c_type, NULL); > >> + /* > >> + * Output c types if no trace types has been > >> + * specified. > >> + */ > >> + if (!trace_size) > >> + trace_size = c_size; > >> + if (trace_type == LTT_TYPE_NONE) > >> + trace_type = c_type; > >> + if (c_type == LTT_TYPE_STRING) > >> + trace_type = LTT_TYPE_STRING; > >> + > >> + largest_align = max_t(int, largest_align, trace_size); > >> + if (largest_align >= ltt_get_alignment()) > >> + goto exit; > >> + > >> + trace_size = 0; > >> + c_size = 0; > >> + trace_type = LTT_TYPE_NONE; > >> + c_size = LTT_TYPE_NONE; > >> + break; > >> + } > >> + } > >> + > >> +exit: > >> + return (largest_align - align_drift) & (largest_align - 1); > > > > Hrm, I think there is something wrong here. > > > > On x86_32, the max alignment is sizeof(u32). > > > > However, if you get a u64 type here, the largest_align will become > > sizeof(u64). This is why, in ltt_vtrace(), I do a > > > > largest_align = 1; /* must be non-zero for ltt_align */ > > data_size = ltt_get_data_size(&closure, serialize_private, > > &largest_align, fmt, &args_copy); > > largest_align = min_t(int, largest_align, sizeof(void *)); > > > > So we should document in the ltt_fmt_largest_align header to always do a > > largest_align = min_t(int, largest_align, sizeof(void *)); on the return > > value (and check that we do it) or we simply do this operation before > > returning. > > > > Mathieu > > I haven't seen this line: > if (largest_align >= ltt_get_alignment()) > goto exit; > > We shouldn't hardcode "sizeof(void *)", we should use ltt_get_alignment(). > My comment was that it's necessary to do a largest_align = min_t(int, largest_align, sizeof(void *)); to make sure the alignment value is never bigger that sizeof(void *). I don't understand how your answer here relates to my concern. (maybe I have problem understanding your answer on my side..). Can you elaborate more please ? Mathieu > > > > >> +} > >> +EXPORT_SYMBOL_GPL(ltt_fmt_largest_align); > >> + > >> +#endif > >> + > >> /* > >> * Calculate data size > >> * Assume that the padding for alignment starts at a sizeof(void *) address. > >> > >> > >> > >> > >> _______________________________________________ > >> ltt-dev mailing list > >> ltt-dev at lists.casi.polymtl.ca > >> http://lists.casi.polymtl.ca/cgi-bin/mailman/listinfo/ltt-dev > >> > > > > > > _______________________________________________ > ltt-dev mailing list > ltt-dev at lists.casi.polymtl.ca > http://lists.casi.polymtl.ca/cgi-bin/mailman/listinfo/ltt-dev > -- Mathieu Desnoyers OpenPGP key fingerprint: 8CD5 52C3 8E3C 4140 715F BA06 3F25 A8FE 3BAE 9A68