Re: [PATCH v2 1/7] x86/apic/x2apic: Fix parallel handling of cluster_mask

[Sean Christopherson <seanjc@google.com>]

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
>