Hi,

This is v3 of a patchset to set the number of cache leaves independently
for each CPU. v1 and v2 can be found here [1] and here [2].

Changes since v2:
  * This version uncovered a NULL-pointer dereference in recent changes to
    cacheinfo[3]. This dereference is observed when the system does not
    configure cacheinfo early during boot nor makes corrections later
    during CPU hotplug; as is the case in x86. Patch 1 fixes this issue.

Changes since v1:
  * Dave Hansen suggested to use the existing per-CPU ci_cpu_cacheinfo
    variable. Now the global variable num_cache_leaves became useless.
  * While here, I noticed that init_cache_level() also became useless:
    x86 does not need ci_cpu_cacheinfo::num_levels.

[1]. https://lore.kernel.org/lkml/20230314231658.30169-1-ricardo.neri-calderon@linux.intel.com/
[2]. https://lore.kernel.org/all/20230424001956.21434-1-ricardo.neri-calderon@linux.intel.com/
[3]. https://lore.kernel.org/all/20230412185759.755408-1-rrendec@redhat.com/

Ricardo Neri (3):
  cacheinfo: Allocate memory for memory if not done from the primary CPU
  x86/cacheinfo: Delete global num_cache_leaves
  x86/cacheinfo: Clean out init_cache_level()

 arch/x86/kernel/cpu/cacheinfo.c | 50 ++++++++++++++++-----------------
 drivers/base/cacheinfo.c        |  6 +++-
 2 files changed, 30 insertions(+), 26 deletions(-)

-- 
2.25.1

On Fri, Aug 04, 2023 at 06:24:18PM -0700, Ricardo Neri wrote:
> Hi,

Hi Ricardo,

> This is v3 of a patchset to set the number of cache leaves independently
> for each CPU. v1 and v2 can be found here [1] and here [2].

I am on CC of your patch set and glanced through it.
Long ago I've touched related code but now I am not really up-to-date
to do a qualified review in this area. First, I would have to look
into documentation to refresh my memory etc. pp.

I've not seen (or it escaped me) information that this was tested on a
variety of machines that might be affected by this change. And there
are no Tested-by-tags.

Even if changes look simple and reasonable they can cause issues.

Thus from my POV it would be good to have some information what tests
were done. I am not asking to test on all possible systems but just
knowing which system(s) was (were) used for functional testing is of
value.

> Changes since v2:
>   * This version uncovered a NULL-pointer dereference in recent changes to
>     cacheinfo[3]. This dereference is observed when the system does not
>     configure cacheinfo early during boot nor makes corrections later
>     during CPU hotplug; as is the case in x86. Patch 1 fixes this issue.
> 
> Changes since v1:
>   * Dave Hansen suggested to use the existing per-CPU ci_cpu_cacheinfo
>     variable. Now the global variable num_cache_leaves became useless.
>   * While here, I noticed that init_cache_level() also became useless:
>     x86 does not need ci_cpu_cacheinfo::num_levels.
> 
> [1]. https://lore.kernel.org/lkml/20230314231658.30169-1-ricardo.neri-calderon@linux.intel.com/
> [2]. https://lore.kernel.org/all/20230424001956.21434-1-ricardo.neri-calderon@linux.intel.com/
> [3]. https://lore.kernel.org/all/20230412185759.755408-1-rrendec@redhat.com/
> 
> Ricardo Neri (3):
>   cacheinfo: Allocate memory for memory if not done from the primary CPU
>   x86/cacheinfo: Delete global num_cache_leaves
>   x86/cacheinfo: Clean out init_cache_level()
> 
>  arch/x86/kernel/cpu/cacheinfo.c | 50 ++++++++++++++++-----------------
>  drivers/base/cacheinfo.c        |  6 +++-
>  2 files changed, 30 insertions(+), 26 deletions(-)
> 
> -- 
> 2.25.1
> 

-- 
Regards,
Andreas

SUSE Software Solutions Germany GmbH
Frankenstrasse 146, 90461 Nürnberg, Germany
GF: Ivo Totev, Andrew McDonald, Werner Knoblich
(HRB 36809, AG Nürnberg)

