| Message ID | 20240414025826.64025-2-cuiyunhui@bytedance.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [v2,1/3] riscv: cacheinfo: remove the useless parameter (node) of ci_leaf_init() | expand |
On Sun, Apr 14, 2024 at 10:58:25AM +0800, Yunhui Cui wrote: > Before cacheinfo can be built correctly, we need to initialize level > and type. Since RSIC-V currently does not have a register group that > describes cache-related attributes like ARM64, we cannot obtain them > directly, so now we obtain cache leaves from the ACPI PPTT table > (acpi_get_cache_info()) and set the cache type through split_levels. > > Suggested-by: Jeremy Linton <jeremy.linton@arm.com> > Suggested-by: Sudeep Holla <sudeep.holla@arm.com> > Signed-off-by: Yunhui Cui <cuiyunhui@bytedance.com> > --- > arch/riscv/kernel/cacheinfo.c | 23 +++++++++++++++++++++++ > 1 file changed, 23 insertions(+) > > diff --git a/arch/riscv/kernel/cacheinfo.c b/arch/riscv/kernel/cacheinfo.c > index 30a6878287ad..ece92aa404e3 100644 > --- a/arch/riscv/kernel/cacheinfo.c > +++ b/arch/riscv/kernel/cacheinfo.c > @@ -6,6 +6,7 @@ > #include <linux/cpu.h> > #include <linux/of.h> > #include <asm/cacheinfo.h> > +#include <linux/acpi.h> > > static struct riscv_cacheinfo_ops *rv_cache_ops; > > @@ -78,6 +79,28 @@ int populate_cache_leaves(unsigned int cpu) > struct device_node *prev = NULL; > int levels = 1, level = 1; > > + if (!acpi_disabled) { > + int ret, idx, fw_levels, split_levels; > + > + ret = acpi_get_cache_info(cpu, &fw_levels, &split_levels); > + if (ret) > + return ret; > + > + /* must be set, so we can drop num_leaves assignment below */ I intentionally added this above comment to check and drop the below statement if it is already set. Please check if the value is already set when we call into this function(which I think is the case). > + this_cpu_ci->num_leaves = fw_levels + split_levels; > + > + for (idx = 0; level <= this_cpu_ci->num_levels && > + idx < this_cpu_ci->num_leaves; idx++, level++) { > + if (level <= split_levels) { > + ci_leaf_init(this_leaf++, CACHE_TYPE_DATA, level); > + ci_leaf_init(this_leaf++, CACHE_TYPE_INST, level); > + } else { > + ci_leaf_init(this_leaf++, CACHE_TYPE_UNIFIED, level); > + } > + } > + return 0; > + } > + > if (of_property_read_bool(np, "cache-size")) > ci_leaf_init(this_leaf++, CACHE_TYPE_UNIFIED, level); > if (of_property_read_bool(np, "i-cache-size")) > -- > 2.20.1 >
Hi Sudeep, On Mon, Apr 15, 2024 at 4:45 PM Sudeep Holla <sudeep.holla@arm.com> wrote: > > On Sun, Apr 14, 2024 at 10:58:25AM +0800, Yunhui Cui wrote: > > Before cacheinfo can be built correctly, we need to initialize level > > and type. Since RSIC-V currently does not have a register group that > > describes cache-related attributes like ARM64, we cannot obtain them > > directly, so now we obtain cache leaves from the ACPI PPTT table > > (acpi_get_cache_info()) and set the cache type through split_levels. > > > > Suggested-by: Jeremy Linton <jeremy.linton@arm.com> > > Suggested-by: Sudeep Holla <sudeep.holla@arm.com> > > Signed-off-by: Yunhui Cui <cuiyunhui@bytedance.com> > > --- > > arch/riscv/kernel/cacheinfo.c | 23 +++++++++++++++++++++++ > > 1 file changed, 23 insertions(+) > > > > diff --git a/arch/riscv/kernel/cacheinfo.c b/arch/riscv/kernel/cacheinfo.c > > index 30a6878287ad..ece92aa404e3 100644 > > --- a/arch/riscv/kernel/cacheinfo.c > > +++ b/arch/riscv/kernel/cacheinfo.c > > @@ -6,6 +6,7 @@ > > #include <linux/cpu.h> > > #include <linux/of.h> > > #include <asm/cacheinfo.h> > > +#include <linux/acpi.h> > > > > static struct riscv_cacheinfo_ops *rv_cache_ops; > > > > @@ -78,6 +79,28 @@ int populate_cache_leaves(unsigned int cpu) > > struct device_node *prev = NULL; > > int levels = 1, level = 1; > > > > + if (!acpi_disabled) { > > + int ret, idx, fw_levels, split_levels; > > + > > + ret = acpi_get_cache_info(cpu, &fw_levels, &split_levels); > > + if (ret) > > + return ret; > > + > > + /* must be set, so we can drop num_leaves assignment below */ > > I intentionally added this above comment to check and drop the below statement > if it is already set. Please check if the value is already set when we call > into this function(which I think is the case). > > > + this_cpu_ci->num_leaves = fw_levels + split_levels; Uh,got it. I understand that there is no need to add this line: "this_cpu_ci->num_leaves = fw_levels + split_levels; " , because in the Master core first it will: smp_prepare_cpus ->init_cpu_topology ->for_each_possible_cpu(cpu) { fetch_cache_info(cpu); //num_leaves and num_levels will be set Then store_cpu_topology->update_siblings_masks->detect_cache_attributes->populate_cache_leaves(). Slave core will follow the logic of smp_callin->store_cpu_topology(). It's the same after I tested it, so I plan to remove that line and update V3, what do you think? Thanks, Yunhui
On Mon, Apr 15, 2024 at 08:03:38PM +0800, yunhui cui wrote: > Hi Sudeep, > > On Mon, Apr 15, 2024 at 4:45 PM Sudeep Holla <sudeep.holla@arm.com> wrote: > > > > On Sun, Apr 14, 2024 at 10:58:25AM +0800, Yunhui Cui wrote: > > > Before cacheinfo can be built correctly, we need to initialize level > > > and type. Since RSIC-V currently does not have a register group that > > > describes cache-related attributes like ARM64, we cannot obtain them > > > directly, so now we obtain cache leaves from the ACPI PPTT table > > > (acpi_get_cache_info()) and set the cache type through split_levels. > > > > > > Suggested-by: Jeremy Linton <jeremy.linton@arm.com> > > > Suggested-by: Sudeep Holla <sudeep.holla@arm.com> > > > Signed-off-by: Yunhui Cui <cuiyunhui@bytedance.com> > > > --- > > > arch/riscv/kernel/cacheinfo.c | 23 +++++++++++++++++++++++ > > > 1 file changed, 23 insertions(+) > > > > > > diff --git a/arch/riscv/kernel/cacheinfo.c b/arch/riscv/kernel/cacheinfo.c > > > index 30a6878287ad..ece92aa404e3 100644 > > > --- a/arch/riscv/kernel/cacheinfo.c > > > +++ b/arch/riscv/kernel/cacheinfo.c > > > @@ -6,6 +6,7 @@ > > > #include <linux/cpu.h> > > > #include <linux/of.h> > > > #include <asm/cacheinfo.h> > > > +#include <linux/acpi.h> > > > > > > static struct riscv_cacheinfo_ops *rv_cache_ops; > > > > > > @@ -78,6 +79,28 @@ int populate_cache_leaves(unsigned int cpu) > > > struct device_node *prev = NULL; > > > int levels = 1, level = 1; > > > > > > + if (!acpi_disabled) { > > > + int ret, idx, fw_levels, split_levels; > > > + > > > + ret = acpi_get_cache_info(cpu, &fw_levels, &split_levels); > > > + if (ret) > > > + return ret; > > > + > > > + /* must be set, so we can drop num_leaves assignment below */ > > > > I intentionally added this above comment to check and drop the below statement > > if it is already set. Please check if the value is already set when we call > > into this function(which I think is the case). > > > > > + this_cpu_ci->num_leaves = fw_levels + split_levels; > > Uh,got it. I understand that there is no need to add this line: > "this_cpu_ci->num_leaves = fw_levels + split_levels; " , because in > the Master core first it will: > smp_prepare_cpus > ->init_cpu_topology > ->for_each_possible_cpu(cpu) { > fetch_cache_info(cpu); //num_leaves and num_levels will be set > Then store_cpu_topology->update_siblings_masks->detect_cache_attributes->populate_cache_leaves(). > > Slave core will follow the logic of smp_callin->store_cpu_topology(). > It's the same after I tested it, so I plan to remove that line and > update V3, what do you think? > Correct, just drop the statement updating "this_cpu_ci->num_leaves".
diff --git a/arch/riscv/kernel/cacheinfo.c b/arch/riscv/kernel/cacheinfo.c index 30a6878287ad..ece92aa404e3 100644 --- a/arch/riscv/kernel/cacheinfo.c +++ b/arch/riscv/kernel/cacheinfo.c @@ -6,6 +6,7 @@ #include <linux/cpu.h> #include <linux/of.h> #include <asm/cacheinfo.h> +#include <linux/acpi.h> static struct riscv_cacheinfo_ops *rv_cache_ops; @@ -78,6 +79,28 @@ int populate_cache_leaves(unsigned int cpu) struct device_node *prev = NULL; int levels = 1, level = 1; + if (!acpi_disabled) { + int ret, idx, fw_levels, split_levels; + + ret = acpi_get_cache_info(cpu, &fw_levels, &split_levels); + if (ret) + return ret; + + /* must be set, so we can drop num_leaves assignment below */ + this_cpu_ci->num_leaves = fw_levels + split_levels; + + for (idx = 0; level <= this_cpu_ci->num_levels && + idx < this_cpu_ci->num_leaves; idx++, level++) { + if (level <= split_levels) { + ci_leaf_init(this_leaf++, CACHE_TYPE_DATA, level); + ci_leaf_init(this_leaf++, CACHE_TYPE_INST, level); + } else { + ci_leaf_init(this_leaf++, CACHE_TYPE_UNIFIED, level); + } + } + return 0; + } + if (of_property_read_bool(np, "cache-size")) ci_leaf_init(this_leaf++, CACHE_TYPE_UNIFIED, level); if (of_property_read_bool(np, "i-cache-size"))
Before cacheinfo can be built correctly, we need to initialize level and type. Since RSIC-V currently does not have a register group that describes cache-related attributes like ARM64, we cannot obtain them directly, so now we obtain cache leaves from the ACPI PPTT table (acpi_get_cache_info()) and set the cache type through split_levels. Suggested-by: Jeremy Linton <jeremy.linton@arm.com> Suggested-by: Sudeep Holla <sudeep.holla@arm.com> Signed-off-by: Yunhui Cui <cuiyunhui@bytedance.com> --- arch/riscv/kernel/cacheinfo.c | 23 +++++++++++++++++++++++ 1 file changed, 23 insertions(+)