Messages in this thread | | | Subject | Re: [PATCH 03/22] perf/x86/intel: Support adaptive PEBSv4 | From | "Liang, Kan" <> | Date | Tue, 19 Mar 2019 17:20:04 -0400 |
| |
On 3/19/2019 10:47 AM, Peter Zijlstra wrote: >> @@ -933,6 +998,19 @@ pebs_update_state(bool needed_cb, struct cpu_hw_events *cpuc, struct pmu *pmu) >> update = true; >> } >> >> + if (x86_pmu.intel_cap.pebs_baseline && add) { >> + u64 pebs_data_cfg; >> + >> + pebs_data_cfg = pebs_update_adaptive_cfg(event); >> + >> + /* Update pebs_record_size if new event requires more data. */ >> + if (pebs_data_cfg & ~cpuc->pebs_data_cfg) { >> + cpuc->pebs_data_cfg |= pebs_data_cfg; >> + adaptive_pebs_record_size_update(); >> + update = true; >> + } >> + } >> + >> if (update) >> pebs_update_threshold(cpuc); >> } > Hurmph.. this only grows the PEBS record. >
Yes, the PEBS record doesn't shrink on the del. Because we have to go through all the existing pebs events for an accurate config. I think it doesn't worth it. There is no harmful for a bigger PEBS record, except little performance impacts. But that's rare case. For most cases, we usually apply the same pebs config for all pebs events.
> >> @@ -947,7 +1025,7 @@ void intel_pmu_pebs_add(struct perf_event *event) >> if (hwc->flags & PERF_X86_EVENT_LARGE_PEBS) >> cpuc->n_large_pebs++; >> >> - pebs_update_state(needed_cb, cpuc, event->ctx->pmu); >> + pebs_update_state(needed_cb, cpuc, event, true); >> } >> >> void intel_pmu_pebs_enable(struct perf_event *event) >> @@ -965,6 +1043,14 @@ void intel_pmu_pebs_enable(struct perf_event *event) >> else if (event->hw.flags & PERF_X86_EVENT_PEBS_ST) >> cpuc->pebs_enabled |= 1ULL << 63; >> >> + if (x86_pmu.intel_cap.pebs_baseline) { >> + hwc->config |= ICL_EVENTSEL_ADAPTIVE; >> + if (cpuc->pebs_data_cfg != cpuc->active_pebs_data_cfg) { >> + wrmsrl(MSR_PEBS_DATA_CFG, cpuc->pebs_data_cfg); >> + cpuc->active_pebs_data_cfg = cpuc->pebs_data_cfg; >> + } >> + } >> + >> /* >> * Use auto-reload if possible to save a MSR write in the PMI. >> * This must be done in pmu::start(), because PERF_EVENT_IOC_PERIOD. >> @@ -991,7 +1077,12 @@ void intel_pmu_pebs_del(struct perf_event *event) >> if (hwc->flags & PERF_X86_EVENT_LARGE_PEBS) >> cpuc->n_large_pebs--; >> >> - pebs_update_state(needed_cb, cpuc, event->ctx->pmu); >> + /* Clear both pebs_data_cfg and pebs_record_size for first PEBS. */ > Weird comment.. > >> + if (x86_pmu.intel_cap.pebs_baseline && !cpuc->n_pebs) { >> + cpuc->pebs_data_cfg = 0; >> + cpuc->pebs_record_size = sizeof(struct pebs_basic); >> + } >> + pebs_update_state(needed_cb, cpuc, event, false); > Why do we have to reset record_size? That'll be updated in > pebs_update_state() on the next add. >
The record_size should be reset for the first PEBS events. Right, I can move the reset in pebs_update_state().
Thanks, Kan
| |