| Message ID | 20230104183033.755668-1-pierre.gondois@arm.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | arch_topology: Build cacheinfo from primary CPU | expand |
On Wed, 4 Jan 2023 19:30:23 +0100, Pierre Gondois wrote: > v2: > - Applied renaming/formatting comments from v1. > - Check CACHE_TYPE_VALID flag in pppt.c. > v3: > - Applied Sudeep's suggestions (for patch 5/5): > - Renaming allocate_cache_info() -> fecth_cache_info() > - Updated error message > - Extract an inline allocate_cache_info() function > - Re-run checkpatch with --strict option > v4: > - Remove RISC-V's implementation of init_cache_level() as not > necessary. > - Add patch: 'cacheinfo: Check 'cache-unified' property to count > cache leaves' to increase the number of leaves at a cache level > when no cache-size property is found. > - In cacheinfo: Use RISC-V's init_cache_level() [...], > make 'levels', 'leaves' and 'level' unsigned int to match > of_property_read_u32()'s parameters signedness. > > [...] Applied to sudeep.holla/linux (for-next/cacheinfo), thanks! [1/6] cacheinfo: Use RISC-V's init_cache_level() as generic OF implementation https://git.kernel.org/sudeep.holla/c/c3719bd9eeb2 [2/6] cacheinfo: Return error code in init_of_cache_level() https://git.kernel.org/sudeep.holla/c/8844c3df001b [3/6] cacheinfo: Check 'cache-unified' property to count cache leaves https://git.kernel.org/sudeep.holla/c/de0df442ee49 [4/6] ACPI: PPTT: Remove acpi_find_cache_levels() https://git.kernel.org/sudeep.holla/c/fa4d566a605b [5/6] ACPI: PPTT: Update acpi_find_last_cache_level() to acpi_get_cache_info() https://git.kernel.org/sudeep.holla/c/bd500361a937 [6/6] arch_topology: Build cacheinfo from primary CPU https://git.kernel.org/sudeep.holla/c/5944ce092b97 -- Regards, Sudeep
On Wed, Jan 18, 2023 at 11:55:59AM +0000, Sudeep Holla wrote: > On Wed, 4 Jan 2023 19:30:23 +0100, Pierre Gondois wrote: > > v2: > > - Applied renaming/formatting comments from v1. > > - Check CACHE_TYPE_VALID flag in pppt.c. > > v3: > > - Applied Sudeep's suggestions (for patch 5/5): > > - Renaming allocate_cache_info() -> fecth_cache_info() > > - Updated error message > > - Extract an inline allocate_cache_info() function > > - Re-run checkpatch with --strict option > > v4: > > - Remove RISC-V's implementation of init_cache_level() as not > > necessary. > > - Add patch: 'cacheinfo: Check 'cache-unified' property to count > > cache leaves' to increase the number of leaves at a cache level > > when no cache-size property is found. > > - In cacheinfo: Use RISC-V's init_cache_level() [...], > > make 'levels', 'leaves' and 'level' unsigned int to match > > of_property_read_u32()'s parameters signedness. > > > > [...] > > Applied to sudeep.holla/linux (for-next/cacheinfo), thanks! > I pushed the changes and then noticed some build warning report by kbuild posted only to you and one list(missing this list). Please post the fix if required on top of my for-next/cacheinfo so that it can be added on the top. Sorry for missing that.
On 1/18/23 13:07, Sudeep Holla wrote: > On Wed, Jan 18, 2023 at 11:55:59AM +0000, Sudeep Holla wrote: >> On Wed, 4 Jan 2023 19:30:23 +0100, Pierre Gondois wrote: >>> v2: >>> - Applied renaming/formatting comments from v1. >>> - Check CACHE_TYPE_VALID flag in pppt.c. >>> v3: >>> - Applied Sudeep's suggestions (for patch 5/5): >>> - Renaming allocate_cache_info() -> fecth_cache_info() >>> - Updated error message >>> - Extract an inline allocate_cache_info() function >>> - Re-run checkpatch with --strict option >>> v4: >>> - Remove RISC-V's implementation of init_cache_level() as not >>> necessary. >>> - Add patch: 'cacheinfo: Check 'cache-unified' property to count >>> cache leaves' to increase the number of leaves at a cache level >>> when no cache-size property is found. >>> - In cacheinfo: Use RISC-V's init_cache_level() [...], >>> make 'levels', 'leaves' and 'level' unsigned int to match >>> of_property_read_u32()'s parameters signedness. >>> >>> [...] >> >> Applied to sudeep.holla/linux (for-next/cacheinfo), thanks! >> > > I pushed the changes and then noticed some build warning report by > kbuild posted only to you and one list(missing this list). Please post the > fix if required on top of my for-next/cacheinfo so that it can be added > on the top. Sorry for missing that. > Hi Sudeep, I think the reported issue can be ignored, the 'levels' and 'split_levels' variables are initialized when used. If necessary, it is straightforward to fix the warning. Regards, Pierre The reported issue: --- Start --- If you fix the issue, kindly add following tag where applicable | Reported-by: kernel test robot <lkp@intel.com> All warnings (new ones prefixed by >>): drivers/base/cacheinfo.c: In function 'fetch_cache_info': >> drivers/base/cacheinfo.c:440:50: warning: 'levels' is used uninitialized [-Wuninitialized] 440 | this_cpu_ci->num_leaves = levels + split_levels; | ~~~~~~~^~~~~~~~~~~~~~ drivers/base/cacheinfo.c:420:22: note: 'levels' was declared here 420 | unsigned int levels, split_levels; | ^~~~~~ >> drivers/base/cacheinfo.c:440:50: warning: 'split_levels' is used uninitialized [-Wuninitialized] 440 | this_cpu_ci->num_leaves = levels + split_levels; | ~~~~~~~^~~~~~~~~~~~~~ drivers/base/cacheinfo.c:420:30: note: 'split_levels' was declared here 420 | unsigned int levels, split_levels; | ^~~~~~~~~~~~ vim +/levels +440 drivers/base/cacheinfo.c 416 417 int fetch_cache_info(unsigned int cpu) 418 { 419 struct cpu_cacheinfo *this_cpu_ci; 420 unsigned int levels, split_levels; 421 int ret; 422 423 if (acpi_disabled) { 424 ret = init_of_cache_level(cpu); 425 if (ret < 0) 426 return ret; 427 } else { 428 ret = acpi_get_cache_info(cpu, &levels, &split_levels); 429 if (ret < 0) 430 return ret; 431 432 this_cpu_ci = get_cpu_cacheinfo(cpu); 433 this_cpu_ci->num_levels = levels; 434 /* 435 * This assumes that: 436 * - there cannot be any split caches (data/instruction) 437 * above a unified cache 438 * - data/instruction caches come by pair 439 */ > 440 this_cpu_ci->num_leaves = levels + split_levels; 441 } 442 if (!cache_leaves(cpu)) 443 return -ENOENT; 444 445 return allocate_cache_info(cpu); 446 } 447 --- End ---
v2: - Applied renaming/formatting comments from v1. - Check CACHE_TYPE_VALID flag in pppt.c. v3: - Applied Sudeep's suggestions (for patch 5/5): - Renaming allocate_cache_info() -> fecth_cache_info() - Updated error message - Extract an inline allocate_cache_info() function - Re-run checkpatch with --strict option v4: - Remove RISC-V's implementation of init_cache_level() as not necessary. - Add patch: 'cacheinfo: Check 'cache-unified' property to count cache leaves' to increase the number of leaves at a cache level when no cache-size property is found. - In cacheinfo: Use RISC-V's init_cache_level() [...], make 'levels', 'leaves' and 'level' unsigned int to match of_property_read_u32()'s parameters signedness. Note: This patchset requires the following patch to be applied first in order to avoid the same bug described in the commit message: https://lore.kernel.org/all/20221116094958.2141072-1-pierre.gondois@arm.com/ [1] and [2] build the CPU topology from the cacheinfo information for both DT/ACPI based systems and remove (struct cpu_topology).llc_id which was used by ACPI only. Creating the cacheinfo for secondary CPUs is done during early boot. Preemption and interrupts are disabled at this stage. On PREEMPT_RT kernels, allocating memory (and parsing the PPTT table for ACPI based systems) triggers a: 'BUG: sleeping function called from invalid context' [4] To prevent this bug, allocate the cacheinfo from the primary CPU when preemption and interrupts are enabled and before booting secondary CPUs. The cache levels/leaves are computed from DT/ACPI PPTT information only, without relying on the arm64 CLIDR_EL1 register. If no cache information is found in the DT/ACPI PPTT, then fallback to the current state, triggering [4] on PREEMPT_RT kernels. Patches to update the arm64 device trees that have incomplete cacheinfo (mostly for missing the 'cache-level' or 'cache-unified' property) have been sent at [3]. Tested platforms: - ACPI + PPTT: Ampere Altra, Ampere eMAG, Cavium ThunderX2, Kunpeng 920, Juno-r2 - DT: rb5, db845c, Juno-r2 [1] https://lore.kernel.org/all/20220704101605.1318280-1-sudeep.holla@arm.com/ [2] https://lore.kernel.org/all/20220720-arch_topo_fixes-v3-0-43d696288e84@arm.com/ [3] https://lore.kernel.org/all/20221107155825.1644604-1-pierre.gondois@arm.com/ [4] On an Ampere Altra, with PREEMPT_RT kernel based on v6.0.0-rc4: [ 7.560791] BUG: sleeping function called from invalid context at kernel/locking/spinlock_rt.c:46 [ 7.560794] in_atomic(): 1, irqs_disabled(): 128, non_block: 0, pid: 0, name: swapper/111 [ 7.560796] preempt_count: 1, expected: 0 [ 7.560797] RCU nest depth: 1, expected: 1 [ 7.560799] 3 locks held by swapper/111/0: [ 7.560800] #0: ffff403e406cae98 (&pcp->lock){+.+.}-{3:3}, at: get_page_from_freelist+0x218/0x12c8 [ 7.560811] #1: ffffc5f8ed09f8e8 (rcu_read_lock){....}-{1:3}, at: rt_spin_trylock+0x48/0xf0 [ 7.560820] #2: ffff403f400b4fd8 (&zone->lock){+.+.}-{3:3}, at: rmqueue_bulk+0x64/0xa80 [ 7.560824] irq event stamp: 0 [ 7.560825] hardirqs last enabled at (0): [<0000000000000000>] 0x0 [ 7.560827] hardirqs last disabled at (0): [<ffffc5f8e9f7d594>] copy_process+0x5dc/0x1ab8 [ 7.560830] softirqs last enabled at (0): [<ffffc5f8e9f7d594>] copy_process+0x5dc/0x1ab8 [ 7.560833] softirqs last disabled at (0): [<0000000000000000>] 0x0 [ 7.560834] Preemption disabled at: [ 7.560835] [<ffffc5f8e9fd3c28>] migrate_enable+0x30/0x130 [ 7.560838] CPU: 111 PID: 0 Comm: swapper/111 Tainted: G W 6.0.0-rc4-[...] [ 7.560841] Call trace: [...] [ 7.560870] __kmalloc+0xbc/0x1e8 [ 7.560873] detect_cache_attributes+0x2d4/0x5f0 [ 7.560876] update_siblings_masks+0x30/0x368 [ 7.560880] store_cpu_topology+0x78/0xb8 [ 7.560883] secondary_start_kernel+0xd0/0x198 [ 7.560885] __secondary_switched+0xb0/0xb4 Pierre Gondois (6): cacheinfo: Use RISC-V's init_cache_level() as generic OF implementation cacheinfo: Return error code in init_of_cache_level() cacheinfo: Check 'cache-unified' property to count cache leaves ACPI: PPTT: Remove acpi_find_cache_levels() ACPI: PPTT: Update acpi_find_last_cache_level() to acpi_get_cache_info() arch_topology: Build cacheinfo from primary CPU arch/arm64/kernel/cacheinfo.c | 11 ++- arch/riscv/kernel/cacheinfo.c | 42 ----------- drivers/acpi/pptt.c | 93 +++++++++++++---------- drivers/base/arch_topology.c | 12 ++- drivers/base/cacheinfo.c | 134 +++++++++++++++++++++++++++++----- include/linux/cacheinfo.h | 11 ++- 6 files changed, 196 insertions(+), 107 deletions(-)