RE: [PATCH v5 6/9] ACPI/PPTT: Add topology parsing code

Hi Jeremy,

 Sorry, I don't have your previous patch emails to reply on right patch
context.
So commenting on top of this patch.

AFAIU, the PPTT v5 patches still rely on CLIDR_EL1 register to know the type
of 
Caches enabled/available on the platform. With PPTT, it should not rely on
architecture
registers. There can be platforms which can report cache availability in
PPTT but not in 
architecture registers.

The following code snippet shows usage of CLIDR_EL1

In arch/arm64/kernel/cacheinfo.c

static inline enum cache_type get_cache_type(int level)
{
         u64 clidr;
 
         if (level > MAX_CACHE_LEVEL)
                 return CACHE_TYPE_NOCACHE;
         clidr = read_sysreg(clidr_el1);
         return CLIDR_CTYPE(clidr, level);
}

static int __populate_cache_leaves(unsigned int cpu)
{
          unsigned int level, idx;
          enum cache_type type;
          struct cpu_cacheinfo *this_cpu_ci = get_cpu_cacheinfo(cpu);
          struct cacheinfo *this_leaf = this_cpu_ci->info_list;
  
          for (idx = 0, level = 1; level <= this_cpu_ci->num_levels &&
               idx < this_cpu_ci->num_leaves; idx++, level++) {
                  type = get_cache_type(level);
                  if (type == CACHE_TYPE_SEPARATE) {
                          ci_leaf_init(this_leaf++, CACHE_TYPE_DATA, level);
                          ci_leaf_init(this_leaf++, CACHE_TYPE_INST, level);
                  } else {
                          ci_leaf_init(this_leaf++, type, level);
                  }
         }
          return 0;
 }

In populate_cache_leaves() the cache type is read from CLIDR_EL1 register.
If CLIDR_EL1 reports CACHE_TYPE_NOCACHE for a particular level then sysfs
entry
/sys/devices/system/cpu/cpu0/index<n>/type is not created and hence
userspace tools
like lstopo will not report this cache level.

Regards
Vijay

-----Original Message-----
From: linux-arm-kernel

[mailto:linux-arm-kernel-bounces@lists.infradead.org]

On Behalf Of Rafael J. Wysocki
Sent: Thursday, December 14, 2017 4:40 AM
To: Jeremy Linton <redacted>
Cc: Mark Rutland <mark.rutland@arm.com>; Jonathan.Zhang@cavium.com;
Jayachandran.Nair@cavium.com; Lorenzo Pieralisi
[off-list ref]; Catalin Marinas [off-list ref];
Rafael J. Wysocki [off-list ref]; jhugo@codeaurora.org; Will Deacon
[off-list ref]; Linux PM [off-list ref]; Rafael J.
Wysocki [off-list ref]; Greg Kroah-Hartman
[off-list ref]; Linux Kernel Mailing List <linux-
kernel@vger.kernel.org>; ACPI Devel Maling List

[off-list ref];

Viresh Kumar [off-list ref]; Hanjun Guo
[off-list ref]; Al Stone [off-list ref]; Sudeep Holla
[off-list ref]; austinwc@codeaurora.org;
wangxiongfeng2@huawei.com; linux-arm-kernel@lists.infradead.org; Len
Brown [off-list ref]
Subject: Re: [PATCH v5 6/9] ACPI/PPTT: Add topology parsing code

On Thu, Dec 14, 2017 at 12:06 AM, Jeremy Linton [off-list ref]
wrote:

quoted

Hi,


On 12/13/2017 04:28 PM, Rafael J. Wysocki wrote:

quoted

On Wed, Dec 13, 2017 at 6:38 PM, Lorenzo Pieralisi
[off-list ref] wrote:

quoted

On Tue, Dec 12, 2017 at 10:13:08AM -0600, Jeremy Linton wrote:

quoted

Hi,

First, thanks for taking a look at this.

On 12/11/2017 07:12 PM, Rafael J. Wysocki wrote:

quoted

On Friday, December 1, 2017 11:23:27 PM CET Jeremy Linton wrote:

quoted

The PPTT can be used to determine the groupings of CPU's at given
levels in the system. Lets add a few routines to the PPTT parsing
code to return a unique id for each unique level in the processor
hierarchy. This can then be matched to build
thread/core/cluster/die/package/etc mappings for each processing
element in the system.

Signed-off-by: Jeremy Linton <redacted>

Why can't this be folded into patch [2/9]?

It can, and I will be happy squash it.

It was requested that the topology portion of the parser be split
out back in v3.

https://www.spinics.net/lists/linux-acpi/msg78487.html

I asked to split cache/topology since I am not familiar with cache
code and Sudeep - who looks after the cache code - won't be able to
review this series in time for v4.16.

OK, so why do we need it in 4.16?

I think its more case of as soon as possible. That is because there
are machines where the topology is completely incorrect due to
assumptions the kernel makes based on registers that aren't defined
for that purpose (say describing which cores are in a physical socket,
or LLC's attached to interconnects or memory controllers).

This incorrect topology information is reported to things like the
kernel scheduler, which then makes poor scheduling decisions resulting
in sub-optimal system performance.

This patchset (and ACPI 6.2) clears up a lot of those problems.

As long as the ACPI tables are as expected that is, I suppose?

Anyway, fair enough, but I don't want to rush it in.

Thanks,
Rafael

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel