| Message ID | 1546878450-20341-11-git-send-email-andrew.murray@arm.com (mailing list archive) |
|---|---|
| State | Not Applicable |
| Headers | show |
| Series | perf/core: Generalise event exclusion checking | expand |
| Context | Check | Description |
|---|---|---|
| snowpatch_ozlabs/apply_patch | success | next/apply_patch Successfully applied |
| snowpatch_ozlabs/checkpatch | success | total: 0 errors, 0 warnings, 0 checks, 147 lines checked |
On Mon, Jan 07, 2019 at 04:27:27PM +0000, Andrew Murray wrote: > For drivers that do not support context exclusion let's advertise the > PERF_PMU_CAP_NOEXCLUDE capability. This ensures that perf will > prevent us from handling events where any exclusion flags are set. > Let's also remove the now unnecessary check for exclusion flags. > > Signed-off-by: Andrew Murray <andrew.murray@arm.com> > --- > arch/x86/events/amd/ibs.c | 13 +------------ > arch/x86/events/amd/power.c | 10 ++-------- > arch/x86/events/intel/cstate.c | 12 +++--------- > arch/x86/events/intel/rapl.c | 9 ++------- > arch/x86/events/intel/uncore_snb.c | 9 ++------- > arch/x86/events/msr.c | 10 ++-------- > 6 files changed, 12 insertions(+), 51 deletions(-) You (correctly) don't add CAP_NO_EXCLUDE to the main x86 pmu code, but then you also don't check if it handles all the various exclude options correctly/consistently. Now; I must admit that that is a bit of a maze, but I think we can at least add exclude_idle and exclude_hv fails in there, nothing uses those afaict. On the various exclude options; they are as follows (IIUC): - exclude_guest: we're a HV/host-kernel and we don't want the counter to run when we run a guest context. - exclude_host: we're a HV/host-kernel and we don't want the counter to run when we run in host context. - exclude_hv: we're a guest and don't want the counter to run in HV context. Now, KVM always implies exclude_hv afaict (for guests), I'm not sure what, if anything Xen does on x86 (IIRC Brendan Gregg once said perf works on Xen) -- nor quite sure who to ask, Boris, Jeurgen?
On Tue, Jan 08, 2019 at 11:48:41AM +0100, Peter Zijlstra wrote: > On Mon, Jan 07, 2019 at 04:27:27PM +0000, Andrew Murray wrote: > > For drivers that do not support context exclusion let's advertise the > > PERF_PMU_CAP_NOEXCLUDE capability. This ensures that perf will > > prevent us from handling events where any exclusion flags are set. > > Let's also remove the now unnecessary check for exclusion flags. > > > > Signed-off-by: Andrew Murray <andrew.murray@arm.com> > > --- > > arch/x86/events/amd/ibs.c | 13 +------------ > > arch/x86/events/amd/power.c | 10 ++-------- > > arch/x86/events/intel/cstate.c | 12 +++--------- > > arch/x86/events/intel/rapl.c | 9 ++------- > > arch/x86/events/intel/uncore_snb.c | 9 ++------- > > arch/x86/events/msr.c | 10 ++-------- > > 6 files changed, 12 insertions(+), 51 deletions(-) > > You (correctly) don't add CAP_NO_EXCLUDE to the main x86 pmu code, but > then you also don't check if it handles all the various exclude options > correctly/consistently. > > Now; I must admit that that is a bit of a maze, but I think we can at > least add exclude_idle and exclude_hv fails in there, nothing uses those > afaict. Yes it took me some time to make sense of it. As per my comments in the other patch, I think you're suggesting that I add additional checks to x86. I think they are needed but I'd prefer to make functional changes in a separate series, I'm happy to do this. > > On the various exclude options; they are as follows (IIUC): > > - exclude_guest: we're a HV/host-kernel and we don't want the counter > to run when we run a guest context. > > - exclude_host: we're a HV/host-kernel and we don't want the counter > to run when we run in host context. > > - exclude_hv: we're a guest and don't want the counter to run in HV > context. > > Now, KVM always implies exclude_hv afaict (for guests), It certaintly does for ARM. > I'm not sure > what, if anything Xen does on x86 (IIRC Brendan Gregg once said perf > works on Xen) -- nor quite sure who to ask, Boris, Jeurgen? Thanks, Andrew Murray >
On 1/8/19 5:48 AM, Peter Zijlstra wrote: > On Mon, Jan 07, 2019 at 04:27:27PM +0000, Andrew Murray wrote: >> For drivers that do not support context exclusion let's advertise the >> PERF_PMU_CAP_NOEXCLUDE capability. This ensures that perf will >> prevent us from handling events where any exclusion flags are set. >> Let's also remove the now unnecessary check for exclusion flags. >> >> Signed-off-by: Andrew Murray <andrew.murray@arm.com> >> --- >> arch/x86/events/amd/ibs.c | 13 +------------ >> arch/x86/events/amd/power.c | 10 ++-------- >> arch/x86/events/intel/cstate.c | 12 +++--------- >> arch/x86/events/intel/rapl.c | 9 ++------- >> arch/x86/events/intel/uncore_snb.c | 9 ++------- >> arch/x86/events/msr.c | 10 ++-------- >> 6 files changed, 12 insertions(+), 51 deletions(-) > You (correctly) don't add CAP_NO_EXCLUDE to the main x86 pmu code, but > then you also don't check if it handles all the various exclude options > correctly/consistently. > > Now; I must admit that that is a bit of a maze, but I think we can at > least add exclude_idle and exclude_hv fails in there, nothing uses those > afaict. > > On the various exclude options; they are as follows (IIUC): > > - exclude_guest: we're a HV/host-kernel and we don't want the counter > to run when we run a guest context. > > - exclude_host: we're a HV/host-kernel and we don't want the counter > to run when we run in host context. > > - exclude_hv: we're a guest and don't want the counter to run in HV > context. > > Now, KVM always implies exclude_hv afaict (for guests), I'm not sure > what, if anything Xen does on x86 (IIRC Brendan Gregg once said perf > works on Xen) -- nor quite sure who to ask, Boris, Jeurgen? perf does work inside guests. VPMU is managed by the Xen and it presents to the guest only samples that are associated with the guest. So from that perspective exclude_hv doesn't seem to be needed. There is a VPMU mode that allows profiling whole system (host and guests) from dom0, and this where exclude_hv might be useful. But this mode, ahem, needs some work. -boris
On Tue, Jan 08, 2019 at 11:36:33AM -0500, Boris Ostrovsky wrote: > On 1/8/19 5:48 AM, Peter Zijlstra wrote: > > On the various exclude options; they are as follows (IIUC): > > > > - exclude_guest: we're a HV/host-kernel and we don't want the counter > > to run when we run a guest context. > > > > - exclude_host: we're a HV/host-kernel and we don't want the counter > > to run when we run in host context. > > > > - exclude_hv: we're a guest and don't want the counter to run in HV > > context. > > > > Now, KVM always implies exclude_hv afaict (for guests), I'm not sure > > what, if anything Xen does on x86 (IIRC Brendan Gregg once said perf > > works on Xen) -- nor quite sure who to ask, Boris, Jeurgen? > > perf does work inside guests. > > VPMU is managed by the Xen and it presents to the guest only samples > that are associated with the guest. So from that perspective exclude_hv > doesn't seem to be needed. > > There is a VPMU mode that allows profiling whole system (host and > guests) from dom0, and this where exclude_hv might be useful. But this > mode, ahem, needs some work. Thanks Boris!
Peter Zijlstra <peterz@infradead.org> writes: > On Mon, Jan 07, 2019 at 04:27:27PM +0000, Andrew Murray wrote: >> For drivers that do not support context exclusion let's advertise the >> PERF_PMU_CAP_NOEXCLUDE capability. This ensures that perf will >> prevent us from handling events where any exclusion flags are set. >> Let's also remove the now unnecessary check for exclusion flags. >> >> Signed-off-by: Andrew Murray <andrew.murray@arm.com> >> --- >> arch/x86/events/amd/ibs.c | 13 +------------ >> arch/x86/events/amd/power.c | 10 ++-------- >> arch/x86/events/intel/cstate.c | 12 +++--------- >> arch/x86/events/intel/rapl.c | 9 ++------- >> arch/x86/events/intel/uncore_snb.c | 9 ++------- >> arch/x86/events/msr.c | 10 ++-------- >> 6 files changed, 12 insertions(+), 51 deletions(-) > > You (correctly) don't add CAP_NO_EXCLUDE to the main x86 pmu code, but > then you also don't check if it handles all the various exclude options > correctly/consistently. > > Now; I must admit that that is a bit of a maze, but I think we can at > least add exclude_idle and exclude_hv fails in there, nothing uses those > afaict. > > On the various exclude options; they are as follows (IIUC): > > - exclude_guest: we're a HV/host-kernel and we don't want the counter > to run when we run a guest context. > > - exclude_host: we're a HV/host-kernel and we don't want the counter > to run when we run in host context. > > - exclude_hv: we're a guest and don't want the counter to run in HV > context. > > Now, KVM always implies exclude_hv afaict (for guests) On Power it mostly does. There's some host code that can run in real mode (MMU off) and therefore doesn't do a full context switch out of the guest (including the PMU), so that's host code that is running while the guest PMCs are still counting. cheers
diff --git a/arch/x86/events/amd/ibs.c b/arch/x86/events/amd/ibs.c index d50bb4d..62f317c 100644 --- a/arch/x86/events/amd/ibs.c +++ b/arch/x86/events/amd/ibs.c @@ -253,15 +253,6 @@ static int perf_ibs_precise_event(struct perf_event *event, u64 *config) return -EOPNOTSUPP; } -static const struct perf_event_attr ibs_notsupp = { - .exclude_user = 1, - .exclude_kernel = 1, - .exclude_hv = 1, - .exclude_idle = 1, - .exclude_host = 1, - .exclude_guest = 1, -}; - static int perf_ibs_init(struct perf_event *event) { struct hw_perf_event *hwc = &event->hw; @@ -282,9 +273,6 @@ static int perf_ibs_init(struct perf_event *event) if (event->pmu != &perf_ibs->pmu) return -ENOENT; - if (perf_flags(&event->attr) & perf_flags(&ibs_notsupp)) - return -EINVAL; - if (config & ~perf_ibs->config_mask) return -EINVAL; @@ -537,6 +525,7 @@ static struct perf_ibs perf_ibs_fetch = { .start = perf_ibs_start, .stop = perf_ibs_stop, .read = perf_ibs_read, + .capabilities = PERF_PMU_CAP_NO_EXCLUDE, }, .msr = MSR_AMD64_IBSFETCHCTL, .config_mask = IBS_FETCH_CONFIG_MASK, diff --git a/arch/x86/events/amd/power.c b/arch/x86/events/amd/power.c index 2aefacf..c5ff084 100644 --- a/arch/x86/events/amd/power.c +++ b/arch/x86/events/amd/power.c @@ -136,14 +136,7 @@ static int pmu_event_init(struct perf_event *event) return -ENOENT; /* Unsupported modes and filters. */ - if (event->attr.exclude_user || - event->attr.exclude_kernel || - event->attr.exclude_hv || - event->attr.exclude_idle || - event->attr.exclude_host || - event->attr.exclude_guest || - /* no sampling */ - event->attr.sample_period) + if (event->attr.sample_period) return -EINVAL; if (cfg != AMD_POWER_EVENTSEL_PKG) @@ -226,6 +219,7 @@ static struct pmu pmu_class = { .start = pmu_event_start, .stop = pmu_event_stop, .read = pmu_event_read, + .capabilities = PERF_PMU_CAP_NO_EXCLUDE, }; static int power_cpu_exit(unsigned int cpu) diff --git a/arch/x86/events/intel/cstate.c b/arch/x86/events/intel/cstate.c index d2e7807..94a4b7f 100644 --- a/arch/x86/events/intel/cstate.c +++ b/arch/x86/events/intel/cstate.c @@ -280,13 +280,7 @@ static int cstate_pmu_event_init(struct perf_event *event) return -ENOENT; /* unsupported modes and filters */ - if (event->attr.exclude_user || - event->attr.exclude_kernel || - event->attr.exclude_hv || - event->attr.exclude_idle || - event->attr.exclude_host || - event->attr.exclude_guest || - event->attr.sample_period) /* no sampling */ + if (event->attr.sample_period) /* no sampling */ return -EINVAL; if (event->cpu < 0) @@ -437,7 +431,7 @@ static struct pmu cstate_core_pmu = { .start = cstate_pmu_event_start, .stop = cstate_pmu_event_stop, .read = cstate_pmu_event_update, - .capabilities = PERF_PMU_CAP_NO_INTERRUPT, + .capabilities = PERF_PMU_CAP_NO_INTERRUPT | PERF_PMU_CAP_NO_EXCLUDE, .module = THIS_MODULE, }; @@ -451,7 +445,7 @@ static struct pmu cstate_pkg_pmu = { .start = cstate_pmu_event_start, .stop = cstate_pmu_event_stop, .read = cstate_pmu_event_update, - .capabilities = PERF_PMU_CAP_NO_INTERRUPT, + .capabilities = PERF_PMU_CAP_NO_INTERRUPT | PERF_PMU_CAP_NO_EXCLUDE, .module = THIS_MODULE, }; diff --git a/arch/x86/events/intel/rapl.c b/arch/x86/events/intel/rapl.c index 91039ff..94dc564 100644 --- a/arch/x86/events/intel/rapl.c +++ b/arch/x86/events/intel/rapl.c @@ -397,13 +397,7 @@ static int rapl_pmu_event_init(struct perf_event *event) return -EINVAL; /* unsupported modes and filters */ - if (event->attr.exclude_user || - event->attr.exclude_kernel || - event->attr.exclude_hv || - event->attr.exclude_idle || - event->attr.exclude_host || - event->attr.exclude_guest || - event->attr.sample_period) /* no sampling */ + if (event->attr.sample_period) /* no sampling */ return -EINVAL; /* must be done before validate_group */ @@ -699,6 +693,7 @@ static int __init init_rapl_pmus(void) rapl_pmus->pmu.stop = rapl_pmu_event_stop; rapl_pmus->pmu.read = rapl_pmu_event_read; rapl_pmus->pmu.module = THIS_MODULE; + rapl_pmus->pmu.capabilities = PERF_PMU_CAP_NO_EXCLUDE; return 0; } diff --git a/arch/x86/events/intel/uncore_snb.c b/arch/x86/events/intel/uncore_snb.c index 2593b0d..b12517f 100644 --- a/arch/x86/events/intel/uncore_snb.c +++ b/arch/x86/events/intel/uncore_snb.c @@ -397,13 +397,7 @@ static int snb_uncore_imc_event_init(struct perf_event *event) return -EINVAL; /* unsupported modes and filters */ - if (event->attr.exclude_user || - event->attr.exclude_kernel || - event->attr.exclude_hv || - event->attr.exclude_idle || - event->attr.exclude_host || - event->attr.exclude_guest || - event->attr.sample_period) /* no sampling */ + if (event->attr.sample_period) /* no sampling */ return -EINVAL; /* @@ -497,6 +491,7 @@ static struct pmu snb_uncore_imc_pmu = { .start = uncore_pmu_event_start, .stop = uncore_pmu_event_stop, .read = uncore_pmu_event_read, + .capabilities = PERF_PMU_CAP_NO_EXCLUDE, }; static struct intel_uncore_ops snb_uncore_imc_ops = { diff --git a/arch/x86/events/msr.c b/arch/x86/events/msr.c index 1b9f85a..a878e62 100644 --- a/arch/x86/events/msr.c +++ b/arch/x86/events/msr.c @@ -160,13 +160,7 @@ static int msr_event_init(struct perf_event *event) return -ENOENT; /* unsupported modes and filters */ - if (event->attr.exclude_user || - event->attr.exclude_kernel || - event->attr.exclude_hv || - event->attr.exclude_idle || - event->attr.exclude_host || - event->attr.exclude_guest || - event->attr.sample_period) /* no sampling */ + if (event->attr.sample_period) /* no sampling */ return -EINVAL; if (cfg >= PERF_MSR_EVENT_MAX) @@ -256,7 +250,7 @@ static struct pmu pmu_msr = { .start = msr_event_start, .stop = msr_event_stop, .read = msr_event_update, - .capabilities = PERF_PMU_CAP_NO_INTERRUPT, + .capabilities = PERF_PMU_CAP_NO_INTERRUPT | PERF_PMU_CAP_NO_EXCLUDE, }; static int __init msr_init(void)
For drivers that do not support context exclusion let's advertise the PERF_PMU_CAP_NOEXCLUDE capability. This ensures that perf will prevent us from handling events where any exclusion flags are set. Let's also remove the now unnecessary check for exclusion flags. Signed-off-by: Andrew Murray <andrew.murray@arm.com> --- arch/x86/events/amd/ibs.c | 13 +------------ arch/x86/events/amd/power.c | 10 ++-------- arch/x86/events/intel/cstate.c | 12 +++--------- arch/x86/events/intel/rapl.c | 9 ++------- arch/x86/events/intel/uncore_snb.c | 9 ++------- arch/x86/events/msr.c | 10 ++-------- 6 files changed, 12 insertions(+), 51 deletions(-)