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

{bp-15285} net: Add buffer pool to replace connection allocation #15361

Merged
merged 2 commits into from
Dec 28, 2024

Conversation

jerpelea
Copy link
Contributor

Summary

patches included:

net: Add buffer pool to replace connection allocation
net: Optimize ipfwd and wrbuffer by buffer pool

Our net socket connection allocations are powerful but redundant because they're implemented once in each protocol. This is not good for further optimizing and extending to other allocations, so maybe we can add a common implementation for the usage.

Impact

RELEASE

Testing

CI

Our net socket connection allocations are powerful but redundant
because they're implemented once in each protocol.  This is not good for
further optimizing and extending to other allocations, so maybe we can
add a common implementation for the usage.

Impact:
1. We add a `struct net_bufpool_s` as pool descriptor, which may use a
   little bit more memory than previous implementation (~28Bytes).
2. We share same functions between pools, so code size may shrink under
   some scenarios.

Signed-off-by: Zhe Weng <[email protected]>
To allow dynamic allocation of these structs, performance keeps the same
as the previous implementation.

Signed-off-by: Zhe Weng <[email protected]>
@github-actions github-actions bot added Area: Networking Effects networking subsystem Size: XL The size of the change in this PR is very large. Consider breaking down the PR into smaller pieces. labels Dec 27, 2024
@nuttxpr
Copy link

nuttxpr commented Dec 27, 2024

[Experimental Bot, please feedback here]

No, this PR does not fully meet the NuttX requirements. While it provides a summary of the changes, it lacks crucial details.

Here's a breakdown of what's missing:

  • Insufficient Summary Detail: While the summary explains the why, it's missing the how. It mentions a buffer pool but doesn't explain how it works or how it replaces the connection allocation. What are the mechanics? What data structures are involved?
  • Missing Issue References: There's no mention of related NuttX or NuttX Apps issues or pull requests. These are essential for tracking the context of the changes.
  • Incomplete Impact Assessment: Simply stating "RELEASE" isn't sufficient. Each impact category (user, build, hardware, documentation, security, compatibility) needs a "YES" or "NO" answer, and if "YES," a description of the impact. For example, does this change require users to modify their code? Does it change any Kconfig options? Does it affect any specific architectures or boards? Does the documentation need to be updated? Are there any potential security implications of using a shared buffer pool?
  • Inadequate Testing Information: "CI" isn't enough. The PR needs to specify the local testing environment (host OS, CPU, compiler) and the target(s) tested (architecture, board, configuration). Critically, it's missing the actual testing logs before and after the change. These logs are crucial for demonstrating that the changes work as intended and don't introduce regressions.

In short, the PR needs to be significantly expanded to provide the required information before it can be properly reviewed.

@xiaoxiang781216 xiaoxiang781216 merged commit 05d4de6 into apache:releases/12.8 Dec 28, 2024
25 of 27 checks passed
@jerpelea jerpelea deleted the bp-15285 branch December 28, 2024 08:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area: Networking Effects networking subsystem Size: XL The size of the change in this PR is very large. Consider breaking down the PR into smaller pieces.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants