Messages in this thread | | | Date | Wed, 21 Sep 2005 12:49:57 -0700 | From | Andrew Morton <> | Subject | Re: [PATCH 07/10] uml: avoid fixing faults while atomic |
| |
"Paolo 'Blaisorblade' Giarrusso" <blaisorblade@yahoo.it> wrote: > > From: Paolo 'Blaisorblade' Giarrusso <blaisorblade@yahoo.it> > > Following i386, we should maybe refuse trying to fault in pages when we're > doing atomic operations, because to handle the fault we could need to take > already taken spinlocks. > > Also, if we're doing an atomic operation (in the sense of in_atomic()) we're > surely in kernel mode and we're surely going to handle adequately the failed > fault, so it's safe to behave this way. > > Currently, on UML SMP is rarely used, and we don't support PREEMPT, so this is > unlikely to create problems right now, but it might in the future. >
That's not really an accurate explanation/understanding of what's going on in there.
There's an extremely special-case in the pagefault handlers where we fail the fault if in_atomic(). It's unrelated to spinlocks (spinlocks don't even cause in_atomic() to become true if !CONFIG_PREEMPT).
It has to do with kmap_atomic(). There's tricksy code in mm/filemap.c which will fault the target page in by hand and will then take an atomic kmap and will then raise current->preempt_count by hand, so in_atomic() becomes true even if !COFNIG_PREEMPT.
So at this stage we expect to be able to do a copy_to/from_user to/from pagecache without taking a fault, because we just faulted the page in by hand. And we're not allowed to take a fault, because we're holding an atomic kmap. But if we _do_ take a fault (extreme memory pressure, racing munmap, etc) then we want to fail the pagefault immediately.
The in_atomic() test in x86's do_page_fault() is in fact a message passed into it from filemap.c's kmap_atomic(). It has accidental side-effects, such as making copy_to_user() fail if inside spinlocks when CONFIG_PREEMPT=y.
So I think this change is only needed if UML implements kmap_atomic, as in arch/i386/mm/highmem.c, which it surely does not do?
> > diff --git a/arch/um/kernel/trap_kern.c b/arch/um/kernel/trap_kern.c > --- a/arch/um/kernel/trap_kern.c > +++ b/arch/um/kernel/trap_kern.c > @@ -40,6 +40,12 @@ int handle_page_fault(unsigned long addr > int err = -EFAULT; > > *code_out = SEGV_MAPERR; > + > + /* If the fault was during atomic operation, don't take the fault, just > + * fail. */ > + if (in_atomic()) > + goto out_nosemaphore; > + > down_read(&mm->mmap_sem); > vma = find_vma(mm, address); > if(!vma) > @@ -90,6 +96,7 @@ survive: > flush_tlb_page(vma, address); > out: > up_read(&mm->mmap_sem); > +out_nosemaphore: > return(err); > > /* - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
| |