Re: Top kernel oopses/warnings for the week of May 30th 2008

[Hugh Dickins <hugh@veritas.com>]

On Mon, 2 Jun 2008, Andrew Morton wrote:
> On Tue, 3 Jun 2008 00:44:38 +0100 (BST)
> Hugh Dickins <hugh@veritas.com> wrote:
> 
> > +					if (in_atomic())
> > +						kunmap_atomic(kaddr, KM_USER0);
> > +					else
> > +						kunmap(kmapped_page);
> 
> eek.
> 
> /*
>  * Are we running in atomic context?  WARNING: this macro cannot
>  * always detect atomic context; in particular, it cannot know about
>  * held spinlocks in non-preemptible kernels.  Thus it should not be
>  * used in the general case to determine whether sleeping is possible.
>  * Do not use in_atomic() in driver code.
>  */
> #define in_atomic()	((preempt_count() & ~PREEMPT_ACTIVE) != PREEMPT_INATOMIC_BASE)

Yes, that comment is all about how a common function cannot be expected
to guess whether it's being called in atomic context or not; but we
know that we don't have any spinlocks held here, therefore it's okay.

Or do you consider fs/exec.c a driver, and shouldn't set bad example?
It is exactly the test that do_page_fault() makes at the other end,
when deciding whether it can handle the fault.

Originally I had a bool atomic there instead.  I switched over to
testing in_atomic() itself because I had it mind to suggest another
patch: it has long seemed wrong to me that we should have to disable
preemption and fault handling there, when often (on many architectures,
or on many pages) it's unnecessary.

So I'd like to change (the various implementations of) kmap_atomic()
to use pagefault_disable() only when the page actually is in highmem.

Hugh