[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH v7 0/2] qemu-qdisk: Implementation of grant copy
From: |
Stefano Stabellini |
Subject: |
Re: [Qemu-devel] [PATCH v7 0/2] qemu-qdisk: Implementation of grant copy operation. |
Date: |
Wed, 14 Sep 2016 18:33:00 -0700 (PDT) |
User-agent: |
Alpine 2.10 (DEB 1266 2009-07-14) |
Hi Wei,
I am happy to queue up this for QEMU, but I'll wait for the first patch
to be committed to Xen before sending a pull request. Is that OK?
Cheers,
Stefano
On Wed, 14 Sep 2016, Paulina Szubarczyk wrote:
> Hi,
>
> It is a proposition for implementation of grant copy operation in qemu-qdisk
> and interface in libxc/libs.
>
> Changes since v6:
> qemu-qdisk:
> -removed blank lines
> -renamed functions free_buffers -> ioreq_free_copy_buffers,
> ioreq_copy -> ioreq_grant_copy
> -merged the if(ioreq_copy) with the conditions above
>
> Changes since v5:
> qemu-qdisk:
> -added checking of every interface in the configure file. Based on
> the Roger's comment that xengnttab_map_grant_ref was added prior
> xengnttab_grant_copy, thus do not need to be check again here
> I dropped this check.
>
> Changes since v4:
> Interface:
> - changed the subject line
> - changed the comment in libs/gnttab/include/xengnttab.h according
> to the David's suggestion.
> - removed unnecessary braces.
>
> qemu-qdisk:
> - in the configure file check only if xengnttab_grant_copy is
> implemented to verify 480 version of Xen.
> - remove r variable and initialization of count to 0 in
> ioreq_copy.
>
> - surround free_buffers, ioreq_init_copy_buffers and ioreq_copy
> by "#if CONFIG_XEN_CTRL_INTERFACE_VERSION >= 480" abort in else
> path because the function should not be called in that case.
> - replace the definition of struct xengnttab_grant_copy_segment
> and a typedef to it with
> 'typedef void* xengnttab_grant_copy_segment_t'.
> - moved the new code in the xen_common.h to the end of the file.
>
> Changes since v3:
> Interface:
> - revert to cast from xengnttab_grant_copy_segment_t
> to ioctl_gntdev_grant_copy.
> - added compile-time check to compare the libs
> xengnttab_grant_copy_segment_t with the ioctl structure.
> The patch relies on Wei patch introducing XENGNTTAB_BUILD_BUG_ON in
> libs/gnttab.
>
> qemu-qdisk:
> - qemu_memalign/qemu_free is used instead function allocating
> memory from xc.
> - removed the get_buffer function instead there is a direct call
> to qemu_memalign.
> - moved ioreq_copy for write operation to ioreq_runio_qemu_aio.
> - added struct xengnttab_grant_copy_segment_t and stub in
> xen_common.h for version of Xen earlier then 480.
> - added checking for version 480 to configure. The test repeats
> all the operation that are required for version < 480 and
> checks if xengnttab_grant_copy() is implemented.
>
> Changes since v2:
> Interface:
> - dropped the changes in libxc/include/xenctrl_compat
> - changed the MINOR version in Makefile
> - replaced 'return -1' -> 'abort()'in libs/gnttab/gnttab_unimp.c
> - moved the struct 'xengnttab_copy_grant_segment' to
> libs/gnttab/include/xengnttab.h
> - added explicit assingment to ioctl_gntdev_grant_copy_segment
> to the linux part
>
> qemu-qdisk:
> - to use the xengnttab_* function directly added -lxengnttab to configure
> and include <xengnttab.h> in include/hw/xen/xen_common.h
> - in ioreq_copy removed an out path, changed a log level, made explicit
> assignement to 'xengnttab_copy_grant_segment'
> * I did not change the way of testing if grant_copy operation is implemented.
> As far as I understand if the code from gnttab_unimp.c is used then the
> gnttab
> device is unavailable and the handler to gntdev would be invalid. But
> if the handler is valid then the ioctl should return operation
> unimplemented
> if the gntdev does not implement the operation.
>
>
> Changes since v1:
> Interface:
> - changed the interface to call grant copy operation to match ioctl
> int xengnttab_grant_copy(xengnttab_handle *xgt,
> uint32_t count,
> xengnttab_grant_copy_segment_t* segs)
>
> - added a struct 'xengnttab_copy_grant_segment' definition to tools/libs
> /gnttab/private.h, tools/libxc/include/xenctrl_compat.h
>
> - changed the function 'osdep_gnttab_grant_copy' which right now just
> call the ioctl
>
> - added a new VER1.1 to tools/libs/gnttab/libxengnttab.map
>
> qemu-qdisk:
> - removed the 'ioreq_write','ioreq_read_init','ioreq_read' functions
> - implemented 'ioreq_init_copy_buffers', 'ioreq_copy'
> - reverted the removal of grant map and introduced conditional invoking
> grant copy or grant map
> - resigned from caching the local buffers on behalf of allocating the
> required amount of pages at once. The cached structure would require
> to have an lock guard and I suppose that the performance improvement
> would degraded.
>
>
> For the functional test I attached the device with a qdisk backend to the
> guest, mounted, performed some reads and writes.
>
> I run fio tests[1] with different iodepth and size of the block. The test can
> be
> accessed on my github[2] but mainly after the warm up I run for 60 seconds:
> fio --time_based \
> --clocksource=clock_gettime \
> --rw=randread \
> --random_distribution=pareto:0.9 \
> --size=10g \
> --direct='1' \
> --ioengine=libaio \
> --filename=$DEV \
> --iodepth=$IODEPTH \
> --bs=$BS \
> --name=$NAME \
> --runtime=$RUNTIME >> $FILENAME
> The test were repeated at least three times.
>
> [1]
> https://docs.google.com/spreadsheets/d/1E6AMiB8ceJpExL6jWpH9u2yy6DZxzhmDUyFf-eUuJ0c/edit?usp=sharing
>
> [2] https://github.com/paulina-szubarczyk/xen-benchmark
> - multitest_with_iodepth.sh
>
>
> Thanks and regards,
> Paulina
>