Re: xen_exit_mmap() questions

From: Boris Ostrovsky
Date: Wed Apr 26 2017 - 20:57:34 EST




On 04/26/2017 06:49 PM, Andy Lutomirski wrote:
On Wed, Apr 26, 2017 at 3:45 PM, Boris Ostrovsky
<boris.ostrovsky@xxxxxxxxxx> wrote:
On 04/26/2017 04:52 PM, Andy Lutomirski wrote:
I was trying to understand xen_drop_mm_ref() to update it for some
changes I'm working on, and I'm wondering whether we need
xen_exit_mmap() at all.

AFAICS the intent is to force all CPUs to drop their lazy uses of the
mm being destroyed so it can be unpinned before tearing down the page
tables, thus making it faster to tear down the page tables. This
seems like it'll speed up xen_set_pud() and xen_set_pmd(), but this
seems like it may be of rather limited value.

Why do you think it's of limited value? Without it we will end up with a
hypercall for each update.

Or is your point that the number of those update is relatively small
when we are tearing down?

The latter. Also, unless I'm missing something, xen_set_pte() doesn't
have the optimization. I haven't looked at exactly how page table
teardown works, but if it clears each PTE individually, then that's
the bulk of the work.



Could we get away with
deleting it?

Also, this code in drop_other_mm_ref() looks dubious to me:

/* If this cpu still has a stale cr3 reference, then make sure
it has been flushed. */
if (this_cpu_read(xen_current_cr3) == __pa(mm->pgd))
load_cr3(swapper_pg_dir);

If cr3 hasn't been flushed to the hypervisor because we're in a lazy
mode, why would load_cr3() help? Shouldn't this be xen_mc_flush()
instead?

load_cr3() actually ends with xen_mc_flush() by way of xen_write_cr3()
-> xen_mc_issue().

xen_mc_issue() does:

if ((paravirt_get_lazy_mode() & mode) == 0)
xen_mc_flush();

I assume the load_cr3() is intended to deal with the case where we're
in lazy mode, but we'll still be in lazy mode, right? Or does it
serve some other purpose?

Of course. I can't read (I ignored the "== 0" part).

Apparently the early version had an explicit flush but then it disappeared (commit 9f79991d4186089e228274196413572cc000143b).

The point of CR3 loading here, I believe, is to make sure the hypervisor knows that the (v)CPU is no longer using the the mm's cr3 (we are loading swapper_pgdir here).

-boris