Re: [PATCH v5 RFC 13/14] tools/libxc: noarch save code

[Andrew Cooper <andrew.cooper3@citrix.com>]

On 19/06/14 03:48, Wen Congyang wrote:
> At 06/12/2014 02:14 AM, Andrew Cooper Wrote:
>> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
>> Signed-off-by: Frediano Ziglio <frediano.ziglio@citrix.com>
>> Signed-off-by: David Vrabel <david.vrabel@citrix.com>
>> ---
>>  tools/libxc/saverestore/save.c |  545 +++++++++++++++++++++++++++++++++++++++-
>>  1 file changed, 544 insertions(+), 1 deletion(-)
>>
>> diff --git a/tools/libxc/saverestore/save.c b/tools/libxc/saverestore/save.c
>> index f6ad734..9ad43a5 100644
>> --- a/tools/libxc/saverestore/save.c
>> +++ b/tools/libxc/saverestore/save.c
>> @@ -1,11 +1,554 @@
>> +#include <assert.h>
>> +#include <arpa/inet.h>
>> +
>>  #include "common.h"
>>  
>> +/*
>> + * Writes an Image header and Domain header into the stream.
>> + */
>> +static int write_headers(struct context *ctx, uint16_t guest_type)
>> +{
>> +    xc_interface *xch = ctx->xch;
>> +    int32_t xen_version = xc_version(xch, XENVER_version, NULL);
>> +    struct ihdr ihdr =
>> +        {
>> +            .marker  = IHDR_MARKER,
>> +            .id      = htonl(IHDR_ID),
>> +            .version = htonl(IHDR_VERSION),
>> +            .options = htons(IHDR_OPT_LITTLE_ENDIAN),
>> +        };
>> +    struct dhdr dhdr =
>> +        {
>> +            .type       = guest_type,
>> +            .page_shift = XC_PAGE_SHIFT,
>> +            .xen_major  = (xen_version >> 16) & 0xffff,
>> +            .xen_minor  = (xen_version)       & 0xffff,
>> +        };
>> +
>> +    if ( xen_version < 0 )
>> +    {
>> +        PERROR("Unable to obtain Xen Version");
>> +        return -1;
>> +    }
>> +
>> +    if ( write_exact(ctx->fd, &ihdr, sizeof(ihdr)) )
>> +    {
>> +        PERROR("Unable to write Image Header to stream");
>> +        return -1;
>> +    }
>> +
>> +    if ( write_exact(ctx->fd, &dhdr, sizeof(dhdr)) )
>> +    {
>> +        PERROR("Unable to write Domain Header to stream");
>> +        return -1;
>> +    }
>> +
>> +    return 0;
>> +}
>> +
>> +/*
>> + * Writes an END record into the stream.
>> + */
>> +static int write_end_record(struct context *ctx)
>> +{
>> +    struct record end = { REC_TYPE_END, 0, NULL };
>> +
>> +    return write_record(ctx, &end);
>> +}
>> +
>> +/*
>> + * Writes a batch of memory as a PAGE_DATA record into the stream.  The batch
>> + * is constructed in ctx->save.batch_pfns.
>> + *
>> + * This function:
>> + * - gets the types for each pfn in the batch.
>> + * - for each pfn with real data:
>> + *   - maps and attempts to localise the pages.
>> + * - construct and writes a PAGE_DATA record into the stream.
>> + */
>> +static int write_batch(struct context *ctx)
>> +{
>> +    xc_interface *xch = ctx->xch;
>> +    xen_pfn_t *mfns = NULL, *types = NULL;
>> +    void *guest_mapping = NULL;
>> +    void **guest_data = NULL;
>> +    void **local_pages = NULL;
>> +    int *errors = NULL, rc = -1;
>> +    unsigned i, p, nr_pages = 0;
>> +    unsigned nr_pfns = ctx->save.nr_batch_pfns;
>> +    void *page, *orig_page;
>> +    uint64_t *rec_pfns = NULL;
>> +    struct rec_page_data_header hdr = { 0 };
>> +    struct record rec =
>> +    {
>> +        .type = REC_TYPE_PAGE_DATA,
>> +    };
>> +
>> +    assert(nr_pfns != 0);
>> +
>> +    /* Mfns of the batch pfns. */
>> +    mfns = malloc(nr_pfns * sizeof(*mfns));
>> +    /* Types of the batch pfns. */
>> +    types = malloc(nr_pfns * sizeof(*types));
>> +    /* Errors from attempting to map the mfns. */
>> +    errors = malloc(nr_pfns * sizeof(*errors));
>> +    /* Pointers to page data to send.  Either mapped mfns or local allocations. */
>> +    guest_data = calloc(nr_pfns, sizeof(*guest_data));
>> +    /* Pointers to locally allocated pages.  Need freeing. */
>> +    local_pages = calloc(nr_pfns, sizeof(*local_pages));
> This function is called too many times, so we will allocate/free
> memory again and again. It may affect the performance.
>
> I think we can allocate at setup stage, and only clear guest_data/
> local_pages here.

We likely can.  It is currently like this because it allowed valgrind to
do a fantastic job of spotting errors when flushing an incomplete batch
at the end of each iteration.

It should be possible to consolidate the allocations and use valgrind
client requests to achieve the same effect, although at this point my
effort is far more focused to getting something which works correctly
ready in the 4.5 timeframe.

>
>> +
>> +    if ( !mfns || !types || !errors || !guest_data || !local_pages )
>> +    {
>> +        ERROR("Unable to allocate arrays for a batch of %u pages",
>> +              nr_pfns);
>> +        goto err;
>> +    }
>> +
>> +    for ( i = 0; i < nr_pfns; ++i )
>> +    {
>> +        types[i] = mfns[i] = ctx->ops.pfn_to_gfn(ctx, ctx->save.batch_pfns[i]);
>> +
>> +        /* Likely a ballooned page. */
>> +        if ( mfns[i] == INVALID_MFN )
>> +            set_bit(ctx->save.batch_pfns[i], ctx->save.deferred_pages);
>> +    }
>> +
>> +    rc = xc_get_pfn_type_batch(xch, ctx->domid, nr_pfns, types);
>> +    if ( rc )
>> +    {
>> +        PERROR("Failed to get types for pfn batch");
>> +        goto err;
>> +    }
>> +    rc = -1;
>> +
>> +    for ( i = 0; i < nr_pfns; ++i )
>> +    {
>> +        switch ( types[i] )
>> +        {
>> +        case XEN_DOMCTL_PFINFO_BROKEN:
>> +        case XEN_DOMCTL_PFINFO_XALLOC:
>> +        case XEN_DOMCTL_PFINFO_XTAB:
>> +            continue;
>> +        }
>> +
>> +        mfns[nr_pages++] = mfns[i];
>> +    }
>> +
>> +    if ( nr_pages > 0 )
>> +    {
>> +        guest_mapping = xc_map_foreign_bulk(
>> +            xch, ctx->domid, PROT_READ, mfns, errors, nr_pages);
>> +        if ( !guest_mapping )
>> +        {
>> +            PERROR("Failed to map guest pages");
>> +            goto err;
>> +        }
> To support remus, we will map/unmap guest memory again and again. It
> also affects the performance. We can cache guest mapping here.
>
> Thanks
> Wen Congyang
>
>

I am not aware of the current code caching mappings into the guest. 
64bit toolstacks would be fine setting up mappings for every gfn and
reusing the mappings, but this really won't work for 32bit toolstacks.

What remus does appear to do is have a limited cache of
previously-written pages for XOR+RLE compression.

~Andrew