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

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 02/05/17 10:43, Tim Deegan wrote:
> At 02:50 -0600 on 02 May (1493693403), Jan Beulich wrote:
>>>>> On 02.05.17 at 10:32, <tim@xxxxxxx> wrote:
>>> At 04:52 -0600 on 28 Apr (1493355160), Jan Beulich wrote:
>>>>>>> On 27.04.17 at 11:51, <tim@xxxxxxx> 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. :)

:(

A 70% performance hit for guest creation is certainly going to cause
problems, but we obviously need to prioritise correctness in this case.

~Andrew

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