From mboxrd@z Thu Jan  1 00:00:00 1970
From: laijs@cn.fujitsu.com (Lai Jiangshan)
Date: Fri, 16 Jan 2009 09:29:17 +0800
Subject: [ltt-dev] [PATCH 11/13] implement ltt-ascii
In-Reply-To: <20090115200214.GB2462@Krystal>
References: <496C5A56.7060203@cn.fujitsu.com> <20090115200214.GB2462@Krystal>
Message-ID: <496FE2ED.5070405@cn.fujitsu.com>

Mathieu Desnoyers wrote:
> * Lai Jiangshan (laijs at cn.fujitsu.com) wrote:
>> implement text output module for lttng.
>>
>> Signed-off-by: Lai Jiangshan <laijs at cn.fujitsu.com>
>> ---
>>  b/ltt/Makefile    |    1
>>  b/ltt/ltt-ascii.c |  442 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
>>  ltt/Kconfig       |    8
>>  3 files changed, 451 insertions(+)

>> +
>> +static u64 calculate_tsc(u64 pre_tsc, u64 read_tsc, unsigned int rflags)
>> +{
>> +	u64 new_tsc = read_tsc;
>> +
>> +	if (rflags != LTT_RFLAG_ID_SIZE_TSC) {
>> +		BUG_ON(read_tsc >> LTT_TSC_BITS);
>> +
>> +		new_tsc = (pre_tsc & ~LTT_TSC_MASK) + read_tsc;
>> +		if (read_tsc < (pre_tsc & LTT_TSC_MASK))
>> +			new_tsc += 1 << LTT_TSC_BITS;
> 
> 1ULL ? (just to play safe)

Yes, good eyes!

> 
>> +	}
>> +
>> +	return new_tsc;
>> +}
>> +

>> +/* Is @value in the region [@left, @right) */
> 
> The comment should state "in a circular buffer".
> 
>> +static int is_in_region(size_t value, size_t left, size_t right, size_t len)
>> +{
>> +	if (right < left)
>> +		right += len;
>> +	if (value < left)
>> +		value += len;
>> +	return value < right;
>> +}
>> +
> 
> /* Assumes buffer size (len) is power of two */
> 
> size_t half_len = len >> 1;
> size_t len_mask = len - 1;
> 
> if ((value - left) & len_mask > half_len)
>   return 0;
> if ((right - value) & len_mask > half_len)
>   return 0;
> return 1;

It's incorrect.

> 
> I'm not convinced my implementation generates faster assembly.. this
> should probably be tried. The good side here is that we only have two
> comparison.
> 
> 

> 
> Yes, that's going to be required. :)
> 
>> +	for_each_possible_cpu(i) {
>> +		curr = &iter->iter_cpu[i];
>> +
>> +		if (!curr->buf)
>> +			continue;
>> +
>> +		if (cpu_iter_eof(curr))
>> +			continue;
> 
> Hrm, how do we deal with the fact that some CPUs or channels wait for
> too long before providing data ? This is the purpose of the planned
> periodical buffer switch. I suspect that here you simply read the
> "oldest available data", thus discarding the data from subbuffers not
> completed yet, which could lead to timestamps going backward. Or am I
> missing something ?
> 
> Ideally, we should just block and wait for data when any given buffer
> have partially complete subbuffers.
> 
> Oh, maybe this only works on stopped traces then ? This periodical
> subbuffer flush trick is only needed if we plan to read from
> non-overwrite traces (normal traces) while tracing is active.

I stop the this channel's tracing.
I spent the most time for implement ltt event traveling, I still
can't implement a safe traveling for non-stopped ltt-relay buffer.

(I also think "non-stopped" not useful for developer, he just
want to see what he just did in the kernel, so we can stop the buffer.
and we have user-tool for his need)

> 

>> +struct dentry *ltt_txt_create(struct ltt_trace_struct *trace,
>> +		struct ltt_channel_struct *ltt_channel)
>> +{
>> +	struct dentry *entry = debugfs_create_file(ltt_channel->channel_name,
>> +			S_IRUSR | S_IRGRP, trace->dentry.txt_root, ltt_channel,
>> +			&ltt_txt_fops);
>> +
> 
> We should handle the error case here. And make sure the ltt_txt_remove
> won't forget to put the channel on error.
> 

when the error case here, we return NULL, and nothing done.
What we need to handle?

> 
>> +	if (entry)
>> +		ltt_relay_get_chan(ltt_channel->trans_channel_data);
>> +
>> +	return entry;
>> +}
>> +EXPORT_SYMBOL_GPL(ltt_txt_create);
>