Re: Fwd: Re: struct page field arrangement

["Jan Beulich" <jbeulich@novell.com>]

>>>> Keir Fraser <keir@xensource.com> 01.03.07 11:22 >>>
>>Can we confident that the mm_pin/mm_unpin code (which walks pagetables and
>>has to find every page to make every one read-only or writable) is safe?
>>Presumably for this to be true we need to be sure that noone can meanwhile
>>concurrently be populating the pagetable we are walking with extra
>>pgds/puds/pmds/ptes...
>
>Since the pin/unpin walking only cares about pgd/pud/pmd entries, synchronization
>is guaranteed through mm->page_table_lock. The pte lock is used only for leaf
>entries, which are of no concern to (un)pinning.

I'm afraid I have to correct myself. Stress testing has shown severe
problems, and after a few hours of staring at this I'm almost certain there
is a race condition here: While no new pte-s can ever appear, the logic in
mm/vmscan.c may try to modify pte-s in an mm currently being unpinned
(at least through ptep_clear_flush_young() called from
page_referenced_one() in mm/rmap.c). If this happens when
xen_pgd_unpin() has already passed the respective pte page, but
mm_walk() hasn't reached the page, yet, the update will fail (if done
directly, ptwr will no pick this up, and if done through a hypercall, the
call would fail, likely producing a BUG()).

Of course we could back out that changeset, but one of the reasons for
submitting it was to get closer to native. Therefore I'm considering
alternatives:
- lock all pte pages right after taking the page table lock in the pin/unpin
  functions, and drop them right before dropping the page table lock
  (this nesting should be no problem, as none of them can ever nest
  elsewhere, since otherwise the non-split-pt-lock case would not work)
- find a way to fix up the possibly resulting page fault (e.g. allow the
  ptwr code to update the page if it is PGT_l1_page_table but has a
  zero type reference count; the PGT_writable case would be more
  difficult and would probably need to be caught in the guest by checking
  the pte and finding it to be writable); the hypercalls don't seem to be
  affected (do_mmu_update seems okay as it doesn't look at the type
  reference count, and do_update_va_mapping can be called only on
  the currently active address space, which cannot be the one being in
  transition)
Dealing with an mm being pinned seems more difficult, as its L1 page
table pages will not be in PGT_l1_page_table state yet. Thus another
alternative could be to make page_check_address() in the kernel
detect the being-(un)pinned status, or even adjust pte_lockptr() to
return the page table lock for mm-s being (un)pinned (this would
perhaps be the cheapest fix).

Jan