From mboxrd@z Thu Jan 1 00:00:00 1970 From: laijs@cn.fujitsu.com (Lai Jiangshan) Date: Fri, 16 Jan 2009 10:41:21 +0800 Subject: [ltt-dev] [PATCH 07/13] implement fmt align In-Reply-To: <20090116023131.GA12116@Krystal> References: <496C5A3C.2040206@cn.fujitsu.com> <20090115192245.GG32620@Krystal> <496FDB52.6000406@cn.fujitsu.com> <20090116020900.GA9885@Krystal> <496FEED8.80706@cn.fujitsu.com> <20090116023131.GA12116@Krystal> Message-ID: <496FF3D1.3050409@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: >>>> 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 Ouch! I did concern the things as you said, but I coded it with mistake. Sorry. > > >>> 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 >> >