[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