| Message ID | 1414387308-27148-15-git-send-email-jiang.liu@linux.intel.com |
|---|---|
| State | Not Applicable |
| Headers | show |
On Mon, 27 Oct 2014, Jiang Liu wrote: > We are going to support ACPI based IOAPIC hotplug, so introduce a rwsem > to protect IOAPIC data structures from IOAPIC hotplug. We choose to > serialize in ACPI instead of in the IOAPIC core because: > 1) currently we are only plan to support ACPI based IOAPIC hotplug > 2) it's much more cleaner and easier > 3) It does't affect IOAPIC discovered by devicetree, SFI and mpparse. I had a last intensive look at this series as I was about to merge it. So I looked at the locking rules here again > +/* > + * Locks related to IOAPIC hotplug > + * Hotplug side: > + * ->lock_device_hotplug() //device_hotplug_lock > + * ->acpi_ioapic_rwsem > + * ->ioapic_lock > + * Interrupt mapping side: > + * ->acpi_ioapic_rwsem > + * ->ioapic_mutex > + * ->ioapic_lock > + */ This looks sane, but I cannot figure out at all why this needs to be a rwsem. > +static DECLARE_RWSEM(acpi_ioapic_rwsem); I think it should be a simple mutex because the rwsem does not protect against concurrent execution what taken for read. And the site which takes it for write is in the early boot process where nothing runs in parallel AFAICT. Thanks, tglx -- To unsubscribe from this list: send the line "unsubscribe linux-pci" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 2014/11/2 2:59, Thomas Gleixner wrote: > On Mon, 27 Oct 2014, Jiang Liu wrote: >> We are going to support ACPI based IOAPIC hotplug, so introduce a rwsem >> to protect IOAPIC data structures from IOAPIC hotplug. We choose to >> serialize in ACPI instead of in the IOAPIC core because: >> 1) currently we are only plan to support ACPI based IOAPIC hotplug >> 2) it's much more cleaner and easier >> 3) It does't affect IOAPIC discovered by devicetree, SFI and mpparse. > > I had a last intensive look at this series as I was about to merge > it. So I looked at the locking rules here again > >> +/* >> + * Locks related to IOAPIC hotplug >> + * Hotplug side: >> + * ->lock_device_hotplug() //device_hotplug_lock >> + * ->acpi_ioapic_rwsem >> + * ->ioapic_lock >> + * Interrupt mapping side: >> + * ->acpi_ioapic_rwsem >> + * ->ioapic_mutex >> + * ->ioapic_lock >> + */ > > This looks sane, but I cannot figure out at all why this needs to be a > rwsem. > >> +static DECLARE_RWSEM(acpi_ioapic_rwsem); > > I think it should be a simple mutex because the rwsem does not protect > against concurrent execution what taken for read. > > And the site which takes it for write is in the early boot process > where nothing runs in parallel AFAICT. Hi Thomas, You are right. It's not on hot path, so a mutex is better than a rwsem here. I will send out an updated version soon. Regards! Gerry > > Thanks, > > tglx > -- To unsubscribe from this list: send the line "unsubscribe linux-pci" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/arch/x86/kernel/acpi/boot.c b/arch/x86/kernel/acpi/boot.c index e077c080a519..3f6b665f5aa6 100644 --- a/arch/x86/kernel/acpi/boot.c +++ b/arch/x86/kernel/acpi/boot.c @@ -76,6 +76,19 @@ int acpi_fix_pin2_polarity __initdata; static u64 acpi_lapic_addr __initdata = APIC_DEFAULT_PHYS_BASE; #endif +/* + * Locks related to IOAPIC hotplug + * Hotplug side: + * ->lock_device_hotplug() //device_hotplug_lock + * ->acpi_ioapic_rwsem + * ->ioapic_lock + * Interrupt mapping side: + * ->acpi_ioapic_rwsem + * ->ioapic_mutex + * ->ioapic_lock + */ +static DECLARE_RWSEM(acpi_ioapic_rwsem); + /* -------------------------------------------------------------------------- Boot-time Configuration -------------------------------------------------------------------------- */ @@ -609,8 +622,10 @@ int acpi_gsi_to_irq(u32 gsi, unsigned int *irqp) if (acpi_irq_model == ACPI_IRQ_MODEL_PIC) { *irqp = gsi; } else { + down_read(&acpi_ioapic_rwsem); irq = mp_map_gsi_to_irq(gsi, IOAPIC_MAP_ALLOC | IOAPIC_MAP_CHECK); + up_read(&acpi_ioapic_rwsem); if (irq < 0) return -1; *irqp = irq; @@ -651,7 +666,9 @@ static int acpi_register_gsi_ioapic(struct device *dev, u32 gsi, int irq = gsi; #ifdef CONFIG_X86_IO_APIC + down_read(&acpi_ioapic_rwsem); irq = mp_register_gsi(dev, gsi, trigger, polarity); + up_read(&acpi_ioapic_rwsem); #endif return irq; @@ -660,7 +677,9 @@ static int acpi_register_gsi_ioapic(struct device *dev, u32 gsi, static void acpi_unregister_gsi_ioapic(u32 gsi) { #ifdef CONFIG_X86_IO_APIC + down_read(&acpi_ioapic_rwsem); mp_unregister_gsi(gsi); + up_read(&acpi_ioapic_rwsem); #endif } @@ -1186,7 +1205,9 @@ static void __init acpi_process_madt(void) /* * Parse MADT IO-APIC entries */ + down_write(&acpi_ioapic_rwsem); error = acpi_parse_madt_ioapic_entries(); + up_write(&acpi_ioapic_rwsem); if (!error) { acpi_set_irq_model_ioapic();
We are going to support ACPI based IOAPIC hotplug, so introduce a rwsem to protect IOAPIC data structures from IOAPIC hotplug. We choose to serialize in ACPI instead of in the IOAPIC core because: 1) currently we are only plan to support ACPI based IOAPIC hotplug 2) it's much more cleaner and easier 3) It does't affect IOAPIC discovered by devicetree, SFI and mpparse. Signed-off-by: Jiang Liu <jiang.liu@linux.intel.com> --- arch/x86/kernel/acpi/boot.c | 21 +++++++++++++++++++++ 1 file changed, 21 insertions(+)