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

["Zhang, Chen" <chen.zhang@xxxxxxxxx>]

> -----Original Message-----
> From: Anthony PERARD [mailto:anthony.perard@xxxxxxxxxx]
> Sent: Tuesday, July 2, 2019 7:52 PM
> To: Zhang, Chen <chen.zhang@xxxxxxxxx>
> Cc: Ian Jackson <ian.jackson@xxxxxxxxxxxxx>; Wei Liu <wl@xxxxxxx>; xen-
> devel@xxxxxxxxxxxxxxxxxxxx; Zhang Chen <zhangckid@xxxxxxxxx>
> Subject: Re: [Xen-devel] [PATCH] tools/libxl: Add iothread support for COLO
> 
> Hi,
> 
> Thanks for the patch, I've got plenty of question.

OK~ Very happy to discuss about that.

> 
> 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?

No, colo compare use the iothread by the "colo_compare_iothread".

> 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.

Keep default.

> 
> > +                    }
> >                      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?

In current COLO design, iothread are mandatory, it's from Qemu maintainer's 
comments to make
Colo-compare thread independent with Qemu main loop for better performance.
I think libxl can use upstream Qemu to run COLO.
Qemu can't creates iothread automatically, because it needs user input ID for 
iothread, then it will be used to other qemu module(need the ID).

> 
> 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' ?
> 

"Colo_iothread" is iothread ID, "colo_compare_iothread" is colo compare used 
iothread's ID.
In current COLO case, two ID must same.
But for future or other case, it can different(maybe RX/TX use two iothread in 
future). 

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

Because user also need input the iothread ID to colo-compare module.
So we integrated the job here.

Thanks
Zhang Chen

> 
> Thanks,
> 
> --
> Anthony PERARD

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