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

UCT/CM/RDMACM: share dummy CQ per device #105

Merged
merged 1 commit into from
Dec 11, 2020

Conversation

evgeny-leksikov
Copy link
Collaborator

@yosefe @alinask pls take a look


iter = kh_put(uct_rdmacm_cm_cqs, &cm->cqs, device_name, &ret);
if (iter == kh_end(&cm->cqs)) {
ibv_destroy_cq(*cq);
Copy link

Choose a reason for hiding this comment

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

pls check return value

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I don't think it makes sense here on out-of-memory flow, usually this is an unrecoverable error.

return UCS_ERR_IO_ERROR;
}

iter = kh_put(uct_rdmacm_cm_cqs, &cm->cqs, device_name, &ret);
Copy link
Owner

Choose a reason for hiding this comment

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

IMO would be simpler to 1st do kh_put and see if already exists by ret, rather than kh_get_kh_put

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

kh_put has value arg to put but on first look up no need to create cq bcz it can be there. we can try put NULL-pointer on first try then rewrite by new cq if it's not present in the hash.
IMHO get/put logic clearer, extra lookup is rare event due to limited amount of devices

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

ahh, just noticed that I was wrong about value arg, will rework then

src/uct/ib/rdmacm/rdmacm_cm.c Outdated Show resolved Hide resolved
src/uct/ib/rdmacm/rdmacm_cm.c Show resolved Hide resolved
src/uct/ib/rdmacm/rdmacm_cm.c Outdated Show resolved Hide resolved
@yosefe
Copy link
Owner

yosefe commented Dec 11, 2020

@evgeny-leksikov pls squash

@evgeny-leksikov
Copy link
Collaborator Author

@evgeny-leksikov pls squash

done

@yosefe yosefe merged commit adbdf9c into yosefe:integration3 Dec 11, 2020
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