| Message ID | 20190726064300.27530-1-chen.zhang@intel.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [V2] tools/libxl: Add iothread support for COLO | expand |
On Fri, Jul 26, 2019 at 02:43:00PM +0800, Zhang Chen wrote: > From: Zhang Chen <chen.zhang@intel.com> > > Xen COLO and KVM COLO shared lots of code in Qemu. > KVM COLO has added the iothread support, so we add it on Xen. It would be useful to expand the comment of the commit and explain why the change is required. I would add the following: The colo-compare object in QEMU now requires an `iothread' property since QEMU 2.11. > Detail: > https://wiki.qemu.org/Features/COLO > > Signed-off-by: Zhang Chen <chen.zhang@intel.com> > --- > diff --git a/tools/libxl/libxl_types.idl b/tools/libxl/libxl_types.idl > index b61399ce36..eda958eb4b 100644 > --- a/tools/libxl/libxl_types.idl > +++ b/tools/libxl/libxl_types.idl > @@ -727,6 +727,7 @@ libxl_device_nic = Struct("device_nic", [ > ("colo_filter_redirector1_queue", string), > ("colo_filter_redirector1_indev", string), > ("colo_filter_redirector1_outdev", string), > + ("colo_iothread", string), > ("colo_compare_pri_in", string), > ("colo_compare_sec_in", string), > ("colo_compare_out", string), > diff --git a/tools/xl/xl_parse.c b/tools/xl/xl_parse.c > index e105bda2bb..0b8189f375 100644 > --- a/tools/xl/xl_parse.c > +++ b/tools/xl/xl_parse.c > @@ -521,6 +521,8 @@ int parse_nic_config(libxl_device_nic *nic, XLU_Config **config, char *token) > replace_string(&nic->colo_filter_redirector1_indev, oparg); > } else if (MATCH_OPTION("colo_filter_redirector1_outdev", token, oparg)) { > replace_string(&nic->colo_filter_redirector1_outdev, oparg); > + } else if (MATCH_OPTION("colo_iothread", token, oparg)) { > + replace_string(&nic->colo_iothread, oparg); > } else if (MATCH_OPTION("colo_compare_pri_in", token, oparg)) { > replace_string(&nic->colo_compare_pri_in, oparg); > } else if (MATCH_OPTION("colo_compare_sec_in", token, oparg)) { What I had in mind while reviewing the v1 of the patch was to remove both `colo_iothread' and `colo_compare_iothread' from the libxl API and xl config option. I don't think there are useful. Why did you keep `colo_iothread'? Thanks,
> -----Original Message----- > From: Anthony PERARD [mailto:anthony.perard@citrix.com] > Sent: Friday, July 26, 2019 9:48 PM > To: Zhang, Chen <chen.zhang@intel.com> > Cc: Ian Jackson <ian.jackson@eu.citrix.com>; Wei Liu <wl@xen.org>; xen- > devel@lists.xenproject.org; Zhang Chen <zhangckid@gmail.com> > Subject: Re: [PATCH V2] tools/libxl: Add iothread support for COLO > > On Fri, Jul 26, 2019 at 02:43:00PM +0800, Zhang Chen wrote: > > From: Zhang Chen <chen.zhang@intel.com> > > > > Xen COLO and KVM COLO shared lots of code in Qemu. > > KVM COLO has added the iothread support, so we add it on Xen. > > It would be useful to expand the comment of the commit and explain why the > change is required. I would add the following: > > The colo-compare object in QEMU now requires an `iothread' property > since QEMU 2.11. > Make sense. I will add it in next version. > > Detail: > > https://wiki.qemu.org/Features/COLO > > > > Signed-off-by: Zhang Chen <chen.zhang@intel.com> > > --- > > > diff --git a/tools/libxl/libxl_types.idl b/tools/libxl/libxl_types.idl > > index b61399ce36..eda958eb4b 100644 > > --- a/tools/libxl/libxl_types.idl > > +++ b/tools/libxl/libxl_types.idl > > @@ -727,6 +727,7 @@ libxl_device_nic = Struct("device_nic", [ > > ("colo_filter_redirector1_queue", string), > > ("colo_filter_redirector1_indev", string), > > ("colo_filter_redirector1_outdev", string), > > + ("colo_iothread", string), > > ("colo_compare_pri_in", string), > > ("colo_compare_sec_in", string), > > ("colo_compare_out", string), > > diff --git a/tools/xl/xl_parse.c b/tools/xl/xl_parse.c index > > e105bda2bb..0b8189f375 100644 > > --- a/tools/xl/xl_parse.c > > +++ b/tools/xl/xl_parse.c > > @@ -521,6 +521,8 @@ int parse_nic_config(libxl_device_nic *nic, > XLU_Config **config, char *token) > > replace_string(&nic->colo_filter_redirector1_indev, oparg); > > } else if (MATCH_OPTION("colo_filter_redirector1_outdev", token, oparg)) > { > > replace_string(&nic->colo_filter_redirector1_outdev, oparg); > > + } else if (MATCH_OPTION("colo_iothread", token, oparg)) { > > + replace_string(&nic->colo_iothread, oparg); > > } else if (MATCH_OPTION("colo_compare_pri_in", token, oparg)) { > > replace_string(&nic->colo_compare_pri_in, oparg); > > } else if (MATCH_OPTION("colo_compare_sec_in", token, oparg)) { > > What I had in mind while reviewing the v1 of the patch was to remove both > `colo_iothread' and `colo_compare_iothread' from the libxl API and xl config > option. I don't think there are useful. Why did you keep `colo_iothread'? Oh, it looks I misunderstood your means. Do you think we just need hard code the iothread name here? For example the "iothread-1"? Thanks Zhang Chen > > Thanks, > > -- > Anthony PERARD
On Fri, Jul 26, 2019 at 02:18:42PM +0000, Zhang, Chen wrote: > > > -----Original Message----- > > From: Anthony PERARD [mailto:anthony.perard@citrix.com] > > Sent: Friday, July 26, 2019 9:48 PM > > > > On Fri, Jul 26, 2019 at 02:43:00PM +0800, Zhang Chen wrote: > > What I had in mind while reviewing the v1 of the patch was to remove both > > `colo_iothread' and `colo_compare_iothread' from the libxl API and xl config > > option. I don't think there are useful. Why did you keep `colo_iothread'? > > Oh, it looks I misunderstood your means. > Do you think we just need hard code the iothread name here? > For example the "iothread-1"? Yes, it would be better to hard code for now, but with a name which better describe where the iothread is used, how about "colo-compare-iothread-1" ? Thanks,
> -----Original Message----- > From: Anthony PERARD [mailto:anthony.perard@citrix.com] > Sent: Friday, July 26, 2019 10:34 PM > To: Zhang, Chen <chen.zhang@intel.com> > Cc: Ian Jackson <ian.jackson@eu.citrix.com>; Wei Liu <wl@xen.org>; xen- > devel@lists.xenproject.org; Zhang Chen <zhangckid@gmail.com> > Subject: Re: [PATCH V2] tools/libxl: Add iothread support for COLO > > On Fri, Jul 26, 2019 at 02:18:42PM +0000, Zhang, Chen wrote: > > > > > -----Original Message----- > > > From: Anthony PERARD [mailto:anthony.perard@citrix.com] > > > Sent: Friday, July 26, 2019 9:48 PM > > > > > > On Fri, Jul 26, 2019 at 02:43:00PM +0800, Zhang Chen wrote: > > > What I had in mind while reviewing the v1 of the patch was to remove > > > both `colo_iothread' and `colo_compare_iothread' from the libxl API > > > and xl config option. I don't think there are useful. Why did you keep > `colo_iothread'? > > > > Oh, it looks I misunderstood your means. > > Do you think we just need hard code the iothread name here? > > For example the "iothread-1"? > > Yes, it would be better to hard code for now, but with a name which better > describe where the iothread is used, how about "colo-compare-iothread-1" ? > It is fine for me. Thanks your comments. Thanks Zhang Chen > Thanks, > > -- > Anthony PERARD
diff --git a/tools/libxl/libxl_dm.c b/tools/libxl/libxl_dm.c index f4fc96415d..95cb30f9e6 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)); + } 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_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", 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_iothread)); } } ioemu_nics++; diff --git a/tools/libxl/libxl_types.idl b/tools/libxl/libxl_types.idl index b61399ce36..eda958eb4b 100644 --- a/tools/libxl/libxl_types.idl +++ b/tools/libxl/libxl_types.idl @@ -727,6 +727,7 @@ libxl_device_nic = Struct("device_nic", [ ("colo_filter_redirector1_queue", string), ("colo_filter_redirector1_indev", string), ("colo_filter_redirector1_outdev", string), + ("colo_iothread", string), ("colo_compare_pri_in", string), ("colo_compare_sec_in", string), ("colo_compare_out", string), diff --git a/tools/xl/xl_parse.c b/tools/xl/xl_parse.c index e105bda2bb..0b8189f375 100644 --- a/tools/xl/xl_parse.c +++ b/tools/xl/xl_parse.c @@ -521,6 +521,8 @@ int parse_nic_config(libxl_device_nic *nic, XLU_Config **config, char *token) replace_string(&nic->colo_filter_redirector1_indev, oparg); } else if (MATCH_OPTION("colo_filter_redirector1_outdev", token, oparg)) { replace_string(&nic->colo_filter_redirector1_outdev, oparg); + } else if (MATCH_OPTION("colo_iothread", token, oparg)) { + replace_string(&nic->colo_iothread, oparg); } else if (MATCH_OPTION("colo_compare_pri_in", token, oparg)) { replace_string(&nic->colo_compare_pri_in, oparg); } else if (MATCH_OPTION("colo_compare_sec_in", token, oparg)) {