[libvirt] [PATCH v5 05/10] conf: Adjust the iothreadsched expectations

[Peter Krempa]

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 <jferlan at redhat.com>
> >> ---
> >>  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 <code>batch</code>, <code>idle</code>, <code>fifo</code>,
> >>          <code>rr</code>) for particular vCPU/IOThread threads (based on
> >>          <code>vcpus</code> and <code>iothreads</code>, leaving out
> >> -        <code>vcpus</code>/<code>iothreads</code> sets the default).  For
> >> -        real-time schedulers (<code>fifo</code>, <code>rr</code>), 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).
> >> +        <code>vcpus</code>/<code>iothreads</code> sets the default). Valid
> >> +        <code>vcpus</code> values start at 0 through one less than the
> >> +        number of vCPU's defined for the domain. Valid <code>iothreads</code>
> >> +        values are described in the <code>iothreadids</code>
> >> +        <a href="#elementsIOThreadsAllocation"><code>description</code></a>.
> >> +        If no <code>iothreadids</code> are defined, then libvirt numbers
> >> +        IOThreads from 1 to the number of <code>iothreads</code> available
> >> +        for the domain. For real-time schedulers (<code>fifo</code>,
> >> +        <code>rr</code>), priority must real-time schedulers
> >> +        (<code>fifo</code>, <code>rr</code>), 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).
> >>          <span class="since">Since 1.2.13</span>
> >>        </dd>
> >>  
> >> 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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: Digital signature
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20150427/b90e0edf/attachment-0001.sig>