Re: [PATCH 30/34] conf: Fix how iothread scheduler info is stored

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 




On 01/14/2016 11:27 AM, Peter Krempa wrote:
> Similarly to previous commit change the way how iothread scheduler info
> is stored and clean up a lot of unnecessary code.

(and hours of careful cut-n-paste ;-))

> ---
>  src/conf/domain_conf.c                             | 141 +++++++--------------
>  src/conf/domain_conf.h                             |   8 +-
>  src/libvirt_private.syms                           |   1 -
>  src/qemu/qemu_driver.c                             |   3 -
>  src/qemu/qemu_process.c                            |  39 +-----
>  .../qemuxml2xmlout-cputune-iothreadsched.xml       |   3 +-
>  6 files changed, 53 insertions(+), 142 deletions(-)
> 
> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
> index e2dda9a..4ca03d9 100644
> --- a/src/conf/domain_conf.c
> +++ b/src/conf/domain_conf.c
> @@ -2559,10 +2559,6 @@ void virDomainDefFree(virDomainDefPtr def)
> 
>      virBitmapFree(def->cputune.emulatorpin);
> 
> -    for (i = 0; i < def->cputune.niothreadsched; i++)
> -        virBitmapFree(def->cputune.iothreadsched[i].ids);
> -    VIR_FREE(def->cputune.iothreadsched);
> -
>      virDomainNumaFree(def->numa);
> 
>      virSysinfoDefFree(def->sysinfo);
> @@ -14649,25 +14645,26 @@ virDomainVcpuThreadSchedParse(xmlNodePtr node,
>  }
> 
> 
> -static int
> -virDomainThreadSchedParse(xmlNodePtr node,
> -                          unsigned int minid,
> -                          unsigned int maxid,
> -                          const char *name,
> -                          virDomainThreadSchedParamPtr sp)
> -{
> -    if (!(sp->ids = virDomainSchedulerParse(node, name, &sp->policy,
> -                                            &sp->priority)))
> -        return -1;
> +static virDomainThreadSchedParamPtr
> +virDomainDefGetIOThreadSched(virDomainDefPtr def,
> +                             unsigned int iothread)
> +{
> +    virDomainIOThreadIDDefPtr iothrinfo;
> 
> -    if (virBitmapNextSetBit(sp->ids, -1) < minid ||
> -        virBitmapLastSetBit(sp->ids) > maxid) {
> -        virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> -                       _("%ssched bitmap is out of range"), name);
> -        return -1;
> -    }
> +    if (!(iothrinfo = virDomainIOThreadIDFind(def, iothread)))
> +        return NULL;
> 
> -    return 0;
> +    return &iothrinfo->sched;
> +}
> +
> +
> +static int
> +virDomainIOThreadSchedParse(xmlNodePtr node,
> +                            virDomainDefPtr def)
> +{
> +    return virDomainThreadSchedParseHelper(node, "iothreads",

Here's somewhere to think about regarding Parse using "iothreads" while
Format uses "iothread"

> +                                           virDomainDefGetIOThreadSched,
> +                                           def);
>  }
> 
> 
> @@ -15215,46 +15212,10 @@ virDomainDefParseXML(xmlDocPtr xml,
>                         _("cannot extract iothreadsched nodes"));
>          goto error;
>      }
> -    if (n) {
> -        if (n > def->niothreadids) {
> -            virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> -                           _("too many iothreadsched nodes in cputune"));
> -            goto error;
> -        }
> 
> -        if (VIR_ALLOC_N(def->cputune.iothreadsched, n) < 0)
> +    for (i = 0; i < n; i++) {
> +        if (virDomainIOThreadSchedParse(nodes[i], def) < 0)
>              goto error;
> -        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)) {
> -                    virReportError(VIR_ERR_XML_DETAIL, "%s",
> -                                   _("iothreadsched attributes 'iothreads' "
> -                                     "must not overlap"));
> -                    goto error;
> -                }
> -            }
> -        }
>      }
>      VIR_FREE(nodes);
> 
> @@ -18448,29 +18409,6 @@ virDomainIOThreadIDDel(virDomainDefPtr def,
>      }
>  }
> 
> -void
> -virDomainIOThreadSchedDelId(virDomainDefPtr def,
> -                            unsigned int iothreadid)
> -{
> -    size_t i;
> -
> -    if (!def->cputune.iothreadsched || !def->cputune.niothreadsched)
> -        return;
> -
> -    for (i = 0; i < def->cputune.niothreadsched; i++) {
> -        if (virBitmapIsBitSet(def->cputune.iothreadsched[i].ids, iothreadid)) {
> -            ignore_value(virBitmapClearBit(def->cputune.iothreadsched[i].ids,
> -                                           iothreadid));
> -            if (virBitmapIsAllClear(def->cputune.iothreadsched[i].ids)) {
> -                virBitmapFree(def->cputune.iothreadsched[i].ids);
> -                VIR_DELETE_ELEMENT(def->cputune.iothreadsched, i,
> -                                   def->cputune.niothreadsched);
> -            }
> -            return;
> -        }
> -    }
> -}
> -
> 
>  static int
>  virDomainEventActionDefFormat(virBufferPtr buf,
> @@ -21665,6 +21603,27 @@ virDomainFormatVcpuSchedDef(virDomainDefPtr def,
> 
> 
>  static int
> +virDomainFormatIOThreadSchedDef(virDomainDefPtr def,
> +                                virBufferPtr buf)
> +{
> +    virBitmapPtr allthreadmap;
> +    int ret;
> +
> +    if (def->niothreadids == 0)
> +        return 0;
> +
> +    if (!(allthreadmap = virDomainIOThreadIDMap(def)))
> +        return -1;
> +
> +    ret = virDomainFormatSchedDef(def, buf, "iothread",
> +                                  virDomainDefGetIOThreadSched, allthreadmap);

Just another place to consider if it's decided the 'name' should
commonly used (eg. notes from patch 29 and 28).

> +
> +    virBitmapFree(allthreadmap);
> +    return ret;
> +}
> +
> +
> +static int
>  virDomainCputuneDefFormat(virBufferPtr buf,
>                            virDomainDefPtr def)
>  {
> @@ -21741,22 +21700,8 @@ virDomainCputuneDefFormat(virBufferPtr buf,
>      if (virDomainFormatVcpuSchedDef(def, &childrenBuf) < 0)
>          goto cleanup;
> 
> -    for (i = 0; i < def->cputune.niothreadsched; i++) {
> -        virDomainThreadSchedParamPtr sp = &def->cputune.iothreadsched[i];
> -        char *ids = NULL;
> -
> -        if (!(ids = virBitmapFormat(sp->ids)))
> -            goto cleanup;
> -
> -        virBufferAsprintf(&childrenBuf, "<iothreadsched iothreads='%s' scheduler='%s'",
> -                          ids, virProcessSchedPolicyTypeToString(sp->policy));
> -        VIR_FREE(ids);
> -
> -        if (sp->policy == VIR_PROC_POLICY_FIFO ||
> -            sp->policy == VIR_PROC_POLICY_RR)
> -            virBufferAsprintf(&childrenBuf, " priority='%d'", sp->priority);
> -        virBufferAddLit(&childrenBuf, "/>\n");
> -    }
> +    if (virDomainFormatIOThreadSchedDef(def, &childrenBuf) < 0)
> +        goto cleanup;
> 
>      if (virBufferUse(&childrenBuf)) {
>          virBufferAddLit(buf, "<cputune>\n");
> diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h
> index 85740bc..b93835a 100644
> --- a/src/conf/domain_conf.h
> +++ b/src/conf/domain_conf.h
> @@ -1898,7 +1898,6 @@ typedef enum {
>  typedef struct _virDomainThreadSchedParam virDomainThreadSchedParam;
>  typedef virDomainThreadSchedParam *virDomainThreadSchedParamPtr;
>  struct _virDomainThreadSchedParam {
> -    virBitmapPtr ids;
>      virProcessSchedPolicy policy;
>      int priority;
>  };
> @@ -2095,6 +2094,8 @@ struct _virDomainIOThreadIDDef {
>      unsigned int iothread_id;
>      int thread_id;
>      virBitmapPtr cpumask;
> +
> +    virDomainThreadSchedParam sched;
>  };
> 
>  void virDomainIOThreadIDDefFree(virDomainIOThreadIDDefPtr def);
> @@ -2111,9 +2112,6 @@ struct _virDomainCputune {
>      unsigned long long emulator_period;
>      long long emulator_quota;
>      virBitmapPtr emulatorpin;
> -
> -    size_t niothreadsched;
> -    virDomainThreadSchedParamPtr iothreadsched;
>  };
> 
> 
> @@ -2124,7 +2122,6 @@ struct _virDomainVcpuInfo {
>      bool online;
>      virBitmapPtr cpumask;
> 
> -    /* note: the sched.ids bitmap is unused so it doens't have to be cleared */
>      virDomainThreadSchedParam sched;
>  };
> 
> @@ -2708,7 +2705,6 @@ virDomainIOThreadIDDefPtr virDomainIOThreadIDAdd(virDomainDefPtr def,
>  virBitmapPtr virDomainIOThreadIDMap(virDomainDefPtr def)
>      ATTRIBUTE_NONNULL(1) ATTRIBUTE_RETURN_CHECK;
>  void virDomainIOThreadIDDel(virDomainDefPtr def, unsigned int iothread_id);
> -void virDomainIOThreadSchedDelId(virDomainDefPtr def, unsigned int iothread_id);
> 
>  unsigned int virDomainDefFormatConvertXMLFlags(unsigned int flags);
> 
> diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
> index 0c84672..cd595b0 100644
> --- a/src/libvirt_private.syms
> +++ b/src/libvirt_private.syms
> @@ -343,7 +343,6 @@ virDomainIOThreadIDDefFree;
>  virDomainIOThreadIDDel;
>  virDomainIOThreadIDFind;
>  virDomainIOThreadIDMap;
> -virDomainIOThreadSchedDelId;
>  virDomainKeyWrapCipherNameTypeFromString;
>  virDomainKeyWrapCipherNameTypeToString;
>  virDomainLeaseDefFree;
> diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
> index dfed936..34e82c2 100644
> --- a/src/qemu/qemu_driver.c
> +++ b/src/qemu/qemu_driver.c
> @@ -6043,8 +6043,6 @@ qemuDomainHotplugDelIOThread(virQEMUDriverPtr driver,
> 
>      virDomainIOThreadIDDel(vm->def, iothread_id);
> 
> -    virDomainIOThreadSchedDelId(vm->def, iothread_id);
> -
>      if (qemuDomainDelCgroupForThread(priv->cgroup,
>                                       VIR_CGROUP_THREAD_IOTHREAD,
>                                       iothread_id) < 0)
> @@ -6134,7 +6132,6 @@ qemuDomainChgIOThread(virQEMUDriverPtr driver,
>              }
> 
>              virDomainIOThreadIDDel(persistentDef, iothread_id);
> -            virDomainIOThreadSchedDelId(persistentDef, iothread_id);
>              persistentDef->iothreads--;
>          }
> 
> diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
> index 3c1c6d8..abafbb1 100644
> --- a/src/qemu/qemu_process.c
> +++ b/src/qemu/qemu_process.c
> @@ -2251,34 +2251,6 @@ qemuProcessSetIOThreadsAffinity(virDomainObjPtr vm)
>      return ret;
>  }
> 
> -/* Set Scheduler parameters for vCPU or I/O threads. */
> -int
> -qemuProcessSetSchedParams(int id,
> -                          pid_t pid,
> -                          size_t nsp,
> -                          virDomainThreadSchedParamPtr sp)
> -{
> -    bool val = false;
> -    size_t i = 0;
> -    virDomainThreadSchedParamPtr s = NULL;
> -
> -    for (i = 0; i < nsp; i++) {
> -        if (virBitmapGetBit(sp[i].ids, id, &val) < 0) {
> -            virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> -                           _("Cannot get bit from bitmap"));
> -        }
> -        if (val) {
> -            s = &sp[i];
> -            break;
> -        }
> -    }
> -
> -    if (!s)
> -        return 0;
> -
> -    return virProcessSetScheduler(pid, s->policy, s->priority);
> -}
> -
>  static int
>  qemuProcessSetSchedulers(virDomainObjPtr vm)
>  {
> @@ -2297,10 +2269,13 @@ qemuProcessSetSchedulers(virDomainObjPtr vm)
>      }
> 
>      for (i = 0; i < vm->def->niothreadids; i++) {
> -        if (qemuProcessSetSchedParams(vm->def->iothreadids[i]->iothread_id,
> -                                      vm->def->iothreadids[i]->thread_id,
> -                                      vm->def->cputune.niothreadsched,
> -                                      vm->def->cputune.iothreadsched) < 0)
> +        virDomainIOThreadIDDefPtr info = vm->def->iothreadids[i];
> +
> +        if (info->sched.policy == VIR_PROC_POLICY_NONE)
> +            continue;
> +
> +        if (virProcessSetScheduler(info->thread_id, info->sched.policy,
> +                                   info->sched.priority) < 0)
>              return -1;
>      }
> 
> diff --git a/tests/qemuxml2xmloutdata/qemuxml2xmlout-cputune-iothreadsched.xml b/tests/qemuxml2xmloutdata/qemuxml2xmlout-cputune-iothreadsched.xml
> index 9f61336..01f1af9 100644
> --- a/tests/qemuxml2xmloutdata/qemuxml2xmlout-cputune-iothreadsched.xml
> +++ b/tests/qemuxml2xmloutdata/qemuxml2xmlout-cputune-iothreadsched.xml
> @@ -13,8 +13,7 @@
>      <vcpupin vcpu='1' cpuset='1'/>
>      <emulatorpin cpuset='1'/>
>      <vcpusched vcpus='0-1' scheduler='fifo' priority='1'/>
> -    <iothreadsched iothreads='1,3' scheduler='batch'/>
> -    <iothreadsched iothreads='2' scheduler='batch'/>
> +    <iothreadsched iothreads='1-3' scheduler='batch'/>

This undoes commit id '8680ea974'  Not sure why there were two 'batch'
entries using different iothreads values...

Not a problem, but I'm wondering why a DO_TEST_DIFFERENT was used now.

John
>    </cputune>
>    <os>
>      <type arch='i686' machine='pc'>hvm</type>
> 

--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list



[Index of Archives]     [Virt Tools]     [Libvirt Users]     [Lib OS Info]     [Fedora Users]     [Fedora Desktop]     [Fedora SELinux]     [Big List of Linux Books]     [Yosemite News]     [KDE Users]     [Fedora Tools]