Re: [Xen-devel] [PATCH v7 0/2] qemu-qdisk: Implementation of grant copy operation.

[Stefano Stabellini <sstabellini@xxxxxxxxxx>]

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
> 

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