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