Messages in this thread | | | Date | Thu, 26 Oct 2023 17:40:25 -0500 | Subject | Re: [PATCH v3] x86/resctrl: mba_MBps: Fall back to total b/w if local b/w unavailable | From | "Moger, Babu" <> |
| |
Hi Tony,
On 10/26/2023 3:02 PM, Tony Luck wrote: > On Intel the various resource director technology (RDT) features are all > orthogonal and independently enumerated. Thus it is possible to have > a system that provides "total" memory bandwidth measurements without > providing "local" bandwidth measurements. > > If local bandwidth measurement is not available, do not give up on > providing the "mba_MBps" feedback option completely, make the code fall > back to using total bandwidth. > > Signed-off-by: Tony Luck <tony.luck@intel.com> > --- > Change since v2: > > Babu doesn't like the global variable. So here's a version without it. > > Note that my preference is still the v2 version. But as I tell newbies > to Linux "Your job isn't to get YOUR patch upstream. You job is to get > the problem fixed.". So taking my own advice I don't really mind > whether v2 or v3 is applied. Hmm. I like v3 better. Few minor comments below. > > arch/x86/kernel/cpu/resctrl/monitor.c | 43 ++++++++++++++++++-------- > arch/x86/kernel/cpu/resctrl/rdtgroup.c | 2 +- > 2 files changed, 31 insertions(+), 14 deletions(-) > > diff --git a/arch/x86/kernel/cpu/resctrl/monitor.c b/arch/x86/kernel/cpu/resctrl/monitor.c > index f136ac046851..29e86310677d 100644 > --- a/arch/x86/kernel/cpu/resctrl/monitor.c > +++ b/arch/x86/kernel/cpu/resctrl/monitor.c > @@ -418,6 +418,20 @@ static int __mon_event_count(u32 rmid, struct rmid_read *rr) > return 0; > } > > +/* > + * For legacy compatibility use the local memory bandwidth to drive > + * the mba_MBps feedback control loop. But on platforms that do not > + * provide the local event fall back to use the total bandwidth event > + * instead. > + */ > +static enum resctrl_event_id pick_mba_mbps_event(void) > +{ > + if (is_mbm_local_enabled()) > + return QOS_L3_MBM_LOCAL_EVENT_ID; > + > + return QOS_L3_MBM_TOTAL_EVENT_ID; > +} > + > /* > * mbm_bw_count() - Update bw count from values previously read by > * __mon_event_count(). > @@ -431,9 +445,11 @@ static int __mon_event_count(u32 rmid, struct rmid_read *rr) > */ > static void mbm_bw_count(u32 rmid, struct rmid_read *rr) > { > - struct mbm_state *m = &rr->d->mbm_local[rmid]; > + enum resctrl_event_id mba_mbps_evt_id = pick_mba_mbps_event();
How about evt_id instead of mba_mbps_evt_id? It seems pretty mouthful for temp variable.
> u64 cur_bw, bytes, cur_bytes; > + struct mbm_state *m; > > + m = get_mbm_state(rr->d, rmid, mba_mbps_evt_id); > cur_bytes = rr->val; > bytes = cur_bytes - m->prev_bw_bytes; > m->prev_bw_bytes = cur_bytes; > @@ -518,6 +534,7 @@ void mon_event_count(void *info) > */ > static void update_mba_bw(struct rdtgroup *rgrp, struct rdt_domain *dom_mbm) > { > + enum resctrl_event_id mba_mbps_evt_id = pick_mba_mbps_event(); same comment as above. > u32 closid, rmid, cur_msr_val, new_msr_val; > struct mbm_state *pmbm_data, *cmbm_data; > u32 cur_bw, delta_bw, user_bw; > @@ -526,14 +543,14 @@ static void update_mba_bw(struct rdtgroup *rgrp, struct rdt_domain *dom_mbm) > struct list_head *head; > struct rdtgroup *entry; > > - if (!is_mbm_local_enabled()) > + if (!is_mbm_enabled()) > return; > > r_mba = &rdt_resources_all[RDT_RESOURCE_MBA].r_resctrl; > > closid = rgrp->closid; > rmid = rgrp->mon.rmid; > - pmbm_data = &dom_mbm->mbm_local[rmid]; > + pmbm_data = get_mbm_state(dom_mbm, rmid, mba_mbps_evt_id); > > dom_mba = get_domain_from_cpu(smp_processor_id(), r_mba); > if (!dom_mba) { > @@ -553,7 +570,7 @@ static void update_mba_bw(struct rdtgroup *rgrp, struct rdt_domain *dom_mbm) > */ > head = &rgrp->mon.crdtgrp_list; > list_for_each_entry(entry, head, mon.crdtgrp_list) { > - cmbm_data = &dom_mbm->mbm_local[entry->mon.rmid]; > + cmbm_data = get_mbm_state(dom_mbm, entry->mon.rmid, mba_mbps_evt_id); > cur_bw += cmbm_data->prev_bw; > delta_bw += cmbm_data->delta_bw; > } > @@ -595,7 +612,7 @@ static void update_mba_bw(struct rdtgroup *rgrp, struct rdt_domain *dom_mbm) > */ > pmbm_data->delta_comp = true; > list_for_each_entry(entry, head, mon.crdtgrp_list) { > - cmbm_data = &dom_mbm->mbm_local[entry->mon.rmid]; > + cmbm_data = get_mbm_state(dom_mbm, entry->mon.rmid, mba_mbps_evt_id); > cmbm_data->delta_comp = true; > } > } > @@ -621,15 +638,15 @@ static void mbm_update(struct rdt_resource *r, struct rdt_domain *d, int rmid) > rr.evtid = QOS_L3_MBM_LOCAL_EVENT_ID; > rr.val = 0; > __mon_event_count(rmid, &rr); > - > - /* > - * Call the MBA software controller only for the > - * control groups and when user has enabled > - * the software controller explicitly. > - */ > - if (is_mba_sc(NULL)) > - mbm_bw_count(rmid, &rr); > } > + > + /* > + * Call the MBA software controller only for the > + * control groups and when user has enabled > + * the software controller explicitly. > + */ > + if (is_mba_sc(NULL)) > + mbm_bw_count(rmid, &rr); > } > > /* > diff --git a/arch/x86/kernel/cpu/resctrl/rdtgroup.c b/arch/x86/kernel/cpu/resctrl/rdtgroup.c > index 69a1de92384a..0c4f8a1b8df0 100644 > --- a/arch/x86/kernel/cpu/resctrl/rdtgroup.c > +++ b/arch/x86/kernel/cpu/resctrl/rdtgroup.c > @@ -2294,7 +2294,7 @@ static bool supports_mba_mbps(void) > { > struct rdt_resource *r = &rdt_resources_all[RDT_RESOURCE_MBA].r_resctrl; > > - return (is_mbm_local_enabled() && > + return (is_mbm_enabled() && > r->alloc_capable && is_mba_linear()); > } >
Otherwise looks good to me.
Thanks
Babu
| |