From mboxrd@z Thu Jan 1 00:00:00 1970 From: compudj@krystal.dyndns.org (Mathieu Desnoyers) Date: Thu, 15 Jan 2009 21:31:31 -0500 Subject: [ltt-dev] [PATCH 07/13] implement fmt align In-Reply-To: <496FEED8.80706@cn.fujitsu.com> References: <496C5A3C.2040206@cn.fujitsu.com> <20090115192245.GG32620@Krystal> <496FDB52.6000406@cn.fujitsu.com> <20090116020900.GA9885@Krystal> <496FEED8.80706@cn.fujitsu.com> Message-ID: <20090116023131.GA12116@Krystal> * Lai Jiangshan (laijs at cn.fujitsu.com) wrote: > Mathieu Desnoyers wrote: > > * 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: > > *You haven't seen this line*: > Ok, this helps :) > >> 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 > > I have insured value is never bigger that sizeof(void *) > Hrm, but when you goto exit, you actually do : +exit: + return (largest_align - align_drift) & (largest_align - 1); If largest_align == sizeof(u64) on a 32-bit machine, then we happen to return an alignment which will be on 64-bits and not 32. Or is there some magic I've missed ? Mathieu > > 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 > >> > > > > > > _______________________________________________ > 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