Re: [PATCH v2] x86/mm: also flush TLB when putting writable foreign page reference

[Tim Deegan <tim@xen.org>]

At 02:50 -0600 on 02 May (1493693403), Jan Beulich wrote:
> >>> On 02.05.17 at 10:32, <tim@xen.org> wrote:
> > At 04:52 -0600 on 28 Apr (1493355160), Jan Beulich wrote:
> >> >>> On 27.04.17 at 11:51, <tim@xen.org> wrote:
> >> > At 03:23 -0600 on 27 Apr (1493263380), Jan Beulich wrote:
> >> >> ... it wouldn't better be the other way around: We use the patch
> >> >> in its current (or even v1) form, and try to do something about
> >> >> performance only if we really find a case where it matters. To be
> >> >> honest, I'm not even sure how I could meaningfully measure the
> >> >> impact here: Simply counting how many extra flushes there would
> >> >> end up being wouldn't seem all that useful, and whether there
> >> >> would be any measurable difference in the overall execution time
> >> >> of e.g. domain creation I would highly doubt (but if it's that what
> >> >> you're after, I could certainly collect a few numbers).
> >> > 
> >> > I think that would be a good idea, just as a sanity-check.
> >> 
> >> As it turns out there is a measurable effect: xc_dom_boot_image()
> >> for a 4Gb PV guest takes about 70% longer now. Otoh it is itself
> >> responsible for less than 10% of the overall time libxl__build_dom()
> >> takes, and that in turn is only a pretty small portion of the overall
> >> "xl create".
> > 
> > Do you think that slowdown is OK?  I'm not sure -- I'd be inclined to
> > avoid it, but could be persuaded, and it's not me doing the work. :)
> 
> Well, if there was a way to avoid it in a clean way without too much
> code churn, I'd be all for avoiding it. The avenues we've explored so
> far either didn't work (using pg_owner's dirty mask) or didn't promise
> to actually reduce the flush overhead in a meaningful way (adding a
> separate mask to be merged into the mask used for the flush in
> __get_page_type()), unless - as has been the case before - I didn't
> fully understand your thoughts there.

Quoting your earlier response:

> Wouldn't it suffice to set bits in this mask in put_page_from_l1e()
> and consume/clear them in __get_page_type()? Right now I can't
> see it being necessary for correctness to fiddle with any of the
> other flushes using the domain dirty mask.
> 
> But then again this may not be much of a win, unless the put
> operations come through in meaningful batches, not interleaved
> by any type changes (the latter ought to be guaranteed during
> domain construction and teardown at least, as the guest itself
> can't do anything at that time to effect type changes).

I'm not sure how much batching there needs to be.  I agree that the
domain creation case should work well though.  Let me think about the
scenarios when dom B is live:

1. Dom A drops its foreign map of page X; dom B immediately changes the
type of page X.  This case isn't helped at all, but I don't see any
way to improve it -- dom A's TLBs need to be flushed right away.

2. Dom A drops its foreign map of page X; dom B immediately changes
the type of page Y.  Now dom A's dirty CPUs are in the new map, but B
may not need to flush them right away.  B can filter by page Y's
timestamp, and flush (and clear) only some of the cpus in the map.

So that seems good, but then there's a risk that cpus never get
cleared from the map, and __get_page_type() ends up doing a lot of
unnecessary work filtering timestaps.  When is it safe to remove a CPU
from that map?
 - obvs safe if we IPI it to flush the TLB (though may need memory
   barriers -- need to think about a race with CPU C putting A _into_
   the map at the same time...)
 - we could track the timestamp of the most recent addition to the
   map, and drop any CPU whose TLB has been flushed since that,
   but that still lets unrelated unmaps keep CPUs alive in the map...
 - we could double-buffer the map: always add CPUs to the active map;
   from time to time, swap maps and flush everything in the non-active
   map (filtered by the TLB timestamp when we last swapped over).

Bah, this is turning into a tar pit.  Let's stick to the v2 patch as
being (relatively) simple and correct, and revisit this if it causes
trouble. :)

Thanks,

Tim.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel