Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752418AbcD3RvA (ORCPT ); Sat, 30 Apr 2016 13:51:00 -0400 Received: from mail-vk0-f54.google.com ([209.85.213.54]:34790 "EHLO mail-vk0-f54.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751225AbcD3Ru6 (ORCPT ); Sat, 30 Apr 2016 13:50:58 -0400 MIME-Version: 1.0 In-Reply-To: References: <1461905018-86355-1-git-send-email-davidcc@google.com> <1461905018-86355-27-git-send-email-davidcc@google.com> Date: Sat, 30 Apr 2016 10:50:56 -0700 Message-ID: Subject: Re: [PATCH 26/32] perf/x86/intel/cqm: integrate CQM cgroups with scheduler From: David Carrillo-Cisneros To: Vikas Shivappa Cc: Peter Zijlstra , Alexander Shishkin , Arnaldo Carvalho de Melo , Ingo Molnar , Tony Luck , Stephane Eranian , Paul Turner , x86@kernel.org, linux-kernel@vger.kernel.org Content-Type: text/plain; charset=UTF-8 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 15733 Lines: 400 In all those cases perf sched_out will call intel_cqm_event_stop that call pqr_cache_update_rmid that sets state->next_rmid to 0. The RMID 0 corresponds to the root monr in the RMID hierarchy. On Fri, Apr 29, 2016 at 4:49 PM Vikas Shivappa wrote: > > > > On Fri, 29 Apr 2016, David Carrillo-Cisneros wrote: > > > When an event is terminated, intel_cqm_event_stop calls > > pqr_cache_update_rmid and sets state->next_rmid to the rmid of its > > parent in the RMID hierarchy. That would make next call to > > __pqr_update to update PQR_ASSOC. > > What about all the cases like context swith to a pid2 which is not monitored.. > (assume event1 still exists) > > > > > On Fri, Apr 29, 2016 at 2:32 PM Vikas Shivappa > > wrote: > >> > >> > >> > >> On Fri, 29 Apr 2016, David Carrillo-Cisneros wrote: > >> > >>> if __intel_cqm_no_event_sched_in does nothing, the PQR_ASSOC msr is > >>> still updated if state->rmid != state->next_rmid in __pqr_update, > >> > >> But due to 2 and 3 below they are equal ? > >> > >> > >>> even if next_rmid_mode == PQR_RMID_MODE_NOEVENT . > >>> > >>> On Fri, Apr 29, 2016 at 2:01 PM, Vikas Shivappa > >>> wrote: > >>>> > >>>> > >>>> On Fri, 29 Apr 2016, David Carrillo-Cisneros wrote: > >>>> > >>>>> Not sure I see the problem you point here. In step 3, PQR_ASSOC is > >>>>> updated with RMID1, __pqr_update is the one called using the scheduler > >>>>> hook, right after perf sched_in . > >>>>> > >>>>> On Fri, Apr 29, 2016 at 1:25 PM, Vikas Shivappa > >>>>> wrote: > >>>>>> > >>>>>> > >>>>>> > >>>>>> On Thu, 28 Apr 2016, David Carrillo-Cisneros wrote: > >>>>>> > >>>>>>> Allow monitored cgroups to update the PQR MSR during task switch even > >>>>>>> without an associated perf_event. > >>>>>>> > >>>>>>> The package RMID for the current monr associated with a monitored > >>>>>>> cgroup is written to hw during task switch (after perf_events is run) > >>>>>>> if perf_event did not write a RMID for an event. > >>>>>>> > >>>>>>> perf_event and any other caller of pqr_cache_update_rmid can update the > >>>>>>> CPU's RMID using one of two modes: > >>>>>>> - PQR_RMID_MODE_NOEVENT: A RMID that do not correspond to an event. > >>>>>>> e.g. the RMID of the root pmonr when no event is scheduled. > >>>>>>> - PQR_RMID_MODE_EVENT: A RMID used by an event. Set during pmu::add > >>>>>>> unset on pmu::del. This mode prevents from using a non-event > >>>>>>> cgroup RMID. > >>>>>>> > >>>>>>> This patch also introduces caching of writes to PQR MSR within the > >>>>>>> per-pcu > >>>>>>> pqr state variable. This interface to update RMIDs and CLOSIDs will be > >>>>>>> also utilized in upcoming versions of Intel's MBM and CAT drivers. > >>>>>>> > >>>>>>> Reviewed-by: Stephane Eranian > >>>>>>> Signed-off-by: David Carrillo-Cisneros > >>>>>>> --- > >>>>>>> arch/x86/events/intel/cqm.c | 65 > >>>>>>> +++++++++++++++++++++++++++++---------- > >>>>>>> arch/x86/events/intel/cqm.h | 2 -- > >>>>>>> arch/x86/include/asm/pqr_common.h | 53 +++++++++++++++++++++++++++---- > >>>>>>> arch/x86/kernel/cpu/pqr_common.c | 46 +++++++++++++++++++++++---- > >>>>>>> 4 files changed, 135 insertions(+), 31 deletions(-) > >>>>>>> > >>>>>>> diff --git a/arch/x86/events/intel/cqm.c b/arch/x86/events/intel/cqm.c > >>>>>>> index daf9fdf..4ece0a4 100644 > >>>>>>> --- a/arch/x86/events/intel/cqm.c > >>>>>>> +++ b/arch/x86/events/intel/cqm.c > >>>>>>> @@ -198,19 +198,6 @@ static inline int cqm_prmid_update(struct prmid > >>>>>>> *prmid) > >>>>>>> return __cqm_prmid_update(prmid, __rmid_min_update_time); > >>>>>>> } > >>>>>>> > >>>>>>> -/* > >>>>>>> - * Updates caller cpu's cache. > >>>>>>> - */ > >>>>>>> -static inline void __update_pqr_prmid(struct prmid *prmid) > >>>>>>> -{ > >>>>>>> - struct intel_pqr_state *state = this_cpu_ptr(&pqr_state); > >>>>>>> - > >>>>>>> - if (state->rmid == prmid->rmid) > >>>>>>> - return; > >>>>>>> - state->rmid = prmid->rmid; > >>>>>>> - wrmsr(MSR_IA32_PQR_ASSOC, prmid->rmid, state->closid); > >>>>>>> -} > >>>>>>> - > >>>>>>> static inline bool __valid_pkg_id(u16 pkg_id) > >>>>>>> { > >>>>>>> return pkg_id < PQR_MAX_NR_PKGS; > >>>>>>> @@ -2531,12 +2518,11 @@ static inline bool cqm_group_leader(struct > >>>>>>> perf_event *event) > >>>>>>> static inline void __intel_cqm_event_start( > >>>>>>> struct perf_event *event, union prmid_summary summary) > >>>>>>> { > >>>>>>> - u16 pkg_id = topology_physical_package_id(smp_processor_id()); > >>>>>>> if (!(event->hw.state & PERF_HES_STOPPED)) > >>>>>>> return; > >>>>>>> - > >>>>>>> event->hw.state &= ~PERF_HES_STOPPED; > >>>>>>> - __update_pqr_prmid(__prmid_from_rmid(pkg_id, > >>>>>>> summary.sched_rmid)); > >>>>>>> + > >>>>>>> + pqr_cache_update_rmid(summary.sched_rmid, PQR_RMID_MODE_EVENT); > >>>>>>> } > >>>>>>> > >>>>>>> static void intel_cqm_event_start(struct perf_event *event, int mode) > >>>>>>> @@ -2566,7 +2552,7 @@ static void intel_cqm_event_stop(struct perf_event > >>>>>>> *event, int mode) > >>>>>>> /* Occupancy of CQM events is obtained at read. No need to read > >>>>>>> * when event is stopped since read on inactive cpus succeed. > >>>>>>> */ > >>>>>>> - __update_pqr_prmid(__prmid_from_rmid(pkg_id, > >>>>>>> summary.sched_rmid)); > >>>>>>> + pqr_cache_update_rmid(summary.sched_rmid, > >>>>>>> PQR_RMID_MODE_NOEVENT); > >>>>>>> } > >>>>>>> > >>>>>>> static int intel_cqm_event_add(struct perf_event *event, int mode) > >>>>>>> @@ -2977,6 +2963,8 @@ static void intel_cqm_cpu_starting(unsigned int > >>>>>>> cpu) > >>>>>>> > >>>>>>> state->rmid = 0; > >>>>>>> state->closid = 0; > >>>>>>> + state->next_rmid = 0; > >>>>>>> + state->next_closid = 0; > >>>>>>> > >>>>>>> /* XXX: lock */ > >>>>>>> /* XXX: Make sure this case is handled when hotplug happens. */ > >>>>>>> @@ -3152,6 +3140,12 @@ static int __init intel_cqm_init(void) > >>>>>>> pr_info("Intel CQM monitoring enabled with at least %u rmids per > >>>>>>> package.\n", > >>>>>>> min_max_rmid + 1); > >>>>>>> > >>>>>>> + /* Make sure pqr_common_enable_key is enabled after > >>>>>>> + * cqm_initialized_key. > >>>>>>> + */ > >>>>>>> + barrier(); > >>>>>>> + > >>>>>>> + static_branch_enable(&pqr_common_enable_key); > >>>>>>> return ret; > >>>>>>> > >>>>>>> error_init_mutex: > >>>>>>> @@ -3163,4 +3157,41 @@ error: > >>>>>>> return ret; > >>>>>>> } > >>>>>>> > >>>>>>> +/* Schedule task without a CQM perf_event. */ > >>>>>>> +inline void __intel_cqm_no_event_sched_in(void) > >>>>>>> +{ > >>>>>>> +#ifdef CONFIG_CGROUP_PERF > >>>>>>> + struct monr *monr; > >>>>>>> + struct pmonr *pmonr; > >>>>>>> + union prmid_summary summary; > >>>>>>> + u16 pkg_id = topology_physical_package_id(smp_processor_id()); > >>>>>>> + struct pmonr *root_pmonr = monr_hrchy_root->pmonrs[pkg_id]; > >>>>>>> + > >>>>>>> + /* Assume CQM enabled is likely given that PQR is enabled. */ > >>>>>>> + if (!static_branch_likely(&cqm_initialized_key)) > >>>>>>> + return; > >>>>>>> + > >>>>>>> + /* Safe to call from_task since we are in scheduler lock. */ > >>>>>>> + monr = monr_from_perf_cgroup(perf_cgroup_from_task(current, > >>>>>>> NULL)); > >>>>>>> + pmonr = monr->pmonrs[pkg_id]; > >>>>>>> + > >>>>>>> + /* Utilize most up to date pmonr summary. */ > >>>>>>> + monr_hrchy_get_next_prmid_summary(pmonr); > >>>>>>> + summary.value = atomic64_read(&pmonr->prmid_summary_atomic); > >>>>>>> + > >>>>>>> + if (!prmid_summary__is_mon_active(summary)) > >>>>>>> + goto no_rmid; > >>>>>>> + > >>>>>>> + if (WARN_ON_ONCE(!__valid_rmid(pkg_id, summary.sched_rmid))) > >>>>>>> + goto no_rmid; > >>>>>>> + > >>>>>>> + pqr_cache_update_rmid(summary.sched_rmid, > >>>>>>> PQR_RMID_MODE_NOEVENT); > >>>>>>> + return; > >>>>>>> + > >>>>>>> +no_rmid: > >>>>>>> + summary.value = > >>>>>>> atomic64_read(&root_pmonr->prmid_summary_atomic); > >>>>>>> + pqr_cache_update_rmid(summary.sched_rmid, > >>>>>>> PQR_RMID_MODE_NOEVENT); > >>>>>>> +#endif > >>>>>>> +} > >>>>>>> + > >>>>>>> device_initcall(intel_cqm_init); > >>>>>>> diff --git a/arch/x86/events/intel/cqm.h b/arch/x86/events/intel/cqm.h > >>>>>>> index 0f3da94..e1f8bd0 100644 > >>>>>>> --- a/arch/x86/events/intel/cqm.h > >>>>>>> +++ b/arch/x86/events/intel/cqm.h > >>>>>>> @@ -82,8 +82,6 @@ union prmid_summary { > >>>>>>> }; > >>>>>>> }; > >>>>>>> > >>>>>>> -# define INVALID_RMID (-1) > >>>>>>> - > >>>>>>> /* A pmonr in (U)state has no sched_rmid, read_rmid can be 0 or > >>>>>>> INVALID_RMID > >>>>>>> * depending on whether monitoring is active or not. > >>>>>>> */ > >>>>>>> diff --git a/arch/x86/include/asm/pqr_common.h > >>>>>>> b/arch/x86/include/asm/pqr_common.h > >>>>>>> index f770637..abbb235 100644 > >>>>>>> --- a/arch/x86/include/asm/pqr_common.h > >>>>>>> +++ b/arch/x86/include/asm/pqr_common.h > >>>>>>> @@ -3,31 +3,72 @@ > >>>>>>> > >>>>>>> #if defined(CONFIG_INTEL_RDT) > >>>>>>> > >>>>>>> +#include > >>>>>>> #include > >>>>>>> #include > >>>>>>> +#include > >>>>>>> > >>>>>>> #define MSR_IA32_PQR_ASSOC 0x0c8f > >>>>>>> +#define INVALID_RMID (-1) > >>>>>>> +#define INVALID_CLOSID (-1) > >>>>>>> + > >>>>>>> + > >>>>>>> +extern struct static_key_false pqr_common_enable_key; > >>>>>>> + > >>>>>>> +enum intel_pqr_rmid_mode { > >>>>>>> + /* RMID has no perf_event associated. */ > >>>>>>> + PQR_RMID_MODE_NOEVENT = 0, > >>>>>>> + /* RMID has a perf_event associated. */ > >>>>>>> + PQR_RMID_MODE_EVENT > >>>>>>> +}; > >>>>>>> > >>>>>>> /** > >>>>>>> * struct intel_pqr_state - State cache for the PQR MSR > >>>>>>> - * @rmid: The cached Resource Monitoring ID > >>>>>>> - * @closid: The cached Class Of Service ID > >>>>>>> + * @rmid: Last rmid written to hw. > >>>>>>> + * @next_rmid: Next rmid to write to hw. > >>>>>>> + * @next_rmid_mode: Next rmid's mode. > >>>>>>> + * @closid: The current Class Of Service ID > >>>>>>> + * @next_closid: The Class Of Service ID to use. > >>>>>>> * > >>>>>>> * The upper 32 bits of MSR_IA32_PQR_ASSOC contain closid and the > >>>>>>> * lower 10 bits rmid. The update to MSR_IA32_PQR_ASSOC always > >>>>>>> * contains both parts, so we need to cache them. > >>>>>>> * > >>>>>>> - * The cache also helps to avoid pointless updates if the value does > >>>>>>> - * not change. > >>>>>>> + * The cache also helps to avoid pointless updates if the value does > >>>>>>> not > >>>>>>> + * change. It also keeps track of the type of RMID set (event vs no > >>>>>>> event) > >>>>>>> + * used to determine when a cgroup RMID is required. > >>>>>>> */ > >>>>>>> struct intel_pqr_state { > >>>>>>> - u32 rmid; > >>>>>>> - u32 closid; > >>>>>>> + u32 rmid; > >>>>>>> + u32 next_rmid; > >>>>>>> + enum intel_pqr_rmid_mode next_rmid_mode; > >>>>>>> + u32 closid; > >>>>>>> + u32 next_closid; > >>>>>>> }; > >>>>>>> > >>>>>>> DECLARE_PER_CPU(struct intel_pqr_state, pqr_state); > >>>>>>> > >>>>>>> #define PQR_MAX_NR_PKGS 8 > >>>>>>> > >>>>>>> +void __pqr_update(void); > >>>>>>> + > >>>>>>> +inline void __intel_cqm_no_event_sched_in(void); > >>>>>>> + > >>>>>>> +inline void pqr_cache_update_rmid(u32 rmid, enum intel_pqr_rmid_mode > >>>>>>> mode); > >>>>>>> + > >>>>>>> +inline void pqr_cache_update_closid(u32 closid); > >>>>>>> + > >>>>>>> +static inline void pqr_update(void) > >>>>>>> +{ > >>>>>>> + if (static_branch_unlikely(&pqr_common_enable_key)) > >>>>>>> + __pqr_update(); > >>>>>>> +} > >>>>>>> + > >>>>>>> +#else > >>>>>>> + > >>>>>>> +static inline void pqr_update(void) > >>>>>>> +{ > >>>>>>> +} > >>>>>>> + > >>>>>>> #endif > >>>>>>> #endif > >>>>>>> diff --git a/arch/x86/kernel/cpu/pqr_common.c > >>>>>>> b/arch/x86/kernel/cpu/pqr_common.c > >>>>>>> index 9eff5d9..d91c127 100644 > >>>>>>> --- a/arch/x86/kernel/cpu/pqr_common.c > >>>>>>> +++ b/arch/x86/kernel/cpu/pqr_common.c > >>>>>>> @@ -1,9 +1,43 @@ > >>>>>>> #include > >>>>>>> > >>>>>>> -/* > >>>>>>> - * The cached intel_pqr_state is strictly per CPU and can never be > >>>>>>> - * updated from a remote CPU. Both functions which modify the state > >>>>>>> - * (intel_cqm_event_start and intel_cqm_event_stop) are called with > >>>>>>> - * interrupts disabled, which is sufficient for the protection. > >>>>>>> - */ > >>>>>>> DEFINE_PER_CPU(struct intel_pqr_state, pqr_state); > >>>>>>> + > >>>>>>> +DEFINE_STATIC_KEY_FALSE(pqr_common_enable_key); > >>>>>>> + > >>>>>>> +inline void pqr_cache_update_rmid(u32 rmid, enum intel_pqr_rmid_mode > >>>>>>> mode) > >>>>>>> +{ > >>>>>>> + struct intel_pqr_state *state = this_cpu_ptr(&pqr_state); > >>>>>>> + > >>>>>>> + state->next_rmid_mode = mode; > >>>>>>> + state->next_rmid = rmid; > >>>>>>> +} > >>>>>>> + > >>>>>>> +inline void pqr_cache_update_closid(u32 closid) > >>>>>>> +{ > >>>>>>> + struct intel_pqr_state *state = this_cpu_ptr(&pqr_state); > >>>>>>> + > >>>>>>> + state->next_closid = closid; > >>>>>>> +} > >>>>>>> + > >>>>>>> +/* Update hw's RMID using cgroup's if perf_event did not. > >>>>>>> + * Sync pqr cache with MSR. > >>>>>>> + */ > >>>>>>> +inline void __pqr_update(void) > >>>>>>> +{ > >>>>>>> + struct intel_pqr_state *state = this_cpu_ptr(&pqr_state); > >>>>>>> + > >>>>>>> + /* If perf_event has set a next_rmid that is used, do not try > >>>>>>> + * to obtain another one from current task. > >>>>>>> + */ > >>>>>>> + if (state->next_rmid_mode == PQR_RMID_MODE_NOEVENT) > >>>>>>> + __intel_cqm_no_event_sched_in(); > >>>>>> > >>>>>> > >>>>>> > >>>>>> if perf_cgroup is not defined then the state is not updated here so the > >>>>>> state might have state->rmid might have IDs ? > >>>>>> > >>>>>> 1.event1 for PID1 has RMID1, > >>>>>> 2.perf sched_in - state->next_rmid = rmid1 > >>>>>> 3.pqr_update - state->rmid = rmid1 > >>>>>> 4.sched_out - write PQR_NOEVENT - > >>>>>> 5.next switch_to - state->rmid not reset nothing changes (when no > >>>>>> perf_cgroup) ? > >>>> > >>>> > >>>> Between 4 and 5 say event1 is dead. Basically on the next context switch(#5) > >>>> if perf sched_in wasnt called the PQR still has RMID1. > >>>> > >>>> When you have perf_cgroup you call the __intel_cqm_no_event.. then it sees > >>>> if there is continuous monitoring or you set the next_rmid to 0- but all the > >>>> code there is inside #ifdef PERF_CGROUP, so without cgroup its never reset > >>>> to zero ? > >>>> > >>>> > >>>> > >>>>>> > >>>>>> > >>>>>>> + > >>>>>>> + /* __intel_cqm_no_event_sched_in might have changed next_rmid. > >>>>>>> */ > >>>>>>> + if (state->rmid == state->next_rmid && > >>>>>>> + state->closid == state->next_closid) > >>>>>>> + return; > >>>>>>> + > >>>>>>> + state->rmid = state->next_rmid; > >>>>>>> + state->closid = state->next_closid; > >>>>>>> + wrmsr(MSR_IA32_PQR_ASSOC, state->rmid, state->closid); > >>>>>>> +} > >>>>>>> -- > >>>>>>> 2.8.0.rc3.226.g39d4020 > >>>>>>> > >>>>>>> > >>>>>> > >>>>> > >>>> > >>> > >