Re: [Xen-devel] [PATCH] tools/libxl: Add iothread support for COLO

[Anthony PERARD <anthony.perard@xxxxxxxxxx>]

Hi,

Thanks for the patch, I've got plenty of question.

On Mon, Jun 10, 2019 at 04:33:36PM +0800, Zhang Chen wrote:
> From: Zhang Chen <chen.zhang@xxxxxxxxx>
> 
> Xen COLO and KVM COLO shared lots of code in Qemu.
> KVM COLO has added the iothread support, so we add it on Xen.
> 
> Detail:
> https://wiki.qemu.org/Features/COLO
> 
> Signed-off-by: Zhang Chen <chen.zhang@xxxxxxxxx>
> ---
>  tools/libxl/libxl_dm.c      | 14 +++++++++++---
>  tools/libxl/libxl_types.idl |  2 ++
>  tools/xl/xl_parse.c         |  4 ++++
>  3 files changed, 17 insertions(+), 3 deletions(-)
> 
> diff --git a/tools/libxl/libxl_dm.c b/tools/libxl/libxl_dm.c
> index f4fc96415d..6bb400efdf 100644
> --- a/tools/libxl/libxl_dm.c
> +++ b/tools/libxl/libxl_dm.c
> @@ -1629,17 +1629,25 @@ static int 
> libxl__build_device_model_args_new(libxl__gc *gc,
>                                       nics[i].colo_filter_redirector1_queue,
>                                       
> nics[i].colo_filter_redirector1_outdev));
>                      }
> +                    if (nics[i].colo_iothread) {
> +                        flexarray_append(dm_args, "-object");
> +                        flexarray_append(dm_args,
> +                           GCSPRINTF("iothread,id=%s",
> +                                     nics[i].colo_iothread));

This creates an iothread object, but it isn't used anywhere. What the
purpose of it?
Also, iothreads have options like "poll-grow", I don't know if you want
to have that configurable or just keep the default values, just
something to keep in mind.

> +                    }
>                      if (nics[i].colo_compare_pri_in &&
>                          nics[i].colo_compare_sec_in &&
>                          nics[i].colo_compare_out &&
> -                        nics[i].colo_compare_notify_dev) {
> +                        nics[i].colo_compare_notify_dev &&
> +                        nics[i].colo_compare_iothread) {
>                          flexarray_append(dm_args, "-object");
>                          flexarray_append(dm_args,
> -                           
> GCSPRINTF("colo-compare,id=c1,primary_in=%s,secondary_in=%s,outdev=%s,notify_dev=%s",
> +                           
> GCSPRINTF("colo-compare,id=c1,primary_in=%s,secondary_in=%s,outdev=%s,notify_dev=%s,iothread=%s",

So, now iothread are mandatory? It would also mean that libxl cann't use
QEMU older that 2.11, I think.
Can't QEMU creates an iothread automatically if none are provided?

Also, it looks like that if one of the colo-compare option is missing,
the colo-compare object isn't created at all with no warning for the
users of libxl.

What's the difference between `colo_iothread' and
`colo_compare_iothread' ?

If a user only as the choice of a iothread id, why not have libxl create
one on its own instead?

Thanks,

-- 
Anthony PERARD

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel