| Message ID | 20220215101551.23101-2-luca.fancellu@arm.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Boot time cpupools | expand |
On 15.02.22 11:15, Luca Fancellu wrote: > With the introduction of boot time cpupools, Xen can create many > different cpupools at boot time other than cpupool with id 0. > > Since these newly created cpupools can't have an > entry in Xenstore, create the entry using xen-init-dom0 > helper with the usual convention: Pool-<cpupool id>. > > Given the change, remove the check for poolid == 0 from > libxl_cpupoolid_to_name(...). > > Signed-off-by: Luca Fancellu <luca.fancellu@arm.com> > --- > tools/helpers/xen-init-dom0.c | 26 +++++++++++++++++++++++++- > tools/libs/light/libxl_utils.c | 3 +-- > 2 files changed, 26 insertions(+), 3 deletions(-) > > diff --git a/tools/helpers/xen-init-dom0.c b/tools/helpers/xen-init-dom0.c > index c99224a4b607..3539f56faeb0 100644 > --- a/tools/helpers/xen-init-dom0.c > +++ b/tools/helpers/xen-init-dom0.c > @@ -43,7 +43,10 @@ int main(int argc, char **argv) > int rc; > struct xs_handle *xsh = NULL; > xc_interface *xch = NULL; > - char *domname_string = NULL, *domid_string = NULL; > + char *domname_string = NULL, *domid_string = NULL, *pool_string = NULL; pool_string seems to be unused. > + char pool_path[strlen("/local/pool") + 12], pool_name[strlen("Pool-") + 5]; I don't like that. Why don't you use pointers and ... > + xc_cpupoolinfo_t *xcinfo; > + unsigned int pool_id = 0; > libxl_uuid uuid; > > /* Accept 0 or 1 argument */ > @@ -114,6 +117,27 @@ int main(int argc, char **argv) > goto out; > } > > + /* Create an entry in xenstore for each cpupool on the system */ > + do { > + xcinfo = xc_cpupool_getinfo(xch, pool_id); > + if (xcinfo != NULL) { > + if (xcinfo->cpupool_id != pool_id) > + pool_id = xcinfo->cpupool_id; > + snprintf(pool_path, sizeof(pool_path), "/local/pool/%d/name", > + pool_id); > + snprintf(pool_name, sizeof(pool_name), "Pool-%d", pool_id); ... use asprintf() here for allocating the strings in the needed size? > + pool_id++; > + if (!xs_write(xsh, XBT_NULL, pool_path, pool_name, > + strlen(pool_name))) { > + fprintf(stderr, "cannot set pool name\n"); > + rc = 1; > + } > + xc_cpupool_infofree(xch, xcinfo); > + if (rc) > + goto out; Moving the call of xc_cpupool_infofree() ahead of the call of xs_write() would drop the need for this last if statement, as you could add the goto to the upper if. > + } > + } while(xcinfo != NULL); > + With doing all of this for being able to assign other domains created at boot to cpupools, shouldn't you add names for other domains than dom0 here, too? Juergen
> On 15 Feb 2022, at 10:33, Juergen Gross <jgross@suse.com> wrote: > > On 15.02.22 11:15, Luca Fancellu wrote: >> With the introduction of boot time cpupools, Xen can create many >> different cpupools at boot time other than cpupool with id 0. >> Since these newly created cpupools can't have an >> entry in Xenstore, create the entry using xen-init-dom0 >> helper with the usual convention: Pool-<cpupool id>. >> Given the change, remove the check for poolid == 0 from >> libxl_cpupoolid_to_name(...). >> Signed-off-by: Luca Fancellu <luca.fancellu@arm.com> >> --- >> tools/helpers/xen-init-dom0.c | 26 +++++++++++++++++++++++++- >> tools/libs/light/libxl_utils.c | 3 +-- >> 2 files changed, 26 insertions(+), 3 deletions(-) >> diff --git a/tools/helpers/xen-init-dom0.c b/tools/helpers/xen-init-dom0.c >> index c99224a4b607..3539f56faeb0 100644 >> --- a/tools/helpers/xen-init-dom0.c >> +++ b/tools/helpers/xen-init-dom0.c >> @@ -43,7 +43,10 @@ int main(int argc, char **argv) >> int rc; >> struct xs_handle *xsh = NULL; >> xc_interface *xch = NULL; >> - char *domname_string = NULL, *domid_string = NULL; >> + char *domname_string = NULL, *domid_string = NULL, *pool_string = NULL; Hi Juergen, > > pool_string seems to be unused. I will remove it > >> + char pool_path[strlen("/local/pool") + 12], pool_name[strlen("Pool-") + 5]; > > I don't like that. Why don't you use pointers and ... > >> + xc_cpupoolinfo_t *xcinfo; >> + unsigned int pool_id = 0; >> libxl_uuid uuid; >> /* Accept 0 or 1 argument */ >> @@ -114,6 +117,27 @@ int main(int argc, char **argv) >> goto out; >> } >> + /* Create an entry in xenstore for each cpupool on the system */ >> + do { >> + xcinfo = xc_cpupool_getinfo(xch, pool_id); >> + if (xcinfo != NULL) { >> + if (xcinfo->cpupool_id != pool_id) >> + pool_id = xcinfo->cpupool_id; >> + snprintf(pool_path, sizeof(pool_path), "/local/pool/%d/name", >> + pool_id); >> + snprintf(pool_name, sizeof(pool_name), "Pool-%d", pool_id); > > ... use asprintf() here for allocating the strings in the needed size? Why would you like more this approach? I was trying to avoid allocation/free operations in a loop that would need also more code to check cases in which memory is not allocated. Instead with the buffers in the stack we don’t have problems. > >> + pool_id++; >> + if (!xs_write(xsh, XBT_NULL, pool_path, pool_name, >> + strlen(pool_name))) { >> + fprintf(stderr, "cannot set pool name\n"); >> + rc = 1; >> + } >> + xc_cpupool_infofree(xch, xcinfo); >> + if (rc) >> + goto out; > > Moving the call of xc_cpupool_infofree() ahead of the call of xs_write() > would drop the need for this last if statement, as you could add the > goto to the upper if. Yes you are right, it would simplify the code > >> + } >> + } while(xcinfo != NULL); >> + > > With doing all of this for being able to assign other domains created > at boot to cpupools, shouldn't you add names for other domains than dom0 > here, too? This serie is more about cpupools, maybe I can do that in another commit out of this serie. Thanks for your review. Cheers, Luca > > > Juergen > <OpenPGP_0xB0DE9DD628BF132F.asc>
On 15.02.22 18:48, Luca Fancellu wrote: > > >> On 15 Feb 2022, at 10:33, Juergen Gross <jgross@suse.com> wrote: >> >> On 15.02.22 11:15, Luca Fancellu wrote: >>> With the introduction of boot time cpupools, Xen can create many >>> different cpupools at boot time other than cpupool with id 0. >>> Since these newly created cpupools can't have an >>> entry in Xenstore, create the entry using xen-init-dom0 >>> helper with the usual convention: Pool-<cpupool id>. >>> Given the change, remove the check for poolid == 0 from >>> libxl_cpupoolid_to_name(...). >>> Signed-off-by: Luca Fancellu <luca.fancellu@arm.com> >>> --- >>> tools/helpers/xen-init-dom0.c | 26 +++++++++++++++++++++++++- >>> tools/libs/light/libxl_utils.c | 3 +-- >>> 2 files changed, 26 insertions(+), 3 deletions(-) >>> diff --git a/tools/helpers/xen-init-dom0.c b/tools/helpers/xen-init-dom0.c >>> index c99224a4b607..3539f56faeb0 100644 >>> --- a/tools/helpers/xen-init-dom0.c >>> +++ b/tools/helpers/xen-init-dom0.c >>> @@ -43,7 +43,10 @@ int main(int argc, char **argv) >>> int rc; >>> struct xs_handle *xsh = NULL; >>> xc_interface *xch = NULL; >>> - char *domname_string = NULL, *domid_string = NULL; >>> + char *domname_string = NULL, *domid_string = NULL, *pool_string = NULL; > > Hi Juergen, > >> >> pool_string seems to be unused. > > I will remove it > >> >>> + char pool_path[strlen("/local/pool") + 12], pool_name[strlen("Pool-") + 5]; >> >> I don't like that. Why don't you use pointers and ... >> >>> + xc_cpupoolinfo_t *xcinfo; >>> + unsigned int pool_id = 0; >>> libxl_uuid uuid; >>> /* Accept 0 or 1 argument */ >>> @@ -114,6 +117,27 @@ int main(int argc, char **argv) >>> goto out; >>> } >>> + /* Create an entry in xenstore for each cpupool on the system */ >>> + do { >>> + xcinfo = xc_cpupool_getinfo(xch, pool_id); >>> + if (xcinfo != NULL) { >>> + if (xcinfo->cpupool_id != pool_id) >>> + pool_id = xcinfo->cpupool_id; >>> + snprintf(pool_path, sizeof(pool_path), "/local/pool/%d/name", >>> + pool_id); >>> + snprintf(pool_name, sizeof(pool_name), "Pool-%d", pool_id); >> >> ... use asprintf() here for allocating the strings in the needed size? > > Why would you like more this approach? I was trying to avoid allocation/free > operations in a loop that would need also more code to check cases in which > memory is not allocated. Instead with the buffers in the stack we don’t have problems. My main concerns are the usage of strlen() for sizing an on-stack array, the duplication of the format strings (once in the arrays definition and once in the snprintf()), and the mixture of strlen() and constants for sizing the arrays. There are actually some errors in your approach for sizing the arrays, showing how fragile your solution is: you are allowing a "positive integer number" for a cpupool-id, which could easily need 10 digits, while your arrays allow only for 5 or 4 digits, depending on the array. And doing the two asprintf() calls and then checking that both arrays are not NULL isn't that much code. BTW, your approach is missing the test that the arrays have been large enough. The performance of that loop shouldn't be that critical that a few additional microseconds really hurt, especially as I don't think any use case will exceed single digit loop iterations. > >> >>> + pool_id++; >>> + if (!xs_write(xsh, XBT_NULL, pool_path, pool_name, >>> + strlen(pool_name))) { >>> + fprintf(stderr, "cannot set pool name\n"); >>> + rc = 1; >>> + } >>> + xc_cpupool_infofree(xch, xcinfo); >>> + if (rc) >>> + goto out; >> >> Moving the call of xc_cpupool_infofree() ahead of the call of xs_write() >> would drop the need for this last if statement, as you could add the >> goto to the upper if. > > Yes you are right, it would simplify the code > >> >>> + } >>> + } while(xcinfo != NULL); >>> + >> >> With doing all of this for being able to assign other domains created >> at boot to cpupools, shouldn't you add names for other domains than dom0 >> here, too? > > This serie is more about cpupools, maybe I can do that in another commit out > of this serie. Fine with me. Juergen
> On 16 Feb 2022, at 06:13, Juergen Gross <jgross@suse.com> wrote: > > On 15.02.22 18:48, Luca Fancellu wrote: >>> On 15 Feb 2022, at 10:33, Juergen Gross <jgross@suse.com> wrote: >>> >>> On 15.02.22 11:15, Luca Fancellu wrote: >>>> With the introduction of boot time cpupools, Xen can create many >>>> different cpupools at boot time other than cpupool with id 0. >>>> Since these newly created cpupools can't have an >>>> entry in Xenstore, create the entry using xen-init-dom0 >>>> helper with the usual convention: Pool-<cpupool id>. >>>> Given the change, remove the check for poolid == 0 from >>>> libxl_cpupoolid_to_name(...). >>>> Signed-off-by: Luca Fancellu <luca.fancellu@arm.com> >>>> --- >>>> tools/helpers/xen-init-dom0.c | 26 +++++++++++++++++++++++++- >>>> tools/libs/light/libxl_utils.c | 3 +-- >>>> 2 files changed, 26 insertions(+), 3 deletions(-) >>>> diff --git a/tools/helpers/xen-init-dom0.c b/tools/helpers/xen-init-dom0.c >>>> index c99224a4b607..3539f56faeb0 100644 >>>> --- a/tools/helpers/xen-init-dom0.c >>>> +++ b/tools/helpers/xen-init-dom0.c >>>> @@ -43,7 +43,10 @@ int main(int argc, char **argv) >>>> int rc; >>>> struct xs_handle *xsh = NULL; >>>> xc_interface *xch = NULL; >>>> - char *domname_string = NULL, *domid_string = NULL; >>>> + char *domname_string = NULL, *domid_string = NULL, *pool_string = NULL; >> Hi Juergen, >>> >>> pool_string seems to be unused. >> I will remove it >>> >>>> + char pool_path[strlen("/local/pool") + 12], pool_name[strlen("Pool-") + 5]; >>> >>> I don't like that. Why don't you use pointers and ... >>> >>>> + xc_cpupoolinfo_t *xcinfo; >>>> + unsigned int pool_id = 0; >>>> libxl_uuid uuid; >>>> /* Accept 0 or 1 argument */ >>>> @@ -114,6 +117,27 @@ int main(int argc, char **argv) >>>> goto out; >>>> } >>>> + /* Create an entry in xenstore for each cpupool on the system */ >>>> + do { >>>> + xcinfo = xc_cpupool_getinfo(xch, pool_id); >>>> + if (xcinfo != NULL) { >>>> + if (xcinfo->cpupool_id != pool_id) >>>> + pool_id = xcinfo->cpupool_id; >>>> + snprintf(pool_path, sizeof(pool_path), "/local/pool/%d/name", >>>> + pool_id); >>>> + snprintf(pool_name, sizeof(pool_name), "Pool-%d", pool_id); >>> >>> ... use asprintf() here for allocating the strings in the needed size? >> Why would you like more this approach? I was trying to avoid allocation/free >> operations in a loop that would need also more code to check cases in which >> memory is not allocated. Instead with the buffers in the stack we don’t have problems. > > My main concerns are the usage of strlen() for sizing an on-stack array, > the duplication of the format strings (once in the arrays definition and > once in the snprintf()), and the mixture of strlen() and constants for > sizing the arrays. > > There are actually some errors in your approach for sizing the arrays, > showing how fragile your solution is: you are allowing a "positive > integer number" for a cpupool-id, which could easily need 10 digits, > while your arrays allow only for 5 or 4 digits, depending on the array. > > And doing the two asprintf() calls and then checking that both arrays > are not NULL isn't that much code. BTW, your approach is missing the > test that the arrays have been large enough. > > The performance of that loop shouldn't be that critical that a few > additional microseconds really hurt, especially as I don't think any > use case will exceed single digit loop iterations. Hi Juergen, Thank you for your explanation, totally makes sense. I took inspiration from libxl_cpupoolid_to_name in libxl_utils.c writing this code but I see the limitation now. I will change it to use asprintf(). Cheers, Luca > >>> >>>> + pool_id++; >>>> + if (!xs_write(xsh, XBT_NULL, pool_path, pool_name, >>>> + strlen(pool_name))) { >>>> + fprintf(stderr, "cannot set pool name\n"); >>>> + rc = 1; >>>> + } >>>> + xc_cpupool_infofree(xch, xcinfo); >>>> + if (rc) >>>> + goto out; >>> >>> Moving the call of xc_cpupool_infofree() ahead of the call of xs_write() >>> would drop the need for this last if statement, as you could add the >>> goto to the upper if. >> Yes you are right, it would simplify the code >>> >>>> + } >>>> + } while(xcinfo != NULL); >>>> + >>> >>> With doing all of this for being able to assign other domains created >>> at boot to cpupools, shouldn't you add names for other domains than dom0 >>> here, too? >> This serie is more about cpupools, maybe I can do that in another commit out >> of this serie. > > Fine with me. > > > Juergen > <OpenPGP_0xB0DE9DD628BF132F.asc>
diff --git a/tools/helpers/xen-init-dom0.c b/tools/helpers/xen-init-dom0.c index c99224a4b607..3539f56faeb0 100644 --- a/tools/helpers/xen-init-dom0.c +++ b/tools/helpers/xen-init-dom0.c @@ -43,7 +43,10 @@ int main(int argc, char **argv) int rc; struct xs_handle *xsh = NULL; xc_interface *xch = NULL; - char *domname_string = NULL, *domid_string = NULL; + char *domname_string = NULL, *domid_string = NULL, *pool_string = NULL; + char pool_path[strlen("/local/pool") + 12], pool_name[strlen("Pool-") + 5]; + xc_cpupoolinfo_t *xcinfo; + unsigned int pool_id = 0; libxl_uuid uuid; /* Accept 0 or 1 argument */ @@ -114,6 +117,27 @@ int main(int argc, char **argv) goto out; } + /* Create an entry in xenstore for each cpupool on the system */ + do { + xcinfo = xc_cpupool_getinfo(xch, pool_id); + if (xcinfo != NULL) { + if (xcinfo->cpupool_id != pool_id) + pool_id = xcinfo->cpupool_id; + snprintf(pool_path, sizeof(pool_path), "/local/pool/%d/name", + pool_id); + snprintf(pool_name, sizeof(pool_name), "Pool-%d", pool_id); + pool_id++; + if (!xs_write(xsh, XBT_NULL, pool_path, pool_name, + strlen(pool_name))) { + fprintf(stderr, "cannot set pool name\n"); + rc = 1; + } + xc_cpupool_infofree(xch, xcinfo); + if (rc) + goto out; + } + } while(xcinfo != NULL); + printf("Done setting up Dom0\n"); out: diff --git a/tools/libs/light/libxl_utils.c b/tools/libs/light/libxl_utils.c index b91c2cafa223..81780da3ff40 100644 --- a/tools/libs/light/libxl_utils.c +++ b/tools/libs/light/libxl_utils.c @@ -151,8 +151,7 @@ char *libxl_cpupoolid_to_name(libxl_ctx *ctx, uint32_t poolid) snprintf(path, sizeof(path), "/local/pool/%d/name", poolid); s = xs_read(ctx->xsh, XBT_NULL, path, &len); - if (!s && (poolid == 0)) - return strdup("Pool-0"); + return s; }
With the introduction of boot time cpupools, Xen can create many different cpupools at boot time other than cpupool with id 0. Since these newly created cpupools can't have an entry in Xenstore, create the entry using xen-init-dom0 helper with the usual convention: Pool-<cpupool id>. Given the change, remove the check for poolid == 0 from libxl_cpupoolid_to_name(...). Signed-off-by: Luca Fancellu <luca.fancellu@arm.com> --- tools/helpers/xen-init-dom0.c | 26 +++++++++++++++++++++++++- tools/libs/light/libxl_utils.c | 3 +-- 2 files changed, 26 insertions(+), 3 deletions(-)