* [ltt-dev] [PATCH 07/13] implement fmt align @ 2009-01-13 9:09 Lai Jiangshan 2009-01-15 19:22 ` Mathieu Desnoyers 0 siblings, 1 reply; 7+ messages in thread From: Lai Jiangshan @ 2009-01-13 9:09 UTC (permalink / raw) 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. Signed-off-by: Lai Jiangshan <laijs at cn.fujitsu.com> --- 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); +} +EXPORT_SYMBOL_GPL(ltt_fmt_largest_align); + +#endif + /* * Calculate data size * Assume that the padding for alignment starts at a sizeof(void *) address. ^ permalink raw reply [flat|nested] 7+ messages in thread
* [ltt-dev] [PATCH 07/13] implement fmt align 2009-01-13 9:09 [ltt-dev] [PATCH 07/13] implement fmt align Lai Jiangshan @ 2009-01-15 19:22 ` Mathieu Desnoyers 2009-01-16 0:56 ` Lai Jiangshan 0 siblings, 1 reply; 7+ messages in thread From: Mathieu Desnoyers @ 2009-01-15 19:22 UTC (permalink / raw) * 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 <laijs at cn.fujitsu.com> > --- > 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 > +} > +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 > -- Mathieu Desnoyers OpenPGP key fingerprint: 8CD5 52C3 8E3C 4140 715F BA06 3F25 A8FE 3BAE 9A68 ^ permalink raw reply [flat|nested] 7+ messages in thread
* [ltt-dev] [PATCH 07/13] implement fmt align 2009-01-15 19:22 ` Mathieu Desnoyers @ 2009-01-16 0:56 ` Lai Jiangshan 2009-01-16 2:09 ` Mathieu Desnoyers 0 siblings, 1 reply; 7+ messages in thread From: Lai Jiangshan @ 2009-01-16 0:56 UTC (permalink / raw) 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 <laijs at cn.fujitsu.com> >> --- >> 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(). > >> +} >> +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 >> > ^ permalink raw reply [flat|nested] 7+ messages in thread
* [ltt-dev] [PATCH 07/13] implement fmt align 2009-01-16 0:56 ` Lai Jiangshan @ 2009-01-16 2:09 ` Mathieu Desnoyers 2009-01-16 2:20 ` Lai Jiangshan 0 siblings, 1 reply; 7+ messages in thread From: Mathieu Desnoyers @ 2009-01-16 2:09 UTC (permalink / raw) * 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 <laijs at cn.fujitsu.com> > >> --- > >> 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 ^ permalink raw reply [flat|nested] 7+ messages in thread
* [ltt-dev] [PATCH 07/13] implement fmt align 2009-01-16 2:09 ` Mathieu Desnoyers @ 2009-01-16 2:20 ` Lai Jiangshan 2009-01-16 2:31 ` Mathieu Desnoyers 0 siblings, 1 reply; 7+ messages in thread From: Lai Jiangshan @ 2009-01-16 2:20 UTC (permalink / raw) 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 <laijs at cn.fujitsu.com> >>>> --- >>>> 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 >> > ^ permalink raw reply [flat|nested] 7+ messages in thread
* [ltt-dev] [PATCH 07/13] implement fmt align 2009-01-16 2:20 ` Lai Jiangshan @ 2009-01-16 2:31 ` Mathieu Desnoyers 2009-01-16 2:41 ` Lai Jiangshan 0 siblings, 1 reply; 7+ messages in thread From: Mathieu Desnoyers @ 2009-01-16 2:31 UTC (permalink / raw) * 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 <laijs at cn.fujitsu.com> > >>>> --- > >>>> 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 ^ permalink raw reply [flat|nested] 7+ messages in thread
* [ltt-dev] [PATCH 07/13] implement fmt align 2009-01-16 2:31 ` Mathieu Desnoyers @ 2009-01-16 2:41 ` Lai Jiangshan 0 siblings, 0 replies; 7+ messages in thread From: Lai Jiangshan @ 2009-01-16 2:41 UTC (permalink / raw) 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 <laijs at cn.fujitsu.com> >>>>>> --- >>>>>> 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 >> > ^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2009-01-16 2:41 UTC | newest] Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2009-01-13 9:09 [ltt-dev] [PATCH 07/13] implement fmt align Lai Jiangshan 2009-01-15 19:22 ` Mathieu Desnoyers 2009-01-16 0:56 ` Lai Jiangshan 2009-01-16 2:09 ` Mathieu Desnoyers 2009-01-16 2:20 ` Lai Jiangshan 2009-01-16 2:31 ` Mathieu Desnoyers 2009-01-16 2:41 ` Lai Jiangshan
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox