Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

WIP: Enable System CPU info for z/OS #3152

Closed
wants to merge 1 commit into from

Conversation

irisbaron
Copy link

@irisbaron irisbaron commented Nov 6, 2018

Enhance port capabilities to provide system cpu info for z/os
Change includes:

  • Add field cpuLoad to hold % info from z/os. On z/os the query returns parentage and not time.
    CpuLoad is the % value as int between 0 and 100. cpuLoad value for other platforms default to -1.
  • update sysinfo.c to obtain cpu utilization for z/os based on cvt and SRM (system resource manager control) data. The utilization includes gcps and ziips.

This change is used in openj9 change (separate PR) where the cpuLoad info is used to get the return value for systemGetCpuLoad mbean.

Signed-off-by: Iris Baron [email protected]

Add field cpuLoad to hold % info from z/os. On z/os the query returns parentage and not time.
CpuLoad is the % value as int between 0 and 100.
Using cvt and SRM (system resource manager control) can access info about cpu utilization of the system. This includes both gcp and ziips.
It is returned as % (between 0 and 100).
Only zos make use of cpuLoad. Other platforms continue to provide time.
@fjeremic fjeremic self-assigned this Nov 6, 2018
@irisbaron irisbaron changed the title Enable system cpu info for z/os Enable system cpu info for z/OS Nov 6, 2018
@charliegracie
Copy link
Contributor

charliegracie commented Nov 7, 2018

I am not sure about this change. If you look in thread/common/thrprof.c in the function omrthread_get_process_times it looks to me like we can get the same type of data on zOS as other platforms so there is no reason to add the special case.

I think we have a different issue of having two different functions providing the same information which should be cleaned up but that is definitely outside of the scope of this PR.

@irisbaron
Copy link
Author

In response to charliegracie: I am aware of the ''omrthread_get_process_times''' and indeed this path is taken for the process cpu info call. However it only provides information for one process at a time using BPX4GTH /BPX1GTH system calls. In order to get the system view I would need to iterate over all processes on the system, which would take long time. Unfortunately there is no other system call to retrieve the system cpu utilization in similar manner.

@irisbaron irisbaron changed the title Enable system cpu info for z/OS Enable System CPU info for z/OS Nov 7, 2018
@charliegracie
Copy link
Contributor

@irisbaron So the omrsysinfo_get_CPU_utilization returns the amount of CPU time used for all processes on other platforms and not just the current process?

If the answer to the above is true I do not think we should be adding this new CPU Load to the current API for zOS only... This is very confusing to an end user consuming the API. If zOS does not provide the correct information then the correct solution is to provide no answer on zOS.

IF the information is truly needed, but I did not see anything is the commit message describing why this type of information would be required, then maybe a new api called omrsysinfo_get_CPU_load should be added.

char cctxx4 [14];
short ccvutili; /* IFA (zAAP) utilization */
short ccvutils; /* SUP (zIIP) utilization */
};
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should delete this block of code as it is already implemented in omrsimap.h [1]. Please reuse J9CVT, J9RMCT, and J9CCT structures to get to the CPU utilization data. There are examples in the codebase of how to use these structures if you search for the struct names. Once that is done let's remove this piece of code.

[1]

typedef _Packed struct J9CCT {
uint8_t cctFiller1[72]; /**< 0:72 Ignore fields not relevant to current implementation */
uint32_t ccvrbswt; /**< 72:4 Recent base system wait time */
uint8_t cctFiller2[4]; /**< 76:4 Ignore fields not relevant to current implementation */
uint32_t ccvrbstd; /**< 80:4 Recent base time of day */
uint8_t cctFiller3[18]; /**< 84:18 Ignore fields not relevant to current implementation */
uint16_t ccvutilp; /**< 102:2 System CPU utilization */
uint8_t cctFiller4[6]; /**< 104:6 Ignore fields not relevant to current implementation */
uint16_t ccvcpuct; /**< 110:2 No of online CPUs */
/**< Ignore rest of the CCT */
} J9CCT;

Copy link
Contributor

@charliegracie charliegracie Nov 7, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am not sure code in sysinfo.c should rely on the omrsimap.h header Seems as though that header is only for sysinfo.c so that is ok. Also when adding new structures please use OMR and not J9 For future reference when adding new structs, types, classes, etc. please use OMR instead of J9.

@@ -2456,6 +2482,13 @@ omrsysinfo_get_CPU_utilization(struct OMRPortLibrary *portLibrary, struct J9Sysi
cpuTime->numberOfCpus = stats.ncpus; /* get the actual number of CPUs against which the time is reported */
cpuTime->cpuTime = (stats.user + stats.sys) * NS_PER_CPU_TICK;
status = 0;

#elif defined(J9ZOS390) /* ZOS */
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The comment here is unnecessary as it is implied by the define. Can we remove?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right! Good catch. I wasn't sure what we do for #elif so I thought it was incorrect. The document mentions this:

  • #elif and #else should have comments that echo the previous condition.

Does that mean this line should have /* defined(AIXPPC) */ as the comment since it's the previous condition?

@@ -520,6 +520,7 @@ typedef struct J9SysinfoCPUTime {
int64_t timestamp; /* time in nanoseconds from a fixed but arbitrary point in time */
int64_t cpuTime; /* cumulative CPU utilization (sum of system and user time in nanoseconds) of all CPUs on the system. */
int32_t numberOfCpus; /* number of CPUs as reported by the operating system */
int32_t cpuLoad; /* CPU utilization % of all processors on the system including gcp and ziips (between 0 and 100) */
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This field is generic and can apply to any platform. The terms "gcp" and "ziips" are meaningless. I think we should only say:

/* CPU utilization % of all processors on the system (between 0 and 100) */

In addition given the code below we should also mention that cpuLoad and cpuTime are mutually exclusive. That is, only one of them is valid and the other will be -1

@@ -515,9 +515,10 @@ omrsysinfo_get_load_average(struct OMRPortLibrary *portLibrary, struct J9PortSys
* The cpuTime and timestamp values have no absolute significance: they should be used only to compute
* differences from previous values.
* On an N-processor system, cpuTimeStats.cpuTime may increase up to N times faster than real time.
* On z/OS use cpuLoad to hold real cpu for the system. Other platform should default to cpuLoad -1.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should not mention platform specifics in the common port library documentation. The point of the common APIs is to abstract platform specifics. As mentioned in the previous review comment this detail should move up to the definition point of cpuLoad and cpuTime. The documentation here should be improved however:

/**
 * Obtain the cumulative CPU utilization of all CPUs on the system.
 *
 * The cumulative CPU utilization can be derived either from the cpuLoad field (if computed) directly or
 * the combination of cpuTime and timestamp values (if computed) by taking differences from previous 
 * values.
 *
 * Note that cpuLoad and cpuTime are mutually exclusive and only one of them will be present. The other 
 * will have all value of -1 signifying that the value could not nr computed.

*
* @param[in] OMRPortLibrary portLibrary The port library.
* @param[out] J9SysinfoCPUTime cpuTime struct to receive the CPU time and a timestamp
* @param[out] J9SysinfoCPUTime cpuTime struct to receive the CPU time or cpuLoad (z/os) and a timestamp
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should remove the mention of z/OS here as the API is platform agnostic.

@fjeremic
Copy link
Contributor

fjeremic commented Nov 7, 2018

@irisbaron So the omrsysinfo_get_CPU_utilization returns the amount of CPU time used for all processes on other platforms and not just the current process?

If the answer to the above is true I do not think we should be adding this new CPU Load to the current API for zOS only... This is very confusing to an end user consuming the API. If zOS does not provide the correct information then the correct solution is to provide no answer on zOS.

IF the information is truly needed, but I did not see anything is the commit message describing why this type of information would be required, then maybe a new api called omrsysinfo_get_CPU_load should be added.

I'm curious about this as well. It seems the omrsysinfo_get_CPU_utilization API can be used to obtain CPU load by calling the API twice and looking at the ratio of the time stamps compared to the CPU time that is provided.

On z/OS we can't get CPU time as we can on other platforms so this API is unimplemented. I presume a downstream project depends on this functionality. @irisbaron can you confirm? And if so is it truly that expensive to call omrsysinfo_get_CPU_utilization on z/OS which will today return UNSUPPORTED and then you can try calling a new API omrsysinfo_get_CPU_load as proposed by @charliegracie which may then return to you the CPU load on z/OS?

I don't believe this would be too expensive at all and it avoids polluting and unnecessarily complicating these APIs and special casing them to overload the meaning of the fields in the J9SysinfoCPUTime struct. Thoughts?

@irisbaron
Copy link
Author

@fjeremic @charliegracie The case for this PR is to enable cpu info for Healthcenter on z/os.
On all platforms the HC cpu view contains process info as well as system info. Out z/os customers expect similar behaviour.
While the process info is easy to obtain, the system view was missing. Thus this PR.
I understand the concern of changing the omrsysinfo_get_CPU_utilization API. While the info is encapsulates in the J9SysinfoCPUTime structs the return answer, at least for z/os will change.
I think that the solution of having an additional API for cpuLoad is good.

As a consumer that needs to obtain the cpu util, it will first make a call to the omrsysinfo_get_CPU_utilization and if notSupported is returned, proceed to requesting omrsysinfo_get_CPU_load. The first call to omrsysinfo_get_CPU_utilization is pretty cheap and immediately return UNSUPPORTED, but it is a JNI call.
The new API will return non-supported for all platforms except z/os and will use a new struct J9SysinfoCPULoad

Is this acceptable?

I also noticed that omrtiGetSystemCpuLoad in OMR_TI.cpp is making a call to getOMRSysInfoSystemCpuTime which in turn calls omrsysinfo_get_CPU_utilization. But I dont see any call to the omrtiGetSystemCpuLoad.
Any idea about this usage?

@fjeremic
Copy link
Contributor

fjeremic commented Nov 7, 2018

As a consumer that needs to obtain the cpu util, it will first make a call to the omrsysinfo_get_CPU_utilization and if notSupported is returned, proceed to requesting omrsysinfo_get_CPU_load. The first call to omrsysinfo_get_CPU_utilization is pretty cheap and immediately return UNSUPPORTED, but it is a JNI call.
The new API will return non-supported for all platforms except z/os and will use a new struct J9SysinfoCPULoad

Is this acceptable?

I think this is a reasonable and acceptable solution. @charliegracie do you agree?

@charliegracie
Copy link
Contributor

Hmmm I am not sure that is exactly the right direction... If we are adding the new CPU Load API and the consumer would want CPU load and not CPU Time I think the consumer should just use the CPU load for all platforms.... clearly CPU load can be calculated for all other platforms since it is currently being return by the consumer for all platforms except zOS.

@fjeremic
Copy link
Contributor

fjeremic commented Nov 7, 2018

Hmmm I am not sure that is exactly the right direction... If we are adding the new CPU Load API and the consumer would want CPU load and not CPU Time I think the consumer should just use the CPU load for all platforms.... clearly CPU load can be calculated for all other platforms since it is currently being return by the consumer for all platforms except zOS.

I think the issue is that to calculate CPU load on non-z/OS platforms today you have to take two snapshots of the CPU time at short intervals and then to the relative calculation using the timestamp and the CPU time to determine the CPU load.

It would be hard to support a "CPU load" API on non-z/OS platforms as it would have to involve somehow calling the CPU time API, waiting a little bit, then calling it again to determine the relative delta and calculate the load. Of course this is all a moot point if non-z/OS platforms can just report the CPU load to begin with.

Briefly looking at Windows I do not see anything other than GetSystemTimes which only retrieves CPU time, not load. Googling around it seems the canonical way to determine CPU load on Windows is to take two snapshots (via GetSystemTimes) and calculate it that way (as described above).

@charliegracie
Copy link
Contributor

Isn't the MBean in OpenJ9 already doing this based on the data currently returned by the existing APIs? If so then the new API for all platforms other than zOS should be able to do exactly what the MBean is doing. Sounds like there just needs to be a primordial snapshot taken on omrsysinfo startup.

@fjeremic
Copy link
Contributor

fjeremic commented Nov 7, 2018

Sounds like there just needs to be a primordial snapshot taken on omrsysinfo startup.

The issue with with this approach is the accuracy of a new proposed omrsysinfo_get_CPU_load API. For non-z/OS it would need to work off of two snapshots. We can cache the "previous" snapshot of the last time the API was called however if the caller of this API waits too long then the information given back will be incorrect.

For example on x86 Linux the implementation of omrsysinfo_get_CPU_load may look like:

omrsysinfo_startup()
{
    previousCPUUtilizationSnapshot = omrsysinfo_get_CPU_utilization();
}

omrsysinfo_get_CPU_load()
{
    currentCPUUtilizationSnapshot = omrsysinfo_get_CPU_utilization();
    calculatedCPULoadFromCPUTime = doMath(currentCPUUtilizationSnapshot, previousCPUUtilizationSnapshot);
    previousCPUUtilizationSnapshot = currentCPUUtilizationSnapshot;

    return calculatedCPULoadFromCPUTime;
}

At the time omrsysinfo_startup is called the CPU load may have been 100%. Say the user waits two seconds and calls omrsysinfo_get_CPU_load. At this point the true system load is 0%, however because the previousCPUUtilizationSnapshot is so old it would caculate the CPU load to be 50% for example, which is totally not true.

In my mind omrsysinfo_get_CPU_load should return the instantaneous CPU load which should be valid for some small unit of time. For z/OS this is easy because we always have instantaneous CPU load. For non-z/OS platforms perhaps we can check how long ago the last snapshot was taken, and if it's "too long" (whatever we determine that to be) then the API should indicate a failing result code.

I suppose we need to be careful how it is defined. We can certainly implement it on all platforms though under the assumption that the user calls the omrsysinfo_get_CPU_load "often enough" so that the result is commutable and reasonably correct.

@charliegracie
Copy link
Contributor

Isn’t the idea to support the MBean? If the new API does exactly what the MBean does right now then there will be no difference. I am not sure I understand the concern.

@fjeremic
Copy link
Contributor

fjeremic commented Nov 7, 2018

Isn’t the idea to support the MBean? If the new API does exactly what the MBean does right now then there will be no difference. I am not sure I understand the concern.

I don't think there is an MBean for instantaneous CPU load. The closest I can find is an average over one minute [1] which is not what is required here as far as I understand. On Linux this bean seems to boil down to the OMR omrsysinfo_get_load_average API which calls getloadavg in stdlib.h.

You can however get "instantaneous" CPU load by calling omrsysinfo_get_CPU_utilization twice within a short period of time and calculating it manually. I assume this is what is desired from this PR. @irisbaron can you confirm?

@charliegracie in your mind what is the specification of a proposed omrsysinfo_get_CPU_load API? What does it calculate? On what time granularity does it work? Does it return an average load over some time period or the instantaneous load of the CPU?

[1] https://docs.oracle.com/javase/6/docs/api/java/lang/management/OperatingSystemMXBean.html#getSystemLoadAverage()

@charliegracie
Copy link
Contributor

I actually have no real idea. I would rather not do anything since this seems very Java specific but if there is a real use case then we should add an API. Many comments ago I asked for the use case. It would be great if a use case could be explained in detail so we could understand what needs to be completed. At this point there is a solution with no description of why it is needed. It would be great if we could have a non Java specific description of the required functionality so we can come up with the right solution to solve the requirement.

@fjeremic
Copy link
Contributor

fjeremic commented Nov 8, 2018

I actually have no real idea. I would rather not do anything since this seems very Java specific but if there is a real use case then we should add an API. Many comments ago I asked for the use case. It would be great if a use case could be explained in detail so we could understand what needs to be completed. At this point there is a solution with no description of why it is needed. It would be great if we could have a non Java specific description of the required functionality so we can come up with the right solution to solve the requirement.

I agree. @irisbaron this is up to you. Please provide more details as per @charliegracie's comment so we can properly review the change.

@irisbaron
Copy link
Author

irisbaron commented Nov 13, 2018

@charliegracie @fjeremic
Let me try and clear up the picture here:

  1. Use case - motivation for this PR

Many comments ago I asked for the use case. It would be great if a use case could be explained in detail so we could understand what needs to be completed.

The use case was described up, many comments before the quoted comment, but let me clarify. The MBean getSystemCpuLoad, is used in health center agent to provide information for system cpu utilization.
Currently this info is not available for z/os, but we get more and more customers on z/os that use z/os and request to see cpu information. This PR is to address this request and provide system cpu info for z/os on health center and beyond.

  1. The MBean getSystemCpuLoad definition is defined here: https://docs.oracle.com/javase/8/docs/jre/api/management/extension/com/sun/management/OperatingSystemMXBean.html#getSystemCpuLoad()
    "Returns the "recent cpu usage" for the whole system" between 0 and 1. "If the system recent cpu usage is not available, the method returns a negative value."

  2. I think the main point of confusion is that the omrsysinfo_get_CPU_utilization is returning an cpuTime and not real utilization percentage. Would be nice to rename this, but we cannot do that now.
    This cpuTime info is used in the implementation of the mbean to retrieve the real utilization based on 2 snapshot reads. The implementation also takes care to check if the time is too short or there was a rollover. Thus the return utilization is not an "instantaneous" utilization but rather it depends on cpuTime delta.
    You can take a look at the MBean implementation under openJ9: https://github.com/eclipse/openj9/blob/master/jcl/src/jdk.management/share/classes/com/ibm/lang/management/internal/CpuUtilizationHelper.java
    In particular calculateCpuLoad helper method which calculates the cpu utilization from 2 snapshots of cpuTime and timestamp.

  3. Since on z/os we cannot get the cpuTime, but rather an instantaneous cpu utilization, there is a need for a change.

  4. One suggestion, continuing the thread of comments above is to provide an additional API: omrsysinfo_get_CPU_load which will mimic the behaviour of the mbean now for the supported platforms and for zos will provide the instantaneous utilization.
    We can initialize the first read on startup or as currently implemented on the mbean, return -1 for the first call. Followup calls will result with the right information. If the time is too long, there was a rollover or any other problem it will return -1 as well.

    Pros:

    • Provide elegant solution to the zos special case.
    • Implementation already exists.
    • Provides basic utility query
       

    Cons:

    • API is not clear - Users will expect to get the current cpu utilization and may not want to
      call this API repeatedly. But I guess it is the same argument for the MBean implantation as well.
    • This change may only be relevant for java...
       
  5. An alternative solution, if we dont want to have full implementation for omrsysinfo_get_CPU_load on non-zos platforms : the API we can return UNSUPPORTED for those platforms . The Mbean would require change, but thats it.

Thoughts?

@fjeremic
Copy link
Contributor

Thanks for the explanation @irisbaron. I personally think we should go with the omrsysinfo_get_CPU_load solution you outlined in comment 5 as it can be implemented on all platforms. The API can be clearly defined in a way which states under which circumstances we will return -1.

@charliegracie
Copy link
Contributor

I agree with @fjeremic that we should use the suggestion 5 from @irisbaron

@fjeremic fjeremic removed their assignment Nov 13, 2018
@charliegracie
Copy link
Contributor

@irisbaron is this something that you are still investigating?

@charliegracie charliegracie changed the title Enable System CPU info for z/OS WIP: Enable System CPU info for z/OS Dec 10, 2018
@irisbaron
Copy link
Author

@irisbaron is this something that you are still investigating?

@charliegracie After some delay, I am now back to work on this item.

@charliegracie
Copy link
Contributor

@irisbaron ping

@0xdaryl
Copy link
Contributor

0xdaryl commented Aug 14, 2019

@irisbaron, there hasn't been an update on this PR in some time. Are you planning on making progress with this WIP PR in the near future? If not, I suggest closing it. You can continue to work on it in your private fork and re-open when it becomes ready for review.

@fjeremic
Copy link
Contributor

I think we can safely close this one. I have this in my personal backlog to resurrect the change with the cleanup required. I will have to prioritize it though with other more important work. We have this PR though as an archival artifact in the meantime. Thanks for all the work here!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants