Re: [Xen-devel] Vanilla Linux and has_foreign_mapping

[Michael Abd-El-Malek <mabdelmalek@xxxxxxx>]

Jeremy Fitzhardinge wrote:
> Michael Abd-El-Malek wrote:
> > Jeremy Fitzhardinge wrote:
> > > Keir Fraser wrote:
> > > > I'm not really familiar with the pv_ops code I'm afraid. But 
> > > > thinking about
> > > > this some more I've realised there's no way really to avoid making the
> > > > early-unpin logic aware of gntdev mappings. This is because if we do 
> > > > pin pte
> > > > pages, and require them to remain pinned across early-unpin, then
> > > > pgd_unpin() must not attempt to make those pte pages writable. That 
> > > > will
> > > > fail, because the pages are still pinned! You'd either need to 
> > > > handle the
> > > > failure to make the page writable, or have a per-page flag to 
> > > > indicate which
> > > > pte pages contain gntdev mappings. Frankly you may as well stick 
> > > > with the
> > > > per-mm-context has_foreign_mappings flag.
> > > >   
> > > So the issue is that a pte page containing a _PAGE_IO pte must remain 
> > > pinned while it contains that mapping?  Would shooting down the 
> > > mapping allow it to be unpinned, or does that need to be deferred 
> > > until some later point (if so, when?)?
> > > I guess the downside is that we'd need to scan the pte looking for 
> > > _PAGE_IO mappings, which is a bit of a pain.  Skipping that would 
> > > mean hiding a flag somewhere...
> > > > Is it a pain to add a pv_ops-subtype-specific flag to mm_context? If 
> > > > so you
> > > > could maintain a set datastructure instead, indicating which 
> > > > mm_contexts
> > > > contain foreign mappings.
> > > >   
> > > So, in 2.6.18-xen mm->has_foreign_mapping makes it skip early-unpin, 
> > > but puts it off until pgd_free().  Presumably that works because all 
> > > the vma's all been unmapped by then...
> > The following patch was sufficient for me.  I delayed the 
> > arch_exit_mmap (which eventually calls into xen) until after 
> > unmap_vmas is called, which calls zap_pte (where I unmap the grant).  
> > Presumably, there is a performance overhead to always doing this 
> > delay, and hence 2.6.18 only did the delay if has_foreign_mappings is 
> > set.  For macrobenchmarks like compilation, I couldn't find a difference.
> > Cheers,
> > Mike
> >
> > diff --git a/mm/mmap.c b/mm/mmap.c
> > index a32d28c..c118b54 100644
> > --- a/mm/mmap.c
> > +++ b/mm/mmap.c
> > @@ -2036,15 +2036,14 @@ void exit_mmap(struct mm_struct *mm)
> >      unsigned long nr_accounted = 0;
> >      unsigned long end;
> >
> > -    /* mm's last user has gone, and its about to be pulled down */
> > -    arch_exit_mmap(mm);
> > -
> >      lru_add_drain();
> >      flush_cache_mm(mm);
> >      tlb = tlb_gather_mmu(mm, 1);
> >      /* Don't update_hiwater_rss(mm) here, do_exit already did */
> >      /* Use -1 here to ensure all VMAs in the mm are unmapped */
> >      end = unmap_vmas(&tlb, vma, 0, -1, &nr_accounted, NULL);
> > +    /* mm's last user has gone, and its about to be pulled down */
> > +    arch_exit_mmap(mm);
> Yeah, that pretty much removes the point of "early unpin".  The problem 
> is that unmap_vmas tears down the pagetable, and if its pinned that 
> means lots of hypercalls.  If it has already been unpinned it can be 
> much more efficient.
> It mainly effects exec/exit performance, and I'm not sure a kernel 
> compile is all that execy/exity - most of the time is just spent in gcc 
> crunching.  Something with more shellscripts might show more of a 
> difference. (I have to admit I haven't benchmarked this myself.)
>    J
How about we do the following:

arch_exit_mmap_pre(mm);

lru_add_drain();
flush_cache_mm(mm);
tlb = tlb_gather_mmu(mm, 1);
/* Don't update_hiwater_rss(mm) here, do_exit already did */
/* Use -1 here to ensure all VMAs in the mm are unmapped */
end = unmap_vmas(&tlb, vma, 0, -1, &nr_accounted, NULL);

arch_exit_mmap_post(mm);

We'll reintroduce has_foreign_mappings.  If has_foreign_mappings is _not_ set, 
then arch_exit_mmap_pre can early unpin the page tables and arch_exit_mmap_post 
will do nothing.  If has_foreign_mappings is set, then arch_exist_mmap_pre won't 
do anything, and arch_exit_mmap_post will do the actual xen_exit_mmap call.
What do you think?

Mike

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxx
http://lists.xensource.com/xen-devel