From mboxrd@z Thu Jan  1 00:00:00 1970
From: laijs@cn.fujitsu.com (Lai Jiangshan)
Date: Fri, 16 Jan 2009 08:47:55 +0800
Subject: [ltt-dev] [PATCH 03/13] read event header from ltt-relay buffer
In-Reply-To: <20090115183652.GC32620@Krystal>
References: <496C5A23.6010003@cn.fujitsu.com> <20090115183652.GC32620@Krystal>
Message-ID: <496FD93B.3000507@cn.fujitsu.com>

Mathieu Desnoyers wrote:
> * Lai Jiangshan (laijs at cn.fujitsu.com) wrote:
>> we implement ltt_read_event_header() read event header from ltt-relay
>> buffer. it does the work oppositely to ltt_write_event_header().
>>
> 
> While I'm at it.. just want to tell you the global design looks very
> good. Let's continue on the small details...
> 
>> Signed-off-by: Lai Jiangshan <laijs at cn.fujitsu.com>
>> ---
>>  ltt-tracer.h |   59 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>>  1 file changed, 59 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
>> @@ -492,6 +506,65 @@ static inline size_t ltt_write_event_header(struct ltt_trace_struct *trace,
>>  	return buf_offset;
>>  }
>>  
>> +/*
>> + * ltt_read_event_header
>> + * buf_offset must aligned on 32 bits
>> + */
>> +static inline size_t ltt_read_event_header( struct rchan_buf *buf,
> 
> Whitespace above before struct.
> This will need some kerneldocs-style header eventually.
> 
>> +		long buf_offset, u64 *tsc, u32 *event_size, u16 *eID,
>> +		unsigned int *rflags)
>> +{
>> +	struct ltt_event_header header;
>> +	u16 small_size;
>> +
>> +	ltt_relay_read(buf, buf_offset, &header, sizeof(header));
>> +	buf_offset += sizeof(header);
>> +
> 
> Random question :
> 
> On what architecture do you test this ? You might want to force the
> 
> ltt/Kconfig
> 
> config LTT_ALIGNMENT
>         bool "Align Linux Trace Toolkit Traces"
>         default y if !HAVE_EFFICIENT_UNALIGNED_ACCESS
>         help
>           This option enables dynamic alignment of data in buffers. The
>           alignment is made on the smallest size between architecture size
>           and the size of the value to be written.
> 
>           Dynamically calculating the offset of the data has a performance cost,
>           but it is more efficient on some architectures (especially 64 bits) to
>           align data than to write it unaligned.
> 
> Because many arch define HAVE_EFFICIENT_UNALIGNED_ACCESS, especially
> x86. And that might be a source of bug. (this is the role of the
> ltt_align() macro).
> 
> Mathieu

caller of ltt_read_event_header() must ensure buf_offset aligned on 32 bits
when LTT_ALIGNMENT, so this code is correct.

> 
>> +	*event_size = INT_MAX;
>> +	*eID = header.id_time >> LTT_TSC_BITS;
>> +	*tsc = header.id_time & LTT_TSC_MASK;
>> +
>> +	switch (*eID) {
>> +	case 29:
>> +		*rflags = LTT_RFLAG_ID_SIZE_TSC;
>> +		ltt_relay_read(buf, buf_offset, eID, sizeof(u16));
>> +		buf_offset += sizeof(u16);
>> +		ltt_relay_read(buf, buf_offset, &small_size, sizeof(u16));
>> +		buf_offset += sizeof(u16);
>> +		if (small_size == 0xFFFFU) {
>> +			ltt_relay_read(buf, buf_offset, event_size, sizeof(u32));
>> +			buf_offset += sizeof(u32);
>> +		} else
>> +			*event_size = small_size;
>> +		buf_offset += ltt_align(buf_offset, sizeof(u64));
>> +		ltt_relay_read(buf, buf_offset, tsc, sizeof(u64));
>> +		buf_offset += sizeof(u64);
>> +		break;
>> +	case 30:
>> +		*rflags = LTT_RFLAG_ID_SIZE;
>> +		ltt_relay_read(buf, buf_offset, eID, sizeof(u16));
>> +		buf_offset += sizeof(u16);
>> +		ltt_relay_read(buf, buf_offset, &small_size, sizeof(u16));
>> +		buf_offset += sizeof(u16);
>> +		if (small_size == 0xFFFFU) {
> 
> Hrm, well, in the previous email I told you we should not hardcode
> "16".. and I notice that I've hardcoded 0xFFFU in
> ltt_write_event_header... Well ideally this one should also become a
> define, for both ltt_write_event_header and ltt_read_event_header.
> 

I did it as what ltt_write_event_header() did, -:)
If I define some macros, I will conflict your job for these.

Once, I tried to use ltt_event_id_t, but I notice it's bad that I define it
for I will mess up the hold text output patch with a lots of irrespective code.

I can't exceed one's duties and meddle in others' affairs. -:)

> 
>> +			ltt_relay_read(buf, buf_offset, event_size, sizeof(u32));
>> +			buf_offset += sizeof(u32);
>> +		} else
>> +			*event_size = small_size;
>> +		break;
>> +	case 31:
>> +		*rflags = LTT_RFLAG_ID;
>> +		ltt_relay_read(buf, buf_offset, eID, sizeof(u16));
>> +		buf_offset += sizeof(u16);
>> +		break;
>> +	default:
>> +		*rflags = 0;
>> +		break;
>> +	}
>> +
>> +	return buf_offset;
>> +}
>> +
>>  /* Lockless LTTng */
>>  
>>  /* Buffer offset macros */
>>
>>
>>
>>
>> _______________________________________________
>> ltt-dev mailing list
>> ltt-dev at lists.casi.polymtl.ca
>> http://lists.casi.polymtl.ca/cgi-bin/mailman/listinfo/ltt-dev
>>
>