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

[catnip] Bad Scatter-Gather Allocation/Free #82

Merged
merged 12 commits into from
Apr 11, 2022
Merged

[catnip] Bad Scatter-Gather Allocation/Free #82

merged 12 commits into from
Apr 11, 2022

Conversation

ppenna
Copy link
Contributor

@ppenna ppenna commented Apr 11, 2022

Description

This PR fixes issue #51.

In summary, the pointer arithmetic used for releasing allocated mbufs is bad. Instead of relying on this technique, I simply changed current code to use the returned mbuf pointer.

Since the management of DPDK buffers is not well design, I've also placed a warning, so that we know when we are relying on this allocation strategy.

Finally, I introduce unit-tests that ensure that the current logic works as expected.

This component will be strongly re-architected in a next PR.

@ppenna ppenna added the bug Something Isn't Working label Apr 11, 2022
@ppenna ppenna requested a review from BrianZill April 11, 2022 15:04
@ppenna ppenna self-assigned this Apr 11, 2022
tests/sga.rs Outdated
//==============================================================================

/// Size of scatter-gather arrays.
const SGA_MAX_SIZE: usize = 1280;
Copy link
Contributor

Choose a reason for hiding this comment

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

Might we want to test allocating different sizes, instead of always the same size?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Nice! I've added tests for 64-byte SGA arrays, to cover the heap-allocated buffers in catnip. Also, as we've discussed, I'll add even more comprehensive tests after I'm done with the re-architecting of this memory management component.

Copy link
Contributor

Choose a reason for hiding this comment

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

This is great, thanks!

@ppenna ppenna requested a review from BrianZill April 11, 2022 18:56
Copy link
Contributor

@BrianZill BrianZill left a comment

Choose a reason for hiding this comment

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

Looks good now.

@ppenna ppenna merged commit 4284fd3 into dev Apr 11, 2022
@ppenna ppenna deleted the bugfix-catnip-sga branch April 11, 2022 21:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something Isn't Working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants