On Fri, 19 Jan 2024 15:56:21 -0500
Mathieu Desnoyers [off-list ref] wrote:

quoted

So when an overflow happens, you just insert a timestamp, and then events
after that is based on that?  

No. Let's use an example to show how it works.

For reference, LTTng uses 5-bit for event ID and 27-bit for timestamps
in the compact event header representation. But for the sake of making this
discussion easier, let's assume a tracer would use 16-bit for timestamps in the
compact representation.

Let's say we have the following ktime_get() values (monotonic timestamp value) for
a sequence of events:

                                   Timestamp (Hex)    Encoding in the trace

Packet header timestamp begin     0x12345678         64-bit: 0x12345678

Event 1                           0x12345678         16-bit: 0x5678
    (When decoded, same value as previous timestamp, no overflow)
Event 2                           0x12347777         16-bit: 0x7777
    (When decoded, going from "0x5678" to "0x7777" does not overflow 16-bit)
Event 3                           0x12350000         16-bit: 0x0000
    (When decoded, going from "0x7777" to "0x0000" overflow 16-bit exactly once
     which allows the trace reader to reconstruct timestamp 0x12350000 from the
     previous timestamp and the 16-bit timestamp encoding.)
Event 4                           0x12370000         64-bit: 0x12370000
    (Encoding over 16-bit not possible because going from 0x12350000 to
     0x12370000 would overflow 16-bit twice, which cannot be detected
     by a trace reader. Therefore use the full 64-bit timestamp in the
     "large" event header representation.)

I think that's basically what I said, but you are just looking at it
differently ;-) Or should I say, you are using bits for optimization.

The events are based off of the last injected timestamp. The above example,
starts with an timestamp injection into the packet header: 0x12345678, with
the lsb 16bits ignore. So in actuality, it inserts 0x12340000, plus adds a
5678 that represents the creation of the header (like we discussed about in
the last tracing meeting).

The first event has: 0x5678 which is based on the previous injected
timestamp of 0x12340000.

the events go on just using a delta from that until you see it overflow (or
too big of a delta to fit in the 16 bits), and you insert a new full
timestamps that everything will be based on:

  0x12370000

Now events following that are just a delta from that timestamp. But you
calculate the delta simply by masking out the lower bits.

But how do you detect the overflow? That last timestamps to know if the tsc
overflowed or not needs to be saved and compared. I would assume you have a
similar race that I have.

quoted

quoted

The fact that Ftrace exposes this ring buffer binary layout as a user-space
ABI makes it tricky to move to the Common Trace Format timestamp encoding.
There are clearly huge simplifications that could be made by moving to this
scheme though. Is there any way to introduce a different timestamp encoding
scheme as an extension to the Ftrace ring buffer ABI ? This would allow us to
introduce this simpler scheme and gradually phase out the more complex delta
encoding when no users are left.  

I'm not sure if there's a path forward. The infrastructure can easily swap
in and out a new implementation. That is, there's not much dependency on
the way the ring buffer works outside the ring buffer itself.

If we were to change the layout, it would likely require a new interface
file to read. The trace_pipe_raw is the only file that exposes the current
ring buffer. We could create a trace_out_raw or some other named file that
has a completely different API and it wouldn't break any existing API.  

Or introduce "trace_pipe_raw2" or some kind of versioned file names as new
ABIs.

I hate "version numbers" ;-)  I would rather have a meaning name instead.

"trace_pipe_raw_ts_delta" or something that gives a hint to why it's
different.

quoted

Although, if we want to change the "default" way, it may need some other
knobs or something, which wouldn't be hard.  

The delta-timestamp-encoding would have to stay there for a while as long
as users have not switched over to trace_pipe_raw2. Then when it's really
gone, the trace_pipe_raw could either go away or return an error when
opened.

quoted

Now I have to ask, what's the motivation for this. The code isn't that
complex anymore. Yes it still has the before/after timestamps, but the
most complexity in that code was due to what happens in the race of
updating the reserved data. But that's no longer the case with the
cmpxchg(). If you look at this patch, that entire else statement was
deleted. And that deleted code was what made me sick to my stomach ;-)
Good riddance!  

The motivation for this would be to further simplify the implementation
of __rb_reserve_next(), rb_add_timestamp(), and rb_try_to_discard(), and
remove a few useless loads, stores, and conditional branches on the fast-path
of __rb_reserve_next(). This would turn the before/after timestamp
stores/loads/comparisons into a simple "last timestamp" field and a comparison
of the current timestamp against the "last timestamp" value to figure out
whether the compact representation can be used.

I don't know whether it's worth the trouble or not, it's really up to you. :)

Yeah, making such a change would require an analysis to know if it would
speed things up enough to make a significant difference to do that effort.

-- Steve