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: Wednesday, July 3, 2019 11:39 AM
> 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
> 
> On Tue, Jul 02, 2019 at 02:07:27PM +0000, Zhang, Chen wrote:
> > > On Mon, Jun 10, 2019 at 04:33:36PM +0800, Zhang Chen wrote:
> > > > 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".
> >
> > >
> > > > +                    }
> > > >                      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,outd
> > > > + ev=%
> > > > + 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.
> 
> What's a "colo-compare module"? Is it something outside of QEMU?
> If not, then I don't think users of libxl needs to provide a name for this 
> iothread.
> Instead of adding "colo*_iothread" could you do something like the following?
> 
> char *colo_compare_iothread_id = GCSPRINTF("colo-compare-iothread-%d", i);
> flexarray_append_pair(dm_args, "-object",
>                       GCSPRINTF("iothread,id=%s", colo_compare_iothread_id); 
> // then
> use `colo_compare_iothread_id' when generating the // "colo-compare" option
> list:
> flexarray_append(dm_args, GCSPRINTF("colo-compare,...,iothread=%s",
>                                     ..., colo_compare_iothread_id));
> 
> I think that will be more than enough for now. iothread can't be reused by
> something else that libxl create, and there can be only one "colo-compare"
> object in QEMU because the id is hard-coded in libxl.
> 
> What do you thing?
> 
> (I'm trying to make using COLO with libxl a little bit easier. libxl can be 
> use to
> hide all the glory details of a QEMU command line.)

Hi Anthony,

Yes, the colo-compare module is a part of Qemu.
Your idea is good for now, easy to use, I can change it in next version.

Thanks
Zhang Chen

> 
> Thanks,
> 
> --
> Anthony PERARD

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