[v2,4/4] x86/resctrl: Rename arch_has_sparse_bitmaps
Commit Message
Both AMD and Intel documentations use capacity bitmasks terminology
rather than capacity bitmaps. Also bitmask term is much more widely
used inside x86 resctrl code.
Unify the naming convention by renaming arch_has_sparse_bitmaps struct
member to arch_has_sparse_bitmasks.
Suggested-by: Reinette Chatre <reinette.chatre@intel.com>
Signed-off-by: Maciej Wieczor-Retman <maciej.wieczor-retman@intel.com>
---
Changelog v2:
- Created this patch.
arch/x86/kernel/cpu/resctrl/core.c | 6 +++---
arch/x86/kernel/cpu/resctrl/ctrlmondata.c | 4 ++--
arch/x86/kernel/cpu/resctrl/rdtgroup.c | 8 ++++----
include/linux/resctrl.h | 6 +++---
4 files changed, 12 insertions(+), 12 deletions(-)
Comments
Hi Maciej
Could you please move this patch to the beginning of this series?
Having it in the end results in a lot of churn with significant
changes to new code introduced in this series. All this can be avoided
by providing a smaller patch at the beginning of the series.
On 9/22/2023 1:48 AM, Maciej Wieczor-Retman wrote:
> Both AMD and Intel documentations use capacity bitmasks terminology
> rather than capacity bitmaps. Also bitmask term is much more widely
> used inside x86 resctrl code.
Since resctrl is intended to support many architectures and Arm coming
soon (and they use bitmap) I do not think we should use the vendor's
language as a motivation. For me there are three reasons supporting the
rename:
* arch_has_sparse_bitmaps is the *only* instance in resctrl that uses
the bitmap term for capacity bitmasks, all other parts of resctrl refers
to it as a bitmask
* bitmask is the established term used in resctrl documentation
(Documentation/arch/x86/resctrl.rst)
* bitmask is the term already exposed to user space via resctrl ("cbm_mask")
Finally, why do the rename as part of this work? This can be motivated
with something like:
A later patch exposes the value of arch_has_sparse_bitmaps to
user space via the existing term of a bitmask. Rename
arch_has_sparse_bitmaps to arch_has_sparse_bitmasks to ensure
consistent terminology throughout resctrl.
>
> Unify the naming convention by renaming arch_has_sparse_bitmaps struct
> member to arch_has_sparse_bitmasks.
>
> Suggested-by: Reinette Chatre <reinette.chatre@intel.com>
> Signed-off-by: Maciej Wieczor-Retman <maciej.wieczor-retman@intel.com>
> ---
...
> diff --git a/include/linux/resctrl.h b/include/linux/resctrl.h
> index 8334eeacfec5..83c2cbf7136d 100644
> --- a/include/linux/resctrl.h
> +++ b/include/linux/resctrl.h
> @@ -57,7 +57,7 @@ struct resctrl_staged_config {
> * @list: all instances of this resource
> * @id: unique id for this instance
> * @cpu_mask: which CPUs share this resource
> - * @rmid_busy_llc: bitmap of which limbo RMIDs are above threshold
> + * @rmid_busy_llc: bitmask of which limbo RMIDs are above threshold
> * @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
Please drop this hunk. rmid_busy_llc is indeed a bitmap.
Reinette
On 2023-09-28 at 13:47:49 -0700, Reinette Chatre wrote:
>Hi Maciej
>
>Could you please move this patch to the beginning of this series?
>Having it in the end results in a lot of churn with significant
>changes to new code introduced in this series. All this can be avoided
>by providing a smaller patch at the beginning of the series.
Sure
>On 9/22/2023 1:48 AM, Maciej Wieczor-Retman wrote:
>> Both AMD and Intel documentations use capacity bitmasks terminology
>> rather than capacity bitmaps. Also bitmask term is much more widely
>> used inside x86 resctrl code.
>
>Since resctrl is intended to support many architectures and Arm coming
>soon (and they use bitmap) I do not think we should use the vendor's
>language as a motivation. For me there are three reasons supporting the
>rename:
>* arch_has_sparse_bitmaps is the *only* instance in resctrl that uses
> the bitmap term for capacity bitmasks, all other parts of resctrl refers
> to it as a bitmask
>* bitmask is the established term used in resctrl documentation
> (Documentation/arch/x86/resctrl.rst)
>* bitmask is the term already exposed to user space via resctrl ("cbm_mask")
>
>Finally, why do the rename as part of this work? This can be motivated
>with something like:
> A later patch exposes the value of arch_has_sparse_bitmaps to
> user space via the existing term of a bitmask. Rename
> arch_has_sparse_bitmaps to arch_has_sparse_bitmasks to ensure
> consistent terminology throughout resctrl.
Okay, I see your point. My only reason for mentioning x86 was that the
changes are done in arch/x86 directory. But I can see that there is no
need to mention it here really. I'll redo the patch message accordingly.
>>
>> Unify the naming convention by renaming arch_has_sparse_bitmaps struct
>> member to arch_has_sparse_bitmasks.
>>
>> Suggested-by: Reinette Chatre <reinette.chatre@intel.com>
>> Signed-off-by: Maciej Wieczor-Retman <maciej.wieczor-retman@intel.com>
>> ---
>
>...
>
>> diff --git a/include/linux/resctrl.h b/include/linux/resctrl.h
>> index 8334eeacfec5..83c2cbf7136d 100644
>> --- a/include/linux/resctrl.h
>> +++ b/include/linux/resctrl.h
>> @@ -57,7 +57,7 @@ struct resctrl_staged_config {
>> * @list: all instances of this resource
>> * @id: unique id for this instance
>> * @cpu_mask: which CPUs share this resource
>> - * @rmid_busy_llc: bitmap of which limbo RMIDs are above threshold
>> + * @rmid_busy_llc: bitmask of which limbo RMIDs are above threshold
>> * @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
>
>Please drop this hunk. rmid_busy_llc is indeed a bitmap.
Okay, thanks for catching this.
@@ -152,7 +152,7 @@ static inline void cache_alloc_hsw_probe(void)
r->cache.cbm_len = 20;
r->cache.shareable_bits = 0xc0000;
r->cache.min_cbm_bits = 2;
- r->cache.arch_has_sparse_bitmaps = false;
+ r->cache.arch_has_sparse_bitmasks = false;
r->alloc_capable = true;
rdt_alloc_capable = true;
@@ -279,7 +279,7 @@ static void rdt_get_cache_alloc_cfg(int idx, struct rdt_resource *r)
r->cache.shareable_bits = ebx & r->default_ctrl;
r->data_width = (r->cache.cbm_len + 3) / 4;
if (boot_cpu_data.x86_vendor == X86_VENDOR_INTEL)
- r->cache.arch_has_sparse_bitmaps = ecx.split.noncont;
+ r->cache.arch_has_sparse_bitmasks = ecx.split.noncont;
r->alloc_capable = true;
}
@@ -895,7 +895,7 @@ static __init void rdt_init_res_defs_amd(void)
if (r->rid == RDT_RESOURCE_L3 ||
r->rid == RDT_RESOURCE_L2) {
- r->cache.arch_has_sparse_bitmaps = true;
+ r->cache.arch_has_sparse_bitmasks = true;
r->cache.arch_has_per_cpu_cfg = true;
r->cache.min_cbm_bits = 0;
} else if (r->rid == RDT_RESOURCE_MBA) {
@@ -115,8 +115,8 @@ static bool cbm_validate(char *buf, u32 *data, struct rdt_resource *r)
first_bit = find_first_bit(&val, cbm_len);
zero_bit = find_next_zero_bit(&val, cbm_len, first_bit);
- /* Are non-contiguous bitmaps allowed? */
- if (!r->cache.arch_has_sparse_bitmaps &&
+ /* Are non-contiguous bitmasks allowed? */
+ if (!r->cache.arch_has_sparse_bitmasks &&
(find_next_bit(&val, cbm_len, zero_bit) < cbm_len)) {
rdt_last_cmd_printf("The mask %lx has non-consecutive 1-bits\n", val);
return false;
@@ -895,13 +895,13 @@ static int rdt_shareable_bits_show(struct kernfs_open_file *of,
return 0;
}
-static int rdt_has_sparse_bitmaps_show(struct kernfs_open_file *of,
- struct seq_file *seq, void *v)
+static int rdt_has_sparse_bitmasks_show(struct kernfs_open_file *of,
+ struct seq_file *seq, void *v)
{
struct resctrl_schema *s = of->kn->parent->priv;
struct rdt_resource *r = s->res;
- seq_printf(seq, "%u\n", r->cache.arch_has_sparse_bitmaps);
+ seq_printf(seq, "%u\n", r->cache.arch_has_sparse_bitmasks);
return 0;
}
@@ -1854,7 +1854,7 @@ static struct rftype res_common_files[] = {
.name = "sparse_masks",
.mode = 0444,
.kf_ops = &rdtgroup_kf_single_ops,
- .seq_show = rdt_has_sparse_bitmaps_show,
+ .seq_show = rdt_has_sparse_bitmasks_show,
.fflags = RF_CTRL_INFO | RFTYPE_RES_CACHE,
},
@@ -57,7 +57,7 @@ struct resctrl_staged_config {
* @list: all instances of this resource
* @id: unique id for this instance
* @cpu_mask: which CPUs share this resource
- * @rmid_busy_llc: bitmap of which limbo RMIDs are above threshold
+ * @rmid_busy_llc: bitmask of which limbo RMIDs are above threshold
* @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
@@ -94,7 +94,7 @@ struct rdt_domain {
* zero CBM.
* @shareable_bits: Bitmask of shareable resource with other
* executing entities
- * @arch_has_sparse_bitmaps: True if a bitmap like f00f is valid.
+ * @arch_has_sparse_bitmasks: True if a bitmask like f00f is valid.
* @arch_has_per_cpu_cfg: True if QOS_CFG register for this cache
* level has CPU scope.
*/
@@ -102,7 +102,7 @@ struct resctrl_cache {
unsigned int cbm_len;
unsigned int min_cbm_bits;
unsigned int shareable_bits;
- bool arch_has_sparse_bitmaps;
+ bool arch_has_sparse_bitmasks;
bool arch_has_per_cpu_cfg;
};