lkml.org 
[lkml]   [2008]   [Jun]   [2]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: Top kernel oopses/warnings for the week of May 30th 2008
On Tue, 3 Jun 2008 01:41:22 +0100 (BST) Hugh Dickins <hugh@veritas.com> wrote:

> 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.

Well, if you're sure.. I didn't look very closely (sorry), nor did you
explain very closely.

I think doing this sort of thing is OK in fs/exec.c from the
should-we-be-doing-this-in there POV, but it should have suitable comments
slapped all over it.

> 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.

So... places like file_read_actor() would be given an open-coded
pagefault_disable() so we preserve out implicit boolean-passing down to
do_page_fault()?

One of the reasons why we (I?) left kmap_atomic() doing
pagefault_disable() for all pages was testing coverage: not many
developers test with highmem nowadays so there's a high risk (almost a
certainty) that people will start adding can-schedule code inside their
kmap_atomic() regions. Probably it's not a terribly good reason...


\
 
 \ /
  Last update: 2008-06-03 03:25    [W:0.059 / U:1.252 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site