| Message ID | 20240820080416.323725-1-a-singh21@ti.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | [v2] tools/helpers/init-dom0less: fix vcpu availability | expand |
On Tue, Aug 20, 2024 at 01:34:17PM +0530, Amneesh Singh wrote: > diff --git a/tools/helpers/init-dom0less.c b/tools/helpers/init-dom0less.c > index fee9345..722a5af 100644 > --- a/tools/helpers/init-dom0less.c > +++ b/tools/helpers/init-dom0less.c > @@ -167,15 +167,20 @@ retry_transaction: > /* /domain */ > if (!do_xs_write_dom(xsh, t, domid, "vm", vm_val_str)) goto err; > if (!do_xs_write_dom(xsh, t, domid, "name", dom_name_str)) goto err; > - if (!do_xs_write_dom(xsh, t, domid, "cpu", "")) goto err; You should probably keep this node even if "*/availability" isn't going to be written. It might be useful for watching everything under the "cpu" node. (libxl create this node independently from all the other "availability" sub-nodes.) > @@ -330,14 +336,24 @@ int main(int argc, char **argv) > > for (i = 0; i < nb_vm; i++) { > domid_t domid = info[i].domid; > + libxl_vcpuinfo *vcpuinfo; > + int nb_vcpu = 0, nr_cpus = 0; > + > > /* Don't need to check for Dom0 */ > if (!domid) > continue; > > + vcpuinfo = libxl_list_vcpu(ctx, domid, &nb_vcpu, &nr_cpus); > + > + if (!vcpuinfo) { > + fprintf(stderr, "libxl_list_vcpu failed.\n"); > + nb_vcpu = 0; Is there any value to keep going if libxl_list_vcpu() fails? Or is the reasoning is that cpu/*/availability was broken before, so it's not important enough to stop init-dom0less? Thanks,
On 16:24-20240820, Anthony PERARD wrote: > On Tue, Aug 20, 2024 at 01:34:17PM +0530, Amneesh Singh wrote: > > diff --git a/tools/helpers/init-dom0less.c b/tools/helpers/init-dom0less.c > > index fee9345..722a5af 100644 > > --- a/tools/helpers/init-dom0less.c > > +++ b/tools/helpers/init-dom0less.c > > @@ -167,15 +167,20 @@ retry_transaction: > > /* /domain */ > > if (!do_xs_write_dom(xsh, t, domid, "vm", vm_val_str)) goto err; > > if (!do_xs_write_dom(xsh, t, domid, "name", dom_name_str)) goto err; > > - if (!do_xs_write_dom(xsh, t, domid, "cpu", "")) goto err; > > You should probably keep this node even if "*/availability" isn't going > to be written. It might be useful for watching everything under the > "cpu" node. (libxl create this node independently from all the other > "availability" sub-nodes.) Gotcha, didn't know that. Guess that can be kept the way it was then. Thanks for pointing it out. > > > @@ -330,14 +336,24 @@ int main(int argc, char **argv) > > > > for (i = 0; i < nb_vm; i++) { > > domid_t domid = info[i].domid; > > + libxl_vcpuinfo *vcpuinfo; > > + int nb_vcpu = 0, nr_cpus = 0; > > + > > > > /* Don't need to check for Dom0 */ > > if (!domid) > > continue; > > > > + vcpuinfo = libxl_list_vcpu(ctx, domid, &nb_vcpu, &nr_cpus); > > + > > + if (!vcpuinfo) { > > + fprintf(stderr, "libxl_list_vcpu failed.\n"); > > + nb_vcpu = 0; > > Is there any value to keep going if libxl_list_vcpu() fails? > Or is the reasoning is that cpu/*/availability was broken before, so > it's not important enough to stop init-dom0less? Yes, so missing cpu/*/availability nodes would mean we cannot pin/remove/add vcpus using xenlight I believe. However, we can still hotplug other stuff like net or block devices. In fact, I was doing exactly this when cpu/*/availability was broken. > > > Thanks, > > -- > > Anthony Perard | Vates XCP-ng Developer > > XCP-ng & Xen Orchestra - Vates solutions > > web: https://urldefense.com/v3/__https://vates.tech__;!!G3vK!TgBV3TUl158hw6AKLTg3uYRP3PP-1Ku8uVFaT9lFV46NIVCn9kL-y82D2xz1j_iUwo58t8yr3hwYJBV9GAMS5zaFOSA$ > Regards Amneesh
On Wed, Aug 21, 2024 at 11:08:59AM +0530, Amneesh Singh wrote: > On 16:24-20240820, Anthony PERARD wrote: > > On Tue, Aug 20, 2024 at 01:34:17PM +0530, Amneesh Singh wrote: > > > @@ -330,14 +336,24 @@ int main(int argc, char **argv) > > > > > > for (i = 0; i < nb_vm; i++) { > > > domid_t domid = info[i].domid; > > > + libxl_vcpuinfo *vcpuinfo; > > > + int nb_vcpu = 0, nr_cpus = 0; > > > + > > > > > > /* Don't need to check for Dom0 */ > > > if (!domid) > > > continue; > > > > > > + vcpuinfo = libxl_list_vcpu(ctx, domid, &nb_vcpu, &nr_cpus); > > > + > > > + if (!vcpuinfo) { > > > + fprintf(stderr, "libxl_list_vcpu failed.\n"); > > > + nb_vcpu = 0; > > > > Is there any value to keep going if libxl_list_vcpu() fails? > > Or is the reasoning is that cpu/*/availability was broken before, so > > it's not important enough to stop init-dom0less? > Yes, so missing cpu/*/availability nodes would mean we cannot > pin/remove/add vcpus using xenlight I believe. However, we can still > hotplug other stuff like net or block devices. In fact, I was doing > exactly this when cpu/*/availability was broken. Without the "availability" nodes, it probably mean that guest (probably PV ones) will just use all available CPUs, it seems that Linux is doing that, and only disable CPUs that are explicitly marked as "offline" via that node. But I guess it's ok. Thanks,
diff --git a/tools/helpers/init-dom0less.c b/tools/helpers/init-dom0less.c index fee9345..722a5af 100644 --- a/tools/helpers/init-dom0less.c +++ b/tools/helpers/init-dom0less.c @@ -99,8 +99,8 @@ static bool do_xs_write_vm(struct xs_handle *xsh, xs_transaction_t t, * domain started by xl/libxl. */ static int create_xenstore(struct xs_handle *xsh, - libxl_dominfo *info, libxl_uuid uuid, - evtchn_port_t xenstore_port) + libxl_dominfo *info, libxl_vcpuinfo *vcpuinfo, + libxl_uuid uuid, evtchn_port_t xenstore_port) { domid_t domid; unsigned int i; @@ -167,15 +167,20 @@ retry_transaction: /* /domain */ if (!do_xs_write_dom(xsh, t, domid, "vm", vm_val_str)) goto err; if (!do_xs_write_dom(xsh, t, domid, "name", dom_name_str)) goto err; - if (!do_xs_write_dom(xsh, t, domid, "cpu", "")) goto err; - for (i = 0; i < info->vcpu_max_id; i++) { - rc = snprintf(cpu_str, STR_MAX_LENGTH, "cpu/%u/availability/", i); - if (rc < 0 || rc >= STR_MAX_LENGTH) - goto err; - rc = -EIO; - if (!do_xs_write_dom(xsh, t, domid, cpu_str, - (info->cpupool & (1 << i)) ? "online" : "offline")) - goto err; + + if (!vcpuinfo) { + fprintf(stderr, "vcpu information unavailable, proceeding without it\n"); + } else { + if (!do_xs_write_dom(xsh, t, domid, "cpu", "")) goto err; + for (i = 0; i <= info->vcpu_max_id; i++) { + rc = snprintf(cpu_str, STR_MAX_LENGTH, "cpu/%u/availability", i); + if (rc < 0 || rc >= STR_MAX_LENGTH) + goto err; + rc = -EIO; + if (!do_xs_write_dom(xsh, t, domid, cpu_str, + vcpuinfo[i].online ? "online" : "offline")) + goto err; + } } if (!do_xs_write_dom(xsh, t, domid, "memory", "")) goto err; @@ -225,7 +230,8 @@ err: static int init_domain(struct xs_handle *xsh, struct xc_interface_core *xch, xenforeignmemory_handle *xfh, - libxl_dominfo *info) + libxl_dominfo *info, + libxl_vcpuinfo *vcpuinfo) { libxl_uuid uuid; uint64_t xenstore_evtchn, xenstore_pfn; @@ -278,7 +284,7 @@ static int init_domain(struct xs_handle *xsh, if (rc < 0) return rc; - rc = create_xenstore(xsh, info, uuid, xenstore_evtchn); + rc = create_xenstore(xsh, info, vcpuinfo, uuid, xenstore_evtchn); if (rc) err(1, "writing to xenstore"); @@ -330,14 +336,24 @@ int main(int argc, char **argv) for (i = 0; i < nb_vm; i++) { domid_t domid = info[i].domid; + libxl_vcpuinfo *vcpuinfo; + int nb_vcpu = 0, nr_cpus = 0; + /* Don't need to check for Dom0 */ if (!domid) continue; + vcpuinfo = libxl_list_vcpu(ctx, domid, &nb_vcpu, &nr_cpus); + + if (!vcpuinfo) { + fprintf(stderr, "libxl_list_vcpu failed.\n"); + nb_vcpu = 0; + } + printf("Checking domid: %u\n", domid); if (!domain_exists(xsh, domid)) { - rc = init_domain(xsh, xch, xfh, &info[i]); + rc = init_domain(xsh, xch, xfh, &info[i], vcpuinfo); if (rc < 0) { fprintf(stderr, "init_domain failed.\n"); goto out; @@ -345,6 +361,8 @@ int main(int argc, char **argv) } else { printf("Domain %u has already been initialized\n", domid); } + + libxl_vcpuinfo_list_free(vcpuinfo, nb_vcpu); } out: libxl_dominfo_list_free(info, nb_vm);
Currently, writing at cpu/<cpu>/availability in xenstore fails for a couple of reasons: a trailing slash in the path and the fact that cpupool isn't a bitmap but the cpupool id. This patch fixes this by just getting libxl_vcpuinfo for each dom0less domain. Signed-off-by: Amneesh Singh <a-singh21@ti.com> --- tools/helpers/init-dom0less.c | 46 ++++++++++++++++++++++++----------- 1 file changed, 32 insertions(+), 14 deletions(-) --- v1 -> v2: - check for libxl_list_vcpu failure - skip writing cpu availability to store if the above failure occurs