This is an archive of the discontinued LLVM Phabricator instance.

[WIP][trace][intelpt] Add TSC to Nanosecond conversion for IntelPT traces
AbandonedPublic

Authored by jj10306 on Feb 25 2022, 3:42 PM.

Details

Reviewers
wallace
davidca
Summary

TSC to timestamp conversion for IntelPT traces and other minor improvements:

  • Add TimestampCounterRate abstract class and concrete implementation PerfTimestampCounterRate that gets conversion values from Linux's perf event API - perf_event_mmap_page
  • Add resource_handle namespace to allow for reuse of file descriptor and munmap resource handles
  • Cache TimestampCounterRateSP on server side in IntelPTCollector and client side in TraceIntelPT
  • Update DecodedThread to allow for access to the conversion values from the TraceCursor
  • Add GetNanos (to be renamed) API to TraceCursor to allow for conversion of a trace instruction's TSC value to nanoseconds
  • Update trace schema and trace save commands to accommodate schema changes
  • Rename IntelPTManager to IntelPTCollector

TODO:

  • add unittests
  • Decide on an descriptive name and other minor comments from @wallace @davidca on previous version of diff
    • Make naming consistent across all changes after the naming is decided upon
  • Decide how to utilize the new TraceCursor::toNanos() API in instruction dump output
  • Make documentation in lldb-gdb-remote.txt` and GetSchema() clean and descriptive given the potential for different flavors of TimestampCounterRate

Diff Detail

Event Timeline

jj10306 requested review of this revision.Feb 25 2022, 3:42 PM
jj10306 created this revision.
Herald added a project: Restricted Project. · View Herald TranscriptFeb 25 2022, 3:42 PM
jj10306 added inline comments.Feb 25 2022, 3:53 PM
lldb/include/lldb/Utility/TraceIntelPTGDBRemotePackets.h
50

What is the suggested way to serialize a u64 into JSON? I thought of two approaches:

  1. Store two separate u32
  2. store the integer as a string

@wallace wdyt?

lldb/source/Plugins/Trace/intel-pt/TraceIntelPT.cpp
22
lldb/source/Plugins/Trace/intel-pt/TraceIntelPT.h
192

Not sure TraceIntelPT is the correct spot to cache the converter on the client side. It will depend on how this new logic is integrated with the decoders, but curious to get your initial feedback on this.

lldb/source/Utility/TraceIntelPTGDBRemotePackets.cpp
75–76
wallace retitled this revision from [trace][intelpt] Add TSC to Nanosecond conversion for IntelPT traces to [WIP][trace][intelpt] Add TSC to Nanosecond conversion for IntelPT traces.Feb 25 2022, 3:58 PM
wallace requested changes to this revision.Feb 25 2022, 4:33 PM

you got the flow correct. I left comments regarding specific details, but good job overall!!

lldb/docs/lldb-gdb-remote.txt
454–502

don't forget to f

lldb/include/lldb/Utility/TraceIntelPTGDBRemotePackets.h
50

you don't need to use int64_t here. You can use the real types here and do the conversion in the fromJson and toJson methods

54

TscToWallTimeConverter is a better name

54

use a class and not a struct. A struct has all its member public and we don't want that

57

ToNanos() is also okay. Up to you

62
63–65

you can get rid of the constructors a

69

you don't need this struct. You should just put the 3 fields here.

85

it's weird for a bag of data to contain a field transmitted by json called coverter. Just call it TimestampCounterRate. Because that's what it is. Then each TimestampCounterRate subclass can have a ToNanos(uint64_t tsc) method that converts to that.
Your documentation should specific if this ToNanos method returns the since the start of the computer, or since the start of the program, or anything else

88

Create a typedef in this file called TimestampCounterRateSP for readability. You can use tsc_rate as your variable names

lldb/source/Plugins/Process/Linux/IntelPTManager.cpp
49

remove it from here

261–263

notice the case is which g_tsc_rate is not None but nullptr. That means that the computation was performed but no rate was found, so the computation won't happen again. We have to make the code in that way because it's possible that checking for some rates is not a no-op. E.g. you can run a little 10ms program and calculate the difference in TSC to get an estimated rate.

Besides that, put all of this in a new function

lldb/source/Plugins/Process/Linux/IntelPTManager.h
252

static llvm::Optional<TimestampCounterRateSP> g_tsc_rate; // by default this is None

lldb/source/Plugins/Trace/intel-pt/TraceIntelPT.cpp
22

memory should be defined as a new include block between lines 9 and 11. That's just the pattern we follow

218

just replace the converter if you got a new one from the state. IT's cheap

lldb/source/Plugins/Trace/intel-pt/TraceIntelPT.h
191

this is null by default, no need to mention it

192

this is the right place to cache it, right next to m_cpu_info

lldb/source/Target/Trace.cpp
193–203

you can combine these blocks

lldb/source/Utility/TraceIntelPTGDBRemotePackets.cpp
56

fail fast whenever you can

59

you don't need to pass PerfTscConverter. make_shared should be smart enough to create the object by itself

59–60

if you do this, then you can fail fast and do the int conversions here, without polluting your class

66–68

delete this

69

return false; if you got here, then you have a kind that is not available

74

o.mapOptional is what you need here. That will return true if "tsc_conversion" is not available or if it's well-formed.

84–86

here you should cast to int64_t

This revision now requires changes to proceed.Feb 25 2022, 4:33 PM
jj10306 marked 21 inline comments as done.Feb 28 2022, 12:12 PM
jj10306 added inline comments.
lldb/include/lldb/Utility/TraceIntelPTGDBRemotePackets.h
50

What is the suggested way to serialize a u64 into JSON? I thought of two approaches:

  1. Store two separate u32
  2. store the integer as a string

@wallace wdyt?

63–65

Need the 3 arg constructor to create an instance of this class in IntelPTManager.cpp and need the default constructor for when we create an uninitialized instance of the class before mapping the values from JSON in the fromJSON method.

lldb/source/Plugins/Process/Linux/IntelPTManager.cpp
49

Without a definition of the static member outside of the class, a linker error occurs.

lldb/source/Plugins/Process/Linux/IntelPTManager.h
252

Is the idea of wrapping the SP in an Optional here allows us to represent 3 different states?

  1. None: we have not yet attempted to fetch/calculate the tsc rate.
  2. nullptr: we tried to fetch the rate but one does not exist
  3. <non-null SP>: the rate that was successfully fetched

Without the Optional, we cannot distinguish between the first two cases.

Please let me know if my understanding is correct.

lldb/source/Utility/TraceIntelPTGDBRemotePackets.cpp
69

in this case should we also set the SP value to be nullptr? Otherwise that value will still be None which, from my understanding, indicates that the TscRate has not yet been fetched whereas nullptr indicates that the we have tried to fetch the rate but it does not exist.

74


From looking at the source of map and mapOptional it appears that the only difference is that map sets Out to None whereas mapOptional leaves it unchanged, they both return true in the case that the property doesn't exist.

Given this, it's not immediately obvious to me which we should be using in this case - do we want the tsc_conversion to be set to None or left unchanged in the case that it's not present in the JSON?

84–86

How should we handle serializing m_time_zero since it's uint64_t and the JSON library only supports int64_t for number types?

jj10306 updated this revision to Diff 411862.Feb 28 2022, 12:18 PM
jj10306 marked 3 inline comments as done.
jj10306 edited the summary of this revision. (Show Details)

Address comments, still need to handle uint64_t JSON serialization correctly. I plan to proceed with decoder and schema changes now that initial feedback has been received and addressed.

wallace requested changes to this revision.Feb 28 2022, 8:54 PM

in terms of functionality, everything is right, but you need to do some improvements

lldb/docs/lldb-gdb-remote.txt
454–502

you have to improve the documentation and formatting following the existing documentation's format

lldb/include/lldb/Target/Trace.h
306

JSON string representing the jLLDBTraceGetState response. It might be invalid.

310

you don't need to return a unique_ptr for this. There's nothing that you want to forbid copies of. Just return

llvm::Optional<TraceGetStateResponse>
lldb/include/lldb/Utility/TraceIntelPTGDBRemotePackets.h
14

undo this

42

TSC is not a very well known concept, so better be very explanatory in the documentation

Class that can be extended to represent different CPU Time Stamp Counter (TSC) frequency rates, which can be used to convert TSCs to common units of time, such as nanoseconds. More on TSCs can be found here https://en.wikipedia.org/wiki/Time_Stamp_Counter.
46
wallace added inline comments.Feb 28 2022, 8:54 PM
lldb/include/lldb/Utility/TraceIntelPTGDBRemotePackets.h
50

i'll respond below

52

modern c++ uses the using keyword.

using TimestampCounterRateSP = std::shared_ptr<TimestampCounterRate>;

use the version you want

54–55

You have to make the documentation very friendly to newcomers.

58

do you need this?

60

why do you do static_cast<int64_t>?

65–67

having the correct types here is the right thing to do

71

The response contains the frequency rate. Whether it's used to convert to nanos or not is a different topic.

Just delete this line.

73

avoid using nullptr when you have an Optional.

lldb/source/Plugins/Process/Linux/IntelPTManager.cpp
49

makes sense

261

sadly you can't put this here. If you do this, then GetState() will only work correctly if StartTrace() was invoked first. What if in the future we have a different flow in which lldb-server uses an additional tracing function besides StartTrace() and that new function forgets to set the tsc_rate?
This is called coupling and should be avoided.

What you should do instead is to create a new static function called llvm::Optional<TimestampCounterRateSP >GetTscRate() that will perform the perf_event request or reuse a previously saved value. It should work regardless of when it's invoked.

652

only set it if not None

if (Optional<...> tsc_rate_sp = IntelPTManager::GetTSCRate())
  state.tsc_rate = tsc_rate_sp;
699

GetTscRate is a better name, because eventually we might want to include other fallback mechanisms

lldb/source/Plugins/Process/Linux/IntelPTManager.h
216

don't forget to add documentation here

lldb/source/Plugins/Trace/intel-pt/TraceIntelPT.cpp
199

return None

209

None

218

make sure that everything works when lldb-server doesn't have support for the perf zero cap

lldb/source/Plugins/Trace/intel-pt/TraceIntelPT.h
191

use the same documentation I suggest above for the state object

lldb/source/Target/Trace.cpp
193–194

you are not storing live_process_state elsewhere. Just use an optional. That's all you need

193–194

you can fail faster to keep everything cleaner

lldb/source/Utility/TraceIntelPTGDBRemotePackets.cpp
60

int64_t, and give proper names

65

path.report("unsupported TSC rate kind");

make sure to write a test with malformed data

70

just return the bool

80

this kind of casting is all fine. Look at this code

#include <iostream>
#include <climits>
using namespace std;

int main() {
  uint64_t x = ULONG_MAX; // 0xFFFFFFFF;
  cout << x << endl;
  cout << (int64_t)x << endl;
  cout << (uint64_t)(int64_t)x << endl;
}

and the output is

18446744073709551615
-1
18446744073709551615

The same works for ULONG_MAX - 1, which is equivalent to -2.

So that means that there's no conversion loss

86–91

the code is simpler is you make strict use of the shared pointer only for valid cases

This revision now requires changes to proceed.Feb 28 2022, 8:54 PM
labath added a subscriber: labath.Mar 1 2022, 8:01 AM
labath added inline comments.
lldb/include/lldb/Utility/TraceIntelPTGDBRemotePackets.h
73

Ideally this would be a plain TimestampCounterRateSP variable and nullptr would denote emptyness. Since (shared) pointers already have a natural empty state, it's best to use that. Adding an extra Optional layer just creates ambiguity.

lldb/source/Plugins/Trace/intel-pt/TraceIntelPT.cpp
22

It's actually a pattern we should not follow. When all of the includes are grouped in one block, then clang-format automatically reorders them to conform to the llvm coding standards.

davidca added inline comments.Mar 3 2022, 10:59 PM
lldb/docs/lldb-gdb-remote.txt
477

why is all this called tsc rate? there is also offset in this conversion. What about something like tsc_conversion_params or tsc_conv

lldb/include/lldb/Utility/TraceIntelPTGDBRemotePackets.h
43

nit, Intel documentation and kernel refers this as TSC, not TimestampCounter, in order to differentiate this awful name from everywhere else that "Timestamp" and "Counter" is used. Perhaps embrace TSC to make it easier to google info?

46

To be precise, it's number of nanoseconds since boot. You should also add that this nanoseconds clock is sched_clock https://www.kernel.org/doc/html/latest/timers/timekeeping.html

In fact, you may want to use sched_clock as the name for this clock, rather than Nanos as in the next lines.

77–79

should this two be static factory methods?

lldb/source/Plugins/Process/Linux/IntelPTManager.cpp
261
  1. this is not a Get* method. This is populating a global. Get* are inexpensive methods that return a value.
  1. if the TSC conversion parameters will be stored in a global, why not to use the Singleton pattern?
  1. TSC parameters change over time. I really think that should be stored within IntelPTManager. Obtaining this parameters is a ~10 microseconds operation, so it's fine if they need to be read next time an IntelPTManager is created.
lldb/source/Plugins/Process/Linux/IntelPTManager.h
216

how is ti

Herald added a project: Restricted Project. · View Herald TranscriptMar 3 2022, 10:59 PM

Thanks @davidca and @labath for chiming in.
@jj10306, please rename IntelPTManager to IntelPTCollector for clarity.

lldb/docs/lldb-gdb-remote.txt
477

tsc_conversion_params seems like a good idea

lldb/include/lldb/Utility/TraceIntelPTGDBRemotePackets.h
43

Ahh!! Then let's do what David says. Let's call it tsc everywhere

73

Good idea

77–79

That's just the pattern most json conversion utils follow. It helps with some templating stuff

lldb/source/Plugins/Process/Linux/IntelPTManager.cpp
261

Can tsc frequency rates change even in modern cpus?. In this case, it's no brainer we need to calculate this number all the time.

I also agree with point number 1.

lldb/source/Plugins/Process/Linux/IntelPTManager.h
216

@davidca, i think you didn't finish your comment

jj10306 marked 41 inline comments as done.EditedMar 8 2022, 6:43 AM

Will update naming and other issues once the couple minor discussions with @wallace and @davidca in the comments are complete.

lldb/docs/lldb-gdb-remote.txt
477

Agreed, making that change!

lldb/include/lldb/Utility/TraceIntelPTGDBRemotePackets.h
43

Which do you think is a better name to replace TimestampCounterRate with - TscConversionParams (as mentioned in the previous comment, "tsc_conversion_params" will be the new key in the TraceGetState packet schema, so this would be consistent with that) or TscConversioninfo or TscConverter (since the function of the class is to provide functionality to convert Tsc to time)?
@davidca @wallace wdyt?

46

Chapter 17.7 of Intel's software developer manual (volume 3) states the following:
"The time-stamp counter (as implemented in the P6 family, Pentium, Pentium M, Pentium 4, Intel Xeon, Intel Core
Solo and Intel Core Duo processors and later processors) is a 64-bit counter that is set to 0 following a RESET of
the processor
."

Does a reset only occur when the system is rebooted? I mentioned the reset in the documentation here to be very specific, but if reset is always the same as boot then I agree it makes sense to change it to "nanoseconds since boot".

I'll add in the documentation that the nanoseconds being referred to here are from the the sched_clock and add the link you shared for reference.

73

Agreed, will revert back to the plain SP.

lldb/source/Plugins/Process/Linux/IntelPTManager.cpp
261

I agree that in its current state this isn't acting as a Get* function at all. However, after the comments @wallace left above, this function acts more like a Get* method as it returns the value of SP inside of the g_tsc_rate to be stored by tsc_rate

@davidca in point 3, are you suggesting that we move this field to be a member variable of IntelPTManager as opposed to a static variable because these conversion params change and thus should be fetched for each instance of the class?

lldb/source/Plugins/Trace/intel-pt/TraceIntelPT.cpp
218

Definitely, added that to the list of planned tests (in addition to malformed response JSON)

jj10306 updated this revision to Diff 413985.Mar 8 2022, 6:47 PM
jj10306 marked 2 inline comments as done.
jj10306 edited the summary of this revision. (Show Details)

Addressed most of the previous diff's comments (other than the ones from @wallace and @davidca that were explicitly mentioned in my previous comment). The naming of the conversion values is currently inconsistent, but I will update the naming and make it consistent across all the new changes once the name is agreed upon.

  • Add client and server side caching of conversion values
  • Update DecodedThread to allow for access to the conversion values from the TraceCursor
  • Add GetNanos (to be renamed) API to TraceCursor to allow for conversion of a trace instruction's TSC value to nanoseconds
  • Update trace schema and trace save commands to accommodate schema changes
  • Add resource_handle namespace to allow for reuse of file descriptor and munmap resource handles
  • Rename IntelPTManager to IntelPTCollector
wallace added inline comments.Mar 8 2022, 8:35 PM
lldb/docs/lldb-gdb-remote.txt
455

The ? After the key name makes Optional<> redundant. Just keep the ?

457

perf-linux might be a better name

458

All the supported kinds must be present below in the documentation, so no need to mention c++ classes here

lldb/include/lldb/Target/TraceCursor.h
193

What about just naming it GetTimestamp and return std::chrono::nanoseconds? That way we use c++ time conversion libraries and we don't include the time granularity in the name

lldb/include/lldb/Utility/TraceIntelPTGDBRemotePackets.h
46

This class is generic, so better not enforce the meaning of the zero tsc. Each trace plugin might be opinionated regarding this timestamp

64

Return a std:: Chrono type. You might want rename this to ToWallTime

lldb/source/Plugins/Process/Linux/IntelPTManager.h
53

Nice

223

Now that we don't have caching, try to move this method to Process Linux, as an static, so that others can use that method more easily.

253

We don't need caching anymore. Just calculate it every time we request the state, following the information that David gave that the rate might change over time.

jj10306 marked 10 inline comments as done.Mar 9 2022, 6:56 AM
jj10306 added inline comments.
lldb/include/lldb/Target/TraceCursor.h
193

sgtm - do you think the GetTimestampCounter API's name should remain the same or change it to something like GetTsc? This file is trace agnostic so tying it to TSC could be confusing since, afaiu, TSC is intel specific, but I noticed the docs of that method mention "TSC", so curious to hear your thoughts.

lldb/include/lldb/Utility/TraceIntelPTGDBRemotePackets.h
43

Bumping this discussion related to naming

46

Good point. Given that this class is generic, would it make more sense to move it from this trace specific file TraceIntelPTGDBRemotePackets.h to TraceGDBRemotePackets.h since that file is trace agnostic?

The main change is to make the counter conversion generic, so that we don't need to add more APIs in the future. Besides that, you don't need to make any drastic change. Good job so far!

lldb/docs/lldb-gdb-remote.txt
455

let's make this more generic for any kind of counter we might want to include in a trace, including ones that could come from PT_WRITE, so let's call it counter_conversion

455–463
476–502

someone who wants to know more about these fields can simply open the perf_event documentation, so let's just point to it to reduce the amount of text here

lldb/include/lldb/Target/Trace.h
312

json_string is not a good name. Let's call it json_response

lldb/include/lldb/Target/TraceCursor.h
183–190

refactor this into a new function generic for all possible counters

you will need to create a new enum in lldb-enumerations.h

enum TraceCounter {

// Timestamp counter (TSC), like the one offered by Intel CPUs.
eTraceCounterTSC,

};

193

just replied above about having a generic method for all kinds of counter. Regarding this one, let's refactor it into

// Get the timestamp associated with the current instruction.
//
// Each trace plug-in might use a different 0 timestamp, e.g. time since last clock reset, time since the program started, etc.
// Also, it might happen that only some instructions of the entire trace have a timestamp associated with them.
//
//   \return
//      The timestamp associated with the current instruction, or llvm::None if not available.
virtual llvm::Optional<std::chrono::nanoseconds> GetTimestamp() = 0;
lldb/include/lldb/Utility/TraceIntelPTGDBRemotePackets.h
46

I imagine that with generic counter conversions we can't have a simple class that works for everything, but we might be able to do this:

template<typename ToType>
class TraceCounterConversion {
  virtual ~TraceCounterConversion() = default;
  virtual ToType ToNanos(uint64_t raw_counter) = 0;
  virtual llvm::json::Value toJSON() = 0;
}

also move it to TraceGDBRemotePackets as you mentioned.

Then the using statement from below could be specific for TSCs

using TraceTSCCounterRateSP = std::shared_ptr<TraceCounterConversion<uint64_t>> ;

That should work, as the intel pt plugin might explicit want a conversion type to uint64_t for its time conversions. Other hypothetical counters might be converted to double or int64_t, for example.

60
class LinuxPerfTscConversion: public TraceCounterConversion<uint64_t> {
lldb/source/Plugins/Process/Linux/IntelPTManager.cpp
1

rename this file to IntelPTCollector

699

rename this to FetchTscRate and move it to a new file lldb/source/Plugins/Process/Linux/Perf.h
you might want to move all the resource_handle functions to that file as well

700

This is the incorrect logging type. Create a new logging type POSIXLog::Trace. Ptrace is the name of the low-level linux tool used for debugging programs. Also change the logging type of the TraceStart method to match this new one

714

append the message of the errno with std::strerror(errno)

now that i think of it, you don't need to log anything here. Just return descriptive error messages and the caller should decide what to do with the errors.

724–725

return the error. The caller should decide whether to fail or to ignore the error

727

include the strerror

738–739

don't log, just return the error

747–754

we don't have caching, so the code should be like

lldb/source/Plugins/Process/Linux/NativeProcessLinux.cpp
315

nice

lldb/source/Plugins/Trace/intel-pt/TraceCursorIntelPT.cpp
94–99

instead of having the decoded thread hand the tsc conversion params all the way up to here, you can create a new method llvm::Optional<std::chrono::nanoseconds> GetTimestamp(uint64_t insn_index)

let's try to have this method as simple as possible and put the implementation details at a lower level when possible

lldb/source/Plugins/Trace/intel-pt/TraceIntelPTSessionFileParser.cpp
34–39
lldb/source/Target/TraceInstructionDumper.cpp
188–191

just like the m_show_tsc boolean there, you should add a m_show_realtime boolean. We can print the times in milliseconds, just like

[123.23ms]

printing in nanoseconds might be too much granularity for being readable

wallace requested changes to this revision.Mar 9 2022, 9:39 PM
This revision now requires changes to proceed.Mar 9 2022, 9:39 PM
wallace added inline comments.Mar 10 2022, 5:40 AM
lldb/docs/lldb-gdb-remote.txt
455

After some more thoughts, let's just call this field 'counters'. And we can put any information related to any kind of counters.

So, we have something like

"counters"?: [{

... The tsc conversion object

}]

Due to the size and expanding scope of this diff, I'm going to split this into several smaller diffs.

jj10306 abandoned this revision.Jun 15 2022, 10:16 AM