Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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: Derived memh #10332
base: master
Are you sure you want to change the base?
UCT: Derived memh #10332
Changes from 11 commits
525c463
15efe3e
4bb43d6
ae21b04
072de42
8885211
53a4b08
e121f20
c4fc840
5a97574
0196e96
4204a9c
7ccaab7
e0faea0
b193dea
78a80ab
9340de1
6ebe17e
58e9362
beb5107
632f86e
334480b
077752c
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
params
is not checked to be non-NULL in other places. I think we can remove this check here as well.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, initially I didn't have this check, added it to fix NULL pointer crash in CI
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you remember the failed test? Looks like an issue in caller function.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't remember it, but I just see my commit named "Fix NPE" that I've made to address that failure: 53a4b08
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, found and fixed this NULL pointer error in gtest: b193dea
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can we make better names, instead of uct_ib_memh_alloc_internal+uct_ib_memh_alloc - uct_ib_memh_alloc+uct_ib_memh_new/init
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
uct_ib_memh_alloc
is an existing function, so we don't want to change its nameI just extracted some common part from
uct_ib_memh_alloc
into _internal so that it can be reused byuct_ib_memh_clone
.Maybe we name it
uct_ib_memh_alloc_common
?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Answering in the opposite order:
2. Right, here malloc would be enough but I'm reusing existing function
uct_ib_memh_alloc_internal
that doescalloc
and calculates the size, just to reuse the common code. I think the overhead is minimal.We could split size calculation into a separate function etc, but I think it's not worth the effort
My understanding is:
Deep copy creates an independent instance of an object, that can be used apart from the original.
Shallow copy creates an "alias" that depends on master copy and cannot be used separately.
The latter is what's implemented in this PR, whatever we name it.
Derived memh is a shallow copy, because it makes a shallow copy of MRs - the most important part of original memh. And original object remains the only owner of the MRs state.
Of course there could be different implementations of derived memh. For example, we can imagine a shallow copy looking like that:
Looks ok, right? Still you need to allocate this object.
What are the issues with this approach:
memh
has always theuct_ib_mem_t
base: uct_rc_mlx5_txqp_tag_inline_post: ((uct_ib_mem_t*)iov->memh)So we must add
uct_ib_mem_t super
field to our copyThis is just to show you that we need to duplicate the significant part of the original memh anyway.
To overcome all these issues I make a shallow copy of the original memh (== they have the same memory layout), and use it interchangeably in all the places. This allows me to minimize the amount of ifs in the code, basically we check for derived handle only during init and cleanup. This helps to avoid any refactoring in the existing functions.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we nee to brainstorm the approaches. I don't like having structs where only some fields used, maybe need a deeper refactoring in the IB memh structure.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
( )
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok