Skip to content
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.

Sign up for GitHub

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

Jump to bottom

DAOS-623 engine: Fix a typo #14329

Merged
merged 3 commits into from
May 9, 2024
Merged

DAOS-623 engine: Fix a typo #14329

merged 3 commits into from
May 9, 2024

Conversation

jolivier23
Copy link
Contributor

tgt_nr should divide by tgt_per_numa, not number of numa nodes. Otherwise, it can trigger the assertion at the end of the function.

Required-githooks: true

Change-Id: I92f65924f9b4b3dce6a756e01e5bfc9e584af0b6

Before requesting gatekeeper:

  • Two review approvals and any prior change requests have been resolved.
  • Testing is complete and all tests passed or there is a reason documented in the PR why it should be force landed and forced-landing tag is set.
  • Features: (or Test-tag*) commit pragma was used or there is a reason documented that there are no appropriate tags for this PR.
  • Commit messages follows the guidelines outlined here.
  • Any tests skipped by the ticket being addressed have been run and passed in the PR.

Gatekeeper:

  • You are the appropriate gatekeeper to be landing the patch.
  • The PR has 2 reviews by people familiar with the code, including appropriate owners.
  • Githooks were used. If not, request that user install them and check copyright dates.
  • Checkpatch issues are resolved. Pay particular attention to ones that will show up on future PRs.
  • All builds have passed. Check non-required builds for any new compiler warnings.
  • Sufficient testing is done. Check feature pragmas and test tags and that tests skipped for the ticket are run and now pass with the changes.
  • If applicable, the PR has addressed any potential version compatibility issues.
  • Check the target branch. If it is master branch, should the PR go to a feature branch? If it is a release branch, does it have merge approval in the JIRA ticket.
  • Extra checks if forced landing is requested
    • Review comments are sufficiently resolved, particularly by prior reviewers that requested changes.
    • No new NLT or valgrind warnings. Check the classic view.
    • Quick-build or Quick-functional is not used.
  • Fix the commit message upon landing. Check the standard here. Edit it to create a single commit. If necessary, ask submitter for a new summary.

@jolivier23
DAOS-623 engine: Fix a typo
7aebba9 
tgt_nr should divide by tgt_per_numa, not number of
numa nodes.  Otherwise, it can trigger the assertion
at the end of the function.

Required-githooks: true

Change-Id: I92f65924f9b4b3dce6a756e01e5bfc9e584af0b6
Signed-off-by: Jeff Olivier <[email protected]>
@jolivier23 jolivier23 requested review from Nasf-Fan and NiuYawei May 8, 2024 01:24
@github-actions GitHub Actions
Copy link

github-actions bot commented May 8, 2024 •
edited
Loading

Ticket title is 'Generic ticket for minor code cleanup and improvement'
Status is 'Resolved'
Labels: 'request_for_2.4.3,request_for_2.6'
https://daosio.atlassian.net/browse/DAOS-623

@jolivier23
Required-githooks: true
5dc0edb 
Signed-off-by: Jeff Olivier <[email protected]>
@daosbuild1
Copy link
Collaborator

Test stage NLT on EL 8.8 completed with status FAILURE. https://build.hpdd.intel.com/job/daos-stack/job/daos/job/PR-14329/1/display/redirect

Nasf-Fan
Nasf-Fan reviewed May 8, 2024
View reviewed changes
src/engine/ult.c
target = base + ((offload + tgt_id) % dss_offload_per_numa_nr);
offload = target + 17; /* Seed next selection */
base = dss_sys_xs_nr + dss_tgt_nr + (socket * dss_offload_per_numa_nr);
target = base + ((offload + tgt_id) % dss_offload_per_numa_nr);
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we need to keep the "target" and "tgt_id" within the same NUMA node or not? If yes, how? It seems that "base" may exceeds the boundary?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Target in this calculation is a helper thread and will be on the same numa node according to the expected layout shown here:

image

Copy link
Contributor Author

Choose a reason for hiding this comment

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

To be more precise, the helper threads are split among numa nodes but base is the xs id of the first helper thread.

Copy link
Contributor

Choose a reason for hiding this comment

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

Thank for the detailed explanation.

@jolivier23 jolivier23 requested a review from Nasf-Fan May 8, 2024 15:21
jolivier23 added a commit that referenced this pull request May 8, 2024
@jolivier23
DAOS-15739 engine: Add single-engine, multi-socket support (#14311)
f16a7dd 
Backport for the following patches
DAOS-13380 engine: refine tgt_nr check (#12405)
DAOS-15739 engine: Add multi-socket support (#14234)
DAOS-623 engine: Fix a typo (#14329)

* DAOS-13380 engine: refine tgt_nr check

1. for non-DAOS_TARGET_OVERSUBSCRIBE case
   fail to start engine if #cores is not enough
2. for DAOS_TARGET_OVERSUBSCRIBE case
   allow to force start engine
The #nr_xs_helpers possibly be reduced for either case.

* DAOS-15739 engine: Add multi-socket support (#14234)

Add a simple multi-socket mode for use cases where a single
engine must be used. Avoids the issue of having all helper
xstreams automatically assigned to a single NUMA node thus
increasing efficiency of synchronizations between I/O and
helper xstreams.

It is the default behavior if all of the following are true

Neither pinned_numa_node nor first_core are used.
No oversubscription is requested
NUMA has uniform number of cores
targets and helpers divide evenly among numa nodes
There is more than one numa node
Update server config logic to ensure first_core is passed
on to engine if it's set while keeping existing behavior
when both first_core: 0 and pinned_numa_node are set.

Signed-off-by: Jeff Olivier <[email protected]>
Signed-off-by: Xuezhao Liu <[email protected]>
Signed-off-by: Tom Nabarro <[email protected]>
Nasf-Fan
Nasf-Fan approved these changes May 9, 2024
View reviewed changes
src/engine/ult.c
target = base + ((offload + tgt_id) % dss_offload_per_numa_nr);
offload = target + 17; /* Seed next selection */
base = dss_sys_xs_nr + dss_tgt_nr + (socket * dss_offload_per_numa_nr);
target = base + ((offload + tgt_id) % dss_offload_per_numa_nr);
Copy link
Contributor

Choose a reason for hiding this comment

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

Thank for the detailed explanation.

@jolivier23 jolivier23 merged commit ad356bf into master May 9, 2024
51 checks passed
@jolivier23 jolivier23 deleted the jvolivie/minor_fix branch May 9, 2024 13:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@NiuYawei NiuYawei NiuYawei approved these changes

@Nasf-Fan Nasf-Fan Nasf-Fan approved these changes

Assignees
No one assigned
Labels
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@jolivier23 @daosbuild1 @NiuYawei @Nasf-Fan