Re: [PATCH 3/3] qemu_driver: use qemuMonitorQueryStats to extract halt poll time
On Sun, Jul 24, 2022 at 10:50:36AM +0530, Amneesh Singh wrote: On Fri, Jul 22, 2022 at 07:09:57PM +0200, Paolo Bonzini wrote: On 7/22/22 17:43, Martin Kletzander wrote: > As mentioned before, all these failures do not have to exit the > function, but rather fallback to the old way. You can even create > two new functions for the new and old implementations and then call > them from here to make the fallback easier to spot (and code). More precisely, they should just "continue;" to the next iteration of the for loop, like if (!success_obj || !fail_obj) continue; found = true; and then go fall back if found is false at the end of the loop. On the other hand, here: if (virJSONValueGetNumberUlong(success_obj, ) < 0) return 0; if (virJSONValueGetNumberUlong(fail_obj, ) < 0) return 0; I am not sure about falling back, because it is really an unexpected situation where the statistic exist but somehow the value cannot be parsed. The main thing I was worried about was libvirtd crashing on possibly expoited qemu process. Whether we fall back or report nothing in this case is not that big of a deal, since there's something wrong already anyway. Then can we just "continue;" in case the value fails to parse as well? That's another option as well. Paolo > I wanted to change this before pushing as well, but I feel like I'm > changing too much of your code. And I also had to rebase this > (although trivially). Would you mind just changing these few last > things so that we can get it in before the rc0 freeze? Alright, as soon as there is a viable check decided for the virJSONValueGet* statements above, I will push a v4 with the changes you mentioned in your reviews. Thank you both for taking the time to review. signature.asc Description: PGP signature
Re: [PATCH 3/3] qemu_driver: use qemuMonitorQueryStats to extract halt poll time
On Fri, Jul 22, 2022 at 07:09:57PM +0200, Paolo Bonzini wrote: > On 7/22/22 17:43, Martin Kletzander wrote: > > As mentioned before, all these failures do not have to exit the > > function, but rather fallback to the old way. You can even create > > two new functions for the new and old implementations and then call > > them from here to make the fallback easier to spot (and code). > > More precisely, they should just "continue;" to the next iteration of > the for loop, like > > > if (!success_obj || !fail_obj) > continue; > > found = true; > > and then go fall back if found is false at the end of the loop. > > On the other hand, here: > > if (virJSONValueGetNumberUlong(success_obj, ) > < 0) > return 0; > > if (virJSONValueGetNumberUlong(fail_obj, ) < 0) > return 0; > > I am not sure about falling back, because it is really an unexpected > situation where the statistic exist but somehow the value cannot be > parsed. Then can we just "continue;" in case the value fails to parse as well? > > Paolo > > > I wanted to change this before pushing as well, but I feel like I'm > > changing too much of your code. And I also had to rebase this > > (although trivially). Would you mind just changing these few last > > things so that we can get it in before the rc0 freeze? Alright, as soon as there is a viable check decided for the virJSONValueGet* statements above, I will push a v4 with the changes you mentioned in your reviews. Thank you both for taking the time to review. > signature.asc Description: PGP signature
Re: [PATCH 3/3] qemu_driver: use qemuMonitorQueryStats to extract halt poll time
On 7/22/22 17:43, Martin Kletzander wrote: As mentioned before, all these failures do not have to exit the function, but rather fallback to the old way. You can even create two new functions for the new and old implementations and then call them from here to make the fallback easier to spot (and code). More precisely, they should just "continue;" to the next iteration of the for loop, like if (!success_obj || !fail_obj) continue; found = true; and then go fall back if found is false at the end of the loop. On the other hand, here: if (virJSONValueGetNumberUlong(success_obj, ) < 0) return 0; if (virJSONValueGetNumberUlong(fail_obj, ) < 0) return 0; I am not sure about falling back, because it is really an unexpected situation where the statistic exist but somehow the value cannot be parsed. Paolo I wanted to change this before pushing as well, but I feel like I'm changing too much of your code. And I also had to rebase this (although trivially). Would you mind just changing these few last things so that we can get it in before the rc0 freeze?
Re: [PATCH 3/3] qemu_driver: use qemuMonitorQueryStats to extract halt poll time
On Thu, Jul 14, 2022 at 11:22:04AM +0530, Amneesh Singh wrote:
Related: https://gitlab.com/libvirt/libvirt/-/issues/276
This patch uses qemuMonitorQueryStats to query "halt_poll_success_ns"
and "halt_poll_fail_ns" for every vCPU. The respective values for each
vCPU are then added together.
Signed-off-by: Amneesh Singh
---
src/qemu/qemu_driver.c | 69 +-
1 file changed, 61 insertions(+), 8 deletions(-)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index da95f947..e1c595e5 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -18066,15 +18066,68 @@ qemuDomainGetStatsCpuCgroup(virDomainObj *dom,
}
static int
-qemuDomainGetStatsCpuHaltPollTime(virDomainObj *dom,
- virTypedParamList *params)
+qemuDomainGetStatsCpuHaltPollTime(virQEMUDriver *driver,
+ virDomainObj *dom,
+ virTypedParamList *params,
+ unsigned int privflags)
{
unsigned long long haltPollSuccess = 0;
unsigned long long haltPollFail = 0;
-pid_t pid = dom->pid;
+qemuDomainObjPrivate *priv = dom->privateData;
+bool queryStatsCap = virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_QUERY_STATS);
-if (virHostCPUGetHaltPollTime(pid, , ) < 0)
-return 0;
+if (queryStatsCap && HAVE_JOB(privflags) && virDomainObjIsActive(dom)) {
+size_t i;
+qemuMonitorQueryStatsTargetType target =
QEMU_MONITOR_QUERY_STATS_TARGET_VCPU;
+qemuMonitorQueryStatsProvider *provider = NULL;
+g_autoptr(GPtrArray) providers = NULL;
+g_autoptr(GPtrArray) queried_stats = NULL;
+provider = qemuMonitorQueryStatsProviderNew(
+QEMU_MONITOR_QUERY_STATS_PROVIDER_KVM,
+QEMU_MONITOR_QUERY_STATS_NAME_HALT_POLL_SUCCESS_NS,
+QEMU_MONITOR_QUERY_STATS_NAME_HALT_POLL_FAIL_NS,
+QEMU_MONITOR_QUERY_STATS_NAME_LAST);
+
+providers = g_ptr_array_new_full(1, (GDestroyNotify)
qemuMonitorQueryStatsProviderFree);
+g_ptr_array_add(providers, provider);
+
+qemuDomainObjEnterMonitor(driver, dom);
+queried_stats = qemuMonitorQueryStats(priv->mon, target, NULL,
providers);
+qemuDomainObjExitMonitor(dom);
+
+if (!queried_stats)
+return 0;
+
+for (i = 0; i < queried_stats->len; i++) {
+unsigned long long curHaltPollSuccess, curHaltPollFail;
+GHashTable *cur_table = queried_stats->pdata[i];
+virJSONValue *success_obj, *fail_obj;
+const char *success_str = qemuMonitorQueryStatsNameTypeToString(
+QEMU_MONITOR_QUERY_STATS_NAME_HALT_POLL_SUCCESS_NS);
+const char *fail_str = qemuMonitorQueryStatsNameTypeToString(
+QEMU_MONITOR_QUERY_STATS_NAME_HALT_POLL_FAIL_NS);
+
+success_obj = g_hash_table_lookup(cur_table, success_str);
+fail_obj = g_hash_table_lookup(cur_table, fail_str);
+
+if (!success_obj || !fail_obj)
+return 0;
+
+if (virJSONValueGetNumberUlong(success_obj, ) <
0)
+return 0;
+
+if (virJSONValueGetNumberUlong(fail_obj, ) < 0)
+return 0;
+
As mentioned before, all these failures do not have to exit the function, but
rather fallback to the old way. You can even create two new functions for the
new and old implementations and then call them from here to make the fallback
easier to spot (and code).
I wanted to change this before pushing as well, but I feel like I'm changing too
much of your code. And I also had to rebase this (although trivially). Would
you mind just changing these few last things so that we can get it in before the
rc0 freeze?
Thanks and have a nice weekend,
Martin
+haltPollSuccess += curHaltPollSuccess;
+haltPollFail += curHaltPollFail;
+}
+} else {
+pid_t pid = dom->pid;
+
+if (virHostCPUGetHaltPollTime(pid, , ) <
0)
+return 0;
+}
if (virTypedParamListAddULLong(params, haltPollSuccess,
"cpu.haltpoll.success.time") < 0 ||
virTypedParamListAddULLong(params, haltPollFail, "cpu.haltpoll.fail.time")
< 0)
@@ -18087,7 +18140,7 @@ static int
qemuDomainGetStatsCpu(virQEMUDriver *driver,
virDomainObj *dom,
virTypedParamList *params,
- unsigned int privflags G_GNUC_UNUSED)
+ unsigned int privflags)
{
if (qemuDomainGetStatsCpuCgroup(dom, params) < 0)
return -1;
@@ -18095,7 +18148,7 @@ qemuDomainGetStatsCpu(virQEMUDriver *driver,
if (qemuDomainGetStatsCpuCache(driver, dom, params) < 0)
return -1;
-if (qemuDomainGetStatsCpuHaltPollTime(dom, params) < 0)
+if (qemuDomainGetStatsCpuHaltPollTime(driver, dom, params, privflags) < 0)
return -1;
return 0;
@@ -18929,7 +18982,7 @@ static virQEMUCapsFlags
[PATCH 3/3] qemu_driver: use qemuMonitorQueryStats to extract halt poll time
Related: https://gitlab.com/libvirt/libvirt/-/issues/276
This patch uses qemuMonitorQueryStats to query "halt_poll_success_ns"
and "halt_poll_fail_ns" for every vCPU. The respective values for each
vCPU are then added together.
Signed-off-by: Amneesh Singh
---
src/qemu/qemu_driver.c | 69 +-
1 file changed, 61 insertions(+), 8 deletions(-)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index da95f947..e1c595e5 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -18066,15 +18066,68 @@ qemuDomainGetStatsCpuCgroup(virDomainObj *dom,
}
static int
-qemuDomainGetStatsCpuHaltPollTime(virDomainObj *dom,
- virTypedParamList *params)
+qemuDomainGetStatsCpuHaltPollTime(virQEMUDriver *driver,
+ virDomainObj *dom,
+ virTypedParamList *params,
+ unsigned int privflags)
{
unsigned long long haltPollSuccess = 0;
unsigned long long haltPollFail = 0;
-pid_t pid = dom->pid;
+qemuDomainObjPrivate *priv = dom->privateData;
+bool queryStatsCap = virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_QUERY_STATS);
-if (virHostCPUGetHaltPollTime(pid, , ) < 0)
-return 0;
+if (queryStatsCap && HAVE_JOB(privflags) && virDomainObjIsActive(dom)) {
+size_t i;
+qemuMonitorQueryStatsTargetType target =
QEMU_MONITOR_QUERY_STATS_TARGET_VCPU;
+qemuMonitorQueryStatsProvider *provider = NULL;
+g_autoptr(GPtrArray) providers = NULL;
+g_autoptr(GPtrArray) queried_stats = NULL;
+provider = qemuMonitorQueryStatsProviderNew(
+QEMU_MONITOR_QUERY_STATS_PROVIDER_KVM,
+QEMU_MONITOR_QUERY_STATS_NAME_HALT_POLL_SUCCESS_NS,
+QEMU_MONITOR_QUERY_STATS_NAME_HALT_POLL_FAIL_NS,
+QEMU_MONITOR_QUERY_STATS_NAME_LAST);
+
+providers = g_ptr_array_new_full(1, (GDestroyNotify)
qemuMonitorQueryStatsProviderFree);
+g_ptr_array_add(providers, provider);
+
+qemuDomainObjEnterMonitor(driver, dom);
+queried_stats = qemuMonitorQueryStats(priv->mon, target, NULL,
providers);
+qemuDomainObjExitMonitor(dom);
+
+if (!queried_stats)
+return 0;
+
+for (i = 0; i < queried_stats->len; i++) {
+unsigned long long curHaltPollSuccess, curHaltPollFail;
+GHashTable *cur_table = queried_stats->pdata[i];
+virJSONValue *success_obj, *fail_obj;
+const char *success_str = qemuMonitorQueryStatsNameTypeToString(
+QEMU_MONITOR_QUERY_STATS_NAME_HALT_POLL_SUCCESS_NS);
+const char *fail_str = qemuMonitorQueryStatsNameTypeToString(
+QEMU_MONITOR_QUERY_STATS_NAME_HALT_POLL_FAIL_NS);
+
+success_obj = g_hash_table_lookup(cur_table, success_str);
+fail_obj = g_hash_table_lookup(cur_table, fail_str);
+
+if (!success_obj || !fail_obj)
+return 0;
+
+if (virJSONValueGetNumberUlong(success_obj, ) <
0)
+return 0;
+
+if (virJSONValueGetNumberUlong(fail_obj, ) < 0)
+return 0;
+
+haltPollSuccess += curHaltPollSuccess;
+haltPollFail += curHaltPollFail;
+}
+} else {
+pid_t pid = dom->pid;
+
+if (virHostCPUGetHaltPollTime(pid, , ) <
0)
+return 0;
+}
if (virTypedParamListAddULLong(params, haltPollSuccess,
"cpu.haltpoll.success.time") < 0 ||
virTypedParamListAddULLong(params, haltPollFail,
"cpu.haltpoll.fail.time") < 0)
@@ -18087,7 +18140,7 @@ static int
qemuDomainGetStatsCpu(virQEMUDriver *driver,
virDomainObj *dom,
virTypedParamList *params,
- unsigned int privflags G_GNUC_UNUSED)
+ unsigned int privflags)
{
if (qemuDomainGetStatsCpuCgroup(dom, params) < 0)
return -1;
@@ -18095,7 +18148,7 @@ qemuDomainGetStatsCpu(virQEMUDriver *driver,
if (qemuDomainGetStatsCpuCache(driver, dom, params) < 0)
return -1;
-if (qemuDomainGetStatsCpuHaltPollTime(dom, params) < 0)
+if (qemuDomainGetStatsCpuHaltPollTime(driver, dom, params, privflags) < 0)
return -1;
return 0;
@@ -18929,7 +18982,7 @@ static virQEMUCapsFlags queryDirtyRateRequired[] = {
static struct qemuDomainGetStatsWorker qemuDomainGetStatsWorkers[] = {
{ qemuDomainGetStatsState, VIR_DOMAIN_STATS_STATE, false, NULL },
-{ qemuDomainGetStatsCpu, VIR_DOMAIN_STATS_CPU_TOTAL, false, NULL },
+{ qemuDomainGetStatsCpu, VIR_DOMAIN_STATS_CPU_TOTAL, true, NULL },
{ qemuDomainGetStatsBalloon, VIR_DOMAIN_STATS_BALLOON, true, NULL },
{ qemuDomainGetStatsVcpu, VIR_DOMAIN_STATS_VCPU, true, NULL },
{ qemuDomainGetStatsInterface, VIR_DOMAIN_STATS_INTERFACE, false, NULL },
--
2.36.1
Re: [PATCH 3/3] qemu_driver: use qemuMonitorQueryStats to extract halt poll time
On 6/29/22 12:47, Amneesh Singh wrote:
> On Wed, Jun 29, 2022 at 09:03:33AM +0200, Michal Prívozník wrote:
>> On 6/28/22 22:25, Martin Kletzander wrote:
>>> On Tue, Jun 28, 2022 at 10:15:28PM +0530, Amneesh Singh wrote:
On Tue, Jun 28, 2022 at 05:23:11PM +0200, Michal Prívozník wrote:
> On 6/24/22 10:14, Amneesh Singh wrote:
>> Related: https://gitlab.com/libvirt/libvirt/-/issues/276
>>
>> This patch uses qemuMonitorQueryStats to query "halt_poll_success_ns"
>> and "halt_poll_fail_ns" for every vCPU. The respective values for each
>> vCPU are then added together.
>>
>> Signed-off-by: Amneesh Singh
>> ---
>> src/qemu/qemu_driver.c | 48
> ++
>> 1 file changed, 44 insertions(+), 4 deletions(-)
>>
>> diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
>> index 3b5c3db6..0a2be6d3 100644
>> --- a/src/qemu/qemu_driver.c
>> +++ b/src/qemu/qemu_driver.c
>> @@ -18057,10 +18057,50 @@
> qemuDomainGetStatsCpuHaltPollTime(virDomainObj *dom,
>> {
>> unsigned long long haltPollSuccess = 0;
>> unsigned long long haltPollFail = 0;
>> - pid_t pid = dom->pid;
>> + qemuDomainObjPrivate *priv = dom->privateData;
>> + bool canQueryStats = virQEMUCapsGet(priv->qemuCaps,
> QEMU_CAPS_QUERY_STATS);
>
> Is this variable really needed? I mean, we can just:
>
> if (virQEMUCapsGet(...) {
>
> } else {
>
> }
>
> But if you want to avoid long lines, then perhaps rename the variable to
> queryStatsCap? This way it's more obvious what the variable reflects.
> Stats can be queried in both cases ;-)
Sure, that sounds doable :)
>
>>
>> - if (virHostCPUGetHaltPollTime(pid, ,
> ) < 0)
>> - return 0;
>> + if (!canQueryStats) {
>> + pid_t pid = dom->pid;
>> +
>> + if (virHostCPUGetHaltPollTime(pid, ,
> ) < 0)
>> + return 0;
>> + } else {
>> + size_t i;
>> + qemuMonitorQueryStatsTargetType target =
> QEMU_MONITOR_QUERY_STATS_TARGET_VCPU;
>> + qemuMonitorQueryStatsProvider *provider = NULL;
>> + g_autoptr(GPtrArray) providers = NULL;
>> + g_autoptr(GPtrArray) queried_stats = NULL;
>> + const char *success_str = "halt_poll_success_ns";
>> + const char *fail_str = "halt_poll_fail_ns";
>> +
>> + provider =
> qemuMonitorQueryStatsProviderNew(QEMU_MONITOR_QUERY_STATS_PROVIDER_KVM);
>> + provider->names = g_new0(char *, 3);
>> + provider->names[0] = g_strdup(success_str),
> provider->names[1] = g_strdup(fail_str);
>
> I'm starting to wonder whether this is a good interface. These ->names[]
> array is never changed, so maybe we can have it as a NULL terminated
> list of constant strings? For instance:
>
> provider = qemuMonitorQueryStatsProviderNew();
> provider->names = {"halt_poll_success_ns", "halt_poll_fail_ns", NULL};
Well, cannot really assign char ** with a scalar initialization, but
what might work is something like
const char *names[] = { success_str, fail_str, NULL };
provider =
qemuMonitorQueryStatsProviderNew(QEMU_MONITOR_QUERY_STATS_PROVIDER_KVM);
provider->names = g_strdupv((char **) names);
>>
>> Yep, I made too much of a shortcut.
>>
>>>
>>> I think what Michal was trying to say is that since you do not change
>>> the array anywhere, there is no need for that to be a dynamically
>>> allocated array that needs to be freed. I, however, am not 100% if you
>>> are going to need this to be dynamic and whether you will be changing
>>> these arrays.
>>
>> Yes. I'm not exactly sure why those strings have to be strdup-ed(). I
>> mean, from the conceptual POV. They are not changed anywhere. And now
>> that I think about it - why they have to be strings at all? They have to
>> be strings at the monitor level, because that's what QEMU expects,
>> however, I can imagine that in our code, in its upper layers those can
>> be just part of enum. Or even better - a bitmask - just like
>> virQEMUCaps. For instance:
>>
>> qemu_monitor.h:
>>
>> typedef enum {
>> QEMU_MONITOR_QUERY_STATS_HALT_POLL_SUCCESS_NS,
>> QEMU_MONITOR_QUERY_STATS_HALT_POLL_FAIL_NS,
>>
>> QEMU_MONITOR_QUERY_STATS_LAST
>> } qemuMonitorQueryStatsFlags;
>>
>>
>> qemu_monitor.c:
>>
>> VIR_ENUM_DECL(qemuMonitorQueryStatsFlags);
>> VIR_ENUM_IMPL(qemuMonitorQueryStatsFlags,
>> QEMU_MONITOR_QUERY_STATS_LAST,
>> "halt_poll_success_ns",
>> "halt_poll_fail_ns"
>> );
>>
>> and when constructing the monitor command
>> qemuMonitorQueryStatsFlagsTypeToString() translates those
>> QEMU_MONITOR_QUERY_STATS_* enum members into their corresponding string
>> representation.
>>
>> Now, qemuMonitorQueryStatsProviderNew() could then behave like
Re: [PATCH 3/3] qemu_driver: use qemuMonitorQueryStats to extract halt poll time
On Wed, Jun 29, 2022 at 09:03:33AM +0200, Michal Prívozník wrote:
> On 6/28/22 22:25, Martin Kletzander wrote:
> > On Tue, Jun 28, 2022 at 10:15:28PM +0530, Amneesh Singh wrote:
> >> On Tue, Jun 28, 2022 at 05:23:11PM +0200, Michal Prívozník wrote:
> >>> On 6/24/22 10:14, Amneesh Singh wrote:
> >>> > Related: https://gitlab.com/libvirt/libvirt/-/issues/276
> >>> >
> >>> > This patch uses qemuMonitorQueryStats to query "halt_poll_success_ns"
> >>> > and "halt_poll_fail_ns" for every vCPU. The respective values for each
> >>> > vCPU are then added together.
> >>> >
> >>> > Signed-off-by: Amneesh Singh
> >>> > ---
> >>> > src/qemu/qemu_driver.c | 48
> >>> ++
> >>> > 1 file changed, 44 insertions(+), 4 deletions(-)
> >>> >
> >>> > diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
> >>> > index 3b5c3db6..0a2be6d3 100644
> >>> > --- a/src/qemu/qemu_driver.c
> >>> > +++ b/src/qemu/qemu_driver.c
> >>> > @@ -18057,10 +18057,50 @@
> >>> qemuDomainGetStatsCpuHaltPollTime(virDomainObj *dom,
> >>> > {
> >>> > unsigned long long haltPollSuccess = 0;
> >>> > unsigned long long haltPollFail = 0;
> >>> > - pid_t pid = dom->pid;
> >>> > + qemuDomainObjPrivate *priv = dom->privateData;
> >>> > + bool canQueryStats = virQEMUCapsGet(priv->qemuCaps,
> >>> QEMU_CAPS_QUERY_STATS);
> >>>
> >>> Is this variable really needed? I mean, we can just:
> >>>
> >>> if (virQEMUCapsGet(...) {
> >>>
> >>> } else {
> >>>
> >>> }
> >>>
> >>> But if you want to avoid long lines, then perhaps rename the variable to
> >>> queryStatsCap? This way it's more obvious what the variable reflects.
> >>> Stats can be queried in both cases ;-)
> >> Sure, that sounds doable :)
> >>>
> >>> >
> >>> > - if (virHostCPUGetHaltPollTime(pid, ,
> >>> ) < 0)
> >>> > - return 0;
> >>> > + if (!canQueryStats) {
> >>> > + pid_t pid = dom->pid;
> >>> > +
> >>> > + if (virHostCPUGetHaltPollTime(pid, ,
> >>> ) < 0)
> >>> > + return 0;
> >>> > + } else {
> >>> > + size_t i;
> >>> > + qemuMonitorQueryStatsTargetType target =
> >>> QEMU_MONITOR_QUERY_STATS_TARGET_VCPU;
> >>> > + qemuMonitorQueryStatsProvider *provider = NULL;
> >>> > + g_autoptr(GPtrArray) providers = NULL;
> >>> > + g_autoptr(GPtrArray) queried_stats = NULL;
> >>> > + const char *success_str = "halt_poll_success_ns";
> >>> > + const char *fail_str = "halt_poll_fail_ns";
> >>> > +
> >>> > + provider =
> >>> qemuMonitorQueryStatsProviderNew(QEMU_MONITOR_QUERY_STATS_PROVIDER_KVM);
> >>> > + provider->names = g_new0(char *, 3);
> >>> > + provider->names[0] = g_strdup(success_str),
> >>> provider->names[1] = g_strdup(fail_str);
> >>>
> >>> I'm starting to wonder whether this is a good interface. These ->names[]
> >>> array is never changed, so maybe we can have it as a NULL terminated
> >>> list of constant strings? For instance:
> >>>
> >>> provider = qemuMonitorQueryStatsProviderNew();
> >>> provider->names = {"halt_poll_success_ns", "halt_poll_fail_ns", NULL};
> >> Well, cannot really assign char ** with a scalar initialization, but
> >> what might work is something like
> >>
> >> const char *names[] = { success_str, fail_str, NULL };
> >>
> >> provider =
> >> qemuMonitorQueryStatsProviderNew(QEMU_MONITOR_QUERY_STATS_PROVIDER_KVM);
> >> provider->names = g_strdupv((char **) names);
> >>
>
> Yep, I made too much of a shortcut.
>
> >
> > I think what Michal was trying to say is that since you do not change
> > the array anywhere, there is no need for that to be a dynamically
> > allocated array that needs to be freed. I, however, am not 100% if you
> > are going to need this to be dynamic and whether you will be changing
> > these arrays.
>
> Yes. I'm not exactly sure why those strings have to be strdup-ed(). I
> mean, from the conceptual POV. They are not changed anywhere. And now
> that I think about it - why they have to be strings at all? They have to
> be strings at the monitor level, because that's what QEMU expects,
> however, I can imagine that in our code, in its upper layers those can
> be just part of enum. Or even better - a bitmask - just like
> virQEMUCaps. For instance:
>
> qemu_monitor.h:
>
> typedef enum {
> QEMU_MONITOR_QUERY_STATS_HALT_POLL_SUCCESS_NS,
> QEMU_MONITOR_QUERY_STATS_HALT_POLL_FAIL_NS,
>
> QEMU_MONITOR_QUERY_STATS_LAST
> } qemuMonitorQueryStatsFlags;
>
>
> qemu_monitor.c:
>
> VIR_ENUM_DECL(qemuMonitorQueryStatsFlags);
> VIR_ENUM_IMPL(qemuMonitorQueryStatsFlags,
> QEMU_MONITOR_QUERY_STATS_LAST,
> "halt_poll_success_ns",
> "halt_poll_fail_ns"
> );
>
> and when constructing the monitor command
> qemuMonitorQueryStatsFlagsTypeToString() translates those
> QEMU_MONITOR_QUERY_STATS_* enum members into their corresponding string
> representation.
>
> Now, qemuMonitorQueryStatsProviderNew() could then behave like this:
>
> provider =
>
Re: [PATCH 3/3] qemu_driver: use qemuMonitorQueryStats to extract halt poll time
On 6/28/22 22:25, Martin Kletzander wrote:
> On Tue, Jun 28, 2022 at 10:15:28PM +0530, Amneesh Singh wrote:
>> On Tue, Jun 28, 2022 at 05:23:11PM +0200, Michal Prívozník wrote:
>>> On 6/24/22 10:14, Amneesh Singh wrote:
>>> > Related: https://gitlab.com/libvirt/libvirt/-/issues/276
>>> >
>>> > This patch uses qemuMonitorQueryStats to query "halt_poll_success_ns"
>>> > and "halt_poll_fail_ns" for every vCPU. The respective values for each
>>> > vCPU are then added together.
>>> >
>>> > Signed-off-by: Amneesh Singh
>>> > ---
>>> > src/qemu/qemu_driver.c | 48
>>> ++
>>> > 1 file changed, 44 insertions(+), 4 deletions(-)
>>> >
>>> > diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
>>> > index 3b5c3db6..0a2be6d3 100644
>>> > --- a/src/qemu/qemu_driver.c
>>> > +++ b/src/qemu/qemu_driver.c
>>> > @@ -18057,10 +18057,50 @@
>>> qemuDomainGetStatsCpuHaltPollTime(virDomainObj *dom,
>>> > {
>>> > unsigned long long haltPollSuccess = 0;
>>> > unsigned long long haltPollFail = 0;
>>> > - pid_t pid = dom->pid;
>>> > + qemuDomainObjPrivate *priv = dom->privateData;
>>> > + bool canQueryStats = virQEMUCapsGet(priv->qemuCaps,
>>> QEMU_CAPS_QUERY_STATS);
>>>
>>> Is this variable really needed? I mean, we can just:
>>>
>>> if (virQEMUCapsGet(...) {
>>>
>>> } else {
>>>
>>> }
>>>
>>> But if you want to avoid long lines, then perhaps rename the variable to
>>> queryStatsCap? This way it's more obvious what the variable reflects.
>>> Stats can be queried in both cases ;-)
>> Sure, that sounds doable :)
>>>
>>> >
>>> > - if (virHostCPUGetHaltPollTime(pid, ,
>>> ) < 0)
>>> > - return 0;
>>> > + if (!canQueryStats) {
>>> > + pid_t pid = dom->pid;
>>> > +
>>> > + if (virHostCPUGetHaltPollTime(pid, ,
>>> ) < 0)
>>> > + return 0;
>>> > + } else {
>>> > + size_t i;
>>> > + qemuMonitorQueryStatsTargetType target =
>>> QEMU_MONITOR_QUERY_STATS_TARGET_VCPU;
>>> > + qemuMonitorQueryStatsProvider *provider = NULL;
>>> > + g_autoptr(GPtrArray) providers = NULL;
>>> > + g_autoptr(GPtrArray) queried_stats = NULL;
>>> > + const char *success_str = "halt_poll_success_ns";
>>> > + const char *fail_str = "halt_poll_fail_ns";
>>> > +
>>> > + provider =
>>> qemuMonitorQueryStatsProviderNew(QEMU_MONITOR_QUERY_STATS_PROVIDER_KVM);
>>> > + provider->names = g_new0(char *, 3);
>>> > + provider->names[0] = g_strdup(success_str),
>>> provider->names[1] = g_strdup(fail_str);
>>>
>>> I'm starting to wonder whether this is a good interface. These ->names[]
>>> array is never changed, so maybe we can have it as a NULL terminated
>>> list of constant strings? For instance:
>>>
>>> provider = qemuMonitorQueryStatsProviderNew();
>>> provider->names = {"halt_poll_success_ns", "halt_poll_fail_ns", NULL};
>> Well, cannot really assign char ** with a scalar initialization, but
>> what might work is something like
>>
>> const char *names[] = { success_str, fail_str, NULL };
>>
>> provider =
>> qemuMonitorQueryStatsProviderNew(QEMU_MONITOR_QUERY_STATS_PROVIDER_KVM);
>> provider->names = g_strdupv((char **) names);
>>
Yep, I made too much of a shortcut.
>
> I think what Michal was trying to say is that since you do not change
> the array anywhere, there is no need for that to be a dynamically
> allocated array that needs to be freed. I, however, am not 100% if you
> are going to need this to be dynamic and whether you will be changing
> these arrays.
Yes. I'm not exactly sure why those strings have to be strdup-ed(). I
mean, from the conceptual POV. They are not changed anywhere. And now
that I think about it - why they have to be strings at all? They have to
be strings at the monitor level, because that's what QEMU expects,
however, I can imagine that in our code, in its upper layers those can
be just part of enum. Or even better - a bitmask - just like
virQEMUCaps. For instance:
qemu_monitor.h:
typedef enum {
QEMU_MONITOR_QUERY_STATS_HALT_POLL_SUCCESS_NS,
QEMU_MONITOR_QUERY_STATS_HALT_POLL_FAIL_NS,
QEMU_MONITOR_QUERY_STATS_LAST
} qemuMonitorQueryStatsFlags;
qemu_monitor.c:
VIR_ENUM_DECL(qemuMonitorQueryStatsFlags);
VIR_ENUM_IMPL(qemuMonitorQueryStatsFlags,
QEMU_MONITOR_QUERY_STATS_LAST,
"halt_poll_success_ns",
"halt_poll_fail_ns"
);
and when constructing the monitor command
qemuMonitorQueryStatsFlagsTypeToString() translates those
QEMU_MONITOR_QUERY_STATS_* enum members into their corresponding string
representation.
Now, qemuMonitorQueryStatsProviderNew() could then behave like this:
provider =
qemuMonitorQueryStatsProviderNew(QEMU_MONITOR_QUERY_STATS_PROVIDER_KVM);
virBitmapSet(provider->names, QEMU_MONITOR_QUERY_STATS_HALT_POLL_SUCCESS_NS);
virBitmapSet(provider->names, QEMU_MONITOR_QUERY_STATS_HALT_POLL_FAIL_NS);
Alternatively, we can have all of that in the
qemuMonitorQueryStatsProviderNew() call with help of
Re: [PATCH 3/3] qemu_driver: use qemuMonitorQueryStats to extract halt poll time
On Tue, Jun 28, 2022 at 10:25:54PM +0200, Martin Kletzander wrote:
> On Tue, Jun 28, 2022 at 10:15:28PM +0530, Amneesh Singh wrote:
> > On Tue, Jun 28, 2022 at 05:23:11PM +0200, Michal Prívozník wrote:
> > > On 6/24/22 10:14, Amneesh Singh wrote:
> > > > Related: https://gitlab.com/libvirt/libvirt/-/issues/276
> > > >
> > > > This patch uses qemuMonitorQueryStats to query "halt_poll_success_ns"
> > > > and "halt_poll_fail_ns" for every vCPU. The respective values for each
> > > > vCPU are then added together.
> > > >
> > > > Signed-off-by: Amneesh Singh
> > > > ---
> > > > src/qemu/qemu_driver.c | 48 ++
> > > > 1 file changed, 44 insertions(+), 4 deletions(-)
> > > >
> > > > diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
> > > > index 3b5c3db6..0a2be6d3 100644
> > > > --- a/src/qemu/qemu_driver.c
> > > > +++ b/src/qemu/qemu_driver.c
> > > > @@ -18057,10 +18057,50 @@
> > > > qemuDomainGetStatsCpuHaltPollTime(virDomainObj *dom,
> > > > {
> > > > unsigned long long haltPollSuccess = 0;
> > > > unsigned long long haltPollFail = 0;
> > > > -pid_t pid = dom->pid;
> > > > +qemuDomainObjPrivate *priv = dom->privateData;
> > > > +bool canQueryStats = virQEMUCapsGet(priv->qemuCaps,
> > > > QEMU_CAPS_QUERY_STATS);
> > >
> > > Is this variable really needed? I mean, we can just:
> > >
> > > if (virQEMUCapsGet(...) {
> > >
> > > } else {
> > >
> > > }
> > >
> > > But if you want to avoid long lines, then perhaps rename the variable to
> > > queryStatsCap? This way it's more obvious what the variable reflects.
> > > Stats can be queried in both cases ;-)
> > Sure, that sounds doable :)
> > >
> > > >
> > > > -if (virHostCPUGetHaltPollTime(pid, ,
> > > > ) < 0)
> > > > -return 0;
> > > > +if (!canQueryStats) {
> > > > +pid_t pid = dom->pid;
> > > > +
> > > > +if (virHostCPUGetHaltPollTime(pid, ,
> > > > ) < 0)
> > > > +return 0;
> > > > +} else {
> > > > +size_t i;
> > > > +qemuMonitorQueryStatsTargetType target =
> > > > QEMU_MONITOR_QUERY_STATS_TARGET_VCPU;
> > > > +qemuMonitorQueryStatsProvider *provider = NULL;
> > > > +g_autoptr(GPtrArray) providers = NULL;
> > > > +g_autoptr(GPtrArray) queried_stats = NULL;
> > > > +const char *success_str = "halt_poll_success_ns";
> > > > +const char *fail_str = "halt_poll_fail_ns";
> > > > +
> > > > +provider =
> > > > qemuMonitorQueryStatsProviderNew(QEMU_MONITOR_QUERY_STATS_PROVIDER_KVM);
> > > > +provider->names = g_new0(char *, 3);
> > > > +provider->names[0] = g_strdup(success_str), provider->names[1]
> > > > = g_strdup(fail_str);
> > >
> > > I'm starting to wonder whether this is a good interface. These ->names[]
> > > array is never changed, so maybe we can have it as a NULL terminated
> > > list of constant strings? For instance:
> > >
> > > provider = qemuMonitorQueryStatsProviderNew();
> > > provider->names = {"halt_poll_success_ns", "halt_poll_fail_ns", NULL};
> > Well, cannot really assign char ** with a scalar initialization, but
> > what might work is something like
> >
> > const char *names[] = { success_str, fail_str, NULL };
> >
> > provider =
> > qemuMonitorQueryStatsProviderNew(QEMU_MONITOR_QUERY_STATS_PROVIDER_KVM);
> > provider->names = g_strdupv((char **) names);
> >
>
> I think what Michal was trying to say is that since you do not change
> the array anywhere, there is no need for that to be a dynamically
> allocated array that needs to be freed. I, however, am not 100% if you
> are going to need this to be dynamic and whether you will be changing
> these arrays.
I don't think there will be any need to have the array mutable, in
which case, maybe something like this should work
provider->names = (const char *[]){ success_str, fail_str, NULL };
provided that provider->names is changed to const char ** or the same
thing can be done with string literals (or non const string variables)
provided that the cast is (char *[]) and names is still char **.
Compound literals are part of C99 though.
>
> > Another thought was using GStrvBuilder but it is not avaiable as per
> > GLIB_VERSION_MAX.
> >
> > I too think that the current approach is not very nice. A variadic
> > function similar to g_strv_builder_add_many that initializes a
> > char ** would be nice.
> > >
>
> If that is something that helps, then we can write it ourselves and only
> use our implementation if glib is too old.
>
> > > > +
> > > > +providers = g_ptr_array_new_full(1, (GDestroyNotify)
> > > > qemuMonitorQueryStatsProviderFree);
> > > > +g_ptr_array_add(providers, provider);
> > > > +
> > > > +queried_stats = qemuMonitorQueryStats(priv->mon, target, NULL,
> > > > providers);
> > >
> > > This issues monitor command without proper checks. Firstly, you need to
> > > check whether job acquiring (that s/false/true/ change done in the last
> >
Re: [PATCH 3/3] qemu_driver: use qemuMonitorQueryStats to extract halt poll time
On Tue, Jun 28, 2022 at 10:15:28PM +0530, Amneesh Singh wrote:
On Tue, Jun 28, 2022 at 05:23:11PM +0200, Michal Prívozník wrote:
On 6/24/22 10:14, Amneesh Singh wrote:
> Related: https://gitlab.com/libvirt/libvirt/-/issues/276
>
> This patch uses qemuMonitorQueryStats to query "halt_poll_success_ns"
> and "halt_poll_fail_ns" for every vCPU. The respective values for each
> vCPU are then added together.
>
> Signed-off-by: Amneesh Singh
> ---
> src/qemu/qemu_driver.c | 48 ++
> 1 file changed, 44 insertions(+), 4 deletions(-)
>
> diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
> index 3b5c3db6..0a2be6d3 100644
> --- a/src/qemu/qemu_driver.c
> +++ b/src/qemu/qemu_driver.c
> @@ -18057,10 +18057,50 @@ qemuDomainGetStatsCpuHaltPollTime(virDomainObj *dom,
> {
> unsigned long long haltPollSuccess = 0;
> unsigned long long haltPollFail = 0;
> -pid_t pid = dom->pid;
> +qemuDomainObjPrivate *priv = dom->privateData;
> +bool canQueryStats = virQEMUCapsGet(priv->qemuCaps,
QEMU_CAPS_QUERY_STATS);
Is this variable really needed? I mean, we can just:
if (virQEMUCapsGet(...) {
} else {
}
But if you want to avoid long lines, then perhaps rename the variable to
queryStatsCap? This way it's more obvious what the variable reflects.
Stats can be queried in both cases ;-)
Sure, that sounds doable :)
>
> -if (virHostCPUGetHaltPollTime(pid, , ) < 0)
> -return 0;
> +if (!canQueryStats) {
> +pid_t pid = dom->pid;
> +
> +if (virHostCPUGetHaltPollTime(pid, , )
< 0)
> +return 0;
> +} else {
> +size_t i;
> +qemuMonitorQueryStatsTargetType target =
QEMU_MONITOR_QUERY_STATS_TARGET_VCPU;
> +qemuMonitorQueryStatsProvider *provider = NULL;
> +g_autoptr(GPtrArray) providers = NULL;
> +g_autoptr(GPtrArray) queried_stats = NULL;
> +const char *success_str = "halt_poll_success_ns";
> +const char *fail_str = "halt_poll_fail_ns";
> +
> +provider =
qemuMonitorQueryStatsProviderNew(QEMU_MONITOR_QUERY_STATS_PROVIDER_KVM);
> +provider->names = g_new0(char *, 3);
> +provider->names[0] = g_strdup(success_str), provider->names[1] =
g_strdup(fail_str);
I'm starting to wonder whether this is a good interface. These ->names[]
array is never changed, so maybe we can have it as a NULL terminated
list of constant strings? For instance:
provider = qemuMonitorQueryStatsProviderNew();
provider->names = {"halt_poll_success_ns", "halt_poll_fail_ns", NULL};
Well, cannot really assign char ** with a scalar initialization, but
what might work is something like
const char *names[] = { success_str, fail_str, NULL };
provider =
qemuMonitorQueryStatsProviderNew(QEMU_MONITOR_QUERY_STATS_PROVIDER_KVM);
provider->names = g_strdupv((char **) names);
I think what Michal was trying to say is that since you do not change
the array anywhere, there is no need for that to be a dynamically
allocated array that needs to be freed. I, however, am not 100% if you
are going to need this to be dynamic and whether you will be changing
these arrays.
Another thought was using GStrvBuilder but it is not avaiable as per
GLIB_VERSION_MAX.
I too think that the current approach is not very nice. A variadic
function similar to g_strv_builder_add_many that initializes a
char ** would be nice.
If that is something that helps, then we can write it ourselves and only
use our implementation if glib is too old.
> +
> +providers = g_ptr_array_new_full(1, (GDestroyNotify)
qemuMonitorQueryStatsProviderFree);
> +g_ptr_array_add(providers, provider);
> +
> +queried_stats = qemuMonitorQueryStats(priv->mon, target, NULL,
providers);
This issues monitor command without proper checks. Firstly, you need to
check whether job acquiring (that s/false/true/ change done in the last
hunk) was successful and the domain is still running:
if (!HAVE_JOB(privflags) || !virDomainObjIsActive(dom)) {
/* code here */
}
Then, you need to so called "enter the monitor" and "exit the monitor".
So what happens here is that @vm is when this function is called. Now,
issuing a monitor command with the domain object lock held is
potentially very dangerous because if QEMU takes long time to reply (or
it doesn't reply at all) the domain object is locked an can't be
destroyed (virsh destroy) - because the first thing that
qemuDomainDestroyFlags() does is locking the domain object. Also, you
want to make sure no other thread is currently talking on the monitor -
so the monitor has to be locked too. We have two convenient functions
for these operations:
qemuDomainObjEnterMonitor()
qemuDomainObjExitMonitor()
This is all described in more detail in
docs/kbase/internals/qemu-threads.rst.
Having said all of this, I think we can fallback to the current behavior
if job wasn't acquired successfully. Therefore the condition at the very
beginning might look like this:
if
Re: [PATCH 3/3] qemu_driver: use qemuMonitorQueryStats to extract halt poll time
On Tue, Jun 28, 2022 at 05:23:11PM +0200, Michal Prívozník wrote:
> On 6/24/22 10:14, Amneesh Singh wrote:
> > Related: https://gitlab.com/libvirt/libvirt/-/issues/276
> >
> > This patch uses qemuMonitorQueryStats to query "halt_poll_success_ns"
> > and "halt_poll_fail_ns" for every vCPU. The respective values for each
> > vCPU are then added together.
> >
> > Signed-off-by: Amneesh Singh
> > ---
> > src/qemu/qemu_driver.c | 48 ++
> > 1 file changed, 44 insertions(+), 4 deletions(-)
> >
> > diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
> > index 3b5c3db6..0a2be6d3 100644
> > --- a/src/qemu/qemu_driver.c
> > +++ b/src/qemu/qemu_driver.c
> > @@ -18057,10 +18057,50 @@ qemuDomainGetStatsCpuHaltPollTime(virDomainObj
> > *dom,
> > {
> > unsigned long long haltPollSuccess = 0;
> > unsigned long long haltPollFail = 0;
> > -pid_t pid = dom->pid;
> > +qemuDomainObjPrivate *priv = dom->privateData;
> > +bool canQueryStats = virQEMUCapsGet(priv->qemuCaps,
> > QEMU_CAPS_QUERY_STATS);
>
> Is this variable really needed? I mean, we can just:
>
> if (virQEMUCapsGet(...) {
>
> } else {
>
> }
>
> But if you want to avoid long lines, then perhaps rename the variable to
> queryStatsCap? This way it's more obvious what the variable reflects.
> Stats can be queried in both cases ;-)
Sure, that sounds doable :)
>
> >
> > -if (virHostCPUGetHaltPollTime(pid, , ) <
> > 0)
> > -return 0;
> > +if (!canQueryStats) {
> > +pid_t pid = dom->pid;
> > +
> > +if (virHostCPUGetHaltPollTime(pid, ,
> > ) < 0)
> > +return 0;
> > +} else {
> > +size_t i;
> > +qemuMonitorQueryStatsTargetType target =
> > QEMU_MONITOR_QUERY_STATS_TARGET_VCPU;
> > +qemuMonitorQueryStatsProvider *provider = NULL;
> > +g_autoptr(GPtrArray) providers = NULL;
> > +g_autoptr(GPtrArray) queried_stats = NULL;
> > +const char *success_str = "halt_poll_success_ns";
> > +const char *fail_str = "halt_poll_fail_ns";
> > +
> > +provider =
> > qemuMonitorQueryStatsProviderNew(QEMU_MONITOR_QUERY_STATS_PROVIDER_KVM);
> > +provider->names = g_new0(char *, 3);
> > +provider->names[0] = g_strdup(success_str), provider->names[1] =
> > g_strdup(fail_str);
>
> I'm starting to wonder whether this is a good interface. These ->names[]
> array is never changed, so maybe we can have it as a NULL terminated
> list of constant strings? For instance:
>
> provider = qemuMonitorQueryStatsProviderNew();
> provider->names = {"halt_poll_success_ns", "halt_poll_fail_ns", NULL};
Well, cannot really assign char ** with a scalar initialization, but
what might work is something like
const char *names[] = { success_str, fail_str, NULL };
provider =
qemuMonitorQueryStatsProviderNew(QEMU_MONITOR_QUERY_STATS_PROVIDER_KVM);
provider->names = g_strdupv((char **) names);
Another thought was using GStrvBuilder but it is not avaiable as per
GLIB_VERSION_MAX.
I too think that the current approach is not very nice. A variadic
function similar to g_strv_builder_add_many that initializes a
char ** would be nice.
>
> > +
> > +providers = g_ptr_array_new_full(1, (GDestroyNotify)
> > qemuMonitorQueryStatsProviderFree);
> > +g_ptr_array_add(providers, provider);
> > +
> > +queried_stats = qemuMonitorQueryStats(priv->mon, target, NULL,
> > providers);
>
> This issues monitor command without proper checks. Firstly, you need to
> check whether job acquiring (that s/false/true/ change done in the last
> hunk) was successful and the domain is still running:
>
> if (!HAVE_JOB(privflags) || !virDomainObjIsActive(dom)) {
>/* code here */
> }
>
> Then, you need to so called "enter the monitor" and "exit the monitor".
> So what happens here is that @vm is when this function is called. Now,
> issuing a monitor command with the domain object lock held is
> potentially very dangerous because if QEMU takes long time to reply (or
> it doesn't reply at all) the domain object is locked an can't be
> destroyed (virsh destroy) - because the first thing that
> qemuDomainDestroyFlags() does is locking the domain object. Also, you
> want to make sure no other thread is currently talking on the monitor -
> so the monitor has to be locked too. We have two convenient functions
> for these operations:
>
> qemuDomainObjEnterMonitor()
> qemuDomainObjExitMonitor()
>
> This is all described in more detail in
> docs/kbase/internals/qemu-threads.rst.
>
> Having said all of this, I think we can fallback to the current behavior
> if job wasn't acquired successfully. Therefore the condition at the very
> beginning might look like this:
>
> if (queryStatsCap && HAVE_JOB() && virDomainObjIsActive()) {
> ...
> qemuDomainEnterMonitor();
> qemuMonitorQueryStats();
> qemuDomainExitMonitor();
> } else {
> virHostCPUGetHaltPollTime();
> }
>
I see that makes sense. I shall do the
Re: [PATCH 3/3] qemu_driver: use qemuMonitorQueryStats to extract halt poll time
On 6/24/22 10:14, Amneesh Singh wrote:
> Related: https://gitlab.com/libvirt/libvirt/-/issues/276
>
> This patch uses qemuMonitorQueryStats to query "halt_poll_success_ns"
> and "halt_poll_fail_ns" for every vCPU. The respective values for each
> vCPU are then added together.
>
> Signed-off-by: Amneesh Singh
> ---
> src/qemu/qemu_driver.c | 48 ++
> 1 file changed, 44 insertions(+), 4 deletions(-)
>
> diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
> index 3b5c3db6..0a2be6d3 100644
> --- a/src/qemu/qemu_driver.c
> +++ b/src/qemu/qemu_driver.c
> @@ -18057,10 +18057,50 @@ qemuDomainGetStatsCpuHaltPollTime(virDomainObj *dom,
> {
> unsigned long long haltPollSuccess = 0;
> unsigned long long haltPollFail = 0;
> -pid_t pid = dom->pid;
> +qemuDomainObjPrivate *priv = dom->privateData;
> +bool canQueryStats = virQEMUCapsGet(priv->qemuCaps,
> QEMU_CAPS_QUERY_STATS);
Is this variable really needed? I mean, we can just:
if (virQEMUCapsGet(...) {
} else {
}
But if you want to avoid long lines, then perhaps rename the variable to
queryStatsCap? This way it's more obvious what the variable reflects.
Stats can be queried in both cases ;-)
>
> -if (virHostCPUGetHaltPollTime(pid, , ) < 0)
> -return 0;
> +if (!canQueryStats) {
> +pid_t pid = dom->pid;
> +
> +if (virHostCPUGetHaltPollTime(pid, , )
> < 0)
> +return 0;
> +} else {
> +size_t i;
> +qemuMonitorQueryStatsTargetType target =
> QEMU_MONITOR_QUERY_STATS_TARGET_VCPU;
> +qemuMonitorQueryStatsProvider *provider = NULL;
> +g_autoptr(GPtrArray) providers = NULL;
> +g_autoptr(GPtrArray) queried_stats = NULL;
> +const char *success_str = "halt_poll_success_ns";
> +const char *fail_str = "halt_poll_fail_ns";
> +
> +provider =
> qemuMonitorQueryStatsProviderNew(QEMU_MONITOR_QUERY_STATS_PROVIDER_KVM);
> +provider->names = g_new0(char *, 3);
> +provider->names[0] = g_strdup(success_str), provider->names[1] =
> g_strdup(fail_str);
I'm starting to wonder whether this is a good interface. These ->names[]
array is never changed, so maybe we can have it as a NULL terminated
list of constant strings? For instance:
provider = qemuMonitorQueryStatsProviderNew();
provider->names = {"halt_poll_success_ns", "halt_poll_fail_ns", NULL};
> +
> +providers = g_ptr_array_new_full(1, (GDestroyNotify)
> qemuMonitorQueryStatsProviderFree);
> +g_ptr_array_add(providers, provider);
> +
> +queried_stats = qemuMonitorQueryStats(priv->mon, target, NULL,
> providers);
This issues monitor command without proper checks. Firstly, you need to
check whether job acquiring (that s/false/true/ change done in the last
hunk) was successful and the domain is still running:
if (!HAVE_JOB(privflags) || !virDomainObjIsActive(dom)) {
/* code here */
}
Then, you need to so called "enter the monitor" and "exit the monitor".
So what happens here is that @vm is when this function is called. Now,
issuing a monitor command with the domain object lock held is
potentially very dangerous because if QEMU takes long time to reply (or
it doesn't reply at all) the domain object is locked an can't be
destroyed (virsh destroy) - because the first thing that
qemuDomainDestroyFlags() does is locking the domain object. Also, you
want to make sure no other thread is currently talking on the monitor -
so the monitor has to be locked too. We have two convenient functions
for these operations:
qemuDomainObjEnterMonitor()
qemuDomainObjExitMonitor()
This is all described in more detail in
docs/kbase/internals/qemu-threads.rst.
Having said all of this, I think we can fallback to the current behavior
if job wasn't acquired successfully. Therefore the condition at the very
beginning might look like this:
if (queryStatsCap && HAVE_JOB() && virDomainObjIsActive()) {
...
qemuDomainEnterMonitor();
qemuMonitorQueryStats();
qemuDomainExitMonitor();
} else {
virHostCPUGetHaltPollTime();
}
> +
> +if (!queried_stats)
> +return 0;
> +
> +for (i = 0; i < queried_stats->len; i++) {
> +unsigned long long curHaltPollSuccess, curHaltPollFail;
> +GHashTable *cur_table = queried_stats->pdata[i];
> +virJSONValue *success, *fail;
> +
> +success = g_hash_table_lookup(cur_table, success_str);
> +fail = g_hash_table_lookup(cur_table, fail_str);
> +
> +ignore_value(virJSONValueGetNumberUlong(success,
> ));
> +ignore_value(virJSONValueGetNumberUlong(fail, ));
I don't think this is a safe thing to do. What if either of @success or
@fail is NULL? That can happen when QEMU didn't return requested member.
True, at that point the 'query-stats' command should have failed, but I
think it's more defensive if we checked these pointers. My worry is that
virJSONValueGetNumberUlong() dereferences
[PATCH 3/3] qemu_driver: use qemuMonitorQueryStats to extract halt poll time
Related: https://gitlab.com/libvirt/libvirt/-/issues/276
This patch uses qemuMonitorQueryStats to query "halt_poll_success_ns"
and "halt_poll_fail_ns" for every vCPU. The respective values for each
vCPU are then added together.
Signed-off-by: Amneesh Singh
---
src/qemu/qemu_driver.c | 48 ++
1 file changed, 44 insertions(+), 4 deletions(-)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 3b5c3db6..0a2be6d3 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -18057,10 +18057,50 @@ qemuDomainGetStatsCpuHaltPollTime(virDomainObj *dom,
{
unsigned long long haltPollSuccess = 0;
unsigned long long haltPollFail = 0;
-pid_t pid = dom->pid;
+qemuDomainObjPrivate *priv = dom->privateData;
+bool canQueryStats = virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_QUERY_STATS);
-if (virHostCPUGetHaltPollTime(pid, , ) < 0)
-return 0;
+if (!canQueryStats) {
+pid_t pid = dom->pid;
+
+if (virHostCPUGetHaltPollTime(pid, , ) <
0)
+return 0;
+} else {
+size_t i;
+qemuMonitorQueryStatsTargetType target =
QEMU_MONITOR_QUERY_STATS_TARGET_VCPU;
+qemuMonitorQueryStatsProvider *provider = NULL;
+g_autoptr(GPtrArray) providers = NULL;
+g_autoptr(GPtrArray) queried_stats = NULL;
+const char *success_str = "halt_poll_success_ns";
+const char *fail_str = "halt_poll_fail_ns";
+
+provider =
qemuMonitorQueryStatsProviderNew(QEMU_MONITOR_QUERY_STATS_PROVIDER_KVM);
+provider->names = g_new0(char *, 3);
+provider->names[0] = g_strdup(success_str), provider->names[1] =
g_strdup(fail_str);
+
+providers = g_ptr_array_new_full(1, (GDestroyNotify)
qemuMonitorQueryStatsProviderFree);
+g_ptr_array_add(providers, provider);
+
+queried_stats = qemuMonitorQueryStats(priv->mon, target, NULL,
providers);
+
+if (!queried_stats)
+return 0;
+
+for (i = 0; i < queried_stats->len; i++) {
+unsigned long long curHaltPollSuccess, curHaltPollFail;
+GHashTable *cur_table = queried_stats->pdata[i];
+virJSONValue *success, *fail;
+
+success = g_hash_table_lookup(cur_table, success_str);
+fail = g_hash_table_lookup(cur_table, fail_str);
+
+ignore_value(virJSONValueGetNumberUlong(success,
));
+ignore_value(virJSONValueGetNumberUlong(fail, ));
+
+haltPollSuccess += curHaltPollSuccess;
+haltPollFail += curHaltPollFail;
+}
+}
if (virTypedParamListAddULLong(params, haltPollSuccess,
"cpu.haltpoll.success.time") < 0 ||
virTypedParamListAddULLong(params, haltPollFail,
"cpu.haltpoll.fail.time") < 0)
@@ -18915,7 +18955,7 @@ static virQEMUCapsFlags queryDirtyRateRequired[] = {
static struct qemuDomainGetStatsWorker qemuDomainGetStatsWorkers[] = {
{ qemuDomainGetStatsState, VIR_DOMAIN_STATS_STATE, false, NULL },
-{ qemuDomainGetStatsCpu, VIR_DOMAIN_STATS_CPU_TOTAL, false, NULL },
+{ qemuDomainGetStatsCpu, VIR_DOMAIN_STATS_CPU_TOTAL, true, NULL },
{ qemuDomainGetStatsBalloon, VIR_DOMAIN_STATS_BALLOON, true, NULL },
{ qemuDomainGetStatsVcpu, VIR_DOMAIN_STATS_VCPU, true, NULL },
{ qemuDomainGetStatsInterface, VIR_DOMAIN_STATS_INTERFACE, false, NULL },
--
2.36.1
