mirrors/reverie
1
0
Fork 0
mirror of https://github.com/facebookexperimental/reverie.git synced 2025-01-23 05:06:23 +00:00
Code Issues Projects Releases Packages Wiki Activity Actions

Adding retry handing for precise timers to account for anomalous wakeups

Browse source 
Summary: Something appears to be causing timer marker events to randomly be sent early or possibly multiple times. This commit adds some sanity check and retry handling to ensure that early wakeups do not impact the implementing tool or at least long runs of single steps (and thus the timeouts that originally sparked this investigation)

Reviewed By: jasonwhite

Differential Revision: D33828008

fbshipit-source-id: 008bf1793964053405643640155269603c4e6bcc
This commit is contained in:
Kevin Guthrie 2022-02-04 13:46:40 -08:00 committed by Facebook GitHub Bot
parent eed2eba46a
commit e5e0e2983d
2 changed files with 154 additions and 31 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

61
reverie-ptrace/src/timer.rs
View file

@ -323,6 +323,20 @@ enum ActiveEvent {
},
}
impl ActiveEvent {
/// Given the current clock, determine if another event is required to get the
/// clock to its expected state
fn reschedule_if_spurious_wakeup(&self, curr_clock: u64) -> Option<TimerEventRequest> {
match self {
ActiveEvent::Precise { clock_target } => (clock_target.saturating_sub(curr_clock)
> MAX_SINGLE_STEP_COUNT)
.then(|| TimerEventRequest::Precise(*clock_target - curr_clock)),
ActiveEvent::Imprecise { clock_min } => (*clock_min > curr_clock)
.then(|| TimerEventRequest::Imprecise(*clock_min - curr_clock)),
}
}
}
impl EventStatus {
pub fn next(self) -> Self {
match self {
@ -356,11 +370,14 @@ const SKID_MARGIN_RCBS: u64 = 50;
/// about perf event throttling, which isn't well-documented.
const SINGLESTEP_TIMEOUT_RCBS: u64 = 5;
/// The maximum single step count we expect can occur when a precise timer event
/// is requested that leaves less than the minimum perf timeout remaining.
const MAX_SINGLE_STEP_COUNT: u64 = SKID_MARGIN_RCBS + SINGLESTEP_TIMEOUT_RCBS;
impl TimerImpl {
pub fn new(guest_pid: Pid, guest_tid: Tid) -> Result<Self, Errno> {
let has_debug_store = CpuId::new()
.get_feature_info()
.map_or(false, |info| info.has_ds());
let cpu = CpuId::new();
let has_debug_store = cpu.get_feature_info().map_or(false, |info| info.has_ds());
let evt = Event::Raw(get_rcb_perf_config());
@ -371,6 +388,10 @@ impl TimerImpl {
.event(evt);
// Check if we can set precise_ip = 1 by checking if debug store is enabled.
debug!(
"Setting precise_ip to {} for cpu {:?}",
has_debug_store, cpu
);
if has_debug_store {
// set precise_ip to lowest value to enable PEBS (TODO: AMD?)
builder.precise_ip(1);
@ -470,6 +491,8 @@ impl TimerImpl {
pub fn finalize_requests(&self) {
if self.send_artificial_signal {
debug!("Sending artificial timer signal");
// Give the guest a kick via an "artificial signal". This gives us something
// to handle in `handle_signal` and thus drives single-stepping.
Errno::result(unsafe {
@ -509,15 +532,30 @@ impl TimerImpl {
// At this point, we've decided that a timer event is to be delivered.
// Ensure any new timer signals don't mess with us while single-stepping
self.disable_timer_before_stepping();
// Last check to see if this an unexpected wakeup (a signal before the minimum expected)
let ctr = self.read_clock();
if let Some(additional_timer_request) = self.event.reschedule_if_spurious_wakeup(ctr) {
warn!("Spurious wakeup - rescheduling new timer event");
if let Err(errno) = self.request_event(additional_timer_request) {
warn!(
"Attempted to reschedule a timer signal after an early wakeup, but failed with - {:?}. A panic will likely follow",
errno
);
} else {
return Err(HandleFailure::Cancelled(task));
};
}
// Before we drive the event to completion, clear `send_artificial_signal` flag so that:
// - another signal isn't generated anytime Timer::finalize_requests() is called
// - spurious SIGSTKFLTs aren't let errantly let through
// Cancellations should prevent spurious timer events in any case.
self.send_artificial_signal = false;
// Ensure any new timer signals don't mess with us while single-stepping
self.disable_timer_before_stepping();
let ctr = self.read_clock();
match self.event {
ActiveEvent::Precise { clock_target } => {
self.attempt_single_step(task, ctr, clock_target).await
@ -527,7 +565,7 @@ impl TimerImpl {
"Imprecise timer event delivered. Ctr val: {}, min val: {}",
ctr, clock_min
);
assert!(ctr >= clock_min);
assert!(ctr >= clock_min, "ctr = {}, clock_min = {}", ctr, clock_min);
Ok(task)
}
}
@ -547,6 +585,15 @@ impl TimerImpl {
ctr,
target
);
assert!(
target - ctr <= MAX_SINGLE_STEP_COUNT,
"Single steps from {} to {} requested ({} steps), but that exceeds the skid margin + minimum perf timer steps ({}). \
This probably indicates a bug",
ctr,
target,
(target - ctr),
MAX_SINGLE_STEP_COUNT
);
debug!("Timer will single-step from ctr {} to {}", ctr, target);
let mut task = task;
loop {

124
reverie-ptrace/src/validation.rs
View file

@ -7,7 +7,7 @@
* LICENSE file in the root directory of this source tree.
*/
use crate::perf::do_branches;
use crate::perf::{do_branches, PerfCounter};
use crate::timer::{get_rcb_perf_config, AMD_VENDOR, INTEL_VENDOR};
use core::mem;
use perf_event_open_sys::bindings as perf;
@ -72,7 +72,11 @@ pub(crate) enum PmuValidationError {
IntelXenPmiBugDetected,
}
fn init_perf_event_attr(perf_attr_type: u32, config: u64) -> perf::perf_event_attr {
fn init_perf_event_attr(
perf_attr_type: u32,
config: u64,
precise_ip: bool,
) -> perf::perf_event_attr {
let mut result = perf::perf_event_attr {
type_: perf_attr_type,
config,
@ -82,19 +86,39 @@ fn init_perf_event_attr(perf_attr_type: u32, config: u64) -> perf::perf_event_at
result.set_exclude_guest(1);
result.set_exclude_kernel(1);
if precise_ip
&& CpuId::new()
.get_feature_info()
.map_or(false, |info| info.has_ds())
{
result.set_precise_ip(1);
// This prevents EINVAL when creating a counter with precise_ip enabled
result.__bindgen_anon_1.sample_period = PerfCounter::DISABLE_SAMPLE_PERIOD;
} else {
// This is the value used for the bug checks which are not originally designed to
// work with precise_ip
result.__bindgen_anon_1.sample_period = 0;
}
result
}
/// Create a template perf_event_attr for ticks
fn ticks_attr() -> perf::perf_event_attr {
init_perf_event_attr(perf::perf_type_id_PERF_TYPE_RAW, get_rcb_perf_config())
fn ticks_attr(precise_ip: bool) -> perf::perf_event_attr {
init_perf_event_attr(
perf::perf_type_id_PERF_TYPE_RAW,
get_rcb_perf_config(),
precise_ip,
)
}
/// Create a template perf_event_attr for cycles
fn cycles_attr() -> perf::perf_event_attr {
fn cycles_attr(precise_ip: bool) -> perf::perf_event_attr {
init_perf_event_attr(
perf::perf_type_id_PERF_TYPE_HARDWARE,
perf::perf_hw_id_PERF_COUNT_HW_CPU_CYCLES.into(),
precise_ip,
)
}
@ -120,18 +144,21 @@ impl Drop for ScopedFd {
/// It checks for a collection of processor features that ensure that the pmu features
/// required from Reverie to function correctly are available and trustworthy
pub(crate) fn check_for_pmu_bugs() -> Result<(), PmuValidationError> {
check_for_ioc_period_bug()
.and(check_working_counters())
.and(check_for_arch_bugs())
check_for_ioc_period_bug(false)?;
check_working_counters(false)?;
check_for_arch_bugs(false)?;
check_for_ioc_period_bug(true)?;
check_working_counters(true)?;
check_for_arch_bugs(true)
}
/// This function is transcribed from the function with the same name in
/// [Mozilla-RR](https://github.com/rr-debugger/rr/blob/master/src/PerfCounters.cc#L227)
/// Checks for a bug in (supposedly) Linux Kernel < 3.7 where period changes
/// do not happen until after the _next_ rollover.
fn check_for_ioc_period_bug() -> Result<(), PmuValidationError> {
fn check_for_ioc_period_bug(precise_ip: bool) -> Result<(), PmuValidationError> {
// Start a cycles counter
let mut attr = ticks_attr();
let mut attr = ticks_attr(precise_ip);
attr.__bindgen_anon_1.sample_period = 0xffffffff;
attr.set_exclude_callchain_kernel(1);
let bug_fd = start_counter(0, -1, &mut attr, None)?;
@ -272,11 +299,9 @@ fn read_counter(fd: &ScopedFd) -> Result<i64, PmuValidationError> {
/// Transcription of the function with the same name in mozilla-rr to check
/// for the bug where hardware counters simply don't work or only one hardware
/// counter works
fn check_working_counters() -> Result<(), PmuValidationError> {
let mut attr = ticks_attr();
attr.__bindgen_anon_1.sample_period = 0;
let mut attr2 = cycles_attr();
attr2.__bindgen_anon_1.sample_period = 0;
fn check_working_counters(precise_ip: bool) -> Result<(), PmuValidationError> {
let mut attr = ticks_attr(precise_ip);
let mut attr2 = cycles_attr(precise_ip);
let fd = start_counter(0, -1, &mut attr, None)?;
let fd2 = start_counter(0, -1, &mut attr2, None)?;
@ -336,7 +361,7 @@ fn is_amd_zen(cpu_feature: FeatureInfo) -> bool {
/// This is a transcription of the function with the same name in Mozilla-RR it will
/// check for bugs specific to cpu architectures
fn check_for_arch_bugs() -> Result<(), PmuValidationError> {
fn check_for_arch_bugs(precise_ip: bool) -> Result<(), PmuValidationError> {
let c = CpuId::new();
let vendor = c.get_vendor_info().unwrap();
let feature_info = c
@ -346,7 +371,7 @@ fn check_for_arch_bugs() -> Result<(), PmuValidationError> {
match vendor_str {
AMD_VENDOR if is_amd_zen(feature_info) => check_for_zen_speclockmap(),
INTEL_VENDOR => check_for_kvm_in_txcp_bug().and(check_for_xen_pmi_bug()),
INTEL_VENDOR => check_for_kvm_in_txcp_bug().and(check_for_xen_pmi_bug(precise_ip)),
s => panic!("Unknown CPU vendor: {}", s),
}
}
@ -361,7 +386,7 @@ fn check_for_zen_speclockmap() -> Result<(), PmuValidationError> {
// 0x25 == RETIRED_LOCK_INSTRUCTIONS - Counts the number of retired locked instructions
// + 0x08 == SPECLOCKMAPCOMMIT
let mut attr = init_perf_event_attr(perf::perf_type_id_PERF_TYPE_RAW, 0x510825);
let mut attr = init_perf_event_attr(perf::perf_type_id_PERF_TYPE_RAW, 0x510825, false);
let fd = start_counter(0, -1, &mut attr, None)?;
@ -389,7 +414,7 @@ fn check_for_zen_speclockmap() -> Result<(), PmuValidationError> {
fn check_for_kvm_in_txcp_bug() -> Result<(), PmuValidationError> {
let mut count: i64 = 0;
let mut attr = ticks_attr();
let mut attr = ticks_attr(false);
attr.config |= IN_TXCP;
attr.__bindgen_anon_1.sample_period = 0;
let mut disabled_txcp = false;
@ -412,9 +437,9 @@ fn check_for_kvm_in_txcp_bug() -> Result<(), PmuValidationError> {
}
}
fn check_for_xen_pmi_bug() -> Result<(), PmuValidationError> {
fn check_for_xen_pmi_bug(precise_ip: bool) -> Result<(), PmuValidationError> {
let mut count: i32;
let mut attr = ticks_attr();
let mut attr = ticks_attr(precise_ip);
attr.__bindgen_anon_1.sample_period = NUM_BRANCHES - 1;
let fd = start_counter(0, -1, &mut attr, None)?;
@ -539,7 +564,7 @@ mod test {
#[test]
fn test_check_for_ioc_period_bug() {
// This assumes the machine running the test will not have this bug
if let Err(pmu_err) = check_for_ioc_period_bug() {
if let Err(pmu_err) = check_for_ioc_period_bug(false) {
panic!("Ioc period bug check failed - {}", pmu_err);
}
}
@ -547,7 +572,7 @@ mod test {
#[test]
fn test_check_working_counters() {
// This assumes the machine running the test will have working counters
if let Err(pmu_err) = check_working_counters() {
if let Err(pmu_err) = check_working_counters(false) {
panic!("Working counters check failed - {}", pmu_err);
}
}
@ -555,8 +580,59 @@ mod test {
#[test]
fn test_check_for_arch_bugs() {
// This assumes the machine running the test will not have arch bugs
if let Err(pmu_err) = check_for_arch_bugs() {
if let Err(pmu_err) = check_for_arch_bugs(false) {
panic!("Architecture-specific bug check failed - {}", pmu_err);
}
}
#[test]
fn test_check_for_ioc_period_bug_precise_ip() {
// This assumes the machine running the test will not have this bug and only runs
// if precise_ip will be enabled
if CpuId::new()
.get_feature_info()
.map_or(false, |info| info.has_ds())
{
if let Err(pmu_err) = check_for_ioc_period_bug(true) {
panic!(
"Ioc period bug check failed when precise_ip was enabled - {}",
pmu_err
);
}
}
}
#[test]
fn test_check_working_counters_precise_ip() {
// This assumes the machine running the test will have working counters and only runs
// if precise_ip will be enabled
if CpuId::new()
.get_feature_info()
.map_or(false, |info| info.has_ds())
{
if let Err(pmu_err) = check_working_counters(true) {
panic!(
"Working counters check failed when precise_ip was enabled - {}",
pmu_err
);
}
}
}
#[test]
fn test_check_for_arch_bugs_precise_ip() {
// This assumes the machine running the test will not have arch bugs and only runs
// if precise_ip will be enabled
if CpuId::new()
.get_feature_info()
.map_or(false, |info| info.has_ds())
{
if let Err(pmu_err) = check_for_arch_bugs(true) {
panic!(
"Architecture-specific bug check failed when precise_ip was enabled - {}",
pmu_err
);
}
}
}
}