On Mon, Feb 27, 2023 at 02:30:38PM +0100, Peter Newman wrote:
> Hi Tony,
>
> On Thu, Jan 26, 2023 at 7:42 PM Tony Luck <tony.luck@xxxxxxxxx> wrote:
> > +static __init int find_snc_ways(void)
> > +{
> > + unsigned long *node_caches;
> > + int cpu, node, ret;
> > +
> > + node_caches = kcalloc(BITS_TO_LONGS(nr_node_ids), sizeof(*node_caches), GFP_KERNEL);
> > + if (!node_caches)
> > + return 1;
> > +
> > + cpus_read_lock();
> > + for_each_node(node) {
>
> Someone tried this patch on a machine with a CPU-less node...
>
> We need to check for this:
>
> + if (cpumask_empty(cpumask_of_node(node)))
> + continue;
>
> > + cpu = cpumask_first(cpumask_of_node(node));
> > + set_bit(get_cpu_cacheinfo_id(cpu, 3), node_caches);
> > + }
> > + cpus_read_unlock();
Peter,
Tell me more about your CPU-less nodes. Your fix avoids a bad
pointer reference (because cpumask_first() returns cpu >= nr_cpu_ids
for an empty bitmask).
But now I'm worried about whether I have the right values in the
formula:
nr_node_ids / bitmap_weight(node_caches, nr_node_ids);
This fix avoids counting the L3 from a non-existent CPU, but still
counts the node in the numerator.
Is your CPU-less node a full (non-SNC) node? Like this:
Socket 0 Socket 1
+--------------------+ +--------------------+
| . | | . |
| SNC 0.0 . SNC 0.1 | | zero . zero |
| . | | CPUs . CPUs |
| . | | . |
| . | | . |
+--------------------+ +--------------------+
| L3 Cache | | L3 Cache |
+--------------------+ +--------------------+
I could fix this case by counting how many CPU-less
nodes I find, and reducing the numerator (the denominator
didn't count the L3 cache from socket 1 because there
are no CPUs there)
(nr_node_ids - n_empty_nodes) / bitmap_weight(node_caches, nr_node_ids);
=> 2 / 1
But that won't work if your CPU-less node is an SNC node
and the other SNC node in the same socket does have some
CPUs:
Socket 0 Socket 1
+--------------------+ +--------------------+
| . | | . |
| SNC 0.0 . SNC 0.1 | | zero . SNC 1.1 |
| . | | CPUs . |
| . | | . |
| . | | . |
+--------------------+ +--------------------+
| L3 Cache | | L3 Cache |
+--------------------+ +--------------------+
This would get 3 / 2 ... i.e. I should still count the
empty node because its cache was counted by its SNC
buddy.
-Tony
