[ltt-dev] [PATCH 11/13] implement ltt-ascii

[Mathieu Desnoyers]

* Lai Jiangshan (laijs at cn.fujitsu.com) wrote:
> 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".
A> > 
> >> +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.
> 

Hrm, so if we have :

len = 2048 (buffer size)
value = 1491
left = 18
right = 0

You test would consider value to be within left and right, while my test
considers value to be too far apart from the left/right region. Oh I
see.. I use the kind of test I wrote here when I do not know which is
left and which is right. In that kind of case, I do not know in which
direction the inner and outer regions are, and I therefore have to
assume (and therefore require) the value to be no more that half an
overflow away from the region. If we can insure this, my test is ok too.
But I just generated the assembly for both, and you solution seems to
result in fewer instructions, so let's keep it.

Also, note that your code works because we only keep the buffer offset
part of the "offset" value, and therefore do not risk overflowing
size_t.


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

It's a good thing to proceed step-by-step as you do. But we could just
disallow read() unless tracing is !active (stopped) for the whole trace
rather than adding the new channel_disabled counter ?

That would make our life easier in the future when we wish to support
live ascii dump while tracing. (which will require periodical buffer
flush)



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

debugfs_create_file() can return -ENODEV if not enabled in the kernel.
Clearly this should never happen because our build depends on it. But
still, I'd be more comfortable dealing with the IS_ERR() case (if new
return values are added to debugfs in the future especially).

Mathieu

> > 
> >> +	if (entry)
> >> +		ltt_relay_get_chan(ltt_channel->trans_channel_data);
> >> +
> >> +	return entry;
> >> +}
> >> +EXPORT_SYMBOL_GPL(ltt_txt_create);
> >
> 
> 
> _______________________________________________
> 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