Re: Vanilla Linux and has_foreign_mapping

[Michael Abd-El-Malek <mabdelmalek@cmu.edu>]

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