Skip to content

layered container support #1747

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

Draft
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

likewhatevs
Copy link
Contributor

I need to debug this because it is still has all tasks going to lo fallback when I run:

https://github.com/likewhatevs/perfstuff/blob/main/noisy-workload.compose.yml

That being said, this does run/pass the verifier and has all the idk components needed to make this work so lmk thoughts on the approach etc.

@likewhatevs likewhatevs marked this pull request as draft April 25, 2025 06:42
@likewhatevs likewhatevs force-pushed the layered-container-support-2 branch from 612f186 to 64f4056 Compare April 25, 2025 13:53
Copy link
Contributor

@etsal etsal left a comment

Choose a reason for hiding this comment

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

So AFAICT The changes are:

  1. Machinery for forwarding cpuset information to the BPF side
  2. The corresponding BPF code
  3. Logic that replicates allow_node_aligned but for cpusets.

If this fixes cpuset related problems I think it is reasonable, but I'm not sure about the naming - contianer enable is a bit confusing because containers aren't really a thing at this level of abstraction. Maybe replace "container" with "cpuset-based workloads"? This way it's clear what the code does concretely.

!(layer->allow_node_aligned && taskc->cpus_node_aligned)) ||
!layer->nr_cpus) {
!(layer->allow_node_aligned && taskc->cpus_node_aligned)) ||
!(enable_container && taskc->cpus_cpuset_aligned) ||
Copy link
Contributor

Choose a reason for hiding this comment

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

Wrt the bug, maybe wrap lines 1354-1355 in parentheses? && has a higher precedence than || so rn all tasks without cpus_cpuset_aligned are getting put into the fallback queue regardless of the value of tasks->all_cpus_allowed.

Copy link
Contributor Author

@likewhatevs likewhatevs Apr 30, 2025

Choose a reason for hiding this comment

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

think that was it, thanks 1000x for that.

well, part of it, went from exclusively using lo fallback to largely using it.

@@ -30,6 +30,7 @@ enum consts {
MAX_TASKS = 131072,
MAX_PATH = 4096,
MAX_NUMA_NODES = 64,
MAX_CONTAINERS = 64,
Copy link
Contributor

@htejun htejun Apr 28, 2025

Choose a reason for hiding this comment

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

Can we use cpuset / CPUSET consistently instead of using both container and cpuset and document the implemented behavior in a comment?

@likewhatevs likewhatevs force-pushed the layered-container-support-2 branch from 64f4056 to 495915d Compare April 30, 2025 23:58
@likewhatevs likewhatevs force-pushed the layered-container-support-2 branch from 495915d to 33cb330 Compare May 6, 2025 15:06
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.

3 participants