On 11/30/20 5:07 AM, Bart Van Assche wrote:
On 11/25/20 10:26 AM, Nicolas Morey-Chaisemartin wrote:
+static int srp_get_num_populated_nodes(void)
+{
+ int num_populated_nodes = 0;
+ int node, cpu;
+
+ for_each_online_node(node) {
+ bool populated = false;
+ for_each_online_cpu(cpu) {
+ if (cpu_to_node(cpu) == node){
+ populated = true;
+ break;
+ }
+ }
+ if (populated) {
+ num_populated_nodes++;
+ break;
+ }
+ }
+
+ return num_populated_nodes;
+}
Does the above code do what it should do? Will the outer loop be left as
soon as one populated node has been found? Can the 'populated' variable
be left out, e.g. as follows (untested):
+ for_each_online_node(node) {
+ for_each_online_cpu(cpu) {
+ if (cpu_to_node(cpu) == node){
+ num_populated_nodes++;
+ break;
+ }
+ }
+ }
Yes your code is better and mine was glitchy...
if (target->ch_count == 0)
target->ch_count =
- max_t(unsigned int, num_online_nodes(),
- min(ch_count ?:
- min(4 * num_online_nodes(),
- ibdev->num_comp_vectors),
- num_online_cpus()));
+ min(ch_count ?:
+ min(4 * num_populated_nodes,
+ ibdev->num_comp_vectors),
+ num_online_cpus());
It seems like "max_t(unsigned int, num_populated_nodes," is missing from
the above expression? I think there should be at least one channel per
NUMA node even if the number of completion vectors is less than the
number of NUMA nodes.
Makes sense. I'll fix that.
After some rethinking, the current code is still broken anyway.
If we take a case like
Node0: CPU 0
Node1: CPU [1-3]
We'd end up with a ch_count of 4 spread equally across 2 nodes.
But because N0 has only one CPU, not all associated channels will be setup/connected correctly.
Would you agree saying that each node should have the same number of channels?
In that case, we need to count the # of available CPU per online node
Then:
num_ch_per_node = max(min(min(min_online_cpu_per_node, 4), (ch_count ?:num_comp_vectors)/num_populated_nodes), 1)
target->ch_count = num_populated_nodes * num_ch_per_node
This way we ensure that:
- There is at least one channel per node (max(..., 1))
- There is up to 4 channel per node min(min_online_cpu_per_node, 4)
- We don't overuse comp_vectors (except if rule #1 requires it) and get a value as close as user specified ch_count respecting above values.
min(..., (ch_count ?:num_comp_vectors)/num_populated_nodes))
This by construct will always be spread equally across nodes and fulfill the legacy constraints we had
- node_idx++;
+ if(cpu_idx)
+ node_idx++;
Has this patch been verified with checkpatch? I think a space is missing
between "if" and "(".
Will fix this next round
Nicolas
