Message-ID: <55EF6519.3070104@redhat.com>
Date:	Tue, 8 Sep 2015 16:45:45 -0600
From:	Al Stone <ahs3@...hat.com>
To:	Marc Zyngier <marc.zyngier@....com>,
	Lukasz Anaczkowski <lukasz.anaczkowski@...el.com>,
	lorenzo.pieralisi@....com, tomasz.nowicki@...aro.org,
	tglx@...utronix.de, mingo@...hat.com, hpa@...or.com,
	x86@...nel.org, jason@...edaemon.net
Cc:	rjw@...ysocki.net, len.brown@...el.com, pavel@....cz,
	linux-pm@...r.kernel.org, linux-kernel@...r.kernel.org,
	linux-acpi@...r.kernel.org
Subject: Re: [PATCH 0/4] Fix how CPUs are enumerated when there's more than
 255 CPUs

On 09/08/2015 10:27 AM, Marc Zyngier wrote:
> On 08/09/15 12:07, Lukasz Anaczkowski wrote:
>> This series of patches attempts to fix how CPUs are enumerated by kernel when
>> there's more than 255 of them on single processor.
>> In such case, BIOS may interleave APIC/X2APIC MADT subtables, to obey requirements
>> specified in ACPI spec. Without this patches, kernel then would first enumerate
>> BSP, then X2APIC then APIC, resulting in low APIC IDs to be assigned with high
>> logical IDs and high APIC IDs to be assigned low logical IDs. Biggest consequence
>> of that could be performance penalties due to wrong L2 cache sharing.
>> More details in patch 4/4.
>>
>> Also, simpler approach has been considered, which did not required ACPI parsing
>> interface changes, however it failed to meet requirements. More details can be
>> found here: https://lkml.org/lkml/2015/9/7/285
>>
>> I've compiled this code successfully for x86/arm64 and verified it on x86. I'd
>> really appreciate if someone could give it a try on arm64.
>> Although interface has changed, semantics stayed the same, thus runtime issues
>> should not appear. Please verify, thanks!
> 
> I really don't see why you cannot provide simple helpers that avoids
> the churn on code that doesn't require this new feature. It just makes
> the code hard(er) to read, and unnecessarily convoluted. ACPI doesn't
> need more of that, really.
> 
> I've come up with this (on top of your series):
> 
> diff --git a/drivers/acpi/numa.c b/drivers/acpi/numa.c
> index f3ccc68..acaa3b4 100644
> --- a/drivers/acpi/numa.c
> +++ b/drivers/acpi/numa.c
> @@ -314,15 +314,9 @@ static int __init
>  acpi_table_parse_srat(enum acpi_srat_type id,
>  		      acpi_tbl_entry_handler handler, unsigned int max_entries)
>  {
> -	struct acpi_subtable_proc srat_proc;
> -
> -	memset(&srat_proc, 0, sizeof(srat_proc));
> -	srat_proc.id = id;
> -	srat_proc.handler = handler;
> -
> -	return acpi_table_parse_entries_array(ACPI_SIG_SRAT,
> -				sizeof(struct acpi_table_srat),
> -				&srat_proc, 1, max_entries);
> +	return acpi_table_parse_entries(ACPI_SIG_SRAT,
> +					    sizeof(struct acpi_table_srat), id,
> +					    handler, max_entries);
>  }
>  
>  int __init acpi_numa_init(void)
> @@ -341,7 +335,6 @@ int __init acpi_numa_init(void)
>  				     acpi_parse_x2apic_affinity, 0);
>  		acpi_table_parse_srat(ACPI_SRAT_TYPE_CPU_AFFINITY,
>  				     acpi_parse_processor_affinity, 0);
> -
>  		cnt = acpi_table_parse_srat(ACPI_SRAT_TYPE_MEMORY_AFFINITY,
>  					    acpi_parse_memory_affinity,
>  					    NR_NODE_MEMBLKS);
> diff --git a/drivers/acpi/tables.c b/drivers/acpi/tables.c
> index bc23220..758b334 100644
> --- a/drivers/acpi/tables.c
> +++ b/drivers/acpi/tables.c
> @@ -215,7 +215,7 @@ void acpi_table_print_madt_entry(struct acpi_subtable_header *header)
>  }
>  
>  /**
> - * acpi_parse_entries - for each proc_num find a subtable with proc->id
> + * acpi_parse_entries_array - for each proc_num find a subtable with proc->id
>   *    and run proc->handler on it. Assumption is that there's only
>   *    single handler for particular entry id.
>   *
> @@ -230,8 +230,8 @@ void acpi_table_print_madt_entry(struct acpi_subtable_header *header)
>   * On success returns sum of all matching entries for all proc handlers.
>   * Oterwise, -ENODEV or -EINVAL is returned.
>   */
> -int __init
> -acpi_parse_entries(char *id, unsigned long table_size,
> +static int __init
> +acpi_parse_entries_array(char *id, unsigned long table_size,
>  		struct acpi_table_header *table_header,
>  		struct acpi_subtable_proc *proc, int proc_num,
>  		unsigned int max_entries)
> @@ -300,6 +300,20 @@ acpi_parse_entries(char *id, unsigned long table_size,
>  	return count;
>  }
>  
> +int __init acpi_parse_entries(char *id, unsigned long table_size,
> +			      acpi_tbl_entry_handler handler,
> + 			      struct acpi_table_header *table_header,
> +			      int entry_id, unsigned int max_entries)
> +{
> +	struct acpi_subtable_proc proc = {
> +		.id		= entry_id,
> +		.handler	= handler,
> +	};
> +
> +	return acpi_parse_entries_array(id, table_size, table_header,
> +					&proc, 1, max_entries);
> +}
> +
>  int __init
>  acpi_table_parse_entries_array(char *id,
>  			 unsigned long table_size,
> @@ -326,8 +340,8 @@ acpi_table_parse_entries_array(char *id,
>  		return -ENODEV;
>  	}
>  
> -	count = acpi_parse_entries(id, table_size, table_header,
> -			proc, proc_num, max_entries);
> +	count = acpi_parse_entries_array(id, table_size, table_header,
> +					 proc, proc_num, max_entries);
>  
>  	early_acpi_os_unmap_memory((char *)table_header, tbl_size);
>  	return count;
> @@ -340,17 +354,13 @@ acpi_table_parse_entries(char *id,
>  			acpi_tbl_entry_handler handler,
>  			unsigned int max_entries)
>  {
> -	struct acpi_subtable_proc proc[1];
> -
> -	if (!handler)
> -		return -EINVAL;
> -
> -	memset(proc, 0, sizeof(proc));
> -	proc[0].id = entry_id;
> -	proc[0].handler = handler;
> +	struct acpi_subtable_proc proc = {
> +		.id		= entry_id,
> +		.handler	= handler,
> +	};
>  
> -	return acpi_table_parse_entries_array(id, table_size, proc, 1,
> -						max_entries);
> +	return acpi_table_parse_entries_array(id, table_size, &proc, 1,
> +					      max_entries);
>  }
>  
>  int __init
> diff --git a/drivers/irqchip/irq-gic.c b/drivers/irqchip/irq-gic.c
> index 581336c..4dd8826 100644
> --- a/drivers/irqchip/irq-gic.c
> +++ b/drivers/irqchip/irq-gic.c
> @@ -1091,16 +1091,12 @@ gic_v2_acpi_init(struct acpi_table_header *table)
>  {
>  	void __iomem *cpu_base, *dist_base;
>  	int count;
> -	struct acpi_subtable_proc gic_proc[1];
> -
> -	memset(gic_proc, 0, sizeof(gic_proc));
> -	gic_proc[0].id = ACPI_MADT_TYPE_GENERIC_INTERRUPT;
> -	gic_proc[0].handler = gic_acpi_parse_madt_cpu;
>  
>  	/* Collect CPU base addresses */
>  	count = acpi_parse_entries(ACPI_SIG_MADT,
>  				   sizeof(struct acpi_table_madt),
> -				   table, gic_proc, 1, 0);
> +				   gic_acpi_parse_madt_cpu, table,
> +				   ACPI_MADT_TYPE_GENERIC_INTERRUPT, 0);
>  	if (count <= 0) {
>  		pr_err("No valid GICC entries exist\n");
>  		return -EINVAL;
> @@ -1110,13 +1106,10 @@ gic_v2_acpi_init(struct acpi_table_header *table)
>  	 * Find distributor base address. We expect one distributor entry since
>  	 * ACPI 5.1 spec neither support multi-GIC instances nor GIC cascade.
>  	 */
> -	memset(gic_proc, 0, sizeof(gic_proc));
> -	gic_proc[0].id = ACPI_MADT_TYPE_GENERIC_DISTRIBUTOR;
> -	gic_proc[0].handler = gic_acpi_parse_madt_distributor;
> -
>  	count = acpi_parse_entries(ACPI_SIG_MADT,
>  				   sizeof(struct acpi_table_madt),
> -				   table, gic_proc, 1, 0);
> +				   gic_acpi_parse_madt_distributor, table,
> +				   ACPI_MADT_TYPE_GENERIC_DISTRIBUTOR, 0);
>  	if (count <= 0) {
>  		pr_err("No valid GICD entries exist\n");
>  		return -EINVAL;
> diff --git a/include/linux/acpi.h b/include/linux/acpi.h
> index 52c8d20..0f6381d 100644
> --- a/include/linux/acpi.h
> +++ b/include/linux/acpi.h
> @@ -152,9 +152,9 @@ int acpi_numa_init (void);
>  int acpi_table_init (void);
>  int acpi_table_parse(char *id, acpi_tbl_table_handler handler);
>  int __init acpi_parse_entries(char *id, unsigned long table_size,
> -			      struct acpi_table_header *table_header,
> -			      struct acpi_subtable_proc *proc, int proc_num,
> -			      unsigned int max_entries);
> +			      acpi_tbl_entry_handler handler,
> + 			      struct acpi_table_header *table_header,
> +			      int entry_id, unsigned int max_entries);
>  int __init acpi_table_parse_entries(char *id, unsigned long table_size,
>  				    int entry_id,
>  				    acpi_tbl_entry_handler handler,
> 
> In my view, this makes the change a lot more palatable, and it
> can fit in exactly two patches:
> 
> 1) add the acpi_subtable_proc stuff with the compatibility helpers
> 2) change arch/x86/kernel/acpi/boot.c to do whatever you want it
>    to do
> 
> It will be a lot easier to review and to verify.
> 
> Thanks,
> 
> 	M.
> 

I have to agree with Marc here.  His changes on top of the prior patches
make this much cleaner.  The MADT parsing is messy enough as it is without
adding changes needed by only the APIC/X2APIC subtables into the interface
for all MADT parsing.

I'm also still not convinced we have to have the ability to call multiple
callbacks for each entry as we cycle through the MADT entries.  I've looked
through the previous comments on these patches and I'm still not getting it;
maybe I'm just being dense (it wouldn't be the first time).

Logically, I see no difference between making two passes through the MADT
subtables, once for each subtable type and noting the info I need, versus
making two callbacks for each subtable, and noting the info I need between
callbacks.  If nothing else, my personal opinion is that the former would
be easier to maintain over time since it would be specific to the users of
APIC/X2APIC and not affect other users parsing MADT subtables.

Obviously, though, I'm not the final arbiter of all things ACPI... this is
just my $0.02 worth...

-- 
ciao,
al
-----------------------------------
Al Stone
Software Engineer
Red Hat, Inc.
ahs3@...hat.com
-----------------------------------
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists