Make CPU affinity used for CPU pinning configurable via json file #1136

Raghav-NI · 2024-12-03T11:15:43Z

What does this Pull Request accomplish?

We want to configure cpu affinity used for cpu pinning for different process configurable via json file.

Why should this Pull Request be merged?

User-Configurable CPU affinity:
- Users can explicitly assign processor for:
  - Sideband stream read/write thread
  - Stream write threads
- Why?
  - Some systems have more CPUs available, while others have fewer CPUs with shared workloads.
  - Configurability allows users to optimize CPU usage based on their system constraints or dedicated tasks.
  - It gives users control to explicitly decide how resources are distributed rather than relying on the scheduler.
Default Behavior: "No Affinity" (-1):
- By default, threads are not pinned to any specific CPU, allowing the OS scheduler to decide the CPU assignment
  dynamically.
Separate CPU Cores for Critical Threads:
- Users can assign separate CPU cores for non- sideband threads, sideband-read/write, and server threads.
- Why?
  - To isolate performance-critical sideband operations (e.g., low-latency tasks) from server threads to minimize contention.
  - Separating cores ensures better performance for threads that are sensitive to cache invalidation, refreshes, or CPU contention.

Best Practices for configuration:
- Use Default (-1): For general systems where CPU pinning is unnecessary, default "no affinity" provides broad compatibility.
- Assign Dedicated Cores: On systems with more CPUs, pin performance-critical threads (e.g., sideband tasks) to isolated cores.
- Avoid Contention: Separate sideband and server threads onto different cores to reduce interference, particularly for real-time or low-latency workloads.

What testing has been done?

I am able to build grpc-server and run it successfully with and without cpu pinning configuration in json file.

…n file

source/config/localhost_config.json

source/server/server_configuration_parser.cpp

astarche

Please respond to questions about server affinity and the high level question about clarifying the purpose/priorities for this change. Also consider that this PR is the first-pass documentation for HOW to use the feature, so include details about best practices for config.

source/server/core_server.cpp

source/server/data_moniker_service.cpp

source/config/localhost_config.json

christag-ni · 2024-12-16T19:25:23Z

We are planning to create a release branch on the 18th, and your PR will need to be completed before then for the changes to be in effect. Do you want these changes included in this release?

Raghav-NI · 2024-12-16T19:35:49Z

We are planning to create a release branch on the 18th, and your PR will need to be completed before then for the changes to be in effect. Do you want these changes included in this release?

Yes, I was on leave, will be resolving comments and this PR should be completed today (17th Dec).

Raghav-NI · 2024-12-17T10:56:11Z

Please respond to questions about server affinity and the high level question about clarifying the purpose/priorities for this change. Also consider that this PR is the first-pass documentation for HOW to use the feature, so include details about best practices for config.

Addressed all the questions and updated PR description.

source/server/data_moniker_service.cpp

source/config/localhost_config.json

doshirohan

I have commented only in few places, but anywhere you are using "stream processor" replace with "cpu affinity", "cpu affinity configuration" or equivalent. "stream processor" sounds like something that is responsible for processing streams.

source/config/localhost_config.json

source/server/moniker_stream_processor.h

source/server/data_moniker_service.cpp

source/server/server_configuration_parser.cpp

christag-ni · 2024-12-18T16:22:29Z

I'll be creating the release branch at 11:30 CST.

Comments addressed and OOO rest of year.

Sumit has reviewed and comments address

### What does this Pull Request accomplish? For core server thread, there were hardcoded values for setting cpu affinity. This change removes that code. ### Why should this Pull Request be merged? Earlier change which made some of the [cpu affinity settings configurable](#1136) through `server_config.json` left out making server thread pinning configurable because this code just sets affinity for main thread that launches server. This doesn't really change anything performance-wise apart from server launch time. CPU pinning for main server thread was added when we added support for moniker based streaming and was taken as is from prototype branch for moniker streaming. This code is not needed. ### What testing has been done? Build passes and able to launch grpc-device server

Raghav Rawat added 2 commits December 3, 2024 16:40

changes to make cpu core for different processes configurable via jso…

b430659

…n file

make default value as -1 for cpu cores

75f11a4

Raghav-NI changed the title ~~Make CPU cores used for CPU pinning configurable via json file~~ Make CPU used for CPU pinning configurable via json file Dec 3, 2024

Raghav-NI changed the title ~~Make CPU used for CPU pinning configurable via json file~~ Make cores used for CPU pinning configurable via json file Dec 3, 2024

define variables for different core as extern in .h file

c4eec83

asumit reviewed Dec 3, 2024

View reviewed changes

source/config/localhost_config.json Outdated Show resolved Hide resolved

source/config/localhost_config.json Outdated Show resolved Hide resolved

