[PATCH v2 0/4] x86/resctrl: Non-contiguous bitmasks in Intel CAT

[PATCH v2 0/4] x86/resctrl: Non-contiguous bitmasks in Intel CAT

[Maciej Wieczor-Retman]

Until recently Intel CPUs didn't support using non-contiguous 1s
in Cache Allocation Technology (CAT). Writing a bitmask with
non-contiguous 1s to the resctrl schemata file would fail.

Intel CPUs that support non-contiguous 1s can be identified through a
CPUID leaf mentioned in the "Intel® Architecture Instruction Set
Extensions Programming Reference" document available at:
https://www.intel.com/content/www/us/en/developer/articles/technical/intel-sdm.html

Add kernel support for detecting if non-contiguous 1s in Cache
Allocation Technology (CAT) are supported by the hardware. Also add a
new resctrl FS file to output this information to the userspace.
Keep the hardcoded value for Haswell CPUs only since they do not have
CPUID enumeration support for Cache allocation.

Since the selftests/resctrl files are going through many rewrites and
cleanups the appropriate selftest is still a work in progress. For
basic selftesting capabilities use the bash script attached below this
paragraph. It checks whether various bitmasks written into resctrl FS
generate output consistent with reported feature support.

#!/bin/bash
# must be run as root, depends on a recent cpuid tool (20230406 or later)
# variables
RESCTRL_INFO="/sys/fs/resctrl/info"
L3_NON_CONT_VAL="${RESCTRL_INFO}/L3/sparse_bitmaps"
L2_NON_CONT_VAL="${RESCTRL_INFO}/L2/sparse_bitmaps"
L3_NON_CONT_CBM="${RESCTRL_INFO}/L3/cbm_mask"
L2_NON_CONT_CBM="${RESCTRL_INFO}/L2/cbm_mask"
L3_CPUID_CMD="cpuid -1 -l 0x10 -s 0x01"
L2_CPUID_CMD="cpuid -1 -l 0x10 -s 0x02"
PASSED_TESTS=0
L3_SUPPORT=0
L2_SUPPORT=0
TESTS=0

