mbox series

[v4,0/6] arch_topology: Build cacheinfo from primary CPU

Message ID 20230104183033.755668-1-pierre.gondois@arm.com
Headers show
Series arch_topology: Build cacheinfo from primary CPU | expand

Message

Pierre Gondois Jan. 4, 2023, 6:30 p.m. UTC
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(-)

Comments

Sudeep Holla Jan. 18, 2023, 11:55 a.m. UTC | #1
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
Sudeep Holla Jan. 18, 2023, 12:07 p.m. UTC | #2
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.
Pierre Gondois Jan. 18, 2023, 2:12 p.m. UTC | #3
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 ---