CP-308857: Update xapi to align with xen patch #6612
Conversation
| caml_release_runtime_system(); | ||
| retval = xc_get_runstate_info(xch, c_domid, &info); | ||
| caml_acquire_runtime_system(); | ||
| retval = xc_get_runstate_info(xch, Int_val(domid), &info); |
There was a problem hiding this comment.
Why is the release no longer required? And why are we using not xc_get_runstate_info2?
There was a problem hiding this comment.
In the xen patch, Xenctrl.domain_get_runstate_info is kept to make the old clients working.
Just revert the commit as xenopsd is not using the new fields: runnable, running, nonaffine from Xenctrl.domain_get_runstate_info2.
There was a problem hiding this comment.
But caml_release_runtime_system still needs to stay
There was a problem hiding this comment.
Ok I see what is going on now, some commits from here got reverted, which makes this PR awkard to review.
In effect this branch won't touch the xenctrlext_stubs.c at all: master...gangj:xen-api:private/gangj/CP-308857, that is why it looks like we're removing the acquire_runtime_system, that got added in this branch: a8782c0
We could reintroduce that, but that can be done independently of this feature branch in another PR (I have a very old draft PR to fix up runtime lock usage).
There was a problem hiding this comment.
i.e. instead of reviewing this PR as is, we should review the feature branch's diff against master (see link above).
|
This needs to get https://github.com/xapi-project/xenctrl patched as well |
|
Will update the xenctrl mock the same way as the new xen patch first. |
| CAMLparam2(xch_val, domid); | ||
| #if defined(XENCTRL_HAS_GET_RUNSTATE_INFO) | ||
| CAMLlocal1(result); | ||
| xc_runstate_info_t info = { 0 }; |
There was a problem hiding this comment.
What's wrong with keeping this initialization of info to { 0 }? otherwise it's going to be indeterminate and might not be initialized in the xenctrl call.
There was a problem hiding this comment.
I think a completely new API got introduced instead, so we leave the old API as it was (which will look like we are changing it if we are looking at the diff vs current branch, but if we look at the diff against master we see that the file is not changed at all, see #6612 (comment)).
This has confused me as well.
gangj
force-pushed
the
private/gangj/CP-308857
branch
from
37b6723 to
6f2a73c
Compare
gangj
force-pushed
the
private/gangj/CP-308857
branch
from
6f2a73c to
1c8afb9
Compare
|
the comparison against master master...gangj:xen-api:private/gangj/CP-308857 looks good to me. Though it's probably good to squash the resulting branch feature/vcpu-runnable to remove the unnecessary commits (like the one that introduces and reverts the same changes). |
gangj
force-pushed
the
private/gangj/CP-308857
branch
from
1c8afb9 to
32c1fd5
Compare
|
Squashed feature branch: master...gangj:xen-api:private/gangj/CP-308857, as we are to change the history of the feature branch, here is the PR for it: #6615, this PR can be closed if #6615 is OK. |
gangj
force-pushed
the
private/gangj/CP-308857
branch
from
32c1fd5 to
ec0b510
Compare
"vcpu", "VCPU" unified to "vCPU". Signed-off-by: Gang Ji <[email protected]>
Adding a new metric 'runnable_any' as % of time that at least one vCPU of the domain is in the runnable state. It is the sum of the following 3 metrics: - runstate_full_contention - runstate_concurrency_hazard - runstate_partial_contention Naming it 'runnable_any' instead of 'runnable' is to resolve one problem with rrd2csv: if we name it 'runnable', rrd2csv will select both "runnable" and "runnable_vcpus" when the 'runnable' is used: > rrd2csv AVERAGE:vm:<vm-uuid>:runnable > timestamp, AVERAGE:vm:<vm-uuid>:runnable, AVERAGE:vm:<vm-uuid>:runnable_vcpus This is because "runnable" is a prefix of "runnable_vcpus". Naming it 'runnable_any', with rrd2csv: * can select only 'runnable_any' if we use: rrd2csv AVERAGE:vm:<vm-uuid>:runnable_any * can select only 'runnable_vcpus' if we use: rrd2csv AVERAGE:vm:<vm-uuid>:runnable_vcpus * can select both 'runnable_any' and 'runnable_vcpus' if we use: rrd2csv AVERAGE:vm:<vm-uuid>:runnable Naming it 'runnable_any' also makes it clearer as the metric is % of time that at least one vCPU of the domain is in the runnable state. Add max to "runnable_any" metric to follow the fix here: Signed-off-by: Bengang Yuan <[email protected]> [Rebase with metric renaming and some fixes] Signed-off-by: Gang Ji <[email protected]>
Adding a new CPU ready RRD metric: "runnable_vcpus" per domain as time % of vCPUs of the domain that are in the runnable state. Signed-off-by: Gang Ji <[email protected]>
Query runstate with the new API: Xenctrl.Runstateinfo.V2.domain_get Signed-off-by: Marcus Granado <[email protected]>
gangj
force-pushed
the
private/gangj/CP-308857
branch
from
ec0b510 to
d355725
Compare
Squashed the history in PR #6612 together with the history in feature/vcpu-runnable.
|
Drop it, the work is done by #6615 |
6 participants
Xenctrl.Runstateinfo.V2.domain_get is added in xen patch for querying runnable time of vCPUs of a domain.