RE: 2.6.17-mm1 - possible recursive locking detected

RE: 2.6.17-mm1 - possible recursive locking detected

[Brown, Len]

 
>> The key thing here is that our recent changes in
>> how the locks are _used_ is okay -- and I think it is.
>
>Well.  We don't know that.  We just know that this report of unokayness
>wasn't right.  With Ingo's Linux-only patch we're in a 
>position to verify
>that the locking is probably OK.

If this were really recursive, my machine would have deadlocked
instead of booting normally like it did, no?

-Len

Re: 2.6.17-mm1 - possible recursive locking detected

[Andrew Morton]

On Thu, 22 Jun 2006 03:51:51 -0400
"Brown, Len" <len.brown@intel.com> wrote:

>  
> >> The key thing here is that our recent changes in
> >> how the locks are _used_ is okay -- and I think it is.
> >
> >Well.  We don't know that.  We just know that this report of unokayness
> >wasn't right.  With Ingo's Linux-only patch we're in a 
> >position to verify
> >that the locking is probably OK.
> 
> If this were really recursive, my machine would have deadlocked
> instead of booting normally like it did, no?

yup.  If it's SMP ;)

Re: 2.6.17-mm1 - possible recursive locking detected

[Andrew Morton]

On Thu, 22 Jun 2006 03:40:36 -0400
"Brown, Len" <len.brown@intel.com> wrote:

> >> Nothing jumps out at me as incorrect above, so 
> >> at this point it looks like a CONFIG_LOCKDEP artifact --
> >> but lets ask the experts:-)
> >
> >Yes, lockdep uses the callsite of spin_lock_init() to detect 
> >the "type" of
> >a lock.
> >
> >But the ACPI obfuscation layers use the same spin_lock_init() site to
> >initialise two not-the-same locks, so lockdep decides those 
> >two locks are of the same "type" and gets confused.
> 
> interesting definition of "type".  I guess it works
> in practice or others would be complaining...

It works out that way, yes.

> >We had earlier decided to remove that ACPI code which kmallocs a single
> >spinlock.  When that's done, lockdep will become unconfused.
> 
> Yes, that change is already on the way.

Is good.

> The key thing here is that our recent changes in
> how the locks are _used_ is okay -- and I think it is.

Well.  We don't know that.  We just know that this report of unokayness
wasn't right.  With Ingo's Linux-only patch we're in a position to verify
that the locking is probably OK.

RE: 2.6.17-mm1 - possible recursive locking detected

[Brown, Len]

>> Nothing jumps out at me as incorrect above, so 
>> at this point it looks like a CONFIG_LOCKDEP artifact --
>> but lets ask the experts:-)
>
>Yes, lockdep uses the callsite of spin_lock_init() to detect 
>the "type" of
>a lock.
>
>But the ACPI obfuscation layers use the same spin_lock_init() site to
>initialise two not-the-same locks, so lockdep decides those 
>two locks are of the same "type" and gets confused.

interesting definition of "type".  I guess it works
in practice or others would be complaining...

>We had earlier decided to remove that ACPI code which kmallocs a single
>spinlock.  When that's done, lockdep will become unconfused.

Yes, that change is already on the way.
The key thing here is that our recent changes in
how the locks are _used_ is okay -- and I think it is.

thanks,
-Len

Re: 2.6.17-mm1 - possible recursive locking detected

[Andrew Morton]

On Thu, 22 Jun 2006 00:28:56 -0400
"Brown, Len" <len.brown@intel.com> wrote:

> >It looks like an ACPI problem.
> 
> Thanks for the note, and the .config, I reproduced it here.
> 
> CONFIG_LOCKDEP complains about this sequence:
> 
> ...
> 	<presumed previous acquire/release acpi_gbl_hardware_lock>
> ...
> acpi_ev_gpe_detect()
> 	spin_lock_irqsave(acpi_gbl_gpe_lock,)
> 
> 	spin_lock_irqsave(acpi_gbl_hardware_lock,) <stack trace is on
> this acquire>
> 	spin_lock_irqrestore(acpi_gbl_hardware_lock,)
> 
> 	...
> 
> 	spin_lock_irqrestore(acpi_gbl_gpe_lock)
> 
> It complains about this only the 1st time, even though
> this same code sequence runs for every (subsequent) ACPI interrupt.
> 
> The intent of the arrangement is that acpi_gbl_hardware_lock is for very
> small critical sections around RMW hardware register access.
> It can be acquired with or without other locks held, but
> nothing else is acquired when it is held.
> 
> Nothing jumps out at me as incorrect above, so 
> at this point it looks like a CONFIG_LOCKDEP artifact --
> but lets ask the experts:-)

