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

Re: [libvirt] [PATCH 1/2] util: rename qemuGetProcessInfo to virProcessGetStat




On 05/23/2017 11:31 PM, Wang King wrote:
> qemuGetProcessInfo is more likely a process utility function, just rename it
> to virProcessGetStat and move it to virprocess.c source file.
> ---
>  src/libvirt_private.syms |  1 +
>  src/qemu/qemu_driver.c   | 83 ++++++++++--------------------------------------
>  src/util/virprocess.c    | 62 ++++++++++++++++++++++++++++++++++++
>  src/util/virprocess.h    |  4 +++
>  4 files changed, 83 insertions(+), 67 deletions(-)
> 

You should follow the guidelines about a cover letter when sending more
than one patch (http://libvirt.org/hacking.html). It would have
especially helped explain why you're doing this...

Beyond that there's quite a bit of linux specific stuff in both of these
calls and moves from the "/proc" file system to the "sysconf" call in
this patch and the comments in the second one related to CONFIG_SCHED*.

In other virprocess.c functions you'll generally note there are two
functions - one to handle linux and the other to return an error if run
on a non linux system (forcing someone to add some code in order to make
this work there).

Not sure I see the need to move and rename these, but you can try again
with a more complete set of patches.

... note one more comment below...

> diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
> index d361454..3681869 100644
> --- a/src/libvirt_private.syms
> +++ b/src/libvirt_private.syms
> @@ -2382,6 +2382,7 @@ virProcessGetMaxMemLock;
>  virProcessGetNamespaces;
>  virProcessGetPids;
>  virProcessGetStartTime;
> +virProcessGetStat;
>  virProcessKill;
>  virProcessKillPainfully;
>  virProcessNamespaceAvailable;
> diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
> index 6c79d4f..a4aa5da 100644
> --- a/src/qemu/qemu_driver.c
> +++ b/src/qemu/qemu_driver.c
> @@ -1378,68 +1378,6 @@ qemuGetSchedInfo(unsigned long long *cpuWait,
>      return ret;
>  }
>  
> -
> -static int
> -qemuGetProcessInfo(unsigned long long *cpuTime, int *lastCpu, long *vm_rss,
> -                   pid_t pid, int tid)
> -{
> -    char *proc;
> -    FILE *pidinfo;
> -    unsigned long long usertime = 0, systime = 0;
> -    long rss = 0;
> -    int cpu = 0;
> -    int ret;
> -
> -    /* In general, we cannot assume pid_t fits in int; but /proc parsing
> -     * is specific to Linux where int works fine.  */
> -    if (tid)
> -        ret = virAsprintf(&proc, "/proc/%d/task/%d/stat", (int) pid, tid);
> -    else
> -        ret = virAsprintf(&proc, "/proc/%d/stat", (int) pid);
> -    if (ret < 0)
> -        return -1;
> -
> -    pidinfo = fopen(proc, "r");
> -    VIR_FREE(proc);
> -
> -    /* See 'man proc' for information about what all these fields are. We're
> -     * only interested in a very few of them */
> -    if (!pidinfo ||
> -        fscanf(pidinfo,
> -               /* pid -> stime */
> -               "%*d (%*[^)]) %*c %*d %*d %*d %*d %*d %*u %*u %*u %*u %*u %llu %llu"
> -               /* cutime -> endcode */
> -               "%*d %*d %*d %*d %*d %*d %*u %*u %ld %*u %*u %*u"
> -               /* startstack -> processor */
> -               "%*u %*u %*u %*u %*u %*u %*u %*u %*u %*u %*d %d",
> -               &usertime, &systime, &rss, &cpu) != 4) {
> -        VIR_WARN("cannot parse process status data");
> -    }
> -
> -    /* We got jiffies
> -     * We want nanoseconds
> -     * _SC_CLK_TCK is jiffies per second
> -     * So calculate thus....
> -     */
> -    if (cpuTime)
> -        *cpuTime = 1000ull * 1000ull * 1000ull * (usertime + systime)
> -            / (unsigned long long)sysconf(_SC_CLK_TCK);
> -    if (lastCpu)
> -        *lastCpu = cpu;
> -
> -    if (vm_rss)
> -        *vm_rss = rss * virGetSystemPageSizeKB();
> -
> -
> -    VIR_DEBUG("Got status for %d/%d user=%llu sys=%llu cpu=%d rss=%ld",
> -              (int) pid, tid, usertime, systime, cpu, rss);
> -
> -    VIR_FORCE_FCLOSE(pidinfo);
> -
> -    return 0;
> -}
> -
> -
>  static int
>  qemuDomainHelperGetVcpus(virDomainObjPtr vm,
>                           virVcpuInfoPtr info,
> @@ -1482,9 +1420,9 @@ qemuDomainHelperGetVcpus(virDomainObjPtr vm,
>              vcpuinfo->number = i;
>              vcpuinfo->state = VIR_VCPU_RUNNING;
>  
> -            if (qemuGetProcessInfo(&vcpuinfo->cpuTime,
> -                                   &vcpuinfo->cpu, NULL,
> -                                   vm->pid, vcpupid) < 0) {
> +            if (virProcessGetStat(vm->pid, vcpupid,
> +                                  &vcpuinfo->cpuTime,
> +                                  &vcpuinfo->cpu, NULL) < 0) {
>                  virReportSystemError(errno, "%s",
>                                       _("cannot get vCPU placement & pCPU time"));
>                  return -1;
> @@ -2641,7 +2579,7 @@ qemuDomainGetInfo(virDomainPtr dom,
>      }
>  
>      if (virDomainObjIsActive(vm)) {
> -        if (qemuGetProcessInfo(&(info->cpuTime), NULL, NULL, vm->pid, 0) < 0) {
> +        if (virProcessGetStat(vm->pid, 0, &(info->cpuTime), NULL, NULL) < 0) {
>              virReportError(VIR_ERR_OPERATION_FAILED, "%s",
>                             _("cannot read cputime for domain"));
>              goto cleanup;
> @@ -8172,6 +8110,8 @@ static int qemuDomainUpdateDeviceFlags(virDomainPtr dom,
>      virDomainDeviceDefPtr dev = NULL, dev_copy = NULL;
>      bool force = (flags & VIR_DOMAIN_DEVICE_MODIFY_FORCE) != 0;
>      int ret = -1;
> +    virQEMUCapsPtr qemuCaps = NULL;
> +    qemuDomainObjPrivatePtr priv;
>      virQEMUDriverConfigPtr cfg = NULL;
>      virCapsPtr caps = NULL;
>      unsigned int parse_flags = VIR_DOMAIN_DEF_PARSE_INACTIVE;
> @@ -8190,6 +8130,8 @@ static int qemuDomainUpdateDeviceFlags(virDomainPtr dom,
>      if (!(vm = qemuDomObjFromDomain(dom)))
>          goto cleanup;
>  
> +    priv = vm->privateData;
> +
>      if (virDomainUpdateDeviceFlagsEnsureACL(dom->conn, vm->def, flags) < 0)
>          goto cleanup;
>  
> @@ -8216,6 +8158,12 @@ static int qemuDomainUpdateDeviceFlags(virDomainPtr dom,
>              goto endjob;
>      }
>  
> +    if (priv->qemuCaps)
> +        qemuCaps = virObjectRef(priv->qemuCaps);
> +    else if (!(qemuCaps = virQEMUCapsCacheLookup(caps, driver->qemuCapsCache,
> +                                                 vm->def->emulator)))
> +        goto endjob;
> +

Is this a related hunk? or something else that should have been in it's
own patch?

John

>      if (flags & VIR_DOMAIN_AFFECT_CONFIG) {
>          /* Make a copy for updated domain. */
>          vmdef = virDomainObjCopyPersistentDef(vm, caps, driver->xmlopt);
> @@ -8263,6 +8211,7 @@ static int qemuDomainUpdateDeviceFlags(virDomainPtr dom,
>      qemuDomainObjEndJob(driver, vm);
>  
>   cleanup:
> +    virObjectUnref(qemuCaps);
>      virDomainDefFree(vmdef);
>      if (dev != dev_copy)
>          virDomainDeviceDefFree(dev_copy);
> @@ -11107,7 +11056,7 @@ qemuDomainMemoryStatsInternal(virQEMUDriverPtr driver,
>          ret = 0;
>      }
>  
> -    if (qemuGetProcessInfo(NULL, NULL, &rss, vm->pid, 0) < 0) {
> +    if (virProcessGetStat(vm->pid, 0, NULL, NULL, &rss) < 0) {
>          virReportError(VIR_ERR_OPERATION_FAILED, "%s",
>                         _("cannot get RSS for domain"));
>      } else {
> diff --git a/src/util/virprocess.c b/src/util/virprocess.c
> index 1fbbbb3..98f4b25 100644
> --- a/src/util/virprocess.c
> +++ b/src/util/virprocess.c
> @@ -1426,3 +1426,65 @@ virProcessSetScheduler(pid_t pid ATTRIBUTE_UNUSED,
>  }
>  
>  #endif /* !HAVE_SCHED_SETSCHEDULER */
> +
> +int
> +virProcessGetStat(pid_t pid, int tid,
> +                  unsigned long long *cpuTime,
> +                  int *lastCpu, long *vm_rss)
> +{
> +    char *proc;
> +    FILE *pidinfo;
> +    unsigned long long usertime = 0, systime = 0;
> +    long rss = 0;
> +    int cpu = 0;
> +    int ret;
> +
> +    /* In general, we cannot assume pid_t fits in int; but /proc parsing
> +     * is specific to Linux where int works fine.  */
> +    if (tid)
> +        ret = virAsprintf(&proc, "/proc/%d/task/%d/stat", (int) pid, tid);
> +    else
> +        ret = virAsprintf(&proc, "/proc/%d/stat", (int) pid);
> +    if (ret < 0)
> +        return -1;
> +
> +    pidinfo = fopen(proc, "r");
> +    VIR_FREE(proc);
> +
> +    /* See 'man proc' for information about what all these fields are. We're
> +     * only interested in a very few of them */
> +    if (!pidinfo ||
> +        fscanf(pidinfo,
> +               /* pid -> stime */
> +               "%*d (%*[^)]) %*c %*d %*d %*d %*d %*d %*u %*u %*u %*u %*u %llu %llu"
> +               /* cutime -> endcode */
> +               "%*d %*d %*d %*d %*d %*d %*u %*u %ld %*u %*u %*u"
> +               /* startstack -> processor */
> +               "%*u %*u %*u %*u %*u %*u %*u %*u %*u %*u %*d %d",
> +               &usertime, &systime, &rss, &cpu) != 4) {
> +        VIR_WARN("cannot parse process status data");
> +    }
> +
> +    /* We got jiffies
> +     * We want nanoseconds
> +     * _SC_CLK_TCK is jiffies per second
> +     * So calculate thus....
> +     */
> +    if (cpuTime)
> +        *cpuTime = 1000ull * 1000ull * 1000ull * (usertime + systime)
> +            / (unsigned long long)sysconf(_SC_CLK_TCK);
> +    if (lastCpu)
> +        *lastCpu = cpu;
> +
> +    if (vm_rss)
> +        *vm_rss = rss * virGetSystemPageSizeKB();
> +
> +
> +    VIR_DEBUG("Got status for %d/%d user=%llu sys=%llu cpu=%d rss=%ld",
> +              (int) pid, tid, usertime, systime, cpu, rss);
> +
> +    VIR_FORCE_FCLOSE(pidinfo);
> +
> +    return 0;
> +
> +}
> diff --git a/src/util/virprocess.h b/src/util/virprocess.h
> index 3c5a882..2a2b91d 100644
> --- a/src/util/virprocess.h
> +++ b/src/util/virprocess.h
> @@ -106,4 +106,8 @@ typedef enum {
>  
>  int virProcessNamespaceAvailable(unsigned int ns);
>  
> +int virProcessGetStat(pid_t pid, int tid,
> +                      unsigned long long *cpuTime,
> +                      int *lastCpu, long *vm_rss);
> +
>  #endif /* __VIR_PROCESS_H__ */
> 


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