-
Notifications
You must be signed in to change notification settings - Fork 261
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
Release CPUs reminders #475
Conversation
/ost |
the code changes lgtm but this description.. it should clarify the reason/context for the changes but it's not easy to parse it - see comments below
by 'we are basing upon' you mean 'we rely on'?
do you mean "we use the cpuTopology list"?
"The overtaken CPU remained pinned"?
"when grouping the VMs and having an affinity set"? what does "there" refer to? |
When we allocate CPUs we rely on the list we return back. This is correct for the scheduling phase. But now we run the same logic on canSchedule. There, when having a group of VMs, we use the cpuTopology list, not the list we selected to allocate. The overtaken CPU remained pinned and that harmed the canSchedule logic for VM groups. When grouping the VMs, when the group is having an affinity set, it is reordered. Therefore, it is required to sort them. Change-Id: Idcae90fed953c4798f2c4bf8893bff0a1b7086ea Signed-off-by: Liran Rotenberg <lrotenbe@redhat.com>
e4f2d2d
to
f1ead12
Compare
thanks, commit changed. |
I still do not understand what is this PR about - do you have an example of incorrect behavior? |
The one in the bug https://bugzilla.redhat.com/show_bug.cgi?id=2079351 . 2nd problem, now when we enter CpuPinningPolicyUnit, we use the same cpuTopology for the group of VMs. When allocating (actually, this may hit CPUPolicyUnit as well now), the logic is to allocate and "drop" the reminder CPU(s) if there are any, the returned list from the allocation function (cpusToAllocate) was the place we looked into. Example from the bug having host with 4:4:1, VM1 with 4:3:1 and VM2 with 2:1:1. |
Yes, it is starting to make sense to me. Do I understand it correctly that at first we pin all CPUs in |
When selecting CPUs to allocate we pass on Why it's like that is a good question which I don't remember (and it was discussed) when implemented. To me it's more greedy approach, when we see that core doesn't have enough threads within to be used, we just release it. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I still do not completely understand how cpusToBeAllocated
is created but since that part has already been tested and works, the changes in this PR make sense.
OST passed last time, the only code change is the commit message. |
When we allocate CPUs we are basing upon the list we return back. This
is correct for the scheduling phase. But now we run the same logic on
canSchedule. There, when having a group of VMs, we are basing on the
cpuTopology list, not the list we selected to allocate. The overtaken
CPU wasn't released from the pinning itself and that harmed the
canSchedule logic for VM groups.
Also, when grouping the VMs, when the group is having an affinity set,
it is reordered. Therefore, it is required to sort them there as well.
Change-Id: Idcae90fed953c4798f2c4bf8893bff0a1b7086ea
Signed-off-by: Liran Rotenberg lrotenbe@redhat.com