Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(EventDescription): Turn EventDescription into real class #276

Closed
wants to merge 2 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions include/lo2s/perf/counter/counter_collection.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -39,11 +39,11 @@ struct CounterCollection
{
if (index == 0)
{
return leader.scale;
return leader.scale();
}
else
{
return counters[index - 1].scale;
return counters[index - 1].scale();
}
}
friend bool operator==(const CounterCollection& lhs, const CounterCollection& rhs)
Expand Down
155 changes: 136 additions & 19 deletions include/lo2s/perf/event_description.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,10 @@

#pragma once

#include <lo2s/config.hpp>
#include <lo2s/error.hpp>
#include <lo2s/execution_scope.hpp>
#include <lo2s/perf/util.hpp>
#include <lo2s/topology.hpp>

#include <set>
Expand All @@ -48,16 +51,54 @@ struct EventDescription
{
EventDescription(const std::string& name, perf_type_id type, std::uint64_t config,
std::uint64_t config1 = 0, std::set<Cpu> cpus = std::set<Cpu>(),
double scale = 1, std::string unit = "#",
Availability availability = Availability::UNAVAILABLE)
: name(name), type(type), config(config), config1(config1), scale(scale), unit(unit),
availability(availability), cpus_(cpus)
double scale = 1, std::string unit = "#")
: name_(name), type_(type), config_(config), config1_(config1), scale_(scale), unit_(unit),
cpus_(cpus)
{
struct perf_event_attr attr = perf_event_attr();

int proc_fd = perf_event_open(&attr, ExecutionScope(Thread(0)), -1, 0);
int sys_fd = perf_event_open(&attr, ExecutionScope(*supported_cpus().begin()), -1, 0);

if (sys_fd == -1 && proc_fd == -1)
{
attr.exclude_kernel = 1;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why don't we just open it with exclude_kernel, if it's for testing only anyways?

proc_fd = perf_event_open(&attr, ExecutionScope(Thread(0)), -1, 0);
sys_fd = perf_event_open(&attr, ExecutionScope(*supported_cpus().begin()), -1, 0);
}

if (sys_fd == -1 && proc_fd == -1)
{
switch (errno)
{
case ENOTSUP:
Log::debug() << "perf event not supported by the running kernel: " << name_;
break;
default:
Log::debug() << "perf event " << name_
<< " not available: " << std::string(std::strerror(errno));
break;
}

availability_ = Availability::UNAVAILABLE;
}
else if (sys_fd == -1)
{
availability_ = Availability::PROCESS_MODE;
}
else if (proc_fd == -1)
{
availability_ = Availability::SYSTEM_MODE;
}
else
{
availability_ = Availability::UNIVERSAL;
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We need an Fd RAII object. sys_fd and proc_fd are never closed, if they succeed in opening.

}

EventDescription()
: name(""), type(static_cast<perf_type_id>(-1)), config(0), config1(0), scale(1), unit("#"),
availability(Availability::UNAVAILABLE)
: name_(""), type_(static_cast<perf_type_id>(-1)), config_(0), config1_(0), scale_(1),
unit_("#"), availability_(Availability::UNAVAILABLE)
{
}

Expand All @@ -79,30 +120,106 @@ struct EventDescription

friend bool operator==(const EventDescription& lhs, const EventDescription& rhs)
{
return (lhs.type == rhs.type) && (lhs.config == rhs.config) && (lhs.config1 == rhs.config1);
return (lhs.type_ == rhs.type_) && (lhs.config_ == rhs.config_) &&
(lhs.config1_ == rhs.config1_);
}

friend bool operator<(const EventDescription& lhs, const EventDescription& rhs)
{
if (lhs.type == rhs.type)
if (lhs.type_ == rhs.type_)
{
if (lhs.config == rhs.config)
if (lhs.config_ == rhs.config_)
{
return lhs.config1 < rhs.config1;
return lhs.config1_ < rhs.config1_;
}
return lhs.config < rhs.config;
return lhs.config_ < rhs.config_;
}
return lhs.type_ < rhs.type_;
}

struct perf_event_attr perf_event_attr() const
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
struct perf_event_attr perf_event_attr() const
perf_event_attr attr() const

Should just be called attr

{
struct perf_event_attr attr;
memset(&attr, 0, sizeof(struct perf_event_attr));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
memset(&attr, 0, sizeof(struct perf_event_attr));
std::memset(&attr, 0, sizeof(struct perf_event_attr));

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, missing header: <cstring>


attr.size = sizeof(struct perf_event_attr);

attr.type = type_;
attr.config = config_;
attr.config1 = config1_;

return attr;
}

std::string name() const
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
std::string name() const
const& std::string name() const

{
return name_;
}

std::string description() const
{
if (availability_ == Availability::UNIVERSAL)
{
return name_;
}
else if (availability_ == Availability::SYSTEM_MODE)
{
return fmt::format("{} [SYS]", name_);
}
else if (availability_ == Availability::PROCESS_MODE)
{
return fmt::format("{} [PROC]", name_);
}

return "";
}

bool is_valid() const
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
bool is_valid() const
bool is_available() const

{
return availability_ != Availability::UNAVAILABLE;
}

double scale() const
{
return scale_;
}

std::string unit() const
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
std::string unit() const
const& std::string unit() const

{
return unit_;
}

int open_counter(ExecutionScope scope, int group_fd)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We really need an Fd RAII object.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Having said that. I don't like the fact, that some parts of the code use this method and others are using perf_event_attr() instead.

{
struct perf_event_attr perf_attr = perf_event_attr();
perf_attr.sample_period = 0;
perf_attr.exclude_kernel = config().exclude_kernel;
// Needed when scaling multiplexed events, and recognize activation phases
perf_attr.read_format = PERF_FORMAT_TOTAL_TIME_ENABLED | PERF_FORMAT_TOTAL_TIME_RUNNING;

#ifndef USE_HW_BREAKPOINT_COMPAT
perf_attr.use_clockid = config().use_clockid;
perf_attr.clockid = config().clockid;
#endif

int fd = perf_try_event_open(&perf_attr, scope, group_fd, 0, config().cgroup_fd);
if (fd < 0)
{
Log::error() << "perf_event_open for counter failed";
throw_errno();
}
return lhs.type < rhs.type;
return fd;
}
std::string name;
perf_type_id type;
std::uint64_t config;
std::uint64_t config1;
double scale;
std::string unit;
Availability availability;

private:
std::string name_;
perf_type_id type_;
std::uint64_t config_;
std::uint64_t config1_;
double scale_;
std::string unit_;
Availability availability_;

std::set<Cpu> cpus_;
};
} // namespace perf
Expand Down
37 changes: 1 addition & 36 deletions include/lo2s/perf/event_provider.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -36,42 +36,7 @@ namespace perf
class EventProvider
{
public:
struct DescriptionCache
{
private:
DescriptionCache()
: description(std::string(), static_cast<perf_type_id>(-1), 0, 0), valid_(false)
{
}

public:
DescriptionCache(const EventDescription& description)
: description(description), valid_(true)
{
}

DescriptionCache(EventDescription&& description)
: description(std::move(description)), valid_(true)
{
}

static DescriptionCache make_invalid()
{
return DescriptionCache();
}

bool is_valid() const
{
return valid_;
}

EventDescription description;

private:
bool valid_;
};

using EventMap = std::unordered_map<std::string, DescriptionCache>;
using EventMap = std::unordered_map<std::string, EventDescription>;

EventProvider();
EventProvider(const EventProvider&) = delete;
Expand Down
30 changes: 17 additions & 13 deletions include/lo2s/perf/sample/reader.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -84,15 +84,7 @@ class Reader : public EventReader<T>
Log::debug() << "initializing event_reader for:" << scope.name()
<< ", enable_on_exec: " << enable_on_exec;

struct perf_event_attr perf_attr = common_perf_event_attrs();

if (config().use_pebs)
{
perf_attr.use_clockid = 0;
}

perf_attr.exclude_kernel = config().exclude_kernel;
perf_attr.sample_period = config().sampling_period;
struct perf_event_attr perf_attr;

if (config().sampling)
{
Expand All @@ -103,19 +95,31 @@ class Reader : public EventReader<T>

Log::debug() << "using sampling event \'" << config().sampling_event
<< "\', period: " << config().sampling_period;

perf_attr.type = sampling_event.type;
perf_attr.config = sampling_event.config;
perf_attr.config1 = sampling_event.config1;
perf_attr = sampling_event.perf_event_attr();

perf_attr.mmap = 1;
perf_attr.disabled = 1;

#ifndef USE_HW_BREAKPOINT_COMPAT
perf_attr.use_clockid = config().use_clockid;
perf_attr.clockid = config().clockid;
#endif
// When we poll on the fd given by perf_event_open, wakeup, when our buffer is 80% full
// Default behaviour is to wakeup on every event, which is horrible performance wise
perf_attr.watermark = 1;
perf_attr.wakeup_watermark =
static_cast<uint32_t>(0.8 * config().mmap_pages * get_page_size());
Comment on lines +103 to +111
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🍝

}
else
{
perf_attr = common_perf_event_attrs();

// Set up a dummy event for recording calling context enter/leaves only
perf_attr.type = PERF_TYPE_SOFTWARE;
perf_attr.config = PERF_COUNT_SW_DUMMY;
}
perf_attr.exclude_kernel = config().exclude_kernel;
perf_attr.sample_period = config().sampling_period;

perf_attr.sample_id_all = 1;
// Generate PERF_RECORD_COMM events to trace changes to the command
Expand Down
3 changes: 1 addition & 2 deletions include/lo2s/perf/util.hpp
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
#pragma once

#include <lo2s/perf/event_description.hpp>
#include <lo2s/execution_scope.hpp>

extern "C"
{
Expand All @@ -19,7 +19,6 @@ struct perf_event_attr common_perf_event_attrs();
void perf_warn_paranoid();
void perf_check_disabled();

int perf_event_description_open(ExecutionScope scope, const EventDescription& desc, int group_fd);
int perf_try_event_open(struct perf_event_attr* perf_attr, ExecutionScope scope, int group_fd,
unsigned long flags, int cgroup_fd = -1);

Expand Down
4 changes: 2 additions & 2 deletions include/lo2s/trace/trace.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -171,9 +171,9 @@ class Trace
otf2::definition::metric_member& get_event_metric_member(perf::EventDescription event)
{
return registry_.emplace<otf2::definition::metric_member>(
ByEventDescription(event), intern(event.name), intern(event.name),
ByEventDescription(event), intern(event.name()), intern(event.name()),
otf2::common::metric_type::other, otf2::common::metric_mode::accumulated_start,
otf2::common::type::Double, otf2::common::base_type::decimal, 0, intern(event.unit));
otf2::common::type::Double, otf2::common::base_type::decimal, 0, intern(event.unit()));
}
otf2::definition::metric_class& perf_metric_class(MeasurementScope scope)
{
Expand Down
26 changes: 3 additions & 23 deletions src/config.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -74,30 +74,10 @@ static inline void print_availability(std::ostream& os, const std::string& descr
std::vector<std::string> event_names;
for (const auto& ev : events)
{
if (ev.availability == perf::Availability::UNAVAILABLE)
if (ev.is_valid())
{
continue;
event_names.push_back(ev.description());
}

std::string availability = "";
std::string cpu = "";
if (ev.availability == perf::Availability::PROCESS_MODE)
{
availability = " *";
}
else if (ev.availability == perf::Availability::SYSTEM_MODE)
{
availability = " #";
}
if (ev.supported_cpus() != Topology::instance().cpus())
{
const auto& cpus = ev.supported_cpus();
cpu =
fmt::format(" [ CPUs {}-{} ]", std::min_element(cpus.begin(), cpus.end())->as_int(),
std::max_element(cpus.begin(), cpus.end())->as_int());
}

event_names.push_back(ev.name + availability + cpu);
}
list_arguments_sorted(os, description, event_names);
}
Expand Down Expand Up @@ -670,7 +650,7 @@ void parse_program_options(int argc, const char** argv)
{
for (const auto& mem_event : platform::get_mem_events())
{
perf_group_events.emplace_back(mem_event.name);
perf_group_events.emplace_back(mem_event.name());
}
perf_group_events.emplace_back("instructions");
perf_group_events.emplace_back("cpu-cycles");
Expand Down
Loading