Xen devel - Feb 2008 - [PATCH][RFC]Remove lock on first guest table walk

Just found this patch in house which I thought sent out
already. :-)

Still one thing remaining is to special case for UP guest
which suffers overhead from this change...

----
Remove lock on first guest table walk

Existing shadow fault path grabs big lock before walking
guest tables, to ensure consistency with shadow content
lest concurrent change from other vcpu in a bad OS.
But this lock brings more lock contention when scaled up
for a good guest which already prevents above case happen.
So this patch tries to remove the lock on first guest
table walk, and then delay check at some special points.

The key is to check whether any guest table update happens
between 1st walk and holding shadow lock. Here we take
two hints for guest table update:
    * write permission removal
    * write emulation
If any above two operations are observed within the race
window, it indicates possiblity that previous walk result
may be inaccurate and re-check is requried. If mismatch,
simply return to trigger another fault.

I made some experiment to sample perfc count for kernel
compile:
<64bit guest>
3.7% of gwalks are re-checked
For re-check, 68% comes from write permission removal
<32bit pae guest>
7.2% of gwalks are re-checked
For re-check, 54.9% comes from write permission removal

Actually previous fast emulation optimization already skip
lots of guest table walks, and thus above ratio can be
smaller if compared to total shadow fault count.

Basically shadow promotion with write permision removal
does suffer higher overhead, but the benefit to reduce
lock contention is more obvious.

Improvement on kernel compile for this patch is:
(64bit Xen, with 2 vcpus guest and 2 physical cpus)
32bit guest: 1.1%
pae guest:   0.4%
64bit guest: 0.5%

Signed-off-by Kevin Tian <kevin.tian@intel.com>

Thanks,
Kevin


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

Hi,

So, the idea seems sound, and avoids the shadow lock altogether on a
bunch more pagefaults, which is nice. 

I think that since PV pagetables are guaranteed to be read-only above
l1e, the guest_map_l1e and guest_get_eff_l1e functions can be allowed to
drop the shadow lock and call guest_walk_tables with shadow_op == 0.
That would mean that there are no callers left setting shadow_op to 1,
and then the shadow_op argument (and the whole mechanism for calling
remove_write_access from guest_walk_tables) could be removed.

I think that in guest_walk_tables, you need to add an rmb() after
reading the version number to stop the compiler from hoisting the
pagetable reads to before the version read.

Coding nits:
 - I''d like to see the "version" field called something more
   descriptive, and moved into the shadow-specific domain state, 
   since HAP won''t be using it.
 - In shadow_check_gwalk, maybe return 1 at the top of the function if
   the version number hasn''t changed, rather than putting most of the 
   function inside an if()?
 - You''ve added a second "not a shadow_fault" printout
without removing
   the first.
 - We can remove the comment at the top of multi.c about reworking the
   guest_walk TLB flush logic. :)

Cheers,

Tim.

-- 
Tim Deegan <Tim.Deegan@citrix.com>
Principal Software Engineer, Citrix Systems (R&D) Ltd.
[Company #02300071, SL9 0DZ, UK.]

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

>From: Tim Deegan
>Sent: 2008年2月21日 22:13
>Hi,
>
>So, the idea seems sound, and avoids the shadow lock altogether on a
>bunch more pagefaults, which is nice. 
>
>I think that since PV pagetables are guaranteed to be read-only above
>l1e, the guest_map_l1e and guest_get_eff_l1e functions can be 
>allowed to
>drop the shadow lock and call guest_walk_tables with shadow_op == 0.
>That would mean that there are no callers left setting shadow_op to 1,
>and then the shadow_op argument (and the whole mechanism for calling
>remove_write_access from guest_walk_tables) could be removed.
Thanks for clarification.
>
>I think that in guest_walk_tables, you need to add an rmb() after
>reading the version number to stop the compiler from hoisting the
>pagetable reads to before the version read.
Yes, my overlook and thanks for pointing out.
>
>Coding nits:
> - I''d like to see the "version" field called something
more
>   descriptive, and moved into the shadow-specific domain state, 
>   since HAP won''t be using it.
Done and renamed to gtable_dirty_version. Sorry if this is still not a
good name and please feel free help polish a better name. :-)
> - In shadow_check_gwalk, maybe return 1 at the top of the function if
>   the version number hasn''t changed, rather than putting most of
the
>   function inside an if()?
Agree.
> - You''ve added a second "not a shadow_fault" printout
without removing
>   the first.
The first one is branched with lock held, and some audit functions also
required lock held. To differentiate, I put a new label. Now I just removed
the new label since only few places use it.
> - We can remove the comment at the top of multi.c about reworking the
>   guest_walk TLB flush logic. :)
Done.

Then updated version attached, and please help check again.

Thanks,
Kevin


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