svn commit: r253135 - in projects/bhyve_npt_pmap/sys/amd64: include vmm

[Neel Natu]

Hi Konstantin,

On Wed, Jul 10, 2013 at 12:48 AM, Konstantin Belousov
<kostikbel at gmail.com> wrote:
> On Wed, Jul 10, 2013 at 12:36:38AM -0700, Neel Natu wrote:
>> Hi Konstantin,
>>
>> On Wed, Jul 10, 2013 at 12:20 AM, Konstantin Belousov
>> <kostikbel at gmail.com> wrote:
>> > On Wed, Jul 10, 2013 at 07:12:55AM +0000, Neel Natu wrote:
>> >> Author: neel
>> >> Date: Wed Jul 10 07:12:55 2013
>> >> New Revision: 253135
>> >> URL: http://svnweb.freebsd.org/changeset/base/253135
>> >>
>> >> Log:
>> >>   Replace vm_gpa2hpa() with a pair of functions vm_gpa_hold()/vm_gpa_release().
>> >>
>> >>   We guarantee that the vm_page backing the 'gpa' is not reclaimed by
>> >>   the page daemon until the caller indicates that they are done using it
>> >>   by calling 'vm_gpa_release()'.
>> >>
>> >> Modified:
>> >>   projects/bhyve_npt_pmap/sys/amd64/include/vmm.h
>> >>   projects/bhyve_npt_pmap/sys/amd64/vmm/vmm.c
>> >>   projects/bhyve_npt_pmap/sys/amd64/vmm/vmm_dev.c
>> >>   projects/bhyve_npt_pmap/sys/amd64/vmm/vmm_instruction_emul.c
>> >>
>> >> Modified: projects/bhyve_npt_pmap/sys/amd64/include/vmm.h
>> >> ==============================================================================
>> >> --- projects/bhyve_npt_pmap/sys/amd64/include/vmm.h   Wed Jul 10 06:46:46 2013        (r253134)
>> >> +++ projects/bhyve_npt_pmap/sys/amd64/include/vmm.h   Wed Jul 10 07:12:55 2013        (r253135)
>> >> @@ -93,6 +93,9 @@ const char *vm_name(struct vm *vm);
>> >>  int vm_malloc(struct vm *vm, vm_paddr_t gpa, size_t len);
>> >>  int vm_map_mmio(struct vm *vm, vm_paddr_t gpa, size_t len, vm_paddr_t hpa);
>> >>  int vm_unmap_mmio(struct vm *vm, vm_paddr_t gpa, size_t len);
>> >> +void *vm_gpa_hold(struct vm *, vm_paddr_t gpa, size_t len, int prot,
>> >> +               void **cookie);
>> >> +void vm_gpa_release(void *cookie);
>> >>  vm_paddr_t vm_gpa2hpa(struct vm *vm, vm_paddr_t gpa, size_t size);
>> >>  int vm_gpabase2memseg(struct vm *vm, vm_paddr_t gpabase,
>> >>             struct vm_memory_segment *seg);
>> >>
>> >> Modified: projects/bhyve_npt_pmap/sys/amd64/vmm/vmm.c
>> >> ==============================================================================
>> >> --- projects/bhyve_npt_pmap/sys/amd64/vmm/vmm.c       Wed Jul 10 06:46:46 2013        (r253134)
>> >> +++ projects/bhyve_npt_pmap/sys/amd64/vmm/vmm.c       Wed Jul 10 07:12:55 2013        (r253135)
>> >> @@ -439,16 +439,48 @@ vm_malloc(struct vm *vm, vm_paddr_t gpa,
>> >>       return (0);
>> >>  }
>> >>
>> >> -vm_paddr_t
>> >> -vm_gpa2hpa(struct vm *vm, vm_paddr_t gpa, size_t len)
>> >> +void *
>> >> +vm_gpa_hold(struct vm *vm, vm_paddr_t gpa, size_t len, int reqprot,
>> >> +         void **cookie)
>> >>  {
>> >> -     vm_paddr_t nextpage;
>> >> +     int rv, pageoff;
>> >> +     vm_page_t m;
>> >> +     struct proc *p;
>> >> +
>> >> +     pageoff = gpa & PAGE_MASK;
>> >> +     if (len > PAGE_SIZE - pageoff)
>> >> +             panic("vm_gpa_hold: invalid gpa/len: 0x%016lx/%lu", gpa, len);
>> >> +
>> >> +     p = curthread->td_proc;
>> >> +
>> >> +     PROC_LOCK(p);
>> >> +     p->p_lock++;
>> >> +     PROC_UNLOCK(p);
>> > Why do you need to hold the process there ?
>>
>> I was following the idiom in trap_pfault() - the comment in trap.c
>> about swapout was enough to convince me :-)
>>
>> > I do not think that hold in the trap handler is really useful, probably
>> > the reverse.
>>
>> I am happy to remove the lock if isn't needed.
>>
>> Could you explain a bit more on why it may be detrimental or point me
>> in the right direction so I can look for myself.
> The hold prevents the swap out of the process. This means that kernel
> stack is assured to be resident for all process threads, and the
> vm_pageout_map_deactivate_pages() is not called for the process. In
> other words, in the low memory condition, the VM choice of the pages
> to reuse is limited, which is probably esp. bad for the large address
> spaces like whole virtual machine.
>
> The swapped-out state of the process interacts correctly with the
> vm_fault_hold(), the synchronization of the access to map and objects
> would do the right thing. IMO the PHOLD() makes the page fault
> potentially faster to proceed by the cost of making the deadlock due to
> low memory condition more probable.

Thanks for the explanation. I submitted a change to get rid of the
'p_lock'ing when resolving the guest fault.

Would the same reasoning lead us to get rid of the 'p_lock' increment
in 'trap_pfault()' as well?

best
Neel