[PATCH v3 1/2] util: Add virGetCpuHaltPollTime

[Yang Fei]

On 2021/7/19 18:20, Michal Prívozník wrote:
> On 7/16/21 12:42 PM, 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);
>>  }
>> +
> 
> Apart from Peter's findings:
> 
> I'm not sure virutil.c is the best placement for this function. I mean,
> virutil.c became dump of functions we did not find a better place for.
> How about src/util/virhostcpu.c ? That seems like a better fit, doesn't it?
> 

Fuction in src/util/virhostcpu.c looks more like getting the physical
properties of the host CPU. Such as the number of CPUs, the frequency, etc.
But this is probably the best placement for this function. I'll move it to
virhostcpu.c next version.

>> +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;
> 
> This variable is pretty much useless. The reason we have it in other
> functions is that they have the following pattern:
> 
>   int ret = -1;
> 
>   if ()
>     goto clenaup;
> 
>   if ()
>     goto cleanup;
> 
>   ret = 0;
>  cleanup:
>   something();
>   return ret;
> 
> But this is not really the case for this function. You can do 'return
> -1' instead of 'return ret' and then plain 'return 0' at the end.
> 

OK, I'll remove the ret in next version.

>> +    bool found = false;
>> +
>> +    if (!(debugFsPath = virFileFindMountPoint("debugfs")))
>> +        return ret;
>> +
>> +    completePath = g_strdup_printf("%s/%s", debugFsPath, "kvm");
>> +    if (virDirOpenIfExists(&dir, completePath) != 1)
>> +        return ret;
>> +
>> +    pidToStr = g_strdup_printf("%d%c", pid, '-');
>> +    while (virDirRead(dir, &ent, NULL) > 0) {
>> +        if (STRPREFIX(ent->d_name, pidToStr)) {
>> +            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) {
> 
> We like to format this as:
> 
>   if (condition1 ||
>       condition2)
> 

OK, I'll do it.

>> +        return ret;
>> +    }
>> +
>> +    ret = 0;
>> +
>> +    return ret;
>> +}
> 
> Michal
> 
> .
> 
Thanks,
Fei.