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

Fixed the issue for seating the shared mode EC vs VP value. #866

Merged
merged 2 commits into from
Oct 1, 2024

Conversation

SamirMulani
Copy link
Contributor

@SamirMulani SamirMulani commented Sep 25, 2024

Fix EC Min/Desired Proc Units Calculation and Max Virtual Proc Units Logic

Previously, for EC, the min_proc_units were calculated by multiplying the user input with the overcommit_ratio. This caused an issue where users were unable to set their desired min_proc_units for EC. To fix this, the overcommit_ratio is no longer used in calculating desired_proc_units, as it should only be applied when determining the virtual proc units. The desired_proc_units, max_proc_units and min_proc_units are now set directly based on the user's inputs for EC.

Additionally, we added logic to calculate the max virtual proc units by fetching the maximum virtual procs configured in the system. We use this value as the max virtual proc count, but there is a condition where the available stealable resource count (multiplied by 2) is compared against the max virtual procs. If the multiplied stealable count is greater than the actual max virtual procs retrieved from the HMC command, we use the higher value, otherwise, we proceed with 2 * int(max_proc_units) for EC.

int(min_proc_units), 2*int(desired_proc_units),
2*int(max_proc_units), min_memory, desired_memory, max_memory))
(sharing_mode, min_proc_units, max_proc_units, desired_proc_units,
int(min_proc_units), overcommit_ratio*int(desired_proc_units), v_max_proc,
Copy link
Collaborator

Choose a reason for hiding this comment

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

@SamirMulani we do not need to have overcommit_ratio as a multiplier for min and max (instead of 2 ) as well

Copy link
Contributor Author

@SamirMulani SamirMulani Sep 26, 2024

Choose a reason for hiding this comment

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

@PraveenPenguin The overcommit_ratio should not be used as a multiplier for the min and max values. For the min, it should be equal to the user input, while for the max, it should either follow stealable count or max v_max_proc value.

@PraveenPenguin
Copy link
Collaborator

@SamirMulani please push other changes as well so it has logically conclusion

Samir Mulani added 2 commits October 1, 2024 14:13
Fix EC Min/Desired Proc Units Calculation and Max Virtual Proc Units Logic

Previously, for EC, the `min_proc_units` were calculated by multiplying the user input with the `overcommit_ratio`. This caused an issue where users were unable to set their desired `min_proc_units` for EC. To fix this, the `overcommit_ratio` is no longer used in calculating `desired_proc_units`, as it should only be applied when determining the virtual proc units. The `desired_proc_units`, `max_proc_units` and `min_proc_units` are now set directly based on the user's inputs for EC.

Additionally, we added logic to calculate the max virtual proc units by fetching the maximum virtual procs configured in the system. We use this value as the max virtual proc count, but there is a condition where the available stable resource count (multiplied by 2) is compared against the max virtual procs. If the multiplied stable count is greater than the actual max virtual procs retrieved from the HMC command, we use the higher value, otherwise, we proceed with 2 * int(max_proc_units) for EC.

Signed-off-by: Samir Mulani <[email protected]>
In the previous implementation, we did not support taking the max_proc_units value from user input. Instead, we first calculated the stealable resources and used that value, with a default setting of max_proc_units=2.
Now, we have added the ability to take max_proc_units as input from a configuration file. If the user does not provide this value, we fall back to the stealable resource count.

Signed-off-by: Samir Mulani <[email protected]>
Copy link
Collaborator

@PraveenPenguin PraveenPenguin left a comment

Choose a reason for hiding this comment

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

LGTM

@PraveenPenguin PraveenPenguin merged commit 59e31e5 into open-power:master Oct 1, 2024
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants