Cqm3 based on resctrl fs with MBM support
v3 cover Cqm3 based on resctrl fs with MBM support
Sending some changes to the V2 version based on Thomas feedback that the worst case limbo processing in IPI was still not acceptable and to improve the cache line access during scheduling.
This also includes a fix during hot cpu processing which was reported.
Patches are based on the tip/x86/cache (where the V2 was merged)
Vikas Shivappa (3):
x86/intel_rdt/cqm: Add fix to clear the default RMID during hotcpu x86/intel_rdt: Modify the intel_pqr_state for better performance x86/intel_rdt/cqm: Improve limbo list processing
arch/x86/include/asm/intel_rdt_sched.h | 30 +++-- arch/x86/kernel/cpu/intel_rdt.c | 34 +++-- arch/x86/kernel/cpu/intel_rdt.h | 10 ++ arch/x86/kernel/cpu/intel_rdt_monitor.c | 209 +++++++++++++------------------ arch/x86/kernel/cpu/intel_rdt_rdtgroup.c | 10 +- 5 files changed, 146 insertions(+), 147 deletions(-)
Signature · 1 line
1.9.1
PATCH v3 1/3 x86/intel_rdt/cqm: Add fix to clear the default RMID during hotcpu
The user configured per cpu default RMID is not cleared during cpu hotplug. This may lead to incorrect RMID values after a cpu goes offline and again comes back online. Clear the per cpu default RMID during cpu offline and online handling.
--- arch/x86/kernel/cpu/intel_rdt.c | 1 + 1 file changed, 1 insertion(+)
arch/x86/kernel/cpu/intel_rdt.c +1 -0
@@ -551,6 +551,7 @@ static void clear_closid_rmid(int cpu)struct intel_pqr_state *state =this_cpu_ptr (&pqr_state);per_cpu (rdt_cpu_default.closid, cpu) =0 ; +per_cpu (rdt_cpu_default.rmid, cpu) =0 ; state->closid =0 ; state->rmid =0 ;wrmsr (IA32_PQR_ASSOC ,0 ,0 );
Signature · 1 line
1.9.1
tip-bot for Vikas Shivappa
merge bot[tip:x86/cache] x86/intel_rdt/cqm: Clear the default RMID during hotcpu
x86/intel_rdt/cqm: Clear the default RMID during hotcpu
Commit-ID: eda61c265f3656be8345fdf0334b3a77829437fc Gitweb: http://git.kernel.org/tip/eda61c265f3656be8345fdf0334b3a77829437fc Author: Vikas Shivappa <[email protected]> AuthorDate: Wed, 9 Aug 2017 11:46:33 -0700 Committer: Thomas Gleixner <[email protected]> CommitDate: Mon, 14 Aug 2017 11:47:46 +0200
x86/intel_rdt/cqm: Clear the default RMID during hotcpu
The user configured per cpu default RMID is not cleared during cpu hotplug. This may lead to incorrect RMID values after a cpu goes offline and again comes back online. Clear the per cpu default RMID during cpu offline and online handling.
--- arch/x86/kernel/cpu/intel_rdt.c | 1 + 1 file changed, 1 insertion(+)
arch/x86/kernel/cpu/intel_rdt.c +1 -0
@@ -551,6 +551,7 @@ static void clear_closid_rmid(int cpu)struct intel_pqr_state *state =this_cpu_ptr (&pqr_state);per_cpu (rdt_cpu_default.closid, cpu) =0 ; +per_cpu (rdt_cpu_default.rmid, cpu) =0 ; state->closid =0 ; state->rmid =0 ;wrmsr (IA32_PQR_ASSOC ,0 ,0 );
PATCH v3 2/3 x86/intel_rdt: Modify the intel_pqr_state for better performance
In reply to Vikas ShivappaCurrently we have pqr_state and rdt_default_state which store the cached CLOSID/RMIDs and the user configured cpu default values respectively. We touch both of these during context switch. Put all of them in one structure so that we can spare a cache line.
--- arch/x86/include/asm/intel_rdt_sched.h | 30 +++++++++++++++++------------- arch/x86/kernel/cpu/intel_rdt.c | 10 ++++------ arch/x86/kernel/cpu/intel_rdt_rdtgroup.c | 10 +++++----- 3 files changed, 26 insertions(+), 24 deletions(-)
arch/x86/include/asm/intel_rdt_sched.h +17 -13
@@ -10,8 +10,10 @@/** * struct intel_pqr_state - State cache for the PQR MSR -* @rmid: The cached Resource Monitoring ID -* @closid: The cached Class Of Service ID +* @cur_rmid: The cached Resource Monitoring ID +* @cur_closid: The cached Class Of Service ID +* @default_rmid: The user assigned Resource Monitoring ID +* @default_closid: The user assigned cached Class Of Service ID * * The upper 32 bits of IA32_PQR_ASSOC contain closid and the * lower 10 bits rmid. The update to IA32_PQR_ASSOC always
@@ -22,12 +24,13 @@* not change. */ struct intel_pqr_state { -u32 rmid; -u32 closid; +u32 cur_rmid; +u32 cur_closid; +u32 default_rmid; +u32 default_closid; }; DECLARE_PER_CPU(struct intel_pqr_state, pqr_state); -DECLARE_PER_CPU_READ_MOSTLY(struct intel_pqr_state, rdt_cpu_default);DECLARE_STATIC_KEY_FALSE (rdt_enable_key);DECLARE_STATIC_KEY_FALSE (rdt_alloc_enable_key);
@@ -49,8 +52,9 @@ struct intel_pqr_state {*/ static void __intel_rdt_sched_in (void ) { -struct intel_pqr_state newstate =this_cpu_read (rdt_cpu_default); -struct intel_pqr_state *curstate =this_cpu_ptr (&pqr_state); +struct intel_pqr_state *state =this_cpu_ptr (&pqr_state); +u32 closid = state->default_closid; +u32 rmid = state->default_rmid;/* * If this task has a closid/rmid assigned, use it.
@@ -58,18 +62,18 @@ static void __intel_rdt_sched_in(void)*/ if (static_branch_likely (&rdt_alloc_enable_key)) {if (current->closid) - newstate.closid = current->closid; + closid = current->closid; }if (static_branch_likely (&rdt_mon_enable_key)) {if (current->rmid) - newstate.rmid = current->rmid; + rmid = current->rmid; } -if (newstate.closid != curstate->closid || - newstate.rmid != curstate->rmid) { - *curstate = newstate; -wrmsr (IA32_PQR_ASSOC , newstate.rmid, newstate.closid); +if (closid != state->cur_closid || rmid != state->cur_rmid) { + state->cur_closid = closid; + state->cur_rmid = rmid; +wrmsr (IA32_PQR_ASSOC , rmid, closid); } }
arch/x86/kernel/cpu/intel_rdt.c +4 -6
@@ -47,8 +47,6 @@*/ DEFINE_PER_CPU(struct intel_pqr_state, pqr_state); -DEFINE_PER_CPU_READ_MOSTLY(struct intel_pqr_state, rdt_cpu_default); -/* * Used to store the max resource name width and max resource data width * to display the schemata in a tabular format
@@ -550,10 +548,10 @@ static void clear_closid_rmid(int cpu) {struct intel_pqr_state *state =this_cpu_ptr (&pqr_state); -per_cpu (rdt_cpu_default.closid, cpu) =0 ; -per_cpu (rdt_cpu_default.rmid, cpu) =0 ; - state->closid =0 ; - state->rmid =0 ; + state->default_closid =0 ; + state->default_rmid =0 ; + state->cur_closid =0 ; + state->cur_rmid =0 ;wrmsr (IA32_PQR_ASSOC ,0 ,0 ); }
arch/x86/kernel/cpu/intel_rdt_rdtgroup.c +5 -5
@@ -202,8 +202,8 @@ static void update_cpu_closid_rmid(void *info)struct rdtgroup *r = info;if (r) { -this_cpu_write (rdt_cpu_default.closid, r->closid); -this_cpu_write (rdt_cpu_default.rmid, r->mon.rmid); +this_cpu_write (pqr_state.default_closid, r->closid); +this_cpu_write (pqr_state.default_rmid, r->mon.rmid); }/*
@@ -1733,7 +1733,7 @@ static int rdtgroup_rmdir_mon(struct kernfs_node *kn, struct rdtgroup *rdtgrp,/* Update per cpu rmid of the moved CPUs first */ for_each_cpu (cpu, &rdtgrp->cpu_mask) -per_cpu (rdt_cpu_default.rmid, cpu) = prdtgrp->mon.rmid; +per_cpu (pqr_state.default_rmid, cpu) = prdtgrp->mon.rmid;/* * Update the MSR on moved CPUs and CPUs which have moved * task running on them.
@@ -1774,8 +1774,8 @@ static int rdtgroup_rmdir_ctrl(struct kernfs_node *kn, struct rdtgroup *rdtgrp,/* Update per cpu closid and rmid of the moved CPUs first */ for_each_cpu (cpu, &rdtgrp->cpu_mask) { -per_cpu (rdt_cpu_default.closid, cpu) = rdtgroup_default.closid; -per_cpu (rdt_cpu_default.rmid, cpu) = rdtgroup_default.mon.rmid; +per_cpu (pqr_state.default_closid, cpu) = rdtgroup_default.closid; +per_cpu (pqr_state.default_rmid, cpu) = rdtgroup_default.mon.rmid; }/*
Signature · 1 line
1.9.1
tip-bot for Vikas Shivappa
merge bot[tip:x86/cache] x86/intel_rdt: Modify the intel_pqr_state for better performance
x86/intel_rdt: Modify the intel_pqr_state for better performance
Commit-ID: a9110b552d44fedbd1221eb0e5bde81da32d9350 Gitweb: http://git.kernel.org/tip/a9110b552d44fedbd1221eb0e5bde81da32d9350 Author: Vikas Shivappa <[email protected]> AuthorDate: Wed, 9 Aug 2017 11:46:34 -0700 Committer: Thomas Gleixner <[email protected]> CommitDate: Mon, 14 Aug 2017 11:47:47 +0200
x86/intel_rdt: Modify the intel_pqr_state for better performance
Currently we have pqr_state and rdt_default_state which store the cached CLOSID/RMIDs and the user configured cpu default values respectively. We touch both of these during context switch. Put all of them in one structure so that we can spare a cache line.
--- arch/x86/include/asm/intel_rdt_sched.h | 30 +++++++++++++++++------------- arch/x86/kernel/cpu/intel_rdt.c | 10 ++++------ arch/x86/kernel/cpu/intel_rdt_rdtgroup.c | 10 +++++----- 3 files changed, 26 insertions(+), 24 deletions(-)
arch/x86/include/asm/intel_rdt_sched.h +17 -13
@@ -10,8 +10,10 @@/** * struct intel_pqr_state - State cache for the PQR MSR -* @rmid: The cached Resource Monitoring ID -* @closid: The cached Class Of Service ID +* @cur_rmid: The cached Resource Monitoring ID +* @cur_closid: The cached Class Of Service ID +* @default_rmid: The user assigned Resource Monitoring ID +* @default_closid: The user assigned cached Class Of Service ID * * The upper 32 bits of IA32_PQR_ASSOC contain closid and the * lower 10 bits rmid. The update to IA32_PQR_ASSOC always
@@ -22,12 +24,13 @@* not change. */ struct intel_pqr_state { -u32 rmid; -u32 closid; +u32 cur_rmid; +u32 cur_closid; +u32 default_rmid; +u32 default_closid; }; DECLARE_PER_CPU(struct intel_pqr_state, pqr_state); -DECLARE_PER_CPU_READ_MOSTLY(struct intel_pqr_state, rdt_cpu_default);DECLARE_STATIC_KEY_FALSE (rdt_enable_key);DECLARE_STATIC_KEY_FALSE (rdt_alloc_enable_key);
@@ -49,8 +52,9 @@ DECLARE_STATIC_KEY_FALSE(rdt_mon_enable_key);*/ static void __intel_rdt_sched_in (void ) { -struct intel_pqr_state newstate =this_cpu_read (rdt_cpu_default); -struct intel_pqr_state *curstate =this_cpu_ptr (&pqr_state); +struct intel_pqr_state *state =this_cpu_ptr (&pqr_state); +u32 closid = state->default_closid; +u32 rmid = state->default_rmid;/* * If this task has a closid/rmid assigned, use it.
@@ -58,18 +62,18 @@ static void __intel_rdt_sched_in(void)*/ if (static_branch_likely (&rdt_alloc_enable_key)) {if (current->closid) - newstate.closid = current->closid; + closid = current->closid; }if (static_branch_likely (&rdt_mon_enable_key)) {if (current->rmid) - newstate.rmid = current->rmid; + rmid = current->rmid; } -if (newstate.closid != curstate->closid || - newstate.rmid != curstate->rmid) { - *curstate = newstate; -wrmsr (IA32_PQR_ASSOC , newstate.rmid, newstate.closid); +if (closid != state->cur_closid || rmid != state->cur_rmid) { + state->cur_closid = closid; + state->cur_rmid = rmid; +wrmsr (IA32_PQR_ASSOC , rmid, closid); } }
arch/x86/kernel/cpu/intel_rdt.c +4 -6
@@ -47,8 +47,6 @@ DEFINE_MUTEX(rdtgroup_mutex);*/ DEFINE_PER_CPU(struct intel_pqr_state, pqr_state); -DEFINE_PER_CPU_READ_MOSTLY(struct intel_pqr_state, rdt_cpu_default); -/* * Used to store the max resource name width and max resource data width * to display the schemata in a tabular format
@@ -550,10 +548,10 @@ static void clear_closid_rmid(int cpu) {struct intel_pqr_state *state =this_cpu_ptr (&pqr_state); -per_cpu (rdt_cpu_default.closid, cpu) =0 ; -per_cpu (rdt_cpu_default.rmid, cpu) =0 ; - state->closid =0 ; - state->rmid =0 ; + state->default_closid =0 ; + state->default_rmid =0 ; + state->cur_closid =0 ; + state->cur_rmid =0 ;wrmsr (IA32_PQR_ASSOC ,0 ,0 ); }
arch/x86/kernel/cpu/intel_rdt_rdtgroup.c +5 -5
@@ -202,8 +202,8 @@ static void update_cpu_closid_rmid(void *info)struct rdtgroup *r = info;if (r) { -this_cpu_write (rdt_cpu_default.closid, r->closid); -this_cpu_write (rdt_cpu_default.rmid, r->mon.rmid); +this_cpu_write (pqr_state.default_closid, r->closid); +this_cpu_write (pqr_state.default_rmid, r->mon.rmid); }/*
@@ -1733,7 +1733,7 @@ static int rdtgroup_rmdir_mon(struct kernfs_node *kn, struct rdtgroup *rdtgrp,/* Update per cpu rmid of the moved CPUs first */ for_each_cpu (cpu, &rdtgrp->cpu_mask) -per_cpu (rdt_cpu_default.rmid, cpu) = prdtgrp->mon.rmid; +per_cpu (pqr_state.default_rmid, cpu) = prdtgrp->mon.rmid;/* * Update the MSR on moved CPUs and CPUs which have moved * task running on them.
@@ -1774,8 +1774,8 @@ static int rdtgroup_rmdir_ctrl(struct kernfs_node *kn, struct rdtgroup *rdtgrp,/* Update per cpu closid and rmid of the moved CPUs first */ for_each_cpu (cpu, &rdtgrp->cpu_mask) { -per_cpu (rdt_cpu_default.closid, cpu) = rdtgroup_default.closid; -per_cpu (rdt_cpu_default.rmid, cpu) = rdtgroup_default.mon.rmid; +per_cpu (pqr_state.default_closid, cpu) = rdtgroup_default.closid; +per_cpu (pqr_state.default_rmid, cpu) = rdtgroup_default.mon.rmid; }/*
PATCH v3 3/3 x86/intel_rdt/cqm: Improve limbo list processing
In reply to Vikas ShivappaDuring a mkdir, we synchronously check the entire limbo list on each package for free RMIDs by sending IPIs and the worst case would end up taking more than tolerable amount of time in IPIs. For ex: On SKL we could end up reading all 192 RMIDs for llc_occupancy.
Modify this to make some improvements mainly avoiding the use of IPIs. We use asynchronous worker threads on each package which would periodically scan the limbo list and move the RMIDs that have:
llc_occupancy < threshold_occupancy
on all packages to free list. The mkdir would simply return -ENOSPC if the limbo list is empty or would return -EBUSY if there are RMIDs on limbo but not on free list. As a side effect of not accessing the limbo list in parallel on IPIs we simplify some of the related data structures.
--- arch/x86/kernel/cpu/intel_rdt.c | 25 +++- arch/x86/kernel/cpu/intel_rdt.h | 10 ++ arch/x86/kernel/cpu/intel_rdt_monitor.c | 209 ++++++++++++++------------------ 3 files changed, 120 insertions(+), 124 deletions(-)
arch/x86/kernel/cpu/intel_rdt.c +21 -4
@@ -426,6 +426,9 @@ static int domain_setup_mon_state(struct rdt_resource *r, struct rdt_domain *d) GFP_KERNEL);if (!d->rmid_busy_llc)return -ENOMEM ; +INIT_DELAYED_WORK (&d->cqm_limbo, cqm_handle_limbo); +if (has_busy_rmid (r, d)) +cqm_setup_limbo_handler (d); }if (is_mbm_total_enabled ()) { tsize =sizeof (*d->mbm_total);
@@ -536,11 +539,25 @@ static void domain_remove_cpu(int cpu, struct rdt_resource *r)list_del (&d->list);if (is_mbm_enabled ())cancel_delayed_work (&d->mbm_over); + +if (is_llc_occupancy_enabled () && +has_busy_rmid (r, d)) +cancel_delayed_work (&d->cqm_limbo); +kfree (d); - }else if (r == &rdt_resources_all[RDT_RESOURCE_L3 ] && - cpu == d->mbm_work_cpu &&is_mbm_enabled ()) { -cancel_delayed_work (&d->mbm_over); -mbm_setup_overflow_handler (d); +return ; + } + +if (r == &rdt_resources_all[RDT_RESOURCE_L3 ]) { +if (is_mbm_enabled () && cpu == d->mbm_work_cpu) { +cancel_delayed_work (&d->mbm_over); +mbm_setup_overflow_handler (d); + } +if (is_llc_occupancy_enabled () && cpu == d->mbm_work_cpu && +has_busy_rmid (r, d)) { +cancel_delayed_work (&d->cqm_limbo); +cqm_setup_limbo_handler (d); + } } }
arch/x86/kernel/cpu/intel_rdt.h +10 -0
@@ -20,6 +20,8 @@#define QOS_L3_MBM_TOTAL_EVENT_ID 0x02#define QOS_L3_MBM_LOCAL_EVENT_ID 0x03 +#define CQM_LIMBOCHECK_INTERVAL 1000 +#define MBM_CNTR_WIDTH 24#define MBM_OVERFLOW_INTERVAL 1000
@@ -187,8 +189,11 @@ struct mbm_state {* @mbm_total: saved state for MBM total bandwidth * @mbm_local: saved state for MBM local bandwidth * @mbm_over: worker to periodically read MBM h/w counters +* @cqm_limbo: worker to periodically read CQM h/w counters * @mbm_work_cpu: * worker cpu for MBM h/w counters +* @cqm_work_cpu: +* worker cpu for CQM h/w counters * @ctrl_val: array of cache or mem ctrl values (indexed by CLOSID) * @new_ctrl: new ctrl value to be loaded * @have_new_ctrl: did user provide new_ctrl for this domain
@@ -201,7 +206,9 @@ struct rdt_domain {struct mbm_state *mbm_total;struct mbm_state *mbm_local;struct delayed_work mbm_over; +struct delayed_work cqm_limbo;int mbm_work_cpu; +int cqm_work_cpu;u32 *ctrl_val;u32 new_ctrl;bool have_new_ctrl;
@@ -424,5 +431,8 @@ void mon_event_read(struct rmid_read *rr, struct rdt_domain *d, struct rdtgroup *rdtgrp, int evtid, int first);void mbm_setup_overflow_handler (struct rdt_domain *dom);void mbm_handle_overflow (struct work_struct *work); +void cqm_setup_limbo_handler (struct rdt_domain *dom); +void cqm_handle_limbo (struct work_struct *work); +bool has_busy_rmid (struct rdt_resource *r,struct rdt_domain *d);#endif /* _ASM_X86_INTEL_RDT_H */
arch/x86/kernel/cpu/intel_rdt_monitor.c +89 -120
281 lines not on this page · raw message
Signature · 1 line
1.9.1
Re: [PATCH 3/3] x86/intel_rdt/cqm: Improve limbo list processing
arch/x86/kernel/cpu/intel_rdt.c static int domain_setup_mon_state(struct rdt_resource *r, struct rdt_domain *d) · line 431
Quoted 7 lines · Vikas Shivappa
arch/x86/kernel/cpu/intel_rdt.c +3
-0@@ -426,6 +426,9 @@ static int domain_setup_mon_state(struct rdt_resource *r, struct rdt_domain *d) GFP_KERNEL);if (!d->rmid_busy_llc)return -ENOMEM ; +INIT_DELAYED_WORK (&d->cqm_limbo, cqm_handle_limbo); +if (has_busy_rmid (r, d)) +cqm_setup_limbo_handler (d);
This is beyond silly. d->rmid_busy_llc is allocated a few lines above. How would a bit be set here?
arch/x86/kernel/cpu/intel_rdt.c static void domain_remove_cpu(int cpu, struct rdt_resource *r) · line 544
Quoted 10 lines
arch/x86/kernel/cpu/intel_rdt.c +3
-0}if (is_mbm_total_enabled ()) { tsize =sizeof (*d->mbm_total);@@ -536,11 +539,25 @@ static void domain_remove_cpu(int cpu, struct rdt_resource *r)list_del (&d->list);if (is_mbm_enabled ())cancel_delayed_work (&d->mbm_over); + +if (is_llc_occupancy_enabled () && +has_busy_rmid (r, d))
What is that line break helping here and why can't you just unconditionally cancel the work?
Quoted 4 lines
arch/x86/kernel/cpu/intel_rdt.c +2
-1+cancel_delayed_work (&d->cqm_limbo); +kfree (d); - } else if (r == &rdt_resources_all[RDT_RESOURCE_L3] &&
arch/x86/kernel/cpu/intel_rdt.c static void domain_remove_cpu(int cpu, struct rdt_resource *r) · line 554
Quoted 10 lines
arch/x86/kernel/cpu/intel_rdt.c +7
-3- cpu == d->mbm_work_cpu && is_mbm_enabled()) { -cancel_delayed_work (&d->mbm_over); -mbm_setup_overflow_handler (d); +return ; + } + +if (r == &rdt_resources_all[RDT_RESOURCE_L3 ]) { +if (is_mbm_enabled () && cpu == d->mbm_work_cpu) { +cancel_delayed_work (&d->mbm_over); +mbm_setup_overflow_handler (d);
I think this is the wrong approach. If the timer is about to fire you essentially double the interval. So you better flush the work, which will reschedule it if needed.
Review static void domain_remove_cpu(int cpu, struct rdt_resource *r) · line 556
Quoted 2 lines
(quoted diff) +2
-0+ } + if (is_llc_occupancy_enabled() && cpu == d->mbm_work_cpu &&
That want's to be d->cbm_work_cpu, right?
arch/x86/kernel/cpu/intel_rdt.c static void domain_remove_cpu(int cpu, struct rdt_resource *r) · line 559
Quoted 3 lines
arch/x86/kernel/cpu/intel_rdt.c +3
-0+ has_busy_rmid(r, d)) { +cancel_delayed_work (&d->cqm_limbo); +cqm_setup_limbo_handler (d);
See above.
Thanks,
tglx
Re: [PATCH 3/3] x86/intel_rdt/cqm: Improve limbo list processing
Quoted 12 lines · Thomas Gleixner
arch/x86/kernel/cpu/intel_rdt.c static int domain_setup_mon_state(struct rdt_resource *r, struct rdt_domain *d) · line 431
Quoted 7 lines · Vikas Shivappa
arch/x86/kernel/cpu/intel_rdt.c +3
-0@@ -426,6 +426,9 @@ static int domain_setup_mon_state(struct rdt_resource *r, struct rdt_domain *d) GFP_KERNEL);if (!d->rmid_busy_llc)return -ENOMEM ; +INIT_DELAYED_WORK (&d->cqm_limbo, cqm_handle_limbo); +if (has_busy_rmid (r, d)) +cqm_setup_limbo_handler (d);This is beyond silly.
d->rmid_busy_llcis allocated a few lines above. How would a bit be set here?
If we logically offline all cpus in a package and bring it back, the worker needs to be scheduled on the package if there were busy RMIDs on this package. Otherwise that RMID never gets freed as its rmid->busy stays 1..
I needed to scan the limbo list and set the bits for all limbo RMIDs after the alloc and before doing the 'has_busy_rmid' check. Will fix
Quoted 14 lines
arch/x86/kernel/cpu/intel_rdt.c static void domain_remove_cpu(int cpu, struct rdt_resource *r) · line 544
Quoted 11 lines
arch/x86/kernel/cpu/intel_rdt.c +3
-0}if (is_mbm_total_enabled ()) { tsize =sizeof (*d->mbm_total);@@ -536,11 +539,25 @@ static void domain_remove_cpu(int cpu, struct rdt_resource *r)list_del (&d->list);if (is_mbm_enabled ())cancel_delayed_work (&d->mbm_over); + +if (is_llc_occupancy_enabled () && +has_busy_rmid (r, d))What is that line break helping here and why can't you just unconditionally cancel the work?
Will fix the line break. The has_busy_rmid makes sure the worker was indeed scheduled - that way we cancel the worker which was actually scheduled..
Quoted 11 lines
Quoted 11 lines
arch/x86/kernel/cpu/intel_rdt.c +5
-4+cancel_delayed_work (&d->cqm_limbo); +kfree (d); - }else if (r == &rdt_resources_all[RDT_RESOURCE_L3 ] && - cpu == d->mbm_work_cpu &&is_mbm_enabled ()) { -cancel_delayed_work (&d->mbm_over); -mbm_setup_overflow_handler (d); +return ; + } +
Quoted 8 lines
arch/x86/kernel/cpu/intel_rdt.c static void domain_remove_cpu(int cpu, struct rdt_resource *r) · line 554
Quoted 4 lines
arch/x86/kernel/cpu/intel_rdt.c +4
-0+if (r == &rdt_resources_all[RDT_RESOURCE_L3 ]) { +if (is_mbm_enabled () && cpu == d->mbm_work_cpu) { +cancel_delayed_work (&d->mbm_over); +mbm_setup_overflow_handler (d);I think this is the wrong approach. If the timer is about to fire you essentially double the interval. So you better flush the work, which will reschedule it if needed.
Ok will fix. We can flush(setup and run it immediately) the work here on the new cpu.
Quoted 5 lines
Review static void domain_remove_cpu(int cpu, struct rdt_resource *r) · line 556
Quoted 3 lines
(quoted diff) +2
-0+ } + if (is_llc_occupancy_enabled() && cpu == d->mbm_work_cpu &&That want's to be
d->cbm_work_cpu, right?
Correct - thanks for pointing , will fix.
Quoted 6 lines
arch/x86/kernel/cpu/intel_rdt.c static void domain_remove_cpu(int cpu, struct rdt_resource *r) · line 559
Quoted 4 lines
arch/x86/kernel/cpu/intel_rdt.c +3
-0+ has_busy_rmid(r, d)) { +cancel_delayed_work (&d->cqm_limbo); +cqm_setup_limbo_handler (d);See above.
For cqm 1s is not a hard requirement, but can flush the work like mbm to keep it uniform..
Thanks,
Vikas
Quoted 5 lines
Thanks,
tglx
Re: [PATCH 3/3] x86/intel_rdt/cqm: Improve limbo list processing
Quoted 18 lines · Shivappa Vikas
Quoted 13 lines · Thomas Gleixner
arch/x86/kernel/cpu/intel_rdt.c static int domain_setup_mon_state(struct rdt_resource · line 432
Quoted 8 lines · Vikas Shivappa
arch/x86/kernel/cpu/intel_rdt.c +3
-0@@ -426,6 +426,9 @@ static int domain_setup_mon_state(struct rdt_resource *r, struct rdt_domain *d) GFP_KERNEL);if (!d->rmid_busy_llc)return -ENOMEM ; +INIT_DELAYED_WORK (&d->cqm_limbo, cqm_handle_limbo); +if (has_busy_rmid (r, d)) +cqm_setup_limbo_handler (d);This is beyond silly.
d->rmid_busy_llcis allocated a few lines above. How would a bit be set here?
Quoted 3 lines
If we logically offline all cpus in a package and bring it back, the worker needs to be scheduled on the package if there were busy RMIDs on this package. Otherwise that RMID never gets freed as its
rmid->busystays 1..I needed to scan the limbo list and set the bits for all limbo RMIDs after the alloc and before doing the '
has_busy_rmid' check. Will fix
Tony pointed out that there is no guarentee that a domain will come back up once its down, so the above issue of rmid->busy staying at > 0 can still happen. So I will delete this -
if (has_busy_rmid(r, d)) cqm_setup_limbo_handler(d);
and add this when a domain is powered down -
for each rmid in d->rmid_busy_llc if (--entry->busy) free_rmid(rmid);
We have no way to know if the L3 was indeed flushed (or package was powered off). This may lead to incorrect counts in rare scenarios but can document the same.
Thanks,
vikas