linux-embedded.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Ming Lei <tom.leiming@gmail.com>
To: Russell King - ARM Linux <linux@arm.linux.org.uk>
Cc: linux-arm-kernel@lists.infradead.org,
	linux-embedded@vger.kernel.org, a.p.zijlstra@chello.nl,
	linux-kernel@vger.kernel.org
Subject: Re: [RESEND PATCH] ARM: fix 'unannotated irqs-on' lockdep warning
Date: Mon, 24 May 2010 11:23:55 +0800	[thread overview]
Message-ID: <20100524112355.18424622@tom-lei> (raw)
In-Reply-To: <20100523194746.GE950@n2100.arm.linux.org.uk>

On Sun, 23 May 2010 20:47:46 +0100
Russell King - ARM Linux <linux@arm.linux.org.uk> wrote:

> Let me explain again.  We have this series of actions:
> 
> - in userspace
> - exception happens
> - cpu disables interrupts itself
> - save state
> - enable interrupts, and tell lockdep that IRQs are unmasked
> - we process the exception, and ultimately call ret_fast_syscall or
>   ret_slow_syscall
> 
> Now, what was happening in existing kernels is:
> 
> POINT A.
> - disable interrupts, and tell lockdep that IRQs are masked
> - check for any work pending
>   - if work pending, call function - with IRQs still masked
>   - go back to point A.
> - restore state
> - resume userspace, which implicitly re-enables IRQs
> 
> This results in a balanced and afaics correct setup.  Lockdep doesn't
> care about the state of userspace - it only cares about state (and its
> code only ever runs) when we're in kernel mode.
> 
> With your change above, what's happening is the above is replaced by:
> 
> POINT A.
> - disable interrupts, but don't tell lockdep that IRQs are masked
> - check for any work pending
>   - if work pending, call function - with IRQs still masked
> 	*but* lockdep believes IRQs are enabled.  Therefore, I believe
> 	false warnings are probable from things like the scheduler,
> 	signal handling paths, etc.
>   - go back to point A.
> - restore state
> - resume userspace, which implicitly re-enables IRQs
> 
> So can you now see why I believe the above change I've quoted is
> wrong?

Yes, you are right, so we can fix it by the two ways below:

	-keep disable_irq in ret_slow_syscall, also add a extra 
	trace_ret_hardirqs_on before resume userspace.
or 
	-replace disable_irq with disable_irq_notrace in ret_slow_syscall
	and ret_fast_syscall, also add a extra trace_ret_hardirqs_off if
	there are works pending

seems the 2nd way is more efficient.

> 
> Moreover, I put to you that it's utterly pointless - and a waste of
> CPU time - telling lockdep about the IRQ masking when an exception

Yes, the patch still tries to remove the pointless trace of IRQ masking,
such as: replace disable_irq with disable_irq_notrace.

> occurs, and it's also pointless telling lockdep about the IRQ
> unmasking when we resume userspace.

Even it is pointless, but if lockdep doesn't see the IRQ unmasking, the 
warning "unannotated irqs-on" will be triggered and lockdep doe not work
any longer, so we have to remove the warning to make lockdep workable on
ARM, could you agree on it?  It is the main purpose of the patch.

The warning was reported before and still exists in 2.6.34 and -next tree:

        http://marc.info/?l=linux-arm-kernel&m=126047420005553&w=2


Thanks,
Ming Lei

  reply	other threads:[~2010-05-24  3:23 UTC|newest]

Thread overview: 12+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2010-05-23 11:48 [RESEND PATCH] ARM: fix 'unannotated irqs-on' lockdep warning tom.leiming
2010-05-23 12:38 ` Russell King - ARM Linux
2010-05-23 13:44   ` Ming Lei
2010-05-23 14:13     ` Russell King - ARM Linux
2010-05-23 15:07       ` Ming Lei
2010-05-23 19:47         ` Russell King - ARM Linux
2010-05-24  3:23           ` Ming Lei [this message]
2010-05-24  7:19             ` Russell King - ARM Linux
2010-05-24 10:14               ` Russell King - ARM Linux
2010-05-24 14:20                 ` Ming Lei
2010-05-24 14:45                   ` Russell King - ARM Linux
2010-05-24 15:19                     ` Ming Lei

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20100524112355.18424622@tom-lei \
    --to=tom.leiming@gmail.com \
    --cc=a.p.zijlstra@chello.nl \
    --cc=linux-arm-kernel@lists.infradead.org \
    --cc=linux-embedded@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux@arm.linux.org.uk \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).