[Hardware][POWER] Add Power (ppc64le)–specific CPU binding for VLLM_CPU_OMP_THREADS_BIND=auto #20387
Conversation
Signed-off-by: Akash Kaothalkar <[email protected]>
Signed-off-by: Akash Kaothalkar <[email protected]>
|
👋 Hi! Thank you for contributing to the vLLM project. 💬 Join our developer Slack at https://slack.vllm.ai to discuss your PR in #pr-reviews, coordinate on features in #feat- channels, or join special interest groups in #sig- channels. Just a reminder: PRs would not trigger full CI run by default. Instead, it would only run Once the PR is approved and ready to go, your PR reviewer(s) can run CI to test the changes comprehensively before merging. To run CI, PR reviewers can either: Add 🚀 |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Summary of Changes
Hello @Akashcodes732, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!
I've identified that this pull request addresses a critical performance regression for vLLM on IBM Power (ppc64le) systems. Specifically, it fixes a significant slowdown observed when the VLLM_CPU_OMP_THREADS_BIND environment variable is set to auto. My analysis shows that this change introduces a Power-specific NUMA and SMT-aware CPU binding strategy, which is designed to restore optimal performance on these systems, bringing auto binding performance in line with or exceeding manual all binding.
Highlights
- Performance Fix for Power Systems: I've noted that this PR resolves a 2-3x performance degradation on IBM Power (ppc64le) systems when
VLLM_CPU_OMP_THREADS_BINDis set toauto. - Architecture-Specific CPU Binding: I see that a new function,
get_cpus_id_binding_based_on_numa_nodes_ppc64le, has been introduced to provide optimal CPU binding logic tailored for Power systems, which typically feature SMT-8. - Conditional Binding Logic: I've observed that the CPU worker initialization has been modified to dynamically apply the Power-specific binding logic only when running on
ppc64learchitecture andVLLM_CPU_OMP_THREADS_BINDisauto.
Using Gemini Code Assist
The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.
Invoking Gemini
You can request assistance from Gemini at any point in your pull request via creating an issue comment (i.e. comment on the pull request page) using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands.
| Feature | Command | Description |
|---|---|---|
| Code Review | /gemini review |
Performs a code review for the current pull request in its current state. |
| Pull Request Summary | /gemini summary |
Provides a summary of the current pull request in its current state. |
| Comment | @gemini-code-assist | Responds in comments when explicitly tagged, both in issue comments and review comments. |
| Help | /gemini help |
Displays a list of available commands. |
Customization
To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.
Limitations & Feedback
Gemini Code Assist is currently in preview and may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments to provide feedback.
You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.
Footnotes
-
Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩
There was a problem hiding this comment.
Code Review
This pull request introduces Power (ppc64le)-specific CPU binding logic to address a performance regression. The changes correctly identify the architecture and dispatch to a new function for thread binding.
My review focuses on improving the maintainability and clarity of the new code. I've identified significant code duplication between the new and existing binding functions and recommend refactoring to a shared helper method. I've also pointed out the use of magic numbers in the new logic and suggested replacing them with named constants to improve readability. These changes will make the code easier to understand and maintain in the long run.
| def get_cpus_id_binding_based_on_numa_nodes_ppc64le(self) -> str: | ||
| """ | ||
| Power (ppc64le) specific: Selects a subset of threads per core for each NUMA node. | ||
| This is robust to SMT mode (SMT-8, SMT-4, etc) because the OS only exposes available threads. | ||
| This maximizes performance by avoiding oversubscription of logical CPUs on Power systems. | ||
| """ | ||
|
|
||
| def select_threads_per_power_core(node_cpu_ids): | ||
| return [cpu for cpu in node_cpu_ids if cpu % 8 < 4] | ||
|
|
||
| rank_to_cpus = self.local_omp_cpuid | ||
| world_size = self.vllm_config.parallel_config.world_size | ||
| libnuma_found = util.find_spec("numa") is not None | ||
| psutil_found = util.find_spec("psutil") is not None | ||
| if libnuma_found and psutil_found: | ||
| import psutil | ||
| from numa import info | ||
| cpus_allow_list = psutil.Process().cpu_affinity() | ||
| numa_size = info.get_num_configured_nodes() | ||
|
|
||
| node_to_cpus = [] | ||
| for i in range(numa_size): | ||
| node_intersect = set( | ||
| info.node_to_cpus(i)).intersection(cpus_allow_list) | ||
| if bool(node_intersect): | ||
| node_to_cpus.append(sorted(list(node_intersect))) | ||
|
|
||
| if world_size > len(node_to_cpus): | ||
| logger.error( | ||
| "Auto thread-binding failed due to " | ||
| "world size: %d is larger than " | ||
| "allowed NUMA nodes number: %d." | ||
| "Please try to bind threads manually.", world_size, | ||
| len(node_to_cpus)) | ||
| else: | ||
| node_cpus_this_rank = node_to_cpus[self.rank] | ||
| node_cpus_this_rank = select_threads_per_power_core( | ||
| node_cpus_this_rank) | ||
| cpu_count_per_numa = len(node_cpus_this_rank) | ||
| num_of_reserved_cpu = min(envs.VLLM_CPU_NUM_OF_RESERVED_CPU, | ||
| cpu_count_per_numa // 2) | ||
| end = cpu_count_per_numa - num_of_reserved_cpu | ||
| rank_to_cpus_list = node_cpus_this_rank[:end] | ||
| rank_to_cpus = ','.join(str(x) for x in rank_to_cpus_list) | ||
| logger.info("ppc64le thread-binding list: %s", rank_to_cpus) | ||
| else: | ||
| logger.warning( | ||
| "Auto thread-binding is not supported due to " | ||
| "the lack of package numa and psutil," | ||
| "fallback to no thread-binding. To get better performance," | ||
| "please try to manually bind threads.") | ||
| return rank_to_cpus |
There was a problem hiding this comment.
This new function get_cpus_id_binding_based_on_numa_nodes_ppc64le is almost a complete copy of the existing get_cpus_id_binding_based_on_numa_nodes function. This introduces significant code duplication, which makes the code harder to maintain. Any bug fix or improvement in one function would need to be manually applied to the other.
To improve maintainability, I recommend refactoring the common logic into a private helper method. This helper method could accept an optional cpu_filter function to handle the architecture-specific logic.
Here's an example of how you could refactor this:
class CpuWorker(WorkerBase):
# ...
def _get_cpus_id_binding_from_numa_nodes(
self,
cpu_filter: Optional[Callable[[List[int]], List[int]]] = None,
log_prefix: str = "Auto"
) -> str:
# ... (common logic from both functions)
else:
node_cpus_this_rank = node_to_cpus[self.rank]
if cpu_filter:
node_cpus_this_rank = cpu_filter(node_cpus_this_rank)
# ... (rest of the common logic)
rank_to_cpus = ','.join(str(x) for x in rank_to_cpus_list)
logger.info("%s thread-binding list: %s", log_prefix, rank_to_cpus)
# ...
return rank_to_cpus
def get_cpus_id_binding_based_on_numa_nodes(self) -> str:
return self._get_cpus_id_binding_from_numa_nodes()
def get_cpus_id_binding_based_on_numa_nodes_ppc64le(self) -> str:
# ...
def select_threads_per_power_core(node_cpu_ids):
# ...
return self._get_cpus_id_binding_from_numa_nodes(
cpu_filter=select_threads_per_power_core,
log_prefix="ppc64le"
)This would require modifying get_cpus_id_binding_based_on_numa_nodes, which is outside the current diff. However, this change would significantly improve the code quality and is highly recommended.
| def select_threads_per_power_core(node_cpu_ids): | ||
| return [cpu for cpu in node_cpu_ids if cpu % 8 < 4] |
There was a problem hiding this comment.
The numbers 8 and 4 in cpu % 8 < 4 are magic numbers, which can make the code harder to understand and maintain. It's better to define them as constants with descriptive names. This will improve readability and make it easier to adjust these values in the future if needed.
Additionally, this logic assumes a specific logical CPU assignment scheme by the OS, which might not be robust across different SMT levels (e.g., SMT-4). While the PR description states this is robust, adding a comment to explain the assumption would be beneficial for future maintainers.
| def select_threads_per_power_core(node_cpu_ids): | |
| return [cpu for cpu in node_cpu_ids if cpu % 8 < 4] | |
| def select_threads_per_power_core(node_cpu_ids): | |
| # For SMT-8 on Power, use 4 threads per core for better performance. | |
| # This assumes logical CPUs for a core are assigned in blocks of 8. | |
| THREADS_PER_CORE = 8 | |
| THREADS_TO_USE = 4 | |
| return [cpu for cpu in node_cpu_ids if cpu % THREADS_PER_CORE < THREADS_TO_USE] |
Signed-off-by: Akash Kaothalkar <[email protected]>
Akashcodes732
changed the title
|
@DarkLight1337 Can you please take a look at this PR ? |
|
Thanks @DarkLight1337 for the heads-up! We're actively working on migrating to the V1 CPU worker and aligning our efforts accordingly. |
|
The same logic would be required to V1 as well, so if reviewed it would be helpful when this would be a part of V1 as well. |
|
This pull request has merge conflicts that must be resolved before it can be |
|
Closing this PR as V0 is deprecated |
2 participants
Essential Elements of an Effective PR Description Checklist
supported_models.mdandexamplesfor a new model.Purpose
This PR addresses performance regression on IBM Power (ppc64le) systems introduced by PR #17930, which enabled automatic CPU binding for OpenMP threads when
VLLM_CPU_OMP_THREADS_BINDis set toauto. On Power systems, which typically feature SMT-8, the default auto-binding logic resulted in suboptimal performance and a 2–3x performance degradation compared toVLLM_CPU_OMP_THREADS_BIND=all.The issue and its impact are described in detail in RFC #20089.
To resolve this, we introduce a new function,
get_cpus_id_binding_based_on_numa_nodes_ppc64le, which implements Power-specific logic for thread binding. This function ensures that, in auto mode, vLLM selects the optimal set of logical CPUs for each NUMA node on Power systems, restoring expected performance.Test Plan
VLLM_CPU_OMP_THREADS_BIND="all"VLLM_CPU_OMP_THREADS_BIND="auto"(before and after this PR)Test Results
main:
After this PR:
With the new Power-specific binding logic, the performance in
automode is restored to match or exceed the manualallbinding, resolving the previous 2–3x performance regression.(Optional) Documentation Update