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

[Konstantin Belousov]

On Wed, Jul 10, 2013 at 09:05:02PM -0700, Neel Natu wrote:
> 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?

Yes, I think so.  At least, it would remove two pairs of process lock'
lock and unlock on the trap path.

IMO, the only useful applications of the PHOLD() is when we need to
ensure that other process does not exit while we manipulate it, or when
the process kstack has to be accessed, e.g. in ptrace(2) to manipulate
the trapframe or pcb. For vm_fault() calls, this means that it is enough
to use PHOLD to interlock the call to vmspace_acquire_ref(). Other uses
are probably vestiges of the time when struct user was swappable.

Curproc cannot exit while thread is executing.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 834 bytes
Desc: not available
URL: <http://lists.freebsd.org/pipermail/svn-src-projects/attachments/20130711/f93c41b9/attachment.sig>