On Tue, Dec 14, 2021, David Woodhouse wrote:
> -static int alloc_clustermask(unsigned int cpu, int node)
> +static int alloc_clustermask(unsigned int cpu, u32 cluster, int node)
> {
> + struct cluster_mask *cmsk = NULL;
> + unsigned int cpu_i;
> + u32 apicid;
> +
> if (per_cpu(cluster_masks, cpu))
> return 0;
> - /*
> - * If a hotplug spare mask exists, check whether it's on the right
> - * node. If not, free it and allocate a new one.
> +
> + /* For the hotplug case, don't always allocate a new one */
> + for_each_present_cpu(cpu_i) {
> + apicid = apic->cpu_present_to_apicid(cpu_i);
> + if (apicid != BAD_APICID && apicid >> 4 == cluster) {
> + cmsk = per_cpu(cluster_masks, cpu_i);
> + if (cmsk)
> + break;
> + }
> + }
> + if (!cmsk) {
> + cmsk = kzalloc_node(sizeof(*cmsk), GFP_KERNEL, node);
> + }
> + if (!cmsk)
> + return -ENOMEM;
This can be,
if (!cmsk) {
cmsk = kzalloc_node(sizeof(*cmsk), GFP_KERNEL, node);
if (!cmsk)
return -ENOMEM;
}
which IMO is more intuitive, and it also "fixes" the unnecessary braces in thew
initial check.
> +
> + cmsk->node = node;
> + cmsk->clusterid = cluster;
> +
> + per_cpu(cluster_masks, cpu) = cmsk;
> +
> + /*
> + * As an optimisation during boot, set the cluster_mask for *all*
> + * present CPUs at once, to prevent *each* of them having to iterate
> + * over the others to find the existing cluster_mask.
> */
> - if (cluster_hotplug_mask) {
> - if (cluster_hotplug_mask->node == node)
> - return 0;
> - kfree(cluster_hotplug_mask);
> + if (system_state < SYSTEM_RUNNING) {
This can be
if (system_state >= SYSTEM_RUNNING)
return 0;
to reduce indentation below.
> + for_each_present_cpu(cpu) {
Reusing @cpu here is all kinds of confusing.
> + u32 apicid = apic->cpu_present_to_apicid(cpu);
This shadows apicid. That's completely unnecessary.
> + if (apicid != BAD_APICID && apicid >> 4 == cluster) {
A helper for retrieving the cluster from a cpu would dedup at least three instances
of this pattern.
> + struct cluster_mask **cpu_cmsk = &per_cpu(cluster_masks, cpu);
> + if (*cpu_cmsk)
> + BUG_ON(*cpu_cmsk != cmsk);
> + else
> + *cpu_cmsk = cmsk;
The if statement is a little confusing because of the double pointer. BUG_ON()
won't return, maybe write it like so?
BUG_ON(*cpu_mask && *cpu_mask != cmsk);
*cpu_cmsk = cmsk;
> + }
> + }
> }
>
> - cluster_hotplug_mask = kzalloc_node(sizeof(*cluster_hotplug_mask),
> - GFP_KERNEL, node);
> - if (!cluster_hotplug_mask)
> - return -ENOMEM;
> - cluster_hotplug_mask->node = node;
> return 0;
> }
>
> static int x2apic_prepare_cpu(unsigned int cpu)
> {
> - if (alloc_clustermask(cpu, cpu_to_node(cpu)) < 0)
> + u32 phys_apicid = apic->cpu_present_to_apicid(cpu);
> + u32 cluster = phys_apicid >> 4;
> + u32 logical_apicid = (cluster << 16) | (1 << (phys_apicid & 0xf));
> +
> + x86_cpu_to_logical_apicid[cpu] = logical_apicid;
> +
> + if (alloc_clustermask(cpu, cluster, cpu_to_node(cpu)) < 0)
> return -ENOMEM;
> if (!zalloc_cpumask_var(&per_cpu(ipi_mask, cpu), GFP_KERNEL))
> return -ENOMEM;
> --
> 2.31.1
>
