[PATCH v3 1/2] util: Add virGetCpuHaltPollTime

[Yang Fei]

On 2021/7/19 17:05, Peter Krempa wrote:
> On Fri, Jul 16, 2021 at 18:42:22 +0800, Yang Fei wrote:
>> Add helper function virGetCpuHaltPollTime to obtain halt polling
>> time. If the kernel support halt polling time statistic, and mount
>> debugfs. This function will take effect on KVM VMs.
>>
>> Signed-off-by: Yang Fei <yangfei85 at huawei.com>
>> ---
>>  src/libvirt_private.syms |  1 +
>>  src/util/virutil.c       | 43 ++++++++++++++++++++++++++++++++++++++++
>>  src/util/virutil.h       |  4 ++++
>>  3 files changed, 48 insertions(+)
>>
>> diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
>> index 68e4b6aab8..64aff4eca4 100644
>> --- a/src/libvirt_private.syms
>> +++ b/src/libvirt_private.syms
>> @@ -3479,6 +3479,7 @@ virDoesUserExist;
>>  virDoubleToStr;
>>  virFormatIntDecimal;
>>  virFormatIntPretty;
>> +virGetCpuHaltPollTime;
>>  virGetDeviceID;
>>  virGetDeviceUnprivSGIO;
>>  virGetGroupID;
>> diff --git a/src/util/virutil.c b/src/util/virutil.c
>> index 311cbbf93a..f5304644c0 100644
>> --- a/src/util/virutil.c
>> +++ b/src/util/virutil.c
>> @@ -1936,3 +1936,46 @@ virPipeNonBlock(int fds[2])
>>  {
>>      return virPipeImpl(fds, true, true);
>>  }
>> +
>> +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;
>> +    g_autoptr(DIR) dir = NULL;
>> +    int ret = -1;
>> +    bool found = false;
>> +
>> +    if (!(debugFsPath = virFileFindMountPoint("debugfs")))
>> +        return ret;
> 
> Here '-1' is returned, but no libvirt error is reported ...
> 
>> +
>> +    completePath = g_strdup_printf("%s/%s", debugFsPath, "kvm");
>> +    if (virDirOpenIfExists(&dir, completePath) != 1)
>> +        return ret;
> 
> While here -1 is reported and an error may be reported (e.g on EACCESS).
> We generally avoid this pattern because the caller cant properly handle
> that.
> 
>> +
>> +    pidToStr = g_strdup_printf("%d%c", pid, '-');
> 
> I'm fairly certain this will fail to compile on mingw since %d isn't the
> proper formatting modifier for pid_t.
> 

In the last version, I try to use %lld but failed in the ninja test.
How about forcibly converts the variable pid to long long?
Like this:
         g_strdup_printf("%lld%c", (long long)pid, '-')
I find that this is used elsewhere in the code.

>> +    while (virDirRead(dir, &ent, NULL) > 0) {
>> +        if (STRPREFIX(ent->d_name, pidToStr)) {
> 
> Any idea what the suffix after '-' in the debugfs entry is? If we can
> figure it out we won't have to look up the correct file.
> 

The full directory name format is "pid-fd", The fd is assigned during VM creation.
As I know, we can find it in /proc/(qemu-kvm pid)/fd, but also need to look up which
one points to "anon_inode:kvm-vm". That may not be any more efficient than look up
it in the kvm directory.

>> +            found = true;
>> +            break;
>> +        }
>> +    }
>> +
>> +    if (!found)
>> +        return ret;
>> +
>> +    if (virFileReadValueUllong(haltPollSuccess, "%s/%s/%s", completePath,
>> +                               ent->d_name, "halt_poll_success_ns") < 0
>> +        || virFileReadValueUllong(haltPollFail, "%s/%s/%s", completePath,
>> +                                  ent->d_name, "halt_poll_fail_ns") < 0) {
> 
> As you can see here, this is one of the functions having terrible
> semantics. It sometimes does set the libvirt error object and sometimes
> doesn't, thats why we try to avoid those.
> 
>> +        return ret;
>> +    }
>> +
>> +    ret = 0;
>> +
>> +    return ret;
>> +}
> 
> .
> 
Thanks,
Fei.