run_test() {
        # L2 or L3
        CACHE_LEVEL=$1
        CACHE_LEVEL_SUPPORT="${CACHE_LEVEL}_SUPPORT"
        echo "Checking ${RESCTRL_INFO}/${CACHE_LEVEL}..."
        if [[ -d "${RESCTRL_INFO}/${CACHE_LEVEL}" ]]; then
                eval "${CACHE_LEVEL_SUPPORT}=1"
                echo "${CACHE_LEVEL} CAT Feature is supported"
        else
                echo "${CACHE_LEVEL} CAT Feature is not supported"
        fi

        if [[ ${!CACHE_LEVEL_SUPPORT} -eq 1 ]]; then
                echo " --- Running tests for ${CACHE_LEVEL} CAT ---"

                # read sysfs entries
                # are non-contiguous cbm supported? (driver sysfs)
                eval "NON_CONT_VAL=${CACHE_LEVEL}_NON_CONT_VAL"
                eval "NON_CONT_FEAT=$( cat ${!NON_CONT_VAL} )"

                # are non-contiguous cbm supported? (cpuid)
                CACHE_CPUID_CMD="${CACHE_LEVEL}_CPUID_CMD"
                NONCONT_CPUID=$(${!CACHE_CPUID_CMD} | grep non-contiguous | grep true)
                NONCONT_CPUID_RET=$(( !$? ))

                # what is the mask size?
                eval "NON_CONT_CBM=${CACHE_LEVEL}_NON_CONT_CBM"
                MAX_MASK=$(( 16#$( cat ${!NON_CONT_CBM} ) ))

                # prepare contiguous and non-contiguous masks for tests
                BC_STRING="l(${MAX_MASK})/l(2)"
                MAX_MASK_BIT_COUNT=$(echo ${BC_STRING} | bc -l)
                MAX_MASK_BIT_COUNT=$(printf "%.0f" "$MAX_MASK_BIT_COUNT")
                BITSHIFT=$(( $MAX_MASK_BIT_COUNT/2 - ($MAX_MASK_BIT_COUNT/2 % 4) ))
                CONT_MASK=$(( $MAX_MASK >> $BITSHIFT ))
                NONCONT_MASK=$(( ~( $MAX_MASK & ( 15<<$BITSHIFT) ) ))
                NONCONT_MASK=$(( $NONCONT_MASK & $MAX_MASK ))

                # test if cpuid reported support matches the sysfs one
                echo " * Testing if CPUID matches ${CACHE_LEVEL}/sparse_bitmaps..."
                TESTS=$((TESTS + 1))
                if [[ $NONCONT_CPUID_RET -eq $NON_CONT_FEAT ]]; then
                        PASSED_TESTS=$((PASSED_TESTS + 1))
                        echo "There is a match!"
                else
                        echo "Error - no match!"
                fi

                # test by writing CBMs to the schemata
                printf " * Writing 0x%x mask to the schemata...\n" ${CONT_MASK}
                TESTS=$((TESTS + 1))
                SCHEMATA=$(printf "${CACHE_LEVEL}:0=%x" $CONT_MASK)
                echo "$SCHEMATA" > /sys/fs/resctrl/schemata
                if [[ $? -eq 0 ]]; then
                        PASSED_TESTS=$((PASSED_TESTS + 1))
                        echo "Contiguous ${CACHE_LEVEL} write correct!"
                else
                        echo "Contiguous ${CACHE_LEVEL} write ERROR!"
                fi

                printf " * Writing 0x%x mask to the schemata...\n" ${NONCONT_MASK}
                TESTS=$((TESTS + 1))
                SCHEMATA=$(printf "${CACHE_LEVEL}:0=%x" $NONCONT_MASK)
                echo "$SCHEMATA" > /sys/fs/resctrl/schemata
                if [[ (($? -eq 0) && ($NON_CONT_FEAT -eq 1)) || \
                        (($? -ne 0) && ($NON_CONT_FEAT -eq 0)) ]]; then
                        PASSED_TESTS=$((PASSED_TESTS + 1))
                        echo "Non-contiguous ${CACHE_LEVEL} write correct!"
                else
                        echo "Non-contiguous ${CACHE_LEVEL} write ERROR!"
                fi
        fi
}

# mount resctrl
mount -t resctrl resctrl /sys/fs/resctrl

run_test L3
run_test L2

echo "TESTS PASSED / ALL TESTS : ${PASSED_TESTS} / ${TESTS}"

# unmount resctrl
umount /sys/fs/resctrl

Changelog v2:
- Change git signature from Wieczor-Retman Maciej to Maciej
  Wieczor-Retman.
- Change bitmap naming convention to bit mask.
- Add patch to change arch_has_sparce_bitmaps name to match bitmask
  naming convention.

Fenghua Yu (2):
  x86/resctrl: Add sparse_masks file in info
  Documentation/x86: Document resctrl's new sparse_masks

Maciej Wieczor-Retman (2):
  x86/resctrl: Enable non-contiguous bits in Intel CAT
  x86/resctrl: Rename arch_has_sparse_bitmaps

 Documentation/arch/x86/resctrl.rst        | 16 ++++++++++++----
 arch/x86/kernel/cpu/resctrl/core.c        | 11 +++++++----
 arch/x86/kernel/cpu/resctrl/ctrlmondata.c | 14 ++++++++------
 arch/x86/kernel/cpu/resctrl/internal.h    |  9 +++++++++
 arch/x86/kernel/cpu/resctrl/rdtgroup.c    | 18 ++++++++++++++++++
 include/linux/resctrl.h                   |  6 +++---
 6 files changed, 57 insertions(+), 17 deletions(-)


base-commit: 27bbf45eae9ca98877a2d52a92a188147cd61b07
-- 
2.42.0

[PATCH v2 1/4] x86/resctrl: Enable non-contiguous bits in Intel CAT

[Maciej Wieczor-Retman]

The setting for non-contiguous 1s support in Intel CAT is
hardcoded to false. On these systems, writing non-contiguous
1s into the schemata file will fail before resctrl passes
the value to the hardware.

In Intel CAT CPUID.0x10.1:ECX[3] and CPUID.0x10.2:ECX[3] stopped
being reserved and now carry information about non-contiguous 1s
value support for L3 and L2 cache respectively. The CAT
capacity bitmask (CBM) supports a non-contiguous 1s value if
the bit is set.

Replace the hardcoded non-contiguous support value with
the support learned from the hardware. Add hardcoded non-contiguous
support value to Haswell probe since it can't make use of CPUID for
Cache allocation.

Originally-by: Fenghua Yu <fenghua.yu@intel.com>
Signed-off-by: Maciej Wieczor-Retman <maciej.wieczor-retman@intel.com>
---
Changelog v2:
- Rewrite part of a comment concerning Haswell. (Reinette)

 arch/x86/kernel/cpu/resctrl/core.c        |  9 ++++++---
 arch/x86/kernel/cpu/resctrl/ctrlmondata.c | 10 ++++++----
 arch/x86/kernel/cpu/resctrl/internal.h    |  9 +++++++++
 3 files changed, 21 insertions(+), 7 deletions(-)

diff --git a/arch/x86/kernel/cpu/resctrl/core.c b/arch/x86/kernel/cpu/resctrl/core.c
index 030d3b409768..c783a873147c 100644
--- a/arch/x86/kernel/cpu/resctrl/core.c
+++ b/arch/x86/kernel/cpu/resctrl/core.c
@@ -152,6 +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->alloc_capable = true;
 
 	rdt_alloc_capable = true;
@@ -267,15 +268,18 @@ static void rdt_get_cache_alloc_cfg(int idx, struct rdt_resource *r)
 {
 	struct rdt_hw_resource *hw_res = resctrl_to_arch_res(r);
 	union cpuid_0x10_1_eax eax;
+	union cpuid_0x10_x_ecx ecx;
 	union cpuid_0x10_x_edx edx;
-	u32 ebx, ecx;
+	u32 ebx;
 
-	cpuid_count(0x00000010, idx, &eax.full, &ebx, &ecx, &edx.full);
+	cpuid_count(0x00000010, idx, &eax.full, &ebx, &ecx.full, &edx.full);
 	hw_res->num_closid = edx.split.cos_max + 1;
 	r->cache.cbm_len = eax.split.cbm_len + 1;
 	r->default_ctrl = BIT_MASK(eax.split.cbm_len + 1) - 1;
 	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->alloc_capable = true;
 }
 
@@ -872,7 +876,6 @@ static __init void rdt_init_res_defs_intel(void)
 
 		if (r->rid == RDT_RESOURCE_L3 ||
 		    r->rid == RDT_RESOURCE_L2) {
-			r->cache.arch_has_sparse_bitmaps = false;
 			r->cache.arch_has_per_cpu_cfg = false;
 			r->cache.min_cbm_bits = 1;
 		} else if (r->rid == RDT_RESOURCE_MBA) {
diff --git a/arch/x86/kernel/cpu/resctrl/ctrlmondata.c b/arch/x86/kernel/cpu/resctrl/ctrlmondata.c
index b44c487727d4..f076f12cf8e8 100644
--- a/arch/x86/kernel/cpu/resctrl/ctrlmondata.c
+++ b/arch/x86/kernel/cpu/resctrl/ctrlmondata.c
@@ -87,10 +87,12 @@ int parse_bw(struct rdt_parse_data *data, struct resctrl_schema *s,
 
 /*
  * Check whether a cache bit mask is valid.
- * For Intel the SDM says:
- *	Please note that all (and only) contiguous '1' combinations
- *	are allowed (e.g. FFFFH, 0FF0H, 003CH, etc.).
- * Additionally Haswell requires at least two bits set.
+ * On Intel CPUs, non-contiguous 1s value support is indicated by CPUID:
+ *   - CPUID.0x10.1:ECX[3]: L3 non-contiguous 1s value supported if 1
+ *   - CPUID.0x10.2:ECX[3]: L2 non-contiguous 1s value supported if 1
+ *
+ * Haswell does not support a non-contiguous 1s value and additionally
+ * requires at least two bits set.
  * AMD allows non-contiguous bitmasks.
  */
 static bool cbm_validate(char *buf, u32 *data, struct rdt_resource *r)
diff --git a/arch/x86/kernel/cpu/resctrl/internal.h b/arch/x86/kernel/cpu/resctrl/internal.h
index 85ceaf9a31ac..c47ef2f13e8e 100644
--- a/arch/x86/kernel/cpu/resctrl/internal.h
+++ b/arch/x86/kernel/cpu/resctrl/internal.h
@@ -492,6 +492,15 @@ union cpuid_0x10_3_eax {
 	unsigned int full;
 };
 
+/* CPUID.(EAX=10H, ECX=ResID).ECX */
+union cpuid_0x10_x_ecx {
+	struct {
+		unsigned int reserved:3;
+		unsigned int noncont:1;
+	} split;
+	unsigned int full;
+};
+
 /* CPUID.(EAX=10H, ECX=ResID).EDX */
 union cpuid_0x10_x_edx {
 	struct {
-- 
2.42.0

Re: [PATCH v2 1/4] x86/resctrl: Enable non-contiguous bits in Intel CAT

[Peter Newman]

Hi Maciej,

On Fri, Sep 22, 2023 at 10:48:23AM +0200, Maciej Wieczor-Retman wrote:
> The setting for non-contiguous 1s support in Intel CAT is
> hardcoded to false. On these systems, writing non-contiguous
> 1s into the schemata file will fail before resctrl passes
> the value to the hardware.
> 
> In Intel CAT CPUID.0x10.1:ECX[3] and CPUID.0x10.2:ECX[3] stopped
> being reserved and now carry information about non-contiguous 1s
> value support for L3 and L2 cache respectively. The CAT
> capacity bitmask (CBM) supports a non-contiguous 1s value if
> the bit is set.

How new of an SDM do I need? The June 2023 revision I downloaded today didn't
list it.

> diff --git a/arch/x86/kernel/cpu/resctrl/core.c b/arch/x86/kernel/cpu/resctrl/core.c
> index 030d3b409768..c783a873147c 100644
> --- a/arch/x86/kernel/cpu/resctrl/core.c
> +++ b/arch/x86/kernel/cpu/resctrl/core.c
> @@ -152,6 +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->alloc_capable = true;
>  
>  	rdt_alloc_capable = true;
> @@ -267,15 +268,18 @@ static void rdt_get_cache_alloc_cfg(int idx, struct rdt_resource *r)
>  {
>  	struct rdt_hw_resource *hw_res = resctrl_to_arch_res(r);
>  	union cpuid_0x10_1_eax eax;
> +	union cpuid_0x10_x_ecx ecx;
>  	union cpuid_0x10_x_edx edx;
> -	u32 ebx, ecx;
> +	u32 ebx;
>  
> -	cpuid_count(0x00000010, idx, &eax.full, &ebx, &ecx, &edx.full);
> +	cpuid_count(0x00000010, idx, &eax.full, &ebx, &ecx.full, &edx.full);
>  	hw_res->num_closid = edx.split.cos_max + 1;
>  	r->cache.cbm_len = eax.split.cbm_len + 1;
>  	r->default_ctrl = BIT_MASK(eax.split.cbm_len + 1) - 1;
>  	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;

This seems to be called after the clearing of arch_has_sparse_bitmaps in
cache_alloc_hsw_probe(). If we can't make use of the CPUID bit on Haswell,
is it safe to use its value here?

Thanks!
-Peter

Re: [PATCH v2 1/4] x86/resctrl: Enable non-contiguous bits in Intel CAT

[Maciej Wieczór-Retman]

Hi, and thanks for the review!

On 2023-09-22 at 16:14:41 +0200, Peter Newman wrote:
>Hi Maciej,
>
>On Fri, Sep 22, 2023 at 10:48:23AM +0200, Maciej Wieczor-Retman wrote:
>> The setting for non-contiguous 1s support in Intel CAT is
>> hardcoded to false. On these systems, writing non-contiguous
>> 1s into the schemata file will fail before resctrl passes
>> the value to the hardware.
>> 
>> In Intel CAT CPUID.0x10.1:ECX[3] and CPUID.0x10.2:ECX[3] stopped
>> being reserved and now carry information about non-contiguous 1s
>> value support for L3 and L2 cache respectively. The CAT
>> capacity bitmask (CBM) supports a non-contiguous 1s value if
>> the bit is set.
>
>How new of an SDM do I need? The June 2023 revision I downloaded today didn't
>list it.

It's not currently in the SDM but in the Intel® Architecture
Instruction Set Extensions and Future Features (which I mentioned in the
second paragraph of the cover letter). My version of the ISA pdf was
from June 2023.

>> diff --git a/arch/x86/kernel/cpu/resctrl/core.c b/arch/x86/kernel/cpu/resctrl/core.c
>> index 030d3b409768..c783a873147c 100644
>> --- a/arch/x86/kernel/cpu/resctrl/core.c
>> +++ b/arch/x86/kernel/cpu/resctrl/core.c
>> @@ -152,6 +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->alloc_capable = true;
>>  
>>  	rdt_alloc_capable = true;
>> @@ -267,15 +268,18 @@ static void rdt_get_cache_alloc_cfg(int idx, struct rdt_resource *r)
>>  {
>>  	struct rdt_hw_resource *hw_res = resctrl_to_arch_res(r);
>>  	union cpuid_0x10_1_eax eax;
>> +	union cpuid_0x10_x_ecx ecx;
>>  	union cpuid_0x10_x_edx edx;
>> -	u32 ebx, ecx;
>> +	u32 ebx;
>>  
>> -	cpuid_count(0x00000010, idx, &eax.full, &ebx, &ecx, &edx.full);
>> +	cpuid_count(0x00000010, idx, &eax.full, &ebx, &ecx.full, &edx.full);
>>  	hw_res->num_closid = edx.split.cos_max + 1;
>>  	r->cache.cbm_len = eax.split.cbm_len + 1;
>>  	r->default_ctrl = BIT_MASK(eax.split.cbm_len + 1) - 1;
>>  	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;
>
>This seems to be called after the clearing of arch_has_sparse_bitmaps in
>cache_alloc_hsw_probe(). If we can't make use of the CPUID bit on Haswell,
>is it safe to use its value here?

I believe the calls go like this for a haswell system:
resctrl_late_init() -> check_quirks() -> __check_quirks_intel() ->
-> cache_alloc_hsw_probe()

There this line is executed:
	rdt_alloc_capable = true;
where rdt_alloc_capable is global in the file scope.

Then later in:
resctrl_late_init() -> get_rdt_resources() -> get_rdt_alloc_resources()

this is executed at the function beginning:
	if (rdt_alloc_capable)
		return true;

So the rest of the get_rdt_alloc_resources() is skipped and calls to
rdt_get_cache_alloc_cfg() never get executed.

-- 
Kind regards
Maciej Wieczór-Retman

Re: [PATCH v2 1/4] x86/resctrl: Enable non-contiguous bits in Intel CAT

[Peter Newman]

Hi Maciej,

On Wed, Sep 27, 2023 at 11:20 AM Maciej Wieczór-Retman
<maciej.wieczor-retman@intel.com> wrote:
> On 2023-09-22 at 16:14:41 +0200, Peter Newman wrote:
> >On Fri, Sep 22, 2023 at 10:48:23AM +0200, Maciej Wieczor-Retman wrote:
> >> In Intel CAT CPUID.0x10.1:ECX[3] and CPUID.0x10.2:ECX[3] stopped
> >> being reserved and now carry information about non-contiguous 1s
> >> value support for L3 and L2 cache respectively. The CAT
> >> capacity bitmask (CBM) supports a non-contiguous 1s value if
> >> the bit is set.
> >
> >How new of an SDM do I need? The June 2023 revision I downloaded today didn't
> >list it.
>
> It's not currently in the SDM but in the Intel® Architecture
> Instruction Set Extensions and Future Features (which I mentioned in the
> second paragraph of the cover letter). My version of the ISA pdf was
> from June 2023.
>

I see it now, thanks!

> >> -    cpuid_count(0x00000010, idx, &eax.full, &ebx, &ecx, &edx.full);
> >> +    cpuid_count(0x00000010, idx, &eax.full, &ebx, &ecx.full, &edx.full);
> >>      hw_res->num_closid = edx.split.cos_max + 1;
> >>      r->cache.cbm_len = eax.split.cbm_len + 1;
> >>      r->default_ctrl = BIT_MASK(eax.split.cbm_len + 1) - 1;
> >>      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;
> >
> >This seems to be called after the clearing of arch_has_sparse_bitmaps in
> >cache_alloc_hsw_probe(). If we can't make use of the CPUID bit on Haswell,
> >is it safe to use its value here?
>
> I believe the calls go like this for a haswell system:
> resctrl_late_init() -> check_quirks() -> __check_quirks_intel() ->
> -> cache_alloc_hsw_probe()
>
> There this line is executed:
>         rdt_alloc_capable = true;
> where rdt_alloc_capable is global in the file scope.
>
> Then later in:
> resctrl_late_init() -> get_rdt_resources() -> get_rdt_alloc_resources()
>
> this is executed at the function beginning:
>         if (rdt_alloc_capable)
>                 return true;
>
> So the rest of the get_rdt_alloc_resources() is skipped and calls to
> rdt_get_cache_alloc_cfg() never get executed.

Yuck. But it works I guess.

The series looks fine to me.

Reviewed-by: Peter Newman <peternewman@google.com>

I applied the series and was able to confirm the behavior was still
correct for contiguous-bitmap Intel hardware and that sprase_bitmaps
is true on AMD and continues to work as expected.

Tested-by: Peter Newman <peternewman@google.com>

I'm not sure if I have access to any Intel hardware with
non-contiguous bitmaps right now. Are you able to say where that would
be implemented?

Thanks!
-Peter

Re: [PATCH v2 1/4] x86/resctrl: Enable non-contiguous bits in Intel CAT

[Maciej Wieczór-Retman]

On 2023-09-27 at 12:08:33 +0200, Peter Newman wrote:
>On Wed, Sep 27, 2023 at 11:20 AM Maciej Wieczór-Retman
><maciej.wieczor-retman@intel.com> wrote:
>> On 2023-09-22 at 16:14:41 +0200, Peter Newman wrote:
>> >On Fri, Sep 22, 2023 at 10:48:23AM +0200, Maciej Wieczor-Retman wrote:
>> >> -    cpuid_count(0x00000010, idx, &eax.full, &ebx, &ecx, &edx.full);
>> >> +    cpuid_count(0x00000010, idx, &eax.full, &ebx, &ecx.full, &edx.full);
>> >>      hw_res->num_closid = edx.split.cos_max + 1;
>> >>      r->cache.cbm_len = eax.split.cbm_len + 1;
>> >>      r->default_ctrl = BIT_MASK(eax.split.cbm_len + 1) - 1;
>> >>      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;
>> >
>> >This seems to be called after the clearing of arch_has_sparse_bitmaps in
>> >cache_alloc_hsw_probe(). If we can't make use of the CPUID bit on Haswell,
>> >is it safe to use its value here?
>>
>> I believe the calls go like this for a haswell system:
>> resctrl_late_init() -> check_quirks() -> __check_quirks_intel() ->
>> -> cache_alloc_hsw_probe()
>>
>> There this line is executed:
>>         rdt_alloc_capable = true;
>> where rdt_alloc_capable is global in the file scope.
>>
>> Then later in:
>> resctrl_late_init() -> get_rdt_resources() -> get_rdt_alloc_resources()
>>
>> this is executed at the function beginning:
>>         if (rdt_alloc_capable)
>>                 return true;
>>
>> So the rest of the get_rdt_alloc_resources() is skipped and calls to
>> rdt_get_cache_alloc_cfg() never get executed.
>
>Yuck. But it works I guess.
>
>The series looks fine to me.
>
>Reviewed-by: Peter Newman <peternewman@google.com>
>
>I applied the series and was able to confirm the behavior was still
>correct for contiguous-bitmap Intel hardware and that sprase_bitmaps
>is true on AMD and continues to work as expected.
>
>Tested-by: Peter Newman <peternewman@google.com>
>
>I'm not sure if I have access to any Intel hardware with
>non-contiguous bitmaps right now. Are you able to say where that would
>be implemented?

Thanks for testing!

Writing non-contiguous bitmasks is supported starting from the upcoming
GNR microarchitecture forward.

That's also why the new CPUID bit meaning is in the ISA pdf and not in
the SDM one currently.

-- 
Kind regards
Maciej Wieczór-Retman

Re: [PATCH v2 1/4] x86/resctrl: Enable non-contiguous bits in Intel CAT

[Tony Luck]

On Wed, Sep 27, 2023 at 12:44:39PM +0200, Maciej Wieczór-Retman wrote:
> Writing non-contiguous bitmasks is supported starting from the upcoming
> GNR microarchitecture forward.
> 
> That's also why the new CPUID bit meaning is in the ISA pdf and not in
> the SDM one currently.

New SDM released today has the non-contiguous bit. See vol 3B Figuer
18-33.

-Tony

Re: [PATCH v2 1/4] x86/resctrl: Enable non-contiguous bits in Intel CAT

[Moger, Babu]

Hi Maciej,

How about this subject line?

x86/resctrl: Enable non-contiguous CBMs on Intel CAT

On 9/22/2023 3:48 AM, Maciej Wieczor-Retman wrote:
> The setting for non-contiguous 1s support in Intel CAT is

> hardcoded to false. On these systems, writing non-contiguous
> 1s into the schemata file will fail before resctrl passes
> the value to the hardware.
>
> In Intel CAT CPUID.0x10.1:ECX[3] and CPUID.0x10.2:ECX[3] stopped
> being reserved and now carry information about non-contiguous 1s
> value support for L3 and L2 cache respectively. The CAT
> capacity bitmask (CBM) supports a non-contiguous 1s value if
> the bit is set.
>
> Replace the hardcoded non-contiguous support value with
> the support learned from the hardware. Add hardcoded non-contiguous
> support value to Haswell probe since it can't make use of CPUID for
> Cache allocation.
>
> Originally-by: Fenghua Yu <fenghua.yu@intel.com>
> Signed-off-by: Maciej Wieczor-Retman <maciej.wieczor-retman@intel.com>
> ---
> Changelog v2:
> - Rewrite part of a comment concerning Haswell. (Reinette)
>
>   arch/x86/kernel/cpu/resctrl/core.c        |  9 ++++++---
>   arch/x86/kernel/cpu/resctrl/ctrlmondata.c | 10 ++++++----
>   arch/x86/kernel/cpu/resctrl/internal.h    |  9 +++++++++
>   3 files changed, 21 insertions(+), 7 deletions(-)
>
> diff --git a/arch/x86/kernel/cpu/resctrl/core.c b/arch/x86/kernel/cpu/resctrl/core.c
> index 030d3b409768..c783a873147c 100644
> --- a/arch/x86/kernel/cpu/resctrl/core.c
> +++ b/arch/x86/kernel/cpu/resctrl/core.c
> @@ -152,6 +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;

Is this change required?

This is always set to false in rdt_init_res_defs_intel().

>   	r->alloc_capable = true;
>   
>   	rdt_alloc_capable = true;
> @@ -267,15 +268,18 @@ static void rdt_get_cache_alloc_cfg(int idx, struct rdt_resource *r)
>   {
>   	struct rdt_hw_resource *hw_res = resctrl_to_arch_res(r);
>   	union cpuid_0x10_1_eax eax;
> +	union cpuid_0x10_x_ecx ecx;
>   	union cpuid_0x10_x_edx edx;
> -	u32 ebx, ecx;
> +	u32 ebx;
>   
> -	cpuid_count(0x00000010, idx, &eax.full, &ebx, &ecx, &edx.full);
> +	cpuid_count(0x00000010, idx, &eax.full, &ebx, &ecx.full, &edx.full);
>   	hw_res->num_closid = edx.split.cos_max + 1;
>   	r->cache.cbm_len = eax.split.cbm_len + 1;
>   	r->default_ctrl = BIT_MASK(eax.split.cbm_len + 1) - 1;
>   	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->alloc_capable = true;
>   }
>   
> @@ -872,7 +876,6 @@ static __init void rdt_init_res_defs_intel(void)
>   
>   		if (r->rid == RDT_RESOURCE_L3 ||
>   		    r->rid == RDT_RESOURCE_L2) {
> -			r->cache.arch_has_sparse_bitmaps = false;

Why do you have to remove this one here?   This seems like a right place 
to initialize.

Thanks

Babu


>   			r->cache.arch_has_per_cpu_cfg = false;
>   			r->cache.min_cbm_bits = 1;
>   		} else if (r->rid == RDT_RESOURCE_MBA) {
> diff --git a/arch/x86/kernel/cpu/resctrl/ctrlmondata.c b/arch/x86/kernel/cpu/resctrl/ctrlmondata.c
> index b44c487727d4..f076f12cf8e8 100644
> --- a/arch/x86/kernel/cpu/resctrl/ctrlmondata.c
> +++ b/arch/x86/kernel/cpu/resctrl/ctrlmondata.c
> @@ -87,10 +87,12 @@ int parse_bw(struct rdt_parse_data *data, struct resctrl_schema *s,
>   
>   /*
>    * Check whether a cache bit mask is valid.
> - * For Intel the SDM says:
> - *	Please note that all (and only) contiguous '1' combinations
> - *	are allowed (e.g. FFFFH, 0FF0H, 003CH, etc.).
> - * Additionally Haswell requires at least two bits set.
> + * On Intel CPUs, non-contiguous 1s value support is indicated by CPUID:
> + *   - CPUID.0x10.1:ECX[3]: L3 non-contiguous 1s value supported if 1
> + *   - CPUID.0x10.2:ECX[3]: L2 non-contiguous 1s value supported if 1
> + *
> + * Haswell does not support a non-contiguous 1s value and additionally
> + * requires at least two bits set.
>    * AMD allows non-contiguous bitmasks.
>    */
>   static bool cbm_validate(char *buf, u32 *data, struct rdt_resource *r)
> diff --git a/arch/x86/kernel/cpu/resctrl/internal.h b/arch/x86/kernel/cpu/resctrl/internal.h
> index 85ceaf9a31ac..c47ef2f13e8e 100644
> --- a/arch/x86/kernel/cpu/resctrl/internal.h
> +++ b/arch/x86/kernel/cpu/resctrl/internal.h
> @@ -492,6 +492,15 @@ union cpuid_0x10_3_eax {
>   	unsigned int full;
>   };
>   
> +/* CPUID.(EAX=10H, ECX=ResID).ECX */
> +union cpuid_0x10_x_ecx {
> +	struct {
> +		unsigned int reserved:3;
> +		unsigned int noncont:1;
> +	} split;
> +	unsigned int full;
> +};
> +
>   /* CPUID.(EAX=10H, ECX=ResID).EDX */
>   union cpuid_0x10_x_edx {
>   	struct {

Re: [PATCH v2 1/4] x86/resctrl: Enable non-contiguous bits in Intel CAT

[Maciej Wieczór-Retman]

Hi, thanks for reviewing the series,

On 2023-09-27 at 17:34:27 -0500, Moger, Babu wrote:
>Hi Maciej,
>
>How about this subject line?
>
>x86/resctrl: Enable non-contiguous CBMs on Intel CAT

Changing "bits" to "CBMs" does indeed seem sensible so I'll do that if
there are no objections.

But I'm not sure the preposition collocation change from "in" to "on"
would be grammatical (at least from what I've read in docs about Intel
CAT so far).

>On 9/22/2023 3:48 AM, Maciej Wieczor-Retman wrote:
>> The setting for non-contiguous 1s support in Intel CAT is
>
>> hardcoded to false. On these systems, writing non-contiguous
>> 1s into the schemata file will fail before resctrl passes
>> the value to the hardware.
>> 
>> In Intel CAT CPUID.0x10.1:ECX[3] and CPUID.0x10.2:ECX[3] stopped
>> being reserved and now carry information about non-contiguous 1s
>> value support for L3 and L2 cache respectively. The CAT
>> capacity bitmask (CBM) supports a non-contiguous 1s value if
>> the bit is set.
>> 
>> Replace the hardcoded non-contiguous support value with
>> the support learned from the hardware. Add hardcoded non-contiguous
>> support value to Haswell probe since it can't make use of CPUID for
>> Cache allocation.
>> 
>> Originally-by: Fenghua Yu <fenghua.yu@intel.com>
>> Signed-off-by: Maciej Wieczor-Retman <maciej.wieczor-retman@intel.com>
>> ---
>> Changelog v2:
>> - Rewrite part of a comment concerning Haswell. (Reinette)
>> 
>>   arch/x86/kernel/cpu/resctrl/core.c        |  9 ++++++---
>>   arch/x86/kernel/cpu/resctrl/ctrlmondata.c | 10 ++++++----
>>   arch/x86/kernel/cpu/resctrl/internal.h    |  9 +++++++++
>>   3 files changed, 21 insertions(+), 7 deletions(-)
>> 
>> diff --git a/arch/x86/kernel/cpu/resctrl/core.c b/arch/x86/kernel/cpu/resctrl/core.c
>> index 030d3b409768..c783a873147c 100644
>> --- a/arch/x86/kernel/cpu/resctrl/core.c
>> +++ b/arch/x86/kernel/cpu/resctrl/core.c
>> @@ -152,6 +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;
>
>Is this change required?
>
>This is always set to false in rdt_init_res_defs_intel().

The logic behind moving this variable initialization from
rdt_init_res_defs_intel() into both cache_alloc_hsw_probe() and
rdt_get_cache_alloc_cfg() is that the variable doesn't really have a
default value anymore. It used to when the CPUID.0x10.1:ECX[3] and
CPUID.0x10.2:ECX[3] bits were reserved.

Now for the general case the variable is dependent on CPUID output.
And only for Haswell case it needs to be hardcoded to "false", so the
assignment makes more sense in Haswell probe rather than in the default
section.

>>   	r->alloc_capable = true;
>>   	rdt_alloc_capable = true;
>> @@ -267,15 +268,18 @@ static void rdt_get_cache_alloc_cfg(int idx, struct rdt_resource *r)
>>   {
>>   	struct rdt_hw_resource *hw_res = resctrl_to_arch_res(r);
>>   	union cpuid_0x10_1_eax eax;
>> +	union cpuid_0x10_x_ecx ecx;
>>   	union cpuid_0x10_x_edx edx;
>> -	u32 ebx, ecx;
>> +	u32 ebx;
>> -	cpuid_count(0x00000010, idx, &eax.full, &ebx, &ecx, &edx.full);
>> +	cpuid_count(0x00000010, idx, &eax.full, &ebx, &ecx.full, &edx.full);
>>   	hw_res->num_closid = edx.split.cos_max + 1;
>>   	r->cache.cbm_len = eax.split.cbm_len + 1;
>>   	r->default_ctrl = BIT_MASK(eax.split.cbm_len + 1) - 1;
>>   	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->alloc_capable = true;
>>   }
>> @@ -872,7 +876,6 @@ static __init void rdt_init_res_defs_intel(void)
>>   		if (r->rid == RDT_RESOURCE_L3 ||
>>   		    r->rid == RDT_RESOURCE_L2) {
>> -			r->cache.arch_has_sparse_bitmaps = false;
>
>Why do you have to remove this one here?   This seems like a right place to
>initialize.

Look at the previous comment.

-- 
Kind regards
Maciej Wieczór-Retman

Re: [PATCH v2 1/4] x86/resctrl: Enable non-contiguous bits in Intel CAT

[Moger, Babu]

Hi Maciej,

On 9/28/23 02:06, Maciej Wieczór-Retman wrote:
> Hi, thanks for reviewing the series,
> 
> On 2023-09-27 at 17:34:27 -0500, Moger, Babu wrote:
>> Hi Maciej,
>>
>> How about this subject line?
>>
>> x86/resctrl: Enable non-contiguous CBMs on Intel CAT
> 
> Changing "bits" to "CBMs" does indeed seem sensible so I'll do that if
> there are no objections.
> 
> But I'm not sure the preposition collocation change from "in" to "on"
> would be grammatical (at least from what I've read in docs about Intel
> CAT so far).
> 
>> On 9/22/2023 3:48 AM, Maciej Wieczor-Retman wrote:
>>> The setting for non-contiguous 1s support in Intel CAT is
>>
>>> hardcoded to false. On these systems, writing non-contiguous
>>> 1s into the schemata file will fail before resctrl passes
>>> the value to the hardware.
>>>
>>> In Intel CAT CPUID.0x10.1:ECX[3] and CPUID.0x10.2:ECX[3] stopped
>>> being reserved and now carry information about non-contiguous 1s
>>> value support for L3 and L2 cache respectively. The CAT
>>> capacity bitmask (CBM) supports a non-contiguous 1s value if
>>> the bit is set.
>>>
>>> Replace the hardcoded non-contiguous support value with
>>> the support learned from the hardware. Add hardcoded non-contiguous
>>> support value to Haswell probe since it can't make use of CPUID for
>>> Cache allocation.
>>>
>>> Originally-by: Fenghua Yu <fenghua.yu@intel.com>
>>> Signed-off-by: Maciej Wieczor-Retman <maciej.wieczor-retman@intel.com>
>>> ---
>>> Changelog v2:
>>> - Rewrite part of a comment concerning Haswell. (Reinette)
>>>
>>>   arch/x86/kernel/cpu/resctrl/core.c        |  9 ++++++---
>>>   arch/x86/kernel/cpu/resctrl/ctrlmondata.c | 10 ++++++----
>>>   arch/x86/kernel/cpu/resctrl/internal.h    |  9 +++++++++
>>>   3 files changed, 21 insertions(+), 7 deletions(-)
>>>
>>> diff --git a/arch/x86/kernel/cpu/resctrl/core.c b/arch/x86/kernel/cpu/resctrl/core.c
>>> index 030d3b409768..c783a873147c 100644
>>> --- a/arch/x86/kernel/cpu/resctrl/core.c
>>> +++ b/arch/x86/kernel/cpu/resctrl/core.c
>>> @@ -152,6 +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;
>>
>> Is this change required?
>>
>> This is always set to false in rdt_init_res_defs_intel().
> 
> The logic behind moving this variable initialization from
> rdt_init_res_defs_intel() into both cache_alloc_hsw_probe() and
> rdt_get_cache_alloc_cfg() is that the variable doesn't really have a
> default value anymore. It used to when the CPUID.0x10.1:ECX[3] and
> CPUID.0x10.2:ECX[3] bits were reserved.
> 
> Now for the general case the variable is dependent on CPUID output.
> And only for Haswell case it needs to be hardcoded to "false", so the
> assignment makes more sense in Haswell probe rather than in the default
> section.

Here is the current sequence order with your change.

1.
resctrl_late_init -> check_quirks -> __check_quirks_intel ->
cache_alloc_hsw_probe
   r->cache.arch_has_sparse_bitmaps = false; (new code)

2. resctrl_late_init -> rdt_init_res_defs -> rdt_init_res_defs_intel
   r->cache.arch_has_sparse_bitmaps = false; (old code)

3. resctrl_late_init -> get_rdt_resources -> get_rdt_alloc_resources ->
rdt_get_cache_alloc_cfg
   r->cache.arch_has_sparse_bitmaps = ecx.split.noncont; (new code)

The code in (3) is going to overwrite whatever is set in (1) or (2).

I would say you can just remove initialization in both (1) and (2). That
makes the code clearer to me. I assume reserved bits in Intel is always 0.

Thanks
Babu


> 
>>>   	r->alloc_capable = true;
>>>   	rdt_alloc_capable = true;
>>> @@ -267,15 +268,18 @@ static void rdt_get_cache_alloc_cfg(int idx, struct rdt_resource *r)
>>>   {
>>>   	struct rdt_hw_resource *hw_res = resctrl_to_arch_res(r);
>>>   	union cpuid_0x10_1_eax eax;
>>> +	union cpuid_0x10_x_ecx ecx;
>>>   	union cpuid_0x10_x_edx edx;
>>> -	u32 ebx, ecx;
>>> +	u32 ebx;
>>> -	cpuid_count(0x00000010, idx, &eax.full, &ebx, &ecx, &edx.full);
>>> +	cpuid_count(0x00000010, idx, &eax.full, &ebx, &ecx.full, &edx.full);
>>>   	hw_res->num_closid = edx.split.cos_max + 1;
>>>   	r->cache.cbm_len = eax.split.cbm_len + 1;
>>>   	r->default_ctrl = BIT_MASK(eax.split.cbm_len + 1) - 1;
>>>   	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->alloc_capable = true;
>>>   }
>>> @@ -872,7 +876,6 @@ static __init void rdt_init_res_defs_intel(void)
>>>   		if (r->rid == RDT_RESOURCE_L3 ||
>>>   		    r->rid == RDT_RESOURCE_L2) {
>>> -			r->cache.arch_has_sparse_bitmaps = false;
>>
>> Why do you have to remove this one here?   This seems like a right place to
>> initialize.
> 
> Look at the previous comment.
> 

-- 
Thanks
Babu Moger

Re: [PATCH v2 1/4] x86/resctrl: Enable non-contiguous bits in Intel CAT

[Reinette Chatre]

Hi Babu,

On 9/28/2023 8:08 AM, Moger, Babu wrote:
> On 9/28/23 02:06, Maciej Wieczór-Retman wrote:
>> On 2023-09-27 at 17:34:27 -0500, Moger, Babu wrote:
>>> On 9/22/2023 3:48 AM, Maciej Wieczor-Retman wrote:
...

>>>> diff --git a/arch/x86/kernel/cpu/resctrl/core.c b/arch/x86/kernel/cpu/resctrl/core.c
>>>> index 030d3b409768..c783a873147c 100644
>>>> --- a/arch/x86/kernel/cpu/resctrl/core.c
>>>> +++ b/arch/x86/kernel/cpu/resctrl/core.c
>>>> @@ -152,6 +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;
>>>
>>> Is this change required?
>>>
>>> This is always set to false in rdt_init_res_defs_intel().
>>
>> The logic behind moving this variable initialization from
>> rdt_init_res_defs_intel() into both cache_alloc_hsw_probe() and
>> rdt_get_cache_alloc_cfg() is that the variable doesn't really have a
>> default value anymore. It used to when the CPUID.0x10.1:ECX[3] and
>> CPUID.0x10.2:ECX[3] bits were reserved.
>>
>> Now for the general case the variable is dependent on CPUID output.
>> And only for Haswell case it needs to be hardcoded to "false", so the
>> assignment makes more sense in Haswell probe rather than in the default
>> section.
> 
> Here is the current sequence order with your change.
> 
> 1.
> resctrl_late_init -> check_quirks -> __check_quirks_intel ->
> cache_alloc_hsw_probe
>    r->cache.arch_has_sparse_bitmaps = false; (new code)
> 
> 2. resctrl_late_init -> rdt_init_res_defs -> rdt_init_res_defs_intel
>    r->cache.arch_has_sparse_bitmaps = false; (old code)
> 
> 3. resctrl_late_init -> get_rdt_resources -> get_rdt_alloc_resources ->
> rdt_get_cache_alloc_cfg
>    r->cache.arch_has_sparse_bitmaps = ecx.split.noncont; (new code)
> 
> The code in (3) is going to overwrite whatever is set in (1) or (2).
> 
> I would say you can just remove initialization in both (1) and (2). That
> makes the code clearer to me. I assume reserved bits in Intel is always 0.
> 

I believe Maciej already addressed this in his response to a similar question
from Peter. Please see:
https://lore.kernel.org/lkml/xnjmmsj5pjskbqeynor2ztha5dmkhxa44j764ohtjhtywy7idb@soobjiql4liy/

Reinette

Re: [PATCH v2 1/4] x86/resctrl: Enable non-contiguous bits in Intel CAT

[Moger, Babu]

Hi Reinette,

On 9/28/23 10:53, Reinette Chatre wrote:
> Hi Babu,
> 
> On 9/28/2023 8:08 AM, Moger, Babu wrote:
>> On 9/28/23 02:06, Maciej Wieczór-Retman wrote:
>>> On 2023-09-27 at 17:34:27 -0500, Moger, Babu wrote:
>>>> On 9/22/2023 3:48 AM, Maciej Wieczor-Retman wrote:
> ...
> 
>>>>> diff --git a/arch/x86/kernel/cpu/resctrl/core.c b/arch/x86/kernel/cpu/resctrl/core.c
>>>>> index 030d3b409768..c783a873147c 100644
>>>>> --- a/arch/x86/kernel/cpu/resctrl/core.c
>>>>> +++ b/arch/x86/kernel/cpu/resctrl/core.c
>>>>> @@ -152,6 +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;
>>>>
>>>> Is this change required?
>>>>
>>>> This is always set to false in rdt_init_res_defs_intel().
>>>
>>> The logic behind moving this variable initialization from
>>> rdt_init_res_defs_intel() into both cache_alloc_hsw_probe() and
>>> rdt_get_cache_alloc_cfg() is that the variable doesn't really have a
>>> default value anymore. It used to when the CPUID.0x10.1:ECX[3] and
>>> CPUID.0x10.2:ECX[3] bits were reserved.
>>>
>>> Now for the general case the variable is dependent on CPUID output.
>>> And only for Haswell case it needs to be hardcoded to "false", so the
>>> assignment makes more sense in Haswell probe rather than in the default
>>> section.
>>
>> Here is the current sequence order with your change.
>>
>> 1.
>> resctrl_late_init -> check_quirks -> __check_quirks_intel ->
>> cache_alloc_hsw_probe
>>    r->cache.arch_has_sparse_bitmaps = false; (new code)
>>
>> 2. resctrl_late_init -> rdt_init_res_defs -> rdt_init_res_defs_intel
>>    r->cache.arch_has_sparse_bitmaps = false; (old code)
>>
>> 3. resctrl_late_init -> get_rdt_resources -> get_rdt_alloc_resources ->
>> rdt_get_cache_alloc_cfg
>>    r->cache.arch_has_sparse_bitmaps = ecx.split.noncont; (new code)
>>
>> The code in (3) is going to overwrite whatever is set in (1) or (2).
>>
>> I would say you can just remove initialization in both (1) and (2). That
>> makes the code clearer to me. I assume reserved bits in Intel is always 0.
>>
> 
> I believe Maciej already addressed this in his response to a similar question
> from Peter. Please see:
> https://lore.kernel.org/lkml/xnjmmsj5pjskbqeynor2ztha5dmkhxa44j764ohtjhtywy7idb@soobjiql4liy/

The rdt_alloc_capable part is kind of hidden. Now it makes sense.
Thanks
Babu Moger

[PATCH v2 2/4] x86/resctrl: Add sparse_masks file in info

[Maciej Wieczor-Retman]

From: Fenghua Yu <fenghua.yu@intel.com>

Add the interface in resctrl FS to show if sparse cache allocations
bit masks are supported on the platform. Reading the file returns
either a "1" if non-contiguous 1s are supported and "0" otherwise.
The file path is /sys/fs/resctrl/info/{resource}/sparse_masks, where
{resource} can be either "L2" or "L3".

Signed-off-by: Fenghua Yu <fenghua.yu@intel.com>
Signed-off-by: Maciej Wieczor-Retman <maciej.wieczor-retman@intel.com>
---
Changelog v2:
- Change bitmap naming convention to bit mask. (Reinette)
- Change file name to "sparse_masks". (Reinette)

 arch/x86/kernel/cpu/resctrl/rdtgroup.c | 18 ++++++++++++++++++
 1 file changed, 18 insertions(+)

diff --git a/arch/x86/kernel/cpu/resctrl/rdtgroup.c b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
index 725344048f85..5383169ff982 100644
--- a/arch/x86/kernel/cpu/resctrl/rdtgroup.c
+++ b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
@@ -895,6 +895,17 @@ 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)
+{
+	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);
+
+	return 0;
+}
+
 /**
  * rdt_bit_usage_show - Display current usage of resources
  *
@@ -1839,6 +1850,13 @@ static struct rftype res_common_files[] = {
 		.seq_show	= rdtgroup_size_show,
 		.fflags		= RF_CTRL_BASE,
 	},
+	{
+		.name		= "sparse_masks",
+		.mode		= 0444,
+		.kf_ops		= &rdtgroup_kf_single_ops,
+		.seq_show	= rdt_has_sparse_bitmaps_show,
+		.fflags		= RF_CTRL_INFO | RFTYPE_RES_CACHE,
+	},
 
 };
 
-- 
2.42.0

Re: [PATCH v2 2/4] x86/resctrl: Add sparse_masks file in info

[Reinette Chatre]

Hi Maciej,

On 9/22/2023 1:48 AM, Maciej Wieczor-Retman wrote:
> From: Fenghua Yu <fenghua.yu@intel.com>
> 
> Add the interface in resctrl FS to show if sparse cache allocations

Should this maybe be "cache allocation"?

> bit masks are supported on the platform. Reading the file returns
> either a "1" if non-contiguous 1s are supported and "0" otherwise.
> The file path is /sys/fs/resctrl/info/{resource}/sparse_masks, where
> {resource} can be either "L2" or "L3".
> 

Reinette

Re: [PATCH v2 2/4] x86/resctrl: Add sparse_masks file in info

[Maciej Wieczór-Retman]

On 2023-09-28 at 13:47:30 -0700, Reinette Chatre wrote:
>Hi Maciej,
>
>On 9/22/2023 1:48 AM, Maciej Wieczor-Retman wrote:
>> From: Fenghua Yu <fenghua.yu@intel.com>
>> 
>> Add the interface in resctrl FS to show if sparse cache allocations
>
>Should this maybe be "cache allocation"?

That does sound a bit better. I'll change it, thanks.

>> bit masks are supported on the platform. Reading the file returns
>> either a "1" if non-contiguous 1s are supported and "0" otherwise.
>> The file path is /sys/fs/resctrl/info/{resource}/sparse_masks, where
>> {resource} can be either "L2" or "L3".
>> 
>
>Reinette

-- 
Kind regards
Maciej Wieczór-Retman

[PATCH v2 3/4] Documentation/x86: Document resctrl's new sparse_masks

[Maciej Wieczor-Retman]

From: Fenghua Yu <fenghua.yu@intel.com>

The documentation mentions that non-contiguous bit masks are not
supported in Intel Cache Allocation Technology (CAT).

Update the documentation on how to determine if sparse bit masks are
allowed in L2 and L3 CAT.

Mention the file with feature support information is located in
the /sys/fs/resctrl/info/{resource}/ directories and enumerate what
are the possible outputs on file read operation.

Signed-off-by: Fenghua Yu <fenghua.yu@intel.com>
Signed-off-by: Maciej Wieczor-Retman <maciej.wieczor-retman@intel.com>
---
Changelog v2:
- Change bitmap naming convention to bit mask. (Reinette)

 Documentation/arch/x86/resctrl.rst | 16 ++++++++++++----
 1 file changed, 12 insertions(+), 4 deletions(-)

diff --git a/Documentation/arch/x86/resctrl.rst b/Documentation/arch/x86/resctrl.rst
index cb05d90111b4..4c6421e2aa31 100644
--- a/Documentation/arch/x86/resctrl.rst
+++ b/Documentation/arch/x86/resctrl.rst
@@ -124,6 +124,13 @@ related to allocation:
 			"P":
 			      Corresponding region is pseudo-locked. No
 			      sharing allowed.
+"sparse_masks":
+		Indicates if non-contiguous 1s value in CBM is supported.
+
+			"0":
+			      Only contiguous 1s value in CBM is supported.
+			"1":
+			      Non-contiguous 1s value in CBM is supported.
 
 Memory bandwidth(MB) subdirectory contains the following files
 with respect to allocation:
@@ -445,12 +452,13 @@ For cache resources we describe the portion of the cache that is available
 for allocation using a bitmask. The maximum value of the mask is defined
 by each cpu model (and may be different for different cache levels). It
 is found using CPUID, but is also provided in the "info" directory of
-the resctrl file system in "info/{resource}/cbm_mask". Intel hardware
+the resctrl file system in "info/{resource}/cbm_mask". Some Intel hardware
 requires that these masks have all the '1' bits in a contiguous block. So
 0x3, 0x6 and 0xC are legal 4-bit masks with two bits set, but 0x5, 0x9
-and 0xA are not.  On a system with a 20-bit mask each bit represents 5%
-of the capacity of the cache. You could partition the cache into four
-equal parts with masks: 0x1f, 0x3e0, 0x7c00, 0xf8000.
+and 0xA are not. Check /sys/fs/resctrl/info/{resource}/sparse_masks
+if non-contiguous 1s value is supported. On a system with a 20-bit mask
+each bit represents 5% of the capacity of the cache. You could partition
+the cache into four equal parts with masks: 0x1f, 0x3e0, 0x7c00, 0xf8000.
 
 Memory bandwidth Allocation and monitoring
 ==========================================
-- 
2.42.0

Re: [PATCH v2 3/4] Documentation/x86: Document resctrl's new sparse_masks

[Moger, Babu]

Hi Maciej

On 9/22/2023 3:48 AM, Maciej Wieczor-Retman wrote:
> From: Fenghua Yu <fenghua.yu@intel.com>
>
> The documentation mentions that non-contiguous bit masks are not
> supported in Intel Cache Allocation Technology (CAT).
>
> Update the documentation on how to determine if sparse bit masks are
> allowed in L2 and L3 CAT.
>
> Mention the file with feature support information is located in
> the /sys/fs/resctrl/info/{resource}/ directories and enumerate what
> are the possible outputs on file read operation.
>
> Signed-off-by: Fenghua Yu <fenghua.yu@intel.com>
> Signed-off-by: Maciej Wieczor-Retman <maciej.wieczor-retman@intel.com>
> ---
> Changelog v2:
> - Change bitmap naming convention to bit mask. (Reinette)
>
>   Documentation/arch/x86/resctrl.rst | 16 ++++++++++++----
>   1 file changed, 12 insertions(+), 4 deletions(-)
>
> diff --git a/Documentation/arch/x86/resctrl.rst b/Documentation/arch/x86/resctrl.rst
> index cb05d90111b4..4c6421e2aa31 100644
> --- a/Documentation/arch/x86/resctrl.rst
> +++ b/Documentation/arch/x86/resctrl.rst
> @@ -124,6 +124,13 @@ related to allocation:
>   			"P":
>   			      Corresponding region is pseudo-locked. No
>   			      sharing allowed.
> +"sparse_masks":
> +		Indicates if non-contiguous 1s value in CBM is supported.
> +
> +			"0":
> +			      Only contiguous 1s value in CBM is supported.

This is little confusing. How about?

Non-contiguous 1s value in CBM is not supported

Thanks
Babu

> +			"1":
> +			      Non-contiguous 1s value in CBM is supported.
>   
>   Memory bandwidth(MB) subdirectory contains the following files
>   with respect to allocation:
> @@ -445,12 +452,13 @@ For cache resources we describe the portion of the cache that is available
>   for allocation using a bitmask. The maximum value of the mask is defined
>   by each cpu model (and may be different for different cache levels). It
>   is found using CPUID, but is also provided in the "info" directory of
> -the resctrl file system in "info/{resource}/cbm_mask". Intel hardware
> +the resctrl file system in "info/{resource}/cbm_mask". Some Intel hardware
>   requires that these masks have all the '1' bits in a contiguous block. So
>   0x3, 0x6 and 0xC are legal 4-bit masks with two bits set, but 0x5, 0x9
> -and 0xA are not.  On a system with a 20-bit mask each bit represents 5%
> -of the capacity of the cache. You could partition the cache into four
> -equal parts with masks: 0x1f, 0x3e0, 0x7c00, 0xf8000.
> +and 0xA are not. Check /sys/fs/resctrl/info/{resource}/sparse_masks
> +if non-contiguous 1s value is supported. On a system with a 20-bit mask
> +each bit represents 5% of the capacity of the cache. You could partition
> +the cache into four equal parts with masks: 0x1f, 0x3e0, 0x7c00, 0xf8000.
>   
>   Memory bandwidth Allocation and monitoring
>   ==========================================

Re: [PATCH v2 3/4] Documentation/x86: Document resctrl's new sparse_masks

[Reinette Chatre]

Hi Babu,

On 9/27/2023 3:47 PM, Moger, Babu wrote:
> On 9/22/2023 3:48 AM, Maciej Wieczor-Retman wrote:
>> From: Fenghua Yu <fenghua.yu@intel.com>
>>
>> The documentation mentions that non-contiguous bit masks are not
>> supported in Intel Cache Allocation Technology (CAT).
>>
>> Update the documentation on how to determine if sparse bit masks are
>> allowed in L2 and L3 CAT.
>>
>> Mention the file with feature support information is located in
>> the /sys/fs/resctrl/info/{resource}/ directories and enumerate what
>> are the possible outputs on file read operation.
>>
>> Signed-off-by: Fenghua Yu <fenghua.yu@intel.com>
>> Signed-off-by: Maciej Wieczor-Retman <maciej.wieczor-retman@intel.com>
>> ---
>> Changelog v2:
>> - Change bitmap naming convention to bit mask. (Reinette)
>>
>>   Documentation/arch/x86/resctrl.rst | 16 ++++++++++++----
>>   1 file changed, 12 insertions(+), 4 deletions(-)
>>
>> diff --git a/Documentation/arch/x86/resctrl.rst b/Documentation/arch/x86/resctrl.rst
>> index cb05d90111b4..4c6421e2aa31 100644
>> --- a/Documentation/arch/x86/resctrl.rst
>> +++ b/Documentation/arch/x86/resctrl.rst
>> @@ -124,6 +124,13 @@ related to allocation:
>>               "P":
>>                     Corresponding region is pseudo-locked. No
>>                     sharing allowed.
>> +"sparse_masks":
>> +        Indicates if non-contiguous 1s value in CBM is supported.
>> +
>> +            "0":
>> +                  Only contiguous 1s value in CBM is supported.
> 
> This is little confusing. How about?
> 
> Non-contiguous 1s value in CBM is not supported
> 

It is not clear to me how changing it to a double
negative reduces confusion.

Reinette

Re: [PATCH v2 3/4] Documentation/x86: Document resctrl's new sparse_masks

[Fenghua Yu]

On 9/27/23 15:58, Reinette Chatre wrote:
> Hi Babu,
> 
> On 9/27/2023 3:47 PM, Moger, Babu wrote:
>> On 9/22/2023 3:48 AM, Maciej Wieczor-Retman wrote:
>>> From: Fenghua Yu <fenghua.yu@intel.com>
>>>
>>> The documentation mentions that non-contiguous bit masks are not
>>> supported in Intel Cache Allocation Technology (CAT).
>>>
>>> Update the documentation on how to determine if sparse bit masks are
>>> allowed in L2 and L3 CAT.
>>>
>>> Mention the file with feature support information is located in
>>> the /sys/fs/resctrl/info/{resource}/ directories and enumerate what
>>> are the possible outputs on file read operation.
>>>
>>> Signed-off-by: Fenghua Yu <fenghua.yu@intel.com>
>>> Signed-off-by: Maciej Wieczor-Retman <maciej.wieczor-retman@intel.com>
>>> ---
>>> Changelog v2:
>>> - Change bitmap naming convention to bit mask. (Reinette)
>>>
>>>    Documentation/arch/x86/resctrl.rst | 16 ++++++++++++----
>>>    1 file changed, 12 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/Documentation/arch/x86/resctrl.rst b/Documentation/arch/x86/resctrl.rst
>>> index cb05d90111b4..4c6421e2aa31 100644
>>> --- a/Documentation/arch/x86/resctrl.rst
>>> +++ b/Documentation/arch/x86/resctrl.rst
>>> @@ -124,6 +124,13 @@ related to allocation:
>>>                "P":
>>>                      Corresponding region is pseudo-locked. No
>>>                      sharing allowed.
>>> +"sparse_masks":
>>> +        Indicates if non-contiguous 1s value in CBM is supported.
>>> +
>>> +            "0":
>>> +                  Only contiguous 1s value in CBM is supported.
>>
>> This is little confusing. How about?
>>
>> Non-contiguous 1s value in CBM is not supported
>>
> 
> It is not clear to me how changing it to a double
> negative reduces confusion.
Agree with Reinette.

The original statement is clearer and more direct to explicitly state 
what is supported without introducing a negative assertion (not supported).

Thanks.

-Fenghua

Re: [PATCH v2 3/4] Documentation/x86: Document resctrl's new sparse_masks

[Moger, Babu]

On 9/27/23 18:02, Fenghua Yu wrote:
> 
> 
> On 9/27/23 15:58, Reinette Chatre wrote:
>> Hi Babu,
>>
>> On 9/27/2023 3:47 PM, Moger, Babu wrote:
>>> On 9/22/2023 3:48 AM, Maciej Wieczor-Retman wrote:
>>>> From: Fenghua Yu <fenghua.yu@intel.com>
>>>>
>>>> The documentation mentions that non-contiguous bit masks are not
>>>> supported in Intel Cache Allocation Technology (CAT).
>>>>
>>>> Update the documentation on how to determine if sparse bit masks are
>>>> allowed in L2 and L3 CAT.
>>>>
>>>> Mention the file with feature support information is located in
>>>> the /sys/fs/resctrl/info/{resource}/ directories and enumerate what
>>>> are the possible outputs on file read operation.
>>>>
>>>> Signed-off-by: Fenghua Yu <fenghua.yu@intel.com>
>>>> Signed-off-by: Maciej Wieczor-Retman <maciej.wieczor-retman@intel.com>
>>>> ---
>>>> Changelog v2:
>>>> - Change bitmap naming convention to bit mask. (Reinette)
>>>>
>>>>    Documentation/arch/x86/resctrl.rst | 16 ++++++++++++----
>>>>    1 file changed, 12 insertions(+), 4 deletions(-)
>>>>
>>>> diff --git a/Documentation/arch/x86/resctrl.rst
>>>> b/Documentation/arch/x86/resctrl.rst
>>>> index cb05d90111b4..4c6421e2aa31 100644
>>>> --- a/Documentation/arch/x86/resctrl.rst
>>>> +++ b/Documentation/arch/x86/resctrl.rst
>>>> @@ -124,6 +124,13 @@ related to allocation:
>>>>                "P":
>>>>                      Corresponding region is pseudo-locked. No
>>>>                      sharing allowed.
>>>> +"sparse_masks":
>>>> +        Indicates if non-contiguous 1s value in CBM is supported.
>>>> +
>>>> +            "0":
>>>> +                  Only contiguous 1s value in CBM is supported.
>>>
>>> This is little confusing. How about?
>>>
>>> Non-contiguous 1s value in CBM is not supported
>>>
>>
>> It is not clear to me how changing it to a double
>> negative reduces confusion.
> Agree with Reinette.
> 
> The original statement is clearer and more direct to explicitly state what
> is supported without introducing a negative assertion (not supported).

Ok. If you all agree, fine with me as well.

-- 
Thanks
Babu Moger

[PATCH v2 4/4] x86/resctrl: Rename arch_has_sparse_bitmaps

[Maciej Wieczor-Retman]

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(-)

diff --git a/arch/x86/kernel/cpu/resctrl/core.c b/arch/x86/kernel/cpu/resctrl/core.c
index c783a873147c..19e0681f0435 100644
--- a/arch/x86/kernel/cpu/resctrl/core.c
+++ b/arch/x86/kernel/cpu/resctrl/core.c
@@ -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) {
diff --git a/arch/x86/kernel/cpu/resctrl/ctrlmondata.c b/arch/x86/kernel/cpu/resctrl/ctrlmondata.c
index f076f12cf8e8..beccb0e87ba7 100644
--- a/arch/x86/kernel/cpu/resctrl/ctrlmondata.c
+++ b/arch/x86/kernel/cpu/resctrl/ctrlmondata.c
@@ -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;
diff --git a/arch/x86/kernel/cpu/resctrl/rdtgroup.c b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
index 5383169ff982..945801898a4d 100644
--- a/arch/x86/kernel/cpu/resctrl/rdtgroup.c
+++ b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
@@ -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,
 	},
 
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
@@ -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;
 };
 
-- 
2.42.0

Re: [PATCH v2 4/4] x86/resctrl: Rename arch_has_sparse_bitmaps

[Reinette Chatre]

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

Re: [PATCH v2 4/4] x86/resctrl: Rename arch_has_sparse_bitmaps

[Maciej Wieczór-Retman]

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.

-- 
Kind regards
Maciej Wieczór-Retman