[ltt-dev] [PATCH 07/13] implement fmt align

[ltt-dev] [PATCH 07/13] implement fmt align

[Lai Jiangshan]

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.

[ltt-dev] [PATCH 07/13] implement fmt align

[Mathieu Desnoyers]

* 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

[ltt-dev] [PATCH 07/13] implement fmt align

[Lai Jiangshan]

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
>>
> 

[ltt-dev] [PATCH 07/13] implement fmt align

[Mathieu Desnoyers]

* 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

[ltt-dev] [PATCH 07/13] implement fmt align

[Lai Jiangshan]

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
>>
> 

[ltt-dev] [PATCH 07/13] implement fmt align

[Mathieu Desnoyers]

* 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

[ltt-dev] [PATCH 07/13] implement fmt align

[Lai Jiangshan]

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
>>
>