LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
From: Hugh Dickins <hugh@veritas.com>
To: Ingo Molnar <mingo@elte.hu>
Cc: Jeremy Fitzhardinge <jeremy@goop.org>,
	"H. Peter Anvin" <hpa@zytor.com>, Andi Kleen <ak@suse.de>,
	Linux Kernel Mailing List <linux-kernel@vger.kernel.org>
Subject: Re: preempt bug in set_pmd_pfn?
Date: Wed, 5 Mar 2008 14:29:59 +0000 (GMT)	[thread overview]
Message-ID: <Pine.LNX.4.64.0803051407310.22243@blonde.site> (raw)
In-Reply-To: <20080305064814.GB28398@elte.hu>

On Wed, 5 Mar 2008, Ingo Molnar wrote:
> * Jeremy Fitzhardinge <jeremy@goop.org> wrote:
> > Ingo Molnar wrote:
> >> * Jeremy Fitzhardinge <jeremy@goop.org> wrote:
> >>   
> >>> I think set_pmd_pfn, which is only called by __set_fixmap, might have a 
> >>> preempt bug in it.
> >>>     
> >>
> >> yes, and we had similar preemption bugs in the past. I guess most places 
> >> are either infrequent or have some natural atomicity anyway. Wanna send a 
> >> patch?
> >
> > Sure.  Should it just disable preemption, or take a lock?  It calls 
> > set_pte_at without holding any pte locks; that seems to be relatively 
> > common.  Is it OK when you're operating on init_mm?
> 
> no, it's not OK to modify the kernel pagetable without locking - taking 
> the pgd_lock should do the trick. Could you send the stacktrace that 
> shows the place that is preemptible?

Please, Ingo, could you give an example of where such additional locking
is actually necessary?

I ask because I went over those places when splitting the page_table_lock
for userspace in 2.6.15.  Some things took init_mm.page_table_lock and
some things didn't, and I concluded that actually none of them needed it.

With the userspace pagetables, we need to guard against racing threads
and vmscan/rmap.  But with the kernel pagetables, we'd already be in
serious trouble if two cpus could be modifying the same pte at the
same time - there needs to be other serialization already e.g. vmalloc
has its own locking for parcelling out areas to different uses, so down
at the page table level there should be no conflict.

Allocation of new page tables, yes, that needs locking, and does use
the page_table_lock for kernel space just as for user space.

That was all two years ago, I may have been wrong then, or a lot may
have changed since.  But I've heard of a grand total of 0 problems
from not having such locking.

And on the original topic of flush TLB without preemption disabled:
again I'm not sure there's a bug there, but it's less clear.  Aren't
some of those __flush_tlb_ones just unnecessary, we're simply filling
a previously empty slot?  And if there's a guarantee that preemption
will itself involve a TLB flush (maybe there's no such guarantee for
these kernel entries, it's quite a different case from the userspace
one, and you'll be worrying about the global bit), if, then it'd be
okay to __flush_tlb_one without disabling preemption.

Hugh

  reply	other threads:[~2008-03-05 14:42 UTC|newest]

Thread overview: 17+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2008-03-04 21:13 Jeremy Fitzhardinge
2008-03-04 21:28 ` Ingo Molnar
2008-03-04 21:27   ` Jeremy Fitzhardinge
2008-03-05  6:48     ` Ingo Molnar
2008-03-05 14:29       ` Hugh Dickins [this message]
2008-03-05 16:48         ` Jeremy Fitzhardinge
2008-03-05 17:38           ` Hugh Dickins
2008-03-05 19:18             ` Jeremy Fitzhardinge
2008-03-05 20:40               ` Hugh Dickins
2008-03-06 12:52               ` Ingo Molnar
2008-03-06 18:19                 ` Jeremy Fitzhardinge
2008-03-05 16:45       ` Jeremy Fitzhardinge
2008-03-05  0:06 ` Andi Kleen
2008-03-05  0:07   ` Jeremy Fitzhardinge
2008-03-05  0:16     ` Andi Kleen
2008-03-05  0:19       ` Jeremy Fitzhardinge
2008-03-05  1:28         ` Andi Kleen

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=Pine.LNX.4.64.0803051407310.22243@blonde.site \
    --to=hugh@veritas.com \
    --cc=ak@suse.de \
    --cc=hpa@zytor.com \
    --cc=jeremy@goop.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=mingo@elte.hu \
    --subject='Re: preempt bug in set_pmd_pfn?' \
    /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

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