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.

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

prov/util: Fix setting of mr_mode #5667

Closed
wants to merge 1 commit into from
Closed

Conversation

zhngaj
Copy link
Contributor

@zhngaj zhngaj commented Feb 25, 2020

If the app passes FI_MR_VIRT_ADDR as hints, and provider
supports FI_MR_BASIC, then FI_MR_VIRT_ADDR will be removed
when calling function fi_alter_domain_attr. Therefore, the created
ofi_mr_map won't be able to support FI_MR_VIRT_ADDR.

Fix such issue by substituting FI_MR_BASIC with FI_MR_VIRT_ADDR,
FI_MR_ALLOCATED, and FI_MR_PROV_KEY.

Signed-off-by: Jie Zhang [email protected]

If the app passes FI_MR_VIRT_ADDR as hints, and provider
supports FI_MR_BASIC, FI_MR_VIRT_ADDR will be removed in
function fi_alter_domain_attr. Therefore, the created
ofi_mr_map won't be able to support FI_MR_VIRT_ADDR.

Fix such issue by substituting FI_MR_BASIC with FI_MR_VIRT_ADDR,
FI_MR_ALLOCATED, and FI_MR_PROV_KEY.

Signed-off-by: Jie Zhang <[email protected]>
@zhngaj
Copy link
Contributor Author

zhngaj commented Feb 25, 2020

Fix issue #5659

@zhngaj zhngaj requested review from a team, shefty and aingerson February 25, 2020 00:59
@@ -1089,6 +1089,10 @@ static void fi_alter_domain_attr(struct fi_domain_attr *attr,
FI_MR_BASIC : FI_MR_SCALABLE;
} else {
if ((hints_mr_mode & attr->mr_mode) != attr->mr_mode) {
if (attr->mr_mode & FI_MR_BASIC) {
attr->mr_mode &= ~FI_MR_BASIC;
attr->mr_mode |= FI_MR_ALLOCATED | FI_MR_PROV_KEY | FI_MR_VIRT_ADDR;
Copy link
Member

@shefty shefty Feb 25, 2020

Choose a reason for hiding this comment

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

The provider should indicate what mr_mode bits it requires. It may also indicate if it can handle BASIC and/or SCALABLE. If the app requests BASIC, then only BASIC may be returned, not the individual bits, which the app may not know how to interpret.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, this is a requirement and the util code is doing the right thing by unsetting the bits that a provider doesn't require.

Copy link
Member

Choose a reason for hiding this comment

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

@rajachan - I didn't completely follow your comment. Are you saying that the current code is correct?

Copy link
Member

@rajachan rajachan Feb 26, 2020

Choose a reason for hiding this comment

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

I was agreeing with you that providers should set the bits they require and the util code should not have to do what this PR is doing. I haven't looked at the shm/uber test failure that necessitated this change yet, but looks like that could be a provider/test bug.

Edit: #5672 makes more sense.

@zhngaj
Copy link
Contributor Author

zhngaj commented Feb 27, 2020

Close this as #5672 has been merged.

@zhngaj zhngaj closed this Feb 27, 2020
@zhngaj zhngaj deleted the mr-mode-fix branch March 27, 2020 23:17
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