-
Notifications
You must be signed in to change notification settings - Fork 17
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
Dynamic allocation memory (part 1) #120
Conversation
7e36d48
to
d2a5e84
Compare
9812d65
to
057cefc
Compare
e7e9c63
to
1f17057
Compare
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 see new dataplane options but nothing of them are used for autotests
In this PR (part 1) new options is useless =( |
stats.extended_chunks_size = std::max((uint64_t)object_type::extended_chunks_size_min, | ||
values.size() / 16); | ||
|
||
for (;;) |
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.
So you just double the size of a LPM tree in case of no memory error?
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.
No. Double size only if not enough chunks on update
} | ||
|
||
eResult result = pointer->fill(stats, values); | ||
if (result != eResult::success) |
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 we really want to double the size of a hashtable in case of ANY error?
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.
fill
can return success, or not enough memory
|
||
for (;;) | ||
{ | ||
pointer = memory_manager->create<object_type>(name.data(), |
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, extended_chunks_size should be encapsulated into the tree but not the updater
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.
The idea was that worker and lookup is not required extended_chunks_size
socket_id, | ||
size); | ||
|
||
void* pointer = rte_malloc_socket(nullptr, |
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 do not like the rte_malloc here as it could be fragile, for instance if we are near the overall allocation limits.
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.
At minimum a complex investigation in rte_malloc behavior should be done before its application for dynamic allocation purposes.
Personally, I am unsure that allocation sequences like A16 A16, A40 F16 F16 A40 (allocate 16GB twice, then reallocate the first one from 16 to 40 and then the second one 16 to 40) would be handled 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.
We will not make things worse in this place
allocate memory on update for some acl structs.
allocate memory on update for some acl structs
#58