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

cpu entitlement when hypervisor is present #30

Closed
ashu-mehra opened this issue Sep 15, 2017 · 3 comments · Fixed by #45
Closed

cpu entitlement when hypervisor is present #30

ashu-mehra opened this issue Sep 15, 2017 · 3 comments · Fixed by #45

Comments

@ashu-mehra
Copy link
Contributor

I believe there are two problems in JIT with respect to getting cpu entitlement information from hypervisor:

  1. I noticed this code in TR_CpuEntitlement::computeAndCacheCpuEntitlement
   if (_numTargetCpu == 0)
      _numTargetCpu = 1; // some correction in case we get it wrong
   if (isHypervisorPresent())
      {
      _guestCpuEntitlement = computeGuestCpuEntitlement();
      // If the number of target CPUs is smaller (bind the JVM to a subset of CPUs), use that value
      if (_numTargetCpu < _guestCpuEntitlement || _guestCpuEntitlement <= 0)

It looks like _guestCpuEntitlement is in percentage, but _numTargetCpu is not. The condition _numTargetCpu < _guestCpuEntitlement is likely to be comparing incorrect values in that case.

  1. Secondly, it appears TR_CpuEntitlement::isHypervisorPresent will always returns false, no matter what.
bool TR_CpuEntitlement::isHypervisorPresent()
   {
   if (_hypervisorPresent == TR_maybe)
      {
      // Caching works because we don't expect the information to change
      // However, we must call this after portlib is up and running
      PORT_ACCESS_FROM_JITCONFIG(_jitConfig);
      if (j9hypervisor_hypervisor_present() > 0) // J9HYPERVISOR_NOT_PRESENT/J9HYPERVISOR_PRESENT/J9PORT_ERROR_HYPERVISOR_UNSUPPORTED
         {
         _hypervisorPresent = TR_yes;
         }
      else
         {
         _hypervisorPresent = TR_no;
         }
      }
   return (_hypervisorPresent == TR_yes);
   }

_hypervisorPresent gets a default value of TR_no. I don't see it ever getting set to TR_maybe which means isHypervisorPresent will always return false.

I hope I have not missed anything here.
@mpirvu, any idea why is it like this? It appears this is just dead code right now.

@mpirvu
Copy link
Contributor

mpirvu commented Sep 15, 2017

Indeed, _hypervisorPresent is not initialized at all. It needs to get initialized to TR_maybe in void init(J9JITConfig *jitConfig) before calling computeAndCacheCpuEntitlement()

@mpirvu
Copy link
Contributor

mpirvu commented Sep 15, 2017

Regarding the first problem, yes the code needs to be changed to read
if (_numTargetCpu * 100 < _guestCpuEntitlement || _guestCpuEntitlement <= 0)

@ashu-mehra
Copy link
Contributor Author

@mpirvu thanks for confirming. I will open a PR to fix the two issues.

ashu-mehra pushed a commit to ashu-mehra/openj9 that referenced this issue Sep 19, 2017
Initialize TR_CpuEntitlement::_hypervisorPresent to TR::maybe.
Fix the condition in TR_CpuEntitlement::computeAndCacheCpuEntitlement
that compares _numTargetCpu with _guestCpuEntitlement.

Issue: eclipse-openj9#30
Signed-off-by: Ashutosh Mehra <[email protected]>
JamesKingdon pushed a commit to JamesKingdon/openj9 that referenced this issue Apr 18, 2019
Initialize TR_CpuEntitlement::_hypervisorPresent to TR::maybe.
Fix the condition in TR_CpuEntitlement::computeAndCacheCpuEntitlement
that compares _numTargetCpu with _guestCpuEntitlement.

Issue: eclipse-openj9#30
Signed-off-by: Ashutosh Mehra <[email protected]>
tajila pushed a commit to tajila/openj9 that referenced this issue Aug 6, 2019
* Fixed up the last iTable for each class
* Set default memory state for the image's invalid itable
* Moved the invalid itable allocation to a function

Signed-off-by: Brady Jessup<[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants