| Message ID | 20230308064734.512457-1-suagrfillet@gmail.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 6a24915145c922b79d3ac78f681137a4c14a6d6b |
| Headers | show |
| Series | Revert "riscv: Set more data to cacheinfo" | expand |
| Context | Check | Description |
|---|---|---|
| conchuod/cover_letter | success | Single patches do not need cover letters |
| conchuod/tree_selection | success | Guessed tree name to be for-next |
| conchuod/fixes_present | success | Fixes tag not required for -next series |
| conchuod/maintainers_pattern | success | MAINTAINERS pattern errors before the patch: 1 and now 1 |
| conchuod/verify_signedoff | success | Signed-off-by tag matches author and committer |
| conchuod/kdoc | success | Errors and warnings before: 0 this patch: 0 |
| conchuod/build_rv64_clang_allmodconfig | success | Errors and warnings before: 21 this patch: 21 |
| conchuod/module_param | success | Was 0 now: 0 |
| conchuod/build_rv64_gcc_allmodconfig | success | Errors and warnings before: 37 this patch: 37 |
| conchuod/alphanumeric_selects | success | Out of order selects before the patch: 728 and now 728 |
| conchuod/build_rv32_defconfig | success | Build OK |
| conchuod/dtb_warn_rv64 | success | Errors and warnings before: 3 this patch: 3 |
| conchuod/header_inline | success | No static functions without inline keyword in header files |
| conchuod/checkpatch | success | total: 0 errors, 0 warnings, 0 checks, 92 lines checked |
| conchuod/source_inline | success | Was 0 now: 0 |
| conchuod/build_rv64_nommu_k210_defconfig | success | Build OK |
| conchuod/verify_fixes | success | No Fixes tag |
| conchuod/build_rv64_nommu_virt_defconfig | success | Build OK |
On Wed, Mar 08, 2023 at 02:47:34PM +0800, Song Shuai wrote: > This reverts commit baf7cbd94b5688f167443a2cc3dcea3300132099. > > There are some duplicate cache attributes populations executed > in both ci_leaf_init() and later cache_setup_properties(). > > Revert the commit baf7cbd94b56 ("riscv: Set more data to cacheinfo") > to setup only the level and type attributes at this early place. > I've attempted to do a little bit of history diving to figure out what commit to "blame" in a fixes tag for this. My initial thought was to blame 03f11f03dbfe ("RISC-V: Parse cpu topology during boot."), but baf7cbd94b56 ("riscv: Set more data to cacheinfo") looks like it came along some months later. I assume you double checked that the stuff in the aux vector is the same before/after this revert? Looking on lore, that seems to be why Zong did it this way in the first place: https://lore.kernel.org/linux-riscv/b6fed9b4c2a3eacb2a9c353c2570d9dc1d0c1c88.1598859038.git.zong.li@sifive.com/ Cheers, Conor. > Signed-off-by: Song Shuai <suagrfillet@gmail.com> > --- > arch/riscv/kernel/cacheinfo.c | 66 ++++++++--------------------------- > 1 file changed, 15 insertions(+), 51 deletions(-) > > diff --git a/arch/riscv/kernel/cacheinfo.c b/arch/riscv/kernel/cacheinfo.c > index 3a13113f1b29..305ebbdc780d 100644 > --- a/arch/riscv/kernel/cacheinfo.c > +++ b/arch/riscv/kernel/cacheinfo.c > @@ -64,53 +64,12 @@ uintptr_t get_cache_geometry(u32 level, enum cache_type type) > 0; > } > > -static void ci_leaf_init(struct cacheinfo *this_leaf, enum cache_type type, > - unsigned int level, unsigned int size, > - unsigned int sets, unsigned int line_size) > +static void ci_leaf_init(struct cacheinfo *this_leaf, > + struct device_node *node, > + enum cache_type type, unsigned int level) > { > this_leaf->level = level; > this_leaf->type = type; > - this_leaf->size = size; > - this_leaf->number_of_sets = sets; > - this_leaf->coherency_line_size = line_size; > - > - /* > - * If the cache is fully associative, there is no need to > - * check the other properties. > - */ > - if (sets == 1) > - return; > - > - /* > - * Set the ways number for n-ways associative, make sure > - * all properties are big than zero. > - */ > - if (sets > 0 && size > 0 && line_size > 0) > - this_leaf->ways_of_associativity = (size / sets) / line_size; > -} > - > -static void fill_cacheinfo(struct cacheinfo **this_leaf, > - struct device_node *node, unsigned int level) > -{ > - unsigned int size, sets, line_size; > - > - if (!of_property_read_u32(node, "cache-size", &size) && > - !of_property_read_u32(node, "cache-block-size", &line_size) && > - !of_property_read_u32(node, "cache-sets", &sets)) { > - ci_leaf_init((*this_leaf)++, CACHE_TYPE_UNIFIED, level, size, sets, line_size); > - } > - > - if (!of_property_read_u32(node, "i-cache-size", &size) && > - !of_property_read_u32(node, "i-cache-sets", &sets) && > - !of_property_read_u32(node, "i-cache-block-size", &line_size)) { > - ci_leaf_init((*this_leaf)++, CACHE_TYPE_INST, level, size, sets, line_size); > - } > - > - if (!of_property_read_u32(node, "d-cache-size", &size) && > - !of_property_read_u32(node, "d-cache-sets", &sets) && > - !of_property_read_u32(node, "d-cache-block-size", &line_size)) { > - ci_leaf_init((*this_leaf)++, CACHE_TYPE_DATA, level, size, sets, line_size); > - } > } > > int populate_cache_leaves(unsigned int cpu) > @@ -121,24 +80,29 @@ int populate_cache_leaves(unsigned int cpu) > struct device_node *prev = NULL; > int levels = 1, level = 1; > > - /* Level 1 caches in cpu node */ > - fill_cacheinfo(&this_leaf, np, level); > + if (of_property_read_bool(np, "cache-size")) > + ci_leaf_init(this_leaf++, np, CACHE_TYPE_UNIFIED, level); > + if (of_property_read_bool(np, "i-cache-size")) > + ci_leaf_init(this_leaf++, np, CACHE_TYPE_INST, level); > + if (of_property_read_bool(np, "d-cache-size")) > + ci_leaf_init(this_leaf++, np, CACHE_TYPE_DATA, level); > > - /* Next level caches in cache nodes */ > prev = np; > while ((np = of_find_next_cache_node(np))) { > of_node_put(prev); > prev = np; > - > if (!of_device_is_compatible(np, "cache")) > break; > if (of_property_read_u32(np, "cache-level", &level)) > break; > if (level <= levels) > break; > - > - fill_cacheinfo(&this_leaf, np, level); > - > + if (of_property_read_bool(np, "cache-size")) > + ci_leaf_init(this_leaf++, np, CACHE_TYPE_UNIFIED, level); > + if (of_property_read_bool(np, "i-cache-size")) > + ci_leaf_init(this_leaf++, np, CACHE_TYPE_INST, level); > + if (of_property_read_bool(np, "d-cache-size")) > + ci_leaf_init(this_leaf++, np, CACHE_TYPE_DATA, level); > levels = level; > } > of_node_put(np); > -- > 2.20.1 >
On Wed, Mar 08, 2023 at 02:47:34PM +0800, Song Shuai wrote: > This reverts commit baf7cbd94b5688f167443a2cc3dcea3300132099. > > There are some duplicate cache attributes populations executed > in both ci_leaf_init() and later cache_setup_properties(). > > Revert the commit baf7cbd94b56 ("riscv: Set more data to cacheinfo") > to setup only the level and type attributes at this early place. > I had noticed the same and had something similar when we did some rework around for v6.1 merge window. But there were lot of other issues to be addressed at the moment and hence deferred this. So for the change in general, but as Conor responded, it would be good to do some checking to ensure nothing breaks and all the requirements this patch(baf7cbd94b56) addressed are already handled in the core. Acked-by: Sudeep Holla <sudeep.holla@arm.com> -- Regards, Sudeep
Hi, Conor & Sudeep : Sudeep Holla <sudeep.holla@arm.com> 于2023年3月8日周三 11:35写道： > > On Wed, Mar 08, 2023 at 02:47:34PM +0800, Song Shuai wrote: > > This reverts commit baf7cbd94b5688f167443a2cc3dcea3300132099. > > > > There are some duplicate cache attributes populations executed > > in both ci_leaf_init() and later cache_setup_properties(). > > > > Revert the commit baf7cbd94b56 ("riscv: Set more data to cacheinfo") > > to setup only the level and type attributes at this early place. > > > > I had noticed the same and had something similar when we did some rework > around for v6.1 merge window. But there were lot of other issues to be > addressed at the moment and hence deferred this. > > So for the change in general, but as Conor responded, it would be good > to do some checking to ensure nothing breaks and all the requirements > this patch(baf7cbd94b56) addressed are already handled in the core. As you suggested, commit (da29dbcda49d "riscv: Add cache information in AUX vector") in the "Get cache information from userland" series should be checked. The commit da29dbcda49d adds the cacheinfo (read from ci_cacheinfo(cpu)) in ELF auxiliary vectors, so process can fetch the cacheinfo through glibc sysconf() after ELF loading. At the same time, the glibc related support was enabled by its commit (15b38ffc10 "riscv: Get cache information through sysconf") With this reverting patch applied, the output of `getconf -a` looks good in Qemu sifive_u machine and rootfs image with glibc-2.35. ``` LEVEL1_ICACHE_SIZE 32768 LEVEL1_ICACHE_ASSOC 8 LEVEL1_ICACHE_LINESIZE 64 LEVEL1_DCACHE_SIZE 32768 LEVEL1_DCACHE_ASSOC 8 LEVEL1_DCACHE_LINESIZE 64 LEVEL2_CACHE_SIZE 2097152 LEVEL2_CACHE_ASSOC 32 LEVEL2_CACHE_LINESIZE 64 ``` > > Acked-by: Sudeep Holla <sudeep.holla@arm.com> > > -- > Regards, > Sudeep
On Thu, Mar 09, 2023 at 07:54:45AM +0000, Song Shuai wrote: > Hi, Conor & Sudeep : > > Sudeep Holla <sudeep.holla@arm.com> 于2023年3月8日周三 11:35写道： > > > > On Wed, Mar 08, 2023 at 02:47:34PM +0800, Song Shuai wrote: > > > This reverts commit baf7cbd94b5688f167443a2cc3dcea3300132099. > > > > > > There are some duplicate cache attributes populations executed > > > in both ci_leaf_init() and later cache_setup_properties(). > > > > > > Revert the commit baf7cbd94b56 ("riscv: Set more data to cacheinfo") > > > to setup only the level and type attributes at this early place. > > > > > > > I had noticed the same and had something similar when we did some rework > > around for v6.1 merge window. But there were lot of other issues to be > > addressed at the moment and hence deferred this. > > > > So for the change in general, but as Conor responded, it would be good > > to do some checking to ensure nothing breaks and all the requirements > > this patch(baf7cbd94b56) addressed are already handled in the core. > > As you suggested, commit (da29dbcda49d "riscv: Add cache information > in AUX vector") > in the "Get cache information from userland" series should be checked. > > The commit da29dbcda49d adds the cacheinfo (read from > ci_cacheinfo(cpu)) in ELF auxiliary vectors, > so process can fetch the cacheinfo through glibc sysconf() after ELF loading. > At the same time, the glibc related support was enabled by its commit > (15b38ffc10 "riscv: Get cache information through sysconf") > > With this reverting patch applied, the output of `getconf -a` looks good > in Qemu sifive_u machine and rootfs image with glibc-2.35. If, as I think you're implying, it is unchanged before/after: Acked-by: Conor Dooley <conor.dooley@microchip.com> Thanks, Conor.
Hello: This patch was applied to riscv/linux.git (for-next) by Palmer Dabbelt <palmer@rivosinc.com>: On Wed, 8 Mar 2023 14:47:34 +0800 you wrote: > This reverts commit baf7cbd94b5688f167443a2cc3dcea3300132099. > > There are some duplicate cache attributes populations executed > in both ci_leaf_init() and later cache_setup_properties(). > > Revert the commit baf7cbd94b56 ("riscv: Set more data to cacheinfo") > to setup only the level and type attributes at this early place. > > [...] Here is the summary with links: - Revert "riscv: Set more data to cacheinfo" https://git.kernel.org/riscv/c/6a24915145c9 You are awesome, thank you!
diff --git a/arch/riscv/kernel/cacheinfo.c b/arch/riscv/kernel/cacheinfo.c index 3a13113f1b29..305ebbdc780d 100644 --- a/arch/riscv/kernel/cacheinfo.c +++ b/arch/riscv/kernel/cacheinfo.c @@ -64,53 +64,12 @@ uintptr_t get_cache_geometry(u32 level, enum cache_type type) 0; } -static void ci_leaf_init(struct cacheinfo *this_leaf, enum cache_type type, - unsigned int level, unsigned int size, - unsigned int sets, unsigned int line_size) +static void ci_leaf_init(struct cacheinfo *this_leaf, + struct device_node *node, + enum cache_type type, unsigned int level) { this_leaf->level = level; this_leaf->type = type; - this_leaf->size = size; - this_leaf->number_of_sets = sets; - this_leaf->coherency_line_size = line_size; - - /* - * If the cache is fully associative, there is no need to - * check the other properties. - */ - if (sets == 1) - return; - - /* - * Set the ways number for n-ways associative, make sure - * all properties are big than zero. - */ - if (sets > 0 && size > 0 && line_size > 0) - this_leaf->ways_of_associativity = (size / sets) / line_size; -} - -static void fill_cacheinfo(struct cacheinfo **this_leaf, - struct device_node *node, unsigned int level) -{ - unsigned int size, sets, line_size; - - if (!of_property_read_u32(node, "cache-size", &size) && - !of_property_read_u32(node, "cache-block-size", &line_size) && - !of_property_read_u32(node, "cache-sets", &sets)) { - ci_leaf_init((*this_leaf)++, CACHE_TYPE_UNIFIED, level, size, sets, line_size); - } - - if (!of_property_read_u32(node, "i-cache-size", &size) && - !of_property_read_u32(node, "i-cache-sets", &sets) && - !of_property_read_u32(node, "i-cache-block-size", &line_size)) { - ci_leaf_init((*this_leaf)++, CACHE_TYPE_INST, level, size, sets, line_size); - } - - if (!of_property_read_u32(node, "d-cache-size", &size) && - !of_property_read_u32(node, "d-cache-sets", &sets) && - !of_property_read_u32(node, "d-cache-block-size", &line_size)) { - ci_leaf_init((*this_leaf)++, CACHE_TYPE_DATA, level, size, sets, line_size); - } } int populate_cache_leaves(unsigned int cpu) @@ -121,24 +80,29 @@ int populate_cache_leaves(unsigned int cpu) struct device_node *prev = NULL; int levels = 1, level = 1; - /* Level 1 caches in cpu node */ - fill_cacheinfo(&this_leaf, np, level); + if (of_property_read_bool(np, "cache-size")) + ci_leaf_init(this_leaf++, np, CACHE_TYPE_UNIFIED, level); + if (of_property_read_bool(np, "i-cache-size")) + ci_leaf_init(this_leaf++, np, CACHE_TYPE_INST, level); + if (of_property_read_bool(np, "d-cache-size")) + ci_leaf_init(this_leaf++, np, CACHE_TYPE_DATA, level); - /* Next level caches in cache nodes */ prev = np; while ((np = of_find_next_cache_node(np))) { of_node_put(prev); prev = np; - if (!of_device_is_compatible(np, "cache")) break; if (of_property_read_u32(np, "cache-level", &level)) break; if (level <= levels) break; - - fill_cacheinfo(&this_leaf, np, level); - + if (of_property_read_bool(np, "cache-size")) + ci_leaf_init(this_leaf++, np, CACHE_TYPE_UNIFIED, level); + if (of_property_read_bool(np, "i-cache-size")) + ci_leaf_init(this_leaf++, np, CACHE_TYPE_INST, level); + if (of_property_read_bool(np, "d-cache-size")) + ci_leaf_init(this_leaf++, np, CACHE_TYPE_DATA, level); levels = level; } of_node_put(np);
This reverts commit baf7cbd94b5688f167443a2cc3dcea3300132099. There are some duplicate cache attributes populations executed in both ci_leaf_init() and later cache_setup_properties(). Revert the commit baf7cbd94b56 ("riscv: Set more data to cacheinfo") to setup only the level and type attributes at this early place. Signed-off-by: Song Shuai <suagrfillet@gmail.com> --- arch/riscv/kernel/cacheinfo.c | 66 ++++++++--------------------------- 1 file changed, 15 insertions(+), 51 deletions(-)