Re: [libvirt] [PATCH v5 05/10] conf: Adjust the iothreadsched expectations
On Mon, Apr 27, 2015 at 11:27:09 -0400, John Ferlan wrote:
>
>
> On 04/27/2015 10:28 AM, Peter Krempa wrote:
> > On Fri, Apr 24, 2015 at 12:05:57 -0400, John Ferlan wrote:
> >> With iothreadid's allowing any 'id' value for an iothread_id, the
> >> iothreadsched code needs a slight adjustment to allow for "any"
> >> unsigned int value in order to create the bitmap of ids that will
> >> have scheduler adjustments. Adjusted the doc description as well.
> >>
> >> Signed-off-by: John Ferlan
> >> ---
> >> docs/formatdomain.html.in| 16
> >>
> >> src/conf/domain_conf.c | 2 +-
> >> .../qemuxml2argv-cputune-iothreadsched-toomuch.xml | 3 ++-
> >> 3 files changed, 15 insertions(+), 6 deletions(-)
> >>
> >> diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in
> >> index 7af6bd7..0767a2a 100644
> >> --- a/docs/formatdomain.html.in
> >> +++ b/docs/formatdomain.html.in
> >> @@ -691,10 +691,18 @@
> >> type (values batch, idle,
> >> fifo,
> >> rr) for particular vCPU/IOThread threads (based on
> >> vcpus and iothreads, leaving out
> >> -vcpus/iothreads sets the default). For
> >> -real-time schedulers (fifo, rr),
> >> priority must
> >> -be specified as well (and is ignored for non-real-time ones). The
> >> value
> >> -range for the priority depends on the host kernel (usually 1-99).
> >> +vcpus/iothreads sets the default). Valid
> >> +vcpus values start at 0 through one less than the
> >> +number of vCPU's defined for the domain. Valid
> >> iothreads
> >> +values are described in the iothreadids
> >> + >> href="#elementsIOThreadsAllocation">description.
> >> +If no iothreadids are defined, then libvirt numbers
> >> +IOThreads from 1 to the number of iothreads available
> >> +for the domain. For real-time schedulers (fifo,
> >> +rr), priority must real-time schedulers
> >> +(fifo, rr), priority must be specified
> >> as
> >> +well (and is ignored for non-real-time ones). The value range
> >> +for the priority depends on the host kernel (usually 1-99).
> >> Since 1.2.13
> >>
> >>
> >> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
> >> index 129908d..9d4c916 100644
> >> --- a/src/conf/domain_conf.c
> >> +++ b/src/conf/domain_conf.c
> >> @@ -14348,7 +14348,7 @@ virDomainDefParseXML(xmlDocPtr xml,
> >>
> >> for (i = 0; i < def->cputune.niothreadsched; i++) {
> >> if (virDomainThreadSchedParse(nodes[i],
> >> - 1, def->iothreads,
> >> + 1, UINT_MAX,
> >>"iothreads",
> >>&def->cputune.iothreadsched[i])
> >> < 0)
> >> goto error;
> >
> > I think this patch should also add code that checks that the provided
> > scheduler info is provided only for valid iothread IDs.
> >
>
> Yuck... I know you eschew inline diffs, but it's just easier if nothing
> else just to make progress:
I only dislike them when they are multi-hunk or generally too big to
review without compiling the code. This one is okay, small enough to see
what's going on.
>
> index b6a8129..7da94bb 100644
> --- a/src/conf/domain_conf.c
> +++ b/src/conf/domain_conf.c
> @@ -14348,12 +14348,24 @@ virDomainDefParseXML(xmlDocPtr xml,
> def->cputune.niothreadsched = n;
>
> for (i = 0; i < def->cputune.niothreadsched; i++) {
> +ssize_t pos = -1;
> +
> if (virDomainThreadSchedParse(nodes[i],
>1, UINT_MAX,
>"iothreads",
>&def->cputune.iothreadsched[i]) <
> 0)
> goto error;
>
> +while ((pos =
> virBitmapNextSetBit(def->cputune.iothreadsched[i].ids,
> + pos)) > -1) {
> +if (!virDomainIOThreadIDFind(def, pos)) {
> +virReportError(VIR_ERR_XML_DETAIL, "%s",
> + _("iothreadsched attribute 'iothreads' "
> + "uses undefined iothread ids"));
> +goto error;
Unfortunately we don't have the string containing the bitmap here.
It would make for a nice error message. Not worth changing though.
> +}
> +}
> +
> for (j = 0; j < i; j++) {
> if (virBitmapOverlaps(def->cputune.iothreadsched[i].ids,
>def->cputune.iothreadsched[j].ids)) {
>
>
>
>
>
ACK with the squash-in.
Peter
signature.asc
Description: Digital signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listin
Re: [libvirt] [PATCH v5 05/10] conf: Adjust the iothreadsched expectations
On 04/27/2015 10:28 AM, Peter Krempa wrote:
> On Fri, Apr 24, 2015 at 12:05:57 -0400, John Ferlan wrote:
>> With iothreadid's allowing any 'id' value for an iothread_id, the
>> iothreadsched code needs a slight adjustment to allow for "any"
>> unsigned int value in order to create the bitmap of ids that will
>> have scheduler adjustments. Adjusted the doc description as well.
>>
>> Signed-off-by: John Ferlan
>> ---
>> docs/formatdomain.html.in| 16
>>
>> src/conf/domain_conf.c | 2 +-
>> .../qemuxml2argv-cputune-iothreadsched-toomuch.xml | 3 ++-
>> 3 files changed, 15 insertions(+), 6 deletions(-)
>>
>> diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in
>> index 7af6bd7..0767a2a 100644
>> --- a/docs/formatdomain.html.in
>> +++ b/docs/formatdomain.html.in
>> @@ -691,10 +691,18 @@
>> type (values batch, idle,
>> fifo,
>> rr) for particular vCPU/IOThread threads (based on
>> vcpus and iothreads, leaving out
>> -vcpus/iothreads sets the default). For
>> -real-time schedulers (fifo, rr), priority
>> must
>> -be specified as well (and is ignored for non-real-time ones). The
>> value
>> -range for the priority depends on the host kernel (usually 1-99).
>> +vcpus/iothreads sets the default). Valid
>> +vcpus values start at 0 through one less than the
>> +number of vCPU's defined for the domain. Valid
>> iothreads
>> +values are described in the iothreadids
>> +description.
>> +If no iothreadids are defined, then libvirt numbers
>> +IOThreads from 1 to the number of iothreads available
>> +for the domain. For real-time schedulers (fifo,
>> +rr), priority must real-time schedulers
>> +(fifo, rr), priority must be specified as
>> +well (and is ignored for non-real-time ones). The value range
>> +for the priority depends on the host kernel (usually 1-99).
>> Since 1.2.13
>>
>>
>> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
>> index 129908d..9d4c916 100644
>> --- a/src/conf/domain_conf.c
>> +++ b/src/conf/domain_conf.c
>> @@ -14348,7 +14348,7 @@ virDomainDefParseXML(xmlDocPtr xml,
>>
>> for (i = 0; i < def->cputune.niothreadsched; i++) {
>> if (virDomainThreadSchedParse(nodes[i],
>> - 1, def->iothreads,
>> + 1, UINT_MAX,
>>"iothreads",
>>&def->cputune.iothreadsched[i]) <
>> 0)
>> goto error;
>
> I think this patch should also add code that checks that the provided
> scheduler info is provided only for valid iothread IDs.
>
Yuck... I know you eschew inline diffs, but it's just easier if nothing
else just to make progress:
index b6a8129..7da94bb 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -14348,12 +14348,24 @@ virDomainDefParseXML(xmlDocPtr xml,
def->cputune.niothreadsched = n;
for (i = 0; i < def->cputune.niothreadsched; i++) {
+ssize_t pos = -1;
+
if (virDomainThreadSchedParse(nodes[i],
1, UINT_MAX,
"iothreads",
&def->cputune.iothreadsched[i]) < 0)
goto error;
+while ((pos =
virBitmapNextSetBit(def->cputune.iothreadsched[i].ids,
+ pos)) > -1) {
+if (!virDomainIOThreadIDFind(def, pos)) {
+virReportError(VIR_ERR_XML_DETAIL, "%s",
+ _("iothreadsched attribute 'iothreads' "
+ "uses undefined iothread ids"));
+goto error;
+}
+}
+
for (j = 0; j < i; j++) {
if (virBitmapOverlaps(def->cputune.iothreadsched[i].ids,
def->cputune.iothreadsched[j].ids)) {
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v5 05/10] conf: Adjust the iothreadsched expectations
On Fri, Apr 24, 2015 at 12:05:57 -0400, John Ferlan wrote:
> With iothreadid's allowing any 'id' value for an iothread_id, the
> iothreadsched code needs a slight adjustment to allow for "any"
> unsigned int value in order to create the bitmap of ids that will
> have scheduler adjustments. Adjusted the doc description as well.
>
> Signed-off-by: John Ferlan
> ---
> docs/formatdomain.html.in| 16
>
> src/conf/domain_conf.c | 2 +-
> .../qemuxml2argv-cputune-iothreadsched-toomuch.xml | 3 ++-
> 3 files changed, 15 insertions(+), 6 deletions(-)
>
> diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in
> index 7af6bd7..0767a2a 100644
> --- a/docs/formatdomain.html.in
> +++ b/docs/formatdomain.html.in
> @@ -691,10 +691,18 @@
> type (values batch, idle,
> fifo,
> rr) for particular vCPU/IOThread threads (based on
> vcpus and iothreads, leaving out
> -vcpus/iothreads sets the default). For
> -real-time schedulers (fifo, rr), priority
> must
> -be specified as well (and is ignored for non-real-time ones). The
> value
> -range for the priority depends on the host kernel (usually 1-99).
> +vcpus/iothreads sets the default). Valid
> +vcpus values start at 0 through one less than the
> +number of vCPU's defined for the domain. Valid iothreads
> +values are described in the iothreadids
> +description.
> +If no iothreadids are defined, then libvirt numbers
> +IOThreads from 1 to the number of iothreads available
> +for the domain. For real-time schedulers (fifo,
> +rr), priority must real-time schedulers
> +(fifo, rr), priority must be specified as
> +well (and is ignored for non-real-time ones). The value range
> +for the priority depends on the host kernel (usually 1-99).
> Since 1.2.13
>
>
> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
> index 129908d..9d4c916 100644
> --- a/src/conf/domain_conf.c
> +++ b/src/conf/domain_conf.c
> @@ -14348,7 +14348,7 @@ virDomainDefParseXML(xmlDocPtr xml,
>
> for (i = 0; i < def->cputune.niothreadsched; i++) {
> if (virDomainThreadSchedParse(nodes[i],
> - 1, def->iothreads,
> + 1, UINT_MAX,
>"iothreads",
>&def->cputune.iothreadsched[i]) <
> 0)
> goto error;
I think this patch should also add code that checks that the provided
scheduler info is provided only for valid iothread IDs.
Peter
signature.asc
Description: Digital signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