Raghav Rawat added 3 commits December 5, 2024 15:35

modified the logic and renamed the struct

083c3fd

fixed naming

7f7e89a

fixed variabe name

c293a20

Raghav-NI requested a review from asumit December 5, 2024 11:07

Raghav Rawat added 3 commits December 5, 2024 16:59

set default value of cores as -1

84839fa

fixed variable name

6075595

renaming

515269b

Raghav-NI changed the title ~~Make cores used for CPU pinning configurable via json file~~ Make CPU affinity used for CPU pinning configurable via json file Dec 6, 2024

renamed struct in localhost_config.json

276e3ab

Raghav-NI requested a review from doshirohan December 6, 2024 06:22

Raghav-NI marked this pull request as ready for review December 6, 2024 06:22

Raghav-NI requested review from astarche, reckenro and maxxboehme as code owners December 6, 2024 06:22

maxxboehme previously requested changes Dec 9, 2024

View reviewed changes

source/server/server_configuration_parser.cpp Outdated Show resolved Hide resolved

initialized struct with -1 as default value

0aa5366

Raghav-NI requested a review from maxxboehme December 10, 2024 12:09

astarche previously requested changes Dec 10, 2024

View reviewed changes

source/server/core_server.cpp Outdated Show resolved Hide resolved

source/server/data_moniker_service.cpp Outdated Show resolved Hide resolved

source/config/localhost_config.json Outdated Show resolved Hide resolved

source/config/localhost_config.json Outdated Show resolved Hide resolved

Raghav Rawat added 2 commits December 17, 2024 15:08

resolved Alex's comments. Updated PR description

1adcf4f

added configuration for stream_read and stream_read_write

19487b8

Raghav-NI requested a review from astarche December 17, 2024 12:28

asumit requested changes Dec 17, 2024

View reviewed changes

source/server/data_moniker_service.cpp Outdated Show resolved Hide resolved

source/server/data_moniker_service.cpp Outdated Show resolved Hide resolved

source/server/data_moniker_service.cpp Outdated Show resolved Hide resolved

asumit requested changes Dec 17, 2024

View reviewed changes

source/server/data_moniker_service.cpp Outdated Show resolved Hide resolved

source/server/data_moniker_service.cpp Outdated Show resolved Hide resolved

cleanup

6861585

Raghav-NI requested a review from asumit December 18, 2024 11:24

doshirohan reviewed Dec 18, 2024

View reviewed changes

source/config/localhost_config.json Outdated Show resolved Hide resolved

doshirohan previously requested changes Dec 18, 2024

View reviewed changes

source/config/localhost_config.json Outdated Show resolved Hide resolved

source/server/moniker_stream_processor.h Show resolved Hide resolved

source/server/data_moniker_service.cpp Outdated Show resolved Hide resolved

source/server/server_configuration_parser.cpp Show resolved Hide resolved

addressed rohan's comments

77bfe21

Raghav-NI requested a review from doshirohan December 18, 2024 14:14

asumit approved these changes Dec 18, 2024

View reviewed changes

reckenro approved these changes Dec 18, 2024

View reviewed changes

Raghav-NI merged commit a3d5698 into main Dec 18, 2024
10 checks passed

Raghav-NI deleted the users/rrawat/cpu_pinning branch December 18, 2024 17:19

doshirohan mentioned this pull request Dec 27, 2024

Remove cpu pinning for core server #1145

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make CPU affinity used for CPU pinning configurable via json file #1136

Make CPU affinity used for CPU pinning configurable via json file #1136

Raghav-NI commented Dec 3, 2024 •

edited

Loading

astarche left a comment

christag-ni commented Dec 16, 2024

Raghav-NI commented Dec 16, 2024 •

edited

Loading

Raghav-NI commented Dec 17, 2024 •

edited

Loading

doshirohan left a comment

christag-ni commented Dec 18, 2024

Make CPU affinity used for CPU pinning configurable via json file #1136

Make CPU affinity used for CPU pinning configurable via json file #1136

Conversation

Raghav-NI commented Dec 3, 2024 • edited Loading

What does this Pull Request accomplish?

Why should this Pull Request be merged?

What testing has been done?

astarche left a comment

Choose a reason for hiding this comment

christag-ni commented Dec 16, 2024

Raghav-NI commented Dec 16, 2024 • edited Loading

Raghav-NI commented Dec 17, 2024 • edited Loading

doshirohan left a comment

Choose a reason for hiding this comment

christag-ni commented Dec 18, 2024

Raghav-NI commented Dec 3, 2024 •

edited

Loading

Raghav-NI commented Dec 16, 2024 •

edited

Loading

Raghav-NI commented Dec 17, 2024 •

edited

Loading