Yes, lockdep uses the callsite of spin_lock_init() to detect the "type" of
a lock.

But the ACPI obfuscation layers use the same spin_lock_init() site to
initialise two not-the-same locks, so lockdep decides those two locks are
of the same "type" and gets confused.

We had earlier decided to remove that ACPI code which kmallocs a single
spinlock.  When that's done, lockdep will become unconfused.

AFACIT it's all used for just two statically allocated locks anwyay.

RE: 2.6.17-mm1 - possible recursive locking detected

[Brown, Len]

>It looks like an ACPI problem.

Thanks for the note, and the .config, I reproduced it here.

CONFIG_LOCKDEP complains about this sequence:

...
	<presumed previous acquire/release acpi_gbl_hardware_lock>
...
acpi_ev_gpe_detect()
	spin_lock_irqsave(acpi_gbl_gpe_lock,)

	spin_lock_irqsave(acpi_gbl_hardware_lock,) <stack trace is on
this acquire>
	spin_lock_irqrestore(acpi_gbl_hardware_lock,)

	...

	spin_lock_irqrestore(acpi_gbl_gpe_lock)

It complains about this only the 1st time, even though
this same code sequence runs for every (subsequent) ACPI interrupt.

The intent of the arrangement is that acpi_gbl_hardware_lock is for very
small critical sections around RMW hardware register access.
It can be acquired with or without other locks held, but
nothing else is acquired when it is held.

Nothing jumps out at me as incorrect above, so 
at this point it looks like a CONFIG_LOCKDEP artifact --
but lets ask the experts:-)

-Len


>Setting up standard PCI resources
>=============================================
>[ INFO: possible recursive locking detected ]
>---------------------------------------------
>idle/1 is trying to acquire lock:
> (lock_ptr){....}, at: [<c021cbd2>] acpi_os_acquire_lock+0x8/0xa
>
>but task is already holding lock:
> (lock_ptr){....}, at: [<c021cbd2>] acpi_os_acquire_lock+0x8/0xa
>
>other info that might help us debug this:
>1 lock held by idle/1:
> #0:  (lock_ptr){....}, at: [<c021cbd2>] acpi_os_acquire_lock+0x8/0xa
>
>stack backtrace:
> [<c0103e89>] show_trace+0xd/0x10
> [<c0104483>] dump_stack+0x19/0x1b
> [<c01395fa>] __lock_acquire+0x7d9/0xa50
> [<c0139a98>] lock_acquire+0x71/0x91
> [<c02f0beb>] _spin_lock_irqsave+0x2c/0x3c
> [<c021cbd2>] acpi_os_acquire_lock+0x8/0xa
> [<c0222d95>] acpi_ev_gpe_detect+0x4d/0x10e
> [<c02215c3>] acpi_ev_sci_xrupt_handler+0x15/0x1d
> [<c021c8b1>] acpi_irq+0xe/0x18
> [<c014d36e>] request_irq+0xbe/0x10c
> [<c021cf33>] acpi_os_install_interrupt_handler+0x59/0x87
> [<c02215e7>] acpi_ev_install_sci_handler+0x1c/0x21
> [<c0220d41>] acpi_ev_install_xrupt_handlers+0x9/0x50
> [<c0231772>] acpi_enable_subsystem+0x7d/0x9a
> [<c0416656>] acpi_init+0x3f/0x170
> [<c01003ae>] _stext+0x116/0x26c
> [<c0101005>] kernel_thread_helper+0x5/0xb
>ACPI: Interpreter enabled
>
>Here is a dmesg log 
>http://www.stardust.webpages.pl/files/mm/2.6.17-mm1/mm-dmesg
>Here is a config file
>http://www.stardust.webpages.pl/files/mm/2.6.17-mm1/mm-config
>
>Regards,
>Michal
>
>-- 
>Michal K. K. Piotrowski
>LTG - Linux Testers Group
>(http://www.stardust.webpages.pl/ltg/wiki/)
>