Re: [Xen-devel] [PATCH RESEND RFC 0/8] Memory scrubbing from idle loop

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 27/02/17 00:37, Boris Ostrovsky wrote:
> (Resending with corrected Tim's address, sorry)
>
> When a domain is destroyed the hypervisor must scrub domain's pages before
> giving them to another guest in order to prevent leaking the deceased
> guest's data. Currently this is done during guest's destruction, possibly
> causing very lengthy cleanup process.

Thanks for getting around to posting these.

As a general set of notes across the series,

s/bool_t/bool/
More careful use of unsigned long, 1u << order
&pg[0].  =>  &pg->

There are also a few tabs vs spaces issues.

> This series adds support for scrubbing released pages from idle loop,
> making guest destruction significantly faster. For example, destroying a
> 1TB guest can now be completed in slightly over 1 minute as opposed to
> about 9 minutes using existing scrubbing algorithm.

What is this 1m now?  I'd have thought that freeing memory would become
an O(1) splice from the domain pagelist onto the to-be-scrubbed list,
although I see that isn't quite how you have implemented it.

As a couple of other thoughts, how about getting rid of the boot time
explicit scrub by initialising the entire heap as needing scrubbing? 
That way, scrubbing will start happening as soon as the APs hit their
idle loops, rather than later in one explicit action once dom0 is
constructed.

How about marking pages as needing scrubbing at the end of alloc(),
rather than in free() ?  That way you don't need to walk the pagelists
just to set the pending_scrub flag.

> The downside of this series is that we sometimes fail to allocate high-order
> sets of pages since dirty pages may not yet be merged into higher-order sets
> while they are waiting to be scrubbed.

I presume there is no sensible way to try and prioritise the pages which
would have greatest effect for recreating higher-order sets?

>
> Briefly, the new algorithm places dirty pages at the end of heap's page list
> for each node/zone/order to avoid having to scan full list while searching
> for dirty pages. One processor form each node checks whether the node has any
> dirty pages and, if such pages are found, scrubs them. Scrubbing itself
> happens without holding heap lock so other users may access heap in the
> meantime. If while idle loop is scrubbing a particular chunk of pages this
> chunk is requested by the heap allocator, scrubbing is immediately stopped.

Why not maintain two lists?  That way it is O(1) to find either a clean
or dirty free page.

>
> On the allocation side, alloc_heap_pages() first tries to satisfy allocation
> request using only clean pages. If this is not possible, the search is
> repeated and dirty pages are scrubbed by the allocator.
>
> This series needs to be fixed up for ARM and may have a few optimizations
> added (and possibly some cleanup). I also need to decide what to do about
> nodes without processors.
>
> I originally intended to add per-node heap locks

Is this why scrubbing is limited to a single processor per node?  What
kind of performance impact does this give?

> and AVX-based scrubbing but decided that this should be done separately.

I agree that optimising our copying/zeroing routines should be orthogonal.

~Andrew

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