On Fri, Sep 01, 2023 at 08:50:31AM +0200, Andreas Herrmann wrote:
> On Fri, Aug 04, 2023 at 06:24:18PM -0700, Ricardo Neri wrote:
> > Hi,
> 
> Hi Ricardo,
> 
> > This is v3 of a patchset to set the number of cache leaves independently
> > for each CPU. v1 and v2 can be found here [1] and here [2].
> 
> I am on CC of your patch set and glanced through it.
> Long ago I've touched related code but now I am not really up-to-date
> to do a qualified review in this area. First, I would have to look
> into documentation to refresh my memory etc. pp.
> 
> I've not seen (or it escaped me) information that this was tested on a
> variety of machines that might be affected by this change. And there
> are no Tested-by-tags.
> 
> Even if changes look simple and reasonable they can cause issues.
> 
> Thus from my POV it would be good to have some information what tests
> were done. I am not asking to test on all possible systems but just
> knowing which system(s) was (were) used for functional testing is of
> value.

Doing a good review -- trying to catch every flaw -- is really hard to
do. Especially when you are not actively doing development work in an
area.

For example see

  commit e33b93650fc5 ("blk-iocost: Pass gendisk to ioc_refresh_params")
  [Breno Leitao <leitao@debian.org>, Tue Feb 28 03:16:54 2023 -0800]

This fixes commit

  ce57b558604e ("blk-rq-qos: make rq_qos_add and rq_qos_del more
  useful") [Christoph Hellwig <hch@lst.de>, Fri Feb 3 16:03:54 2023
  +0100]

I had reviewed the latter (see
https://marc.info/?i=Y8plg6OAa4lrnyZZ@suselix) and the entire patch
series. I've compared the original code with the patch and walked
through every single hunk of the diff and tried to think it
through. The changes made sense to me. Then came the bug report(s) and
I felt that I had failed tremendously. To err is human.

What this shows (and it is already known): with every patch new errors
are potentially introduced in the kernel. Functional, and higher
level testing can help to spot them before a kernel is deployed in the
field.

At a higher level view this proves another thing.
Linux kernel development is a transparent example of
"peer-review-process".

In our scientific age it is often postulated that peer review is the
way to go[1] and that it kind of guarantees that what's published, or
rolled out, is reasonable and "works".

The above sample shows that this process will not catch all flaws and
that proper, transparent and reliable tests are required before
anything is deployed in the field.

This is true for every branch of science.

If it is purposely not done something is fishy.


[1] Also some state that it is "the only way to go" and every thing
figured out without a peer-review-process is false and can't be
trusted. Of course this is a false statement.

-- 
Regards,
Andreas

SUSE Software Solutions Germany GmbH
Frankenstrasse 146, 90461 Nürnberg, Germany
GF: Ivo Totev, Andrew McDonald, Werner Knoblich
(HRB 36809, AG Nürnberg)

On Fri, Sep 01, 2023 at 09:52:54AM +0200, Andreas Herrmann wrote:
> On Fri, Sep 01, 2023 at 08:50:31AM +0200, Andreas Herrmann wrote:
> > On Fri, Aug 04, 2023 at 06:24:18PM -0700, Ricardo Neri wrote:
> > > Hi,
> > 
> > Hi Ricardo,
> > 
> > > This is v3 of a patchset to set the number of cache leaves independently
> > > for each CPU. v1 and v2 can be found here [1] and here [2].
> > 
> > I am on CC of your patch set and glanced through it.
> > Long ago I've touched related code but now I am not really up-to-date
> > to do a qualified review in this area. First, I would have to look
> > into documentation to refresh my memory etc. pp.
> > 
> > I've not seen (or it escaped me) information that this was tested on a
> > variety of machines that might be affected by this change. And there
> > are no Tested-by-tags.
> > 
> > Even if changes look simple and reasonable they can cause issues.
> > 
> > Thus from my POV it would be good to have some information what tests
> > were done. I am not asking to test on all possible systems but just
> > knowing which system(s) was (were) used for functional testing is of
> > value.
> 
> Doing a good review -- trying to catch every flaw -- is really hard to
> do. Especially when you are not actively doing development work in an
> area.
> 
> For example see
> 
>   commit e33b93650fc5 ("blk-iocost: Pass gendisk to ioc_refresh_params")
>   [Breno Leitao <leitao@debian.org>, Tue Feb 28 03:16:54 2023 -0800]
> 
> This fixes commit
> 
>   ce57b558604e ("blk-rq-qos: make rq_qos_add and rq_qos_del more
>   useful") [Christoph Hellwig <hch@lst.de>, Fri Feb 3 16:03:54 2023
>   +0100]
> 
> I had reviewed the latter (see
> https://marc.info/?i=Y8plg6OAa4lrnyZZ@suselix) and the entire patch
> series. I've compared the original code with the patch and walked
> through every single hunk of the diff and tried to think it
> through. The changes made sense to me. Then came the bug report(s) and
> I felt that I had failed tremendously. To err is human.
> 
> What this shows (and it is already known): with every patch new errors
> are potentially introduced in the kernel. Functional, and higher
> level testing can help to spot them before a kernel is deployed in the
> field.
> 
> At a higher level view this proves another thing.
> Linux kernel development is a transparent example of
> "peer-review-process".
> 
> In our scientific age it is often postulated that peer review is the
> way to go[1] and that it kind of guarantees that what's published, or
> rolled out, is reasonable and "works".
> 
> The above sample shows that this process will not catch all flaws and
> that proper, transparent and reliable tests are required before
> anything is deployed in the field.
> 
> This is true for every branch of science.
> 
> If it is purposely not done something is fishy.
> 
> 
> [1] Also some state that it is "the only way to go" and every thing
> figured out without a peer-review-process is false and can't be
> trusted. Of course this is a false statement.

Hi Andreas,

Agreed. Testing is important. For the specific case of these patches, I
booted CONFIG_PREEMPT_RT and !CONFIG_PREEMPT_RT kernels. Then I
  a) Ensured that the splat reported in commit 5944ce092b97
     ("arch_topology: Build cacheinfo from primary CPU") was not observed.

  b) Ensured that /sys/devices/system/cpu/cpuX/cache is present.

  c) Ensured that the contents /sys/devices/system/cpu/cpuX/cache is the
     same before and after my patches.

I tested on the following systems: Intel Alder Lake, Intel Meteor
Lake, 2-socket Intel Icelake server, 2-socket Intel Cascade Lake server,
2-socket Intel Skylake server, 4-socket Intel Broadwell server, 2-socket
Intel Haswell server, 2-socket AMD Rome server, and 2-socket AMD Milan
server.

Thanks and BR,
Ricardo

On Mon, Sep 11, 2023 at 08:23:50PM -0700, Ricardo Neri wrote:
> Hi Andreas,
> 
> Agreed. Testing is important. For the specific case of these patches, I
> booted CONFIG_PREEMPT_RT and !CONFIG_PREEMPT_RT kernels. Then I
>   a) Ensured that the splat reported in commit 5944ce092b97
>      ("arch_topology: Build cacheinfo from primary CPU") was not observed.
> 
>   b) Ensured that /sys/devices/system/cpu/cpuX/cache is present.
> 
>   c) Ensured that the contents /sys/devices/system/cpu/cpuX/cache is the
>      same before and after my patches.
> 
> I tested on the following systems: Intel Alder Lake, Intel Meteor
> Lake, 2-socket Intel Icelake server, 2-socket Intel Cascade Lake server,
> 2-socket Intel Skylake server, 4-socket Intel Broadwell server, 2-socket
> Intel Haswell server, 2-socket AMD Rome server, and 2-socket AMD Milan
> server.
> 
> Thanks and BR,
> Ricardo


Thanks for all the tests and info.

-- 
Regards,
Andreas

SUSE Software Solutions Germany GmbH
Frankenstrasse 146, 90461 Nürnberg, Germany
GF: Ivo Totev, Andrew McDonald, Werner Knoblich
(HRB 36809, AG Nürnberg)