Re: copy_one_pte()

["Daniel J Blueman" <daniel.blueman@gmail.com>]

On 03/04/07, Zoltan Menyhart <Zoltan.Menyhart@bull.net> wrote:
> Daniel J Blueman wrote:
> > On 13 Mar, 20:20, Zoltan Menyhart <Zoltan.Menyh...@free.fr> wrote:
> >
> >> I had a look at copy_one_pte().
> >> I cannot see any ioproc_update_page() call, not even for the COW pages.
> >> Is it intentional?
> >
> > There could be an ioproc_update_range() call in
> > memory.c:copy_pte_range(), after the pte_unmap_unlock(), and this
> > would be an optimisation, since explicitly updating the mappings in
> > the Quadrics RDMA NIC avoids the need for the NIC to trap and update
> > it's MMU later.
>
> I can agree that this optimization is a good idea.
>
> Can you please confirm that cp->update_range() understands the concept of
> the COW? (I.e. the card should not write anything into the COW pages.)
>
> > The code which implements [1] this takes the pagetable locks with
> > pte_offset_map_lock(), and uses one of the kmap_atomic slots, so has
> > to be after the pagetables are unlocked. The
> > ioproc_invalidate_page/range() calls are different and can't live with
> > this race, so have to be used with the pagetable locks held.
>
> I can see two issues here:
>
> - Performance problem: page_table_lock is more expensive than a split lock,
>   and taking page_table_lock after the split lock released is even worse.

I have introduced an ioproc_update_page_locked() call for this and
related paths, and updated the kernel patches; this avoids a race
condition I was seeing in multi-threaded processes taking concurrent
page faults under the split locks. The split lock is now acquired
once, and the changes allowed me to do other some optimisations on
this path too.

> - Synchronization problem: do_wp_page() is protected by the appropriate
>   split lock, i.e. the COW page can be broken up while you are inside
>   cp->update_range() under the protection of page_table_lock.

The split locks were used previously via pte_offset_map_lock(), so
this race wasn't possible previously.

> I think the PTE modification and the cp->update_xxx() should be protected
> by the very same lock, without releasing it between the two operations.
> I think the PTE modification and the cp->update_xxx() should be an
> atomic operation with respect to the VMM activity.

This does make most sense and is now the case, although atomiticity
wasn't a problem, since it's fine to use ioproc_update_page/range()
speculatively.

Dan

> Thanks,
>
> Zoltan
-- 
Daniel J Blueman