Messages in this thread |  | | Subject | Re: [PATCH 2/2] x86/resctrl: Initialize new resource group with default MBA values | From | Xiaochen Shen <> | Date | Wed, 17 Apr 2019 04:51:18 +0800 |
| |
Hi Boris,
Thank you very much for code review. I will fix these issues in v2 patch.
Please find more comments inline. Thank you.
On 4/15/2019 19:34, Borislav Petkov wrote: > On Wed, Apr 10, 2019 at 03:55:28AM +0800, Xiaochen Shen wrote: >> Currently when a new resource group is created, the allocation values >> of MBA resource are not initialized and remain meaningless data. >> >> For example: >> mkdir /sys/fs/resctrl/p1 >> cat /sys/fs/resctrl/p1/schemata >> MB:0=100;1=100 >> >> echo "MB:0=10;1=20" > /sys/fs/resctrl/p1/schemata >> cat /sys/fs/resctrl/p1/schemata >> MB:0= 10;1= 20 >> >> rmdir /sys/fs/resctrl/p1 >> mkdir /sys/fs/resctrl/p2 >> cat /sys/fs/resctrl/p2/schemata >> MB:0= 10;1= 20 >> >> When the new group is created, it is reasonable to initialize MBA >> resource with default values. >> >> Initialize MBA resource and cache resources in separate functions. > > Please format your commit message by indenting the examples:
OK. Thank you.
> > x86/resctrl: Initialize a new resource group with default MBA values > > Currently, when a new resource group is created, the allocation values > of the MBA resource are not initialized and remain meaningless data. > > For example: > > mkdir /sys/fs/resctrl/p1 > cat /sys/fs/resctrl/p1/schemata > MB:0=100;1=100 > > echo "MB:0=10;1=20" > /sys/fs/resctrl/p1/schemata > cat /sys/fs/resctrl/p1/schemata > MB:0= 10;1= 20 > > rmdir /sys/fs/resctrl/p1 > mkdir /sys/fs/resctrl/p2 > cat /sys/fs/resctrl/p2/schemata > MB:0= 10;1= 20 > > Therefore, when the new group is created, it is reasonable to initialize > MBA resource with default values. > > Initialize the MBA resource and cache resources in separate functions." > > Thx. > >> >> Signed-off-by: Xiaochen Shen <xiaochen.shen@intel.com> >> Reviewed-by: Fenghua Yu <fenghua.yu@intel.com> >> Reviewed-by: Reinette Chatre <reinette.chatre@intel.com> >> --- >> arch/x86/kernel/cpu/resctrl/ctrlmondata.c | 4 +- >> arch/x86/kernel/cpu/resctrl/rdtgroup.c | 139 ++++++++++++++++-------------- >> 2 files changed, 75 insertions(+), 68 deletions(-) >> >> diff --git a/arch/x86/kernel/cpu/resctrl/ctrlmondata.c b/arch/x86/kernel/cpu/resctrl/ctrlmondata.c >> index 2dbd990..576bb6a 100644 >> --- a/arch/x86/kernel/cpu/resctrl/ctrlmondata.c >> +++ b/arch/x86/kernel/cpu/resctrl/ctrlmondata.c >> @@ -342,10 +342,10 @@ int update_domains(struct rdt_resource *r, int closid) >> if (cpumask_empty(cpu_mask) || mba_sc) >> goto done; >> cpu = get_cpu(); >> - /* Update CBM on this cpu if it's in cpu_mask. */ >> + /* Update resource control msr on this cpu if it's in cpu_mask. */ > > s/cpu/CPU/g >
OK.
>> if (cpumask_test_cpu(cpu, cpu_mask)) >> rdt_ctrl_update(&msr_param); >> - /* Update CBM on other cpus. */ >> + /* Update resource control msr on other cpus. */ > > Ditto.
OK.
> >> smp_call_function_many(cpu_mask, rdt_ctrl_update, &msr_param, 1); >> put_cpu(); >> >> diff --git a/arch/x86/kernel/cpu/resctrl/rdtgroup.c b/arch/x86/kernel/cpu/resctrl/rdtgroup.c >> index 08e0333..9f12a02 100644 >> --- a/arch/x86/kernel/cpu/resctrl/rdtgroup.c >> +++ b/arch/x86/kernel/cpu/resctrl/rdtgroup.c >> @@ -2516,8 +2516,8 @@ static void cbm_ensure_valid(u32 *_val, struct rdt_resource *r) >> bitmap_clear(val, zero_bit, cbm_len - zero_bit); >> } >> >> -/** >> - * rdtgroup_init_alloc - Initialize the new RDT group's allocations >> +/* >> + * Initialize cache resources with default values. >> * >> * A new RDT group is being created on an allocation capable (CAT) >> * supporting system. Set this group up to start off with all usable >> @@ -2526,85 +2526,92 @@ static void cbm_ensure_valid(u32 *_val, struct rdt_resource *r) >> * All-zero CBM is invalid. If there are no more shareable bits available >> * on any domain then the entire allocation will fail. >> */ >> -static int rdtgroup_init_alloc(struct rdtgroup *rdtgrp) >> +static int rdtgroup_init_cat(struct rdt_resource *r, u32 closid) >> { >> struct rdt_resource *r_cdp = NULL; >> struct rdt_domain *d_cdp = NULL; >> u32 used_b = 0, unused_b = 0; >> - u32 closid = rdtgrp->closid; >> - struct rdt_resource *r; >> unsigned long tmp_cbm; >> enum rdtgrp_mode mode; >> struct rdt_domain *d; >> u32 peer_ctl, *ctrl; >> - int i, ret; >> + int i; >> >> - for_each_alloc_enabled_rdt_resource(r) { >> + list_for_each_entry(d, &r->domains, list) { >> + rdt_cdp_peer_get(r, d, &r_cdp, &d_cdp); >> + d->have_new_ctrl = false; >> + d->new_ctrl = r->cache.shareable_bits; >> + used_b = r->cache.shareable_bits; >> + ctrl = d->ctrl_val; >> + for (i = 0; i < closids_supported(); i++, ctrl++) { >> + if (closid_allocated(i) && i != closid) { >> + mode = rdtgroup_mode_by_closid(i); >> + if (mode == RDT_MODE_PSEUDO_LOCKSETUP) >> + break; >> + /* >> + * If CDP is active include peer >> + * domain's usage to ensure there >> + * is no overlap with an exclusive >> + * group. >> + */ >> + if (d_cdp) >> + peer_ctl = d_cdp->ctrl_val[i]; >> + else >> + peer_ctl = 0; >> + used_b |= *ctrl | peer_ctl; >> + if (mode == RDT_MODE_SHAREABLE) >> + d->new_ctrl |= *ctrl | peer_ctl; >> + } >> + } >> + if (d->plr && d->plr->cbm > 0) >> + used_b |= d->plr->cbm; >> + unused_b = used_b ^ (BIT_MASK(r->cache.cbm_len) - 1); >> + unused_b &= BIT_MASK(r->cache.cbm_len) - 1; >> + d->new_ctrl |= unused_b; >> + cbm_ensure_valid(&d->new_ctrl, r); >> /* >> - * Only initialize default allocations for CBM cache >> - * resources >> + * Assign the u32 CBM to an unsigned long to ensure >> + * that bitmap_weight() does not access out-of-bound >> + * memory. >> */ > > So all this code working on a rdt_domain *d pointer could be carved out > into a separate function called something like: > > __init_one_rdt_domain(d, ...) > > and this will make the code more readable and save us at least 2 > indentation levels. > > Please do that in a preparatory patch.
Good suggestion. I will do it in v2 patch.
> >> - if (r->rid == RDT_RESOURCE_MBA) >> - continue; > > Then, after having done that, it would be very obvious when you do this > above because you won't be calling that __init_one_rdt_domain() function > for an MBA anyway. > > Thx. >
In this patch we initialize MBA resource and cache resources in separate functions rdtgroup_init_cat() and rdtgroup_init_mba(). If __init_one_rdt_domain() is only called by rdtgroup_init_cat(), how about using function name "__init_one_rdt_domain_cat()"?
Thank you.
Best regards, Xiaochen
|  |