Messages in this thread | | | Date | Fri, 1 Apr 2022 15:55:13 -0700 | Subject | Re: [PATCH v3 19/21] x86/resctrl: Rename and change the units of resctrl_cqm_threshold | From | Reinette Chatre <> |
| |
Hi James,
On 3/30/2022 9:45 AM, James Morse wrote: > Hi Reinette, > > On 17/03/2022 17:00, Reinette Chatre wrote: >> On 2/17/2022 10:21 AM, James Morse wrote: >>> resctrl_cqm_threshold is stored in a hardware specific chunk size, >>> but exposed to user-space as bytes. >>> >>> This means the filesystem parts of resctrl need to know how the hardware >>> counts, to convert the user provided byte value to chunks. The interface >>> between the architecture's resctrl code and the filesystem ought to >>> treat everything as bytes. >>> >>> Change the unit of resctrl_cqm_threshold to bytes. resctrl_arch_rmid_read() >>> still returns its value in chunks, so this needs converting to bytes. >>> As all the callers have been touched, rename the variable to >>> resctrl_rmid_realloc_threshold, which describes what the value is for. > >>> @@ -762,10 +763,7 @@ int rdt_get_mon_l3_config(struct rdt_resource *r) >>> * >>> * For a 35MB LLC and 56 RMIDs, this is ~1.8% of the LLC. >>> */ >>> - resctrl_cqm_threshold = cl_size * 1024 / r->num_rmid; >>> - >>> - /* h/w works in units of "boot_cpu_data.x86_cache_occ_scale" */ >>> - resctrl_cqm_threshold /= hw_res->mon_scale; >>> + resctrl_rmid_realloc_threshold = cl_size * 1024 / r->num_rmid; >>> >>> ret = dom_data_init(r); >>> if (ret) >>> diff --git a/arch/x86/kernel/cpu/resctrl/rdtgroup.c b/arch/x86/kernel/cpu/resctrl/rdtgroup.c >>> index 7ec089d72ab7..93b3697027df 100644 >>> --- a/arch/x86/kernel/cpu/resctrl/rdtgroup.c >>> +++ b/arch/x86/kernel/cpu/resctrl/rdtgroup.c >>> @@ -1030,10 +1030,7 @@ static int rdt_delay_linear_show(struct kernfs_open_file *of, >>> static int max_threshold_occ_show(struct kernfs_open_file *of, >>> struct seq_file *seq, void *v) >>> { >>> - struct rdt_resource *r = of->kn->parent->priv; >>> - struct rdt_hw_resource *hw_res = resctrl_to_arch_res(r); >>> - >>> - seq_printf(seq, "%u\n", resctrl_cqm_threshold * hw_res->mon_scale); >>> + seq_printf(seq, "%u\n", resctrl_rmid_realloc_threshold); >>> >>> return 0; >>> } >> >> >> This change has some user visible impact that I am still digesting but thought >> that I would share for your consideration. >> >> As seen in the above two snippets, the original code did: >> >> resctrl_cqm_threshold /= hw_res->mon_scale; /* resctrl_cqm_threshold used internally */ >> >> resctrl_cqm_threshold * hw_res->mon_scale; /* this is displayed to user */ >> >> The original loss due to truncation during the division is not recovered >> when the value is displayed to the user the user may see significant differences >> before and after this patch. >> >> I tried this out on a system with a large cache and the before and after >> information is significant: >> Before this patch: >> info/L3_MON/max_threshold_occupancy:147456 >> >> After this patch: >> info/L3_MON/max_threshold_occupancy:196608 > > Hmm. I hadn't considered that information would be lost by the current way of doing this. > It looks like this happens because num_rmid isn't necessarily a power of 2. > > >> As I understand this change indeed represents the information more accurately but >> I found it noteworthy that this is not just a simple "change the units" and >> may thus have broader impact and may indeed result in different behavior that >> should be considered. > > I agree it more accurately reflects resctrl's calculation of "the number > of lines tagged per RMID if all RMIDs have the same number of lines", but if that > produces a number the hardware will never actually measure, then the rounding is still > happening, but somewhere else. > > I think the right thing to do is round resctrl_rmid_realloc_threshold down to the nearest > multiple of hw_res->mon_scale in rdt_get_mon_l3_config(). This way the filesystem parts > still handle things in bytes, and the architecture code provides the quantised value that > will actually get measured. Its this value that should be reported to user-space. > > It doesn't look like the 'Upscaling Factor' is guaranteed to be a power of 2, so I can't > use the round_down() helpers. > > I've added this to the commit message: > | Neither r->num_rmid nor hw_res->mon_scale are guaranteed to be a power > | of 2, so the existing code introduces a rounding error from resctrl's > | theoretical fraction of the cache usage. This behaviour is kept as it > | ensures the user visible value matches the value read from hardware > | when the rmid will be reallocated. > > and the hunk below, which fixes it for me. > > > > Thanks, > > James > > ---------------%<--------------- > diff --git a/arch/x86/kernel/cpu/resctrl/monitor.c b/arch/x86/kernel/cpu/resctrl/monitor.c > index b18e227d585c..fb81d650c457 100644 > --- a/arch/x86/kernel/cpu/resctrl/monitor.c > +++ b/arch/x86/kernel/cpu/resctrl/monitor.c > @@ -753,6 +753,7 @@ int rdt_get_mon_l3_config(struct rdt_resource *r) > unsigned int mbm_offset = boot_cpu_data.x86_cache_mbm_width_offset; > struct rdt_hw_resource *hw_res = resctrl_to_arch_res(r); > unsigned int cl_size = boot_cpu_data.x86_cache_size; > + u64 threshold; > int ret; > > hw_res->mon_scale = boot_cpu_data.x86_cache_occ_scale; > @@ -771,7 +772,15 @@ int rdt_get_mon_l3_config(struct rdt_resource *r) > * > * For a 35MB LLC and 56 RMIDs, this is ~1.8% of the LLC. > */ > - resctrl_rmid_realloc_threshold = cl_size * 1024 / r->num_rmid; > + threshold = cl_size * 1024 / r->num_rmid; > + > + /* > + * Because num_rmid may not be a power of two, round the value > + * to the nearest multiple of hw_res->mon_scale so it matches a > + * value the hardware will measure. mon_scale may not be a power of 2. > + */ > + threshold /= hw_res->mon_scale; > + resctrl_rmid_realloc_threshold = threshold * hw_res->mon_scale; > > ret = dom_data_init(r); > if (ret) > ---------------%<---------------
Thank you for the added explanation. From what I can tell this also restores current behavior.
Reinette
| |