[ltt-dev] [PATCH 03/13] read event header from ltt-relay buffer

[ltt-dev] [PATCH 03/13] read event header from ltt-relay buffer

[Lai Jiangshan]

we implement ltt_read_event_header() read event header from ltt-relay
buffer. it does the work oppositely to ltt_write_event_header().

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,
+		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);
+
+	*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) {
+			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] [PATCH 03/13] read event header from ltt-relay buffer

[Mathieu Desnoyers]

* 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

> +	*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.

Mathieu

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

-- 
Mathieu Desnoyers
OpenPGP key fingerprint: 8CD5 52C3 8E3C 4140 715F  BA06 3F25 A8FE 3BAE 9A68

[ltt-dev] [PATCH 03/13] read event header from ltt-relay buffer

[Lai Jiangshan]

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