From mboxrd@z Thu Jan 1 00:00:00 1970 From: laijs@cn.fujitsu.com (Lai Jiangshan) Date: Fri, 16 Jan 2009 10:20:08 +0800 Subject: [ltt-dev] [PATCH 07/13] implement fmt align In-Reply-To: <20090116020900.GA9885@Krystal> References: <496C5A3C.2040206@cn.fujitsu.com> <20090115192245.GG32620@Krystal> <496FDB52.6000406@cn.fujitsu.com> <20090116020900.GA9885@Krystal> Message-ID: <496FEED8.80706@cn.fujitsu.com> 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*: >> 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 *) > 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 >> >