Re: [PATCH] RDMA/srp: Fix support for unpopulated NUMA nodes

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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;
+            }
+        }
+    }

>      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.

> -        node_idx++;
> +        if(cpu_idx)
> +            node_idx++;

Has this patch been verified with checkpatch? I think a space is missing
between "if" and "(".

Thanks,

Bart.



[Index of Archives]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Photo]     [Yosemite News]     [Yosemite Photos]     [Linux Kernel]     [Linux SCSI]     [XFree86]

  Powered by Linux