ovspinning: Set affinity of each thread #4470
Conversation
github-actions
bot
added
the
area/unit-testing
|
Changes unknown |
| for _, taskID := range taskIDs { | ||
| err = unix.SchedSetaffinity(taskID, ¤tProcessCPUs) | ||
| if err != nil { | ||
| return fmt.Errorf("can't set CPU affinity of task(%d) PID(%d) to %s: %w", taskID, targetPID, printCPUSet(currentProcessCPUs), err) |
There was a problem hiding this comment.
This needs to be a bit resilient. Imagine OVS is reconfiguring its threads at this very time and one of the threads disappears while you are in the loop. The error will end the processing and the rest of the threads will not be configured.
Maybe that is not an issue since atm OVS always recreates all its threads, but it is still a hidden race.
There was a problem hiding this comment.
good point! going to fix this race by not breaking the loop.
|
Just one concurrency related comment, but otherwise the logic looks good to me. I would add the bug id to the title, but I see OVN-k is not following that pattern. |
zeeke
force-pushed
the
OCPBUGS-35347
branch
2 times, most recently
from
e656ca1
to
9652299
Compare
Yes, I'll take care of linking the issue when this fix lands downstream |
OVS CPU pinning ensures the CPU consumed by the processes `ovsdb-server` and `ovs-vswitchd` are the same as the OVNkube container. `ovs-vswitchd` process has threads and calling `SchedSetaffinity()` on the main PID only is does not guarantee that the already spawned threads inherit that affinity. The following is an example of such a situation: ``` sh-4.4# taskset -apc $(pgrep ovs-vswitchd) pid 4059's current affinity list: 0-2,6,9,15-66,69,70,73,79-127 pid 4335's current affinity list: 0-2,32,33,64-66,96,97 pid 2715051's current affinity list: 0-2,5,6,15-66,69,70,73,79-127 pid 2715052's current affinity list: 0-2,5,6,15-66,69,70,73,79-127 pid 2715053's current affinity list: 0-2,5,6,15-66,69,70,73,79-127 ... ``` This changes makes the CPU align routine loop over all the threads of the given PID and invoke `SchedSetaffinity` on all of them. To test these changes, a simple `sleep 10` process is not enough, as it doesn't spawn any thread. A go version of the sleep is therefore provided for testing purpose, in the form of a simple `main`. Signed-off-by: Andrea Panattoni <apanatto@redhat.com>
|
Changes unknown |
|
/lgtm |
|
Has two +1's merging this. |
|
@ricky-rav : please use the GUI to add the lgtm so that I can merge this without overriding the branch protections |
OVS CPU pinning ensures the CPU consumed by the processes
ovsdb-serverandovs-vswitchdare the same as the OVNkube container.ovs-vswitchdprocess has threads and callingSchedSetaffinity()on the main PID only does not guarantee that the already spawned threads inherit that affinity. The following is an example of such a situation:This changes makes the CPU align routine loop over all the threads of the given PID and invoke
SchedSetaffinityon all of them.To test these changes, a simple
sleep 10process is not enough, as it doesn't spawn any thread. A go version of the sleep is therefore provided for testing purpose, in the form of a simplemain.What this PR does and why is it needed
Which issue(s) this PR fixes
Special notes for reviewers
How to verify it
Details to documentation updates
Description for the changelog
Does this PR introduce a user-facing change?
cc @MarSik, @ricky-rav