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
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 <[email protected]>
Signed-off-by: Maciej Wieczor-Retman <[email protected]>
---
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
From: Fenghua Yu <[email protected]>
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 <[email protected]>
Signed-off-by: Maciej Wieczor-Retman <[email protected]>
---
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
From: Fenghua Yu <[email protected]>
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 <[email protected]>
Signed-off-by: Maciej Wieczor-Retman <[email protected]>
---
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
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
><[email protected]> 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 <[email protected]>
>
>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 <[email protected]>
>
>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
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
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 <[email protected]>
> Signed-off-by: Maciej Wieczor-Retman <[email protected]>
> ---
> 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 {
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 <[email protected]>
>>>
>>> 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 <[email protected]>
>>> Signed-off-by: Maciej Wieczor-Retman <[email protected]>
>>> ---
>>> 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
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 <[email protected]>
>>
>> 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 <[email protected]>
>> Signed-off-by: Maciej Wieczor-Retman <[email protected]>
>> ---
>> 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
Hi Maciej
On 9/22/2023 3:48 AM, Maciej Wieczor-Retman wrote:
> From: Fenghua Yu <[email protected]>
>
> 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 <[email protected]>
> Signed-off-by: Maciej Wieczor-Retman <[email protected]>
> ---
> 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
> ==========================================
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 <[email protected]>
>>>>
>>>> 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 <[email protected]>
>>>> Signed-off-by: Maciej Wieczor-Retman <[email protected]>
>>>> ---
>>>> 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
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 <[email protected]>
>> Signed-off-by: Maciej Wieczor-Retman <[email protected]>
>> ---
>> 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
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 <[email protected]>
>>> Signed-off-by: Maciej Wieczor-Retman <[email protected]>
>>> ---
>>> 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
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
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
Hi Maciej,
On 9/22/2023 1:48 AM, Maciej Wieczor-Retman wrote:
> From: Fenghua Yu <[email protected]>
>
> 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
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 <[email protected]>
>>
>> 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