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

[Anthony PERARD <anthony.perard@xxxxxxxxxx>]

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

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

Thanks,

-- 
Anthony PERARD

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