Re: [PATCH] domstats:add haltpolling time statistic interface

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

 




On 2021/7/2 15:45, Peter Krempa wrote:
> On Fri, Jul 02, 2021 at 12:53:29 +0800, Yang Fei wrote:
>> This patch add the ability to statistic the halt polling time when
>> VM execute HLT(arm is WFI).
>>
>> In actual services, the execution of the HLT instruction by the
>> guest is an important cause of virtualization overhead. The halt
>> polling feature is introduced to solve this problem. When a guest
>> idle VM exit occurs, the host continues polling for a period of
>> time to reduce the guest service delay. This mechanism may cause
>> the CPU usage to be 100% when the physical CPU is idle. If the
>> guest service model is woken up at an interval to process a small
>> amount of traffic, and the interval is shorter than kvm halt_poll_ns.
>> The host polls the block time of the entire VM and the CPU usage
>> increases to 100%.
>>
>> The kernel provides the capability of collecting statistics on the
>> halt polling time after v5.8, Introduced by commit
>>  <cb953129bfe5c0f2da835a0469930873fb7e71df>. It is rendered in debugfs.
>> Therefore, we can use this kernel feature to provide the halt poll
>> time to the user to obtain a more accurate CPU usage as required.
> 
> Note that I'm reviewing just the code side, not the justification or
> usefullnes of the data reported.
> 
>>
>> Signed-off-by: Yang Fei <yangfei85@xxxxxxxxxx>
>> ---
>>  src/libvirt_private.syms |  2 +
>>  src/qemu/qemu_driver.c   | 29 +++++++++++++
>>  src/util/virutil.c       | 89 ++++++++++++++++++++++++++++++++++++++++
>>  src/util/virutil.h       |  9 ++++
>>  4 files changed, 129 insertions(+)
>>
>> diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
>> index 68e4b6aab8..f92213b8c2 100644
>> --- a/src/libvirt_private.syms
>> +++ b/src/libvirt_private.syms
>> @@ -3479,6 +3479,8 @@ virDoesUserExist;
>>  virDoubleToStr;
>>  virFormatIntDecimal;
>>  virFormatIntPretty;
>> +virGetCpuHaltPollTime;
>> +virGetDebugFsKvmValue;
>>  virGetDeviceID;
>>  virGetDeviceUnprivSGIO;
>>  virGetGroupID;
>> diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
>> index 235f575901..3a2b530ecf 100644
>> --- a/src/qemu/qemu_driver.c
>> +++ b/src/qemu/qemu_driver.c
>> @@ -17812,6 +17812,32 @@ qemuDomainGetStatsCpuCache(virQEMUDriver *driver,
>>      return ret;
>>  }
>>  
>> +#ifdef __linux__
>> +static int
>> +qemuDomainGetStatsCpuHaltPollTime(virDomainObj *dom,
>> +                                 virTypedParamList *params)
>> +{
>> +    unsigned long long haltPollSuccess = 0;
>> +    unsigned long long haltPollFail = 0;
>> +    pid_t pid = dom->pid;
>> +
>> +    if (virGetCpuHaltPollTime(pid, &haltPollSuccess, &haltPollFail) != 0)
>> +        return -1;
>> +    if (virTypedParamListAddULLong(params, haltPollSuccess, "haltpollsuccess.time") < 0)
>> +        return -1;
>> +    if (virTypedParamListAddULLong(params, haltPollFail, "haltpollfail.time") < 0)
>> +        return -1;
>> +
>> +    return 0;
>> +}
>> +#else
>> +static int
>> +qemuDomainGetStatsCpuHaltPollTime(virDomainObj *dom,
>> +                                 virTypedParamList *params)
>> +{
>> +    return -1;
> 
> This breaks domstats on non-linux ...
> 
>> +}
>> +#endif
>>  
>>  static int
>>  qemuDomainGetStatsCpuCgroup(virDomainObj *dom,
>> @@ -17852,6 +17878,9 @@ qemuDomainGetStatsCpu(virQEMUDriver *driver,
>>      if (qemuDomainGetStatsCpuCache(driver, dom, params) < 0)
>>          return -1;
>>  
>> +    if (qemuDomainGetStatsCpuHaltPollTime(dom, params) < 0)
>> +        return -1;
> 
> since you'd return -1 here.
> 
> Additionally the qemuDomainGetStatsCpuHaltPollTime function IMO should
> not be conditionally compiled on linux. The helpers below should and the
> qemu code should just conditionally fill in the data if it's available.
> 

My consideration is not to compile this feature on non-Linux platforms,
since the function of get halt polling time is based on the capabilities of
Linux kernel.
How about return 0 in non-linux platform?

>> +
>>      return 0;
>>  }
>>  
>> diff --git a/src/util/virutil.c b/src/util/virutil.c
>> index 311cbbf93a..8715deaca5 100644
>> --- a/src/util/virutil.c
>> +++ b/src/util/virutil.c
>> @@ -1936,3 +1936,92 @@ virPipeNonBlock(int fds[2])
>>  {
>>      return virPipeImpl(fds, true, true);
>>  }
>> +
>> +int
>> +virGetDebugFsKvmValue(struct dirent *ent,
>> +                    const char *path,
>> +                    const char *filename,
>> +                    unsigned long long *value)
> 
> This helper should be added in a separate patch, to separate it from the
> other changes, especially the qemu driver change.
> 

I will remove it to a separate patch in next version.

>> +{
>> +    g_autofree char *valToStr = NULL;
>> +    g_autofree char *valPath = NULL;
>> +    int rc = -1;
>> +    int ret = -1;
>> +
>> +    valPath = g_strdup_printf("%s/%s/%s", path, ent->d_name, filename);
>> +
>> +    if ((rc = virFileReadAll(valPath, 16, &valToStr)) < 0) {
> 
> Why just 16 bytes?
> 

Cause the data type of halt_poll_success_ns and halt_poll_fail_ns is u64,
it only requires 8 bytes of memory.

>> +        virReportError(VIR_ERR_INTERNAL_ERROR,
>> +                       _("Unable to read from '%s'"), valPath);
>> +        goto cleanup;
>> +    }
>> +
>> +    /* Terminated with '\n' has sometimes harmful effects to the caller */
> 
> To the caller? I don't quite understand what you mean here.
> 

The code under this comment is copied from virCgroupGetValueRaw
which can only be used to obtain Cgroup data.
I plan extract this function to virutil.c in an independent patch. So that
we can call this function directly.

>> +    if (rc > 0 && (valToStr)[rc - 1] == '\n')
>> +        (valToStr)[rc - 1] = '\0';
>> +
>> +    /* 10 is a Cardinality, must be between 2 and 36 inclusive,
>> +     * or special value 0. Used in fuction strtoull()
>> +     */
> 
> What's the point of this comment?
> 

Since 10 here is a magic number, I've given an comment to explain it.

>> +    if (virStrToLong_ull(valToStr, NULL, 10, value) < 0) {
>> +        virReportError(VIR_ERR_INTERNAL_ERROR,
>> +                       _("Unable to parse '%s' as an integer"), valToStr);
>> +        goto cleanup;
>> +    }
>> +
>> +    ret = 0;
>> +cleanup:
> 
> No need for cleanup label since it's not cleaning up anything.
> 

OK, I will remove it in the next version.

>> +    return ret;
>> +}
>> +
>> +int
>> +virGetCpuHaltPollTime(pid_t pid,
>> +                      unsigned long long *haltPollSuccess,
>> +                      unsigned long long *haltPollFail)
>> +{
>> +    g_autofree char *pidToStr = NULL;
>> +    g_autofree char *debugFsPath = NULL;
>> +    g_autofree char *completePath = NULL;
>> +    struct dirent *ent = NULL;
>> +    DIR *dir = NULL;
>> +    int ret = -1;
>> +    int flag = 0;
>> +
>> +    if (!(debugFsPath = virFileFindMountPoint("debugfs"))) {
>> +        virReportSystemError(errno, "%s",
>> +                             _("unable to find debugfs mountpoint"));
>> +        goto cleanup;
>> +    }
>> +
>> +    completePath = g_strdup_printf("%s/%s", debugFsPath, "kvm");
>> +    if (virDirOpen(&dir, completePath) < 0) {
>> +        virReportError(VIR_ERR_INTERNAL_ERROR,
>> +                       "%s %s", "Can not open directory", completePath);
>> +        return ret;
>> +    }
>> +
>> +    pidToStr = g_strdup_printf("%d", pid);
> 
> %d for pid_t is invalid.
> 

Should I use %lld?

>> +    while (virDirRead(dir, &ent, NULL) > 0) {
>> +        if (strncmp(ent->d_name, pidToStr, strlen(pidToStr)) == 0 &&
> 
> We mandate use of our wrappers, in this case STRPREFIX
> 
>> +            ent->d_name[strlen(pidToStr)] == '-') {
> 
> Formatting the pidToStr including the '-' will simplify the code since
> you can match the full prefix.
> 

OK, I'll use the STRPREFIX in next version.

>> +            flag = 1;
>> +            break;
>> +        }
>> +    }
>> +
>> +    if (flag == 0) {
>> +        virReportError(VIR_ERR_INTERNAL_ERROR,
>> +                       _("Could not find VM(Pid %s) in '%s'"), pidToStr, completePath);
> 
> Reporting errors in optional stats code is not acceptable. The domstats
> code is best-effort and should return what it can.
> 

So, the function should not report any errors or return any error number in any case.  If something
happends, e.g. debugfs isn't mounted, the caller should not add halt_poll_success_ns and
halt_poll_fail_ns to domstats.  Have I got that right?

>> +        goto cleanup;
>> +    }
>> +
>> +    if (virGetDebugFsKvmValue(ent, completePath, "halt_poll_success_ns", haltPollSuccess) < 0 ||
>> +        virGetDebugFsKvmValue(ent, completePath, "halt_poll_fail_ns", haltPollFail) < 0) {
>> +        goto cleanup;
>> +    }
>> +
>> +    ret = 0;
>> +cleanup:
>> +    closedir(dir);
>> +    return ret;
>> +}
> 
> .
> 

Thanks,
Fei.




[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]

  Powered by Linux