search

OpenFastPath / ofp

OpenFastPath project
BSD 3-Clause "New" or "Revised" License
349 stars 126 forks source link

Miscellaneous compile time and run time fixes #275

Closed WonderfulVoid closed 2 years ago

WonderfulVoid commented 2 years ago

Miscellaneous fixes that were required when porting OFP to a bare metal environment with a custom ODP implementation. Commit c0f98a8 should be of special interest since I think without this, all port hash table computations will collapse into slot 0 of the hash table (the uninitialized port hash mask is probably still 0) which is detrimental to performance.

bogdanPricope commented 2 years ago

Hi Ola,

Nice catch. It is nice to see that ofp is still dragging attention. I am not sure who can review and push this change as Nokia & Enea are not active. But ARM has a maintainer in this project, right? Myself, I am working on a commercial version of ofp, named nfp: https://github.com/NetInoSoftware/nfp

Best regards, Bogdan

WonderfulVoid commented 2 years ago

I talked to someone at Nokia, they said they would handle GitHub pull requests.

bogdanPricope commented 2 years ago

Welcome back, @JereLeppanen !

JereLeppanen commented 2 years ago

Heh, thanks Bogdan. No comment on whether I really ever left, or came back.

JereLeppanen commented 2 years ago

CI is now using github actions, so if you update this PR, please remember to rebase so that we get a CI run.

WonderfulVoid commented 2 years ago

I updated one commit (088351e) and removed another (the NULL check). Should I "Update with merge commit" (which seems to be default) or "Update with rebase" (which feels more natural to me)?

JereLeppanen commented 2 years ago

Should I "Update with merge commit" (which seems to be default) or "Update with rebase" (which feels more natural to me)?

Rebase, please.

JereLeppanen commented 2 years ago

Any progress here? I think all that is needed is the rebase and also the commits need descriptions and signed-off-by tags.

WonderfulVoid commented 2 years ago

Sorry for the delay, I was in Powerpoint mode for a month... I rebased and updated the OFP_CONCAT commit per your request. What kind of descriptions do you want for the commits? And who can sign off? I am all alone here with OFP work...

JereLeppanen commented 2 years ago

Your signed-off-by tag is enough. Please also add

Reviewed-by: Jere Leppänen <jere.leppanen@nokia.com>

Description for each commit should state what is changed and why. Just one or two sentences is enough for these commits.

I missed this earlier, but the subject lines of the first and last commits should be prefixed with "api:", i.e.

api: Add OFP_ prefix to _IN_PORT_T_DECLARED symbol
api: Add OFP_ prefix to SOMAXCONN symbol

And finally, if you like, you could fix the checkpatch warnings: https://github.com/OpenFastPath/ofp/runs/5738785370?check_suite_focus=true You didn't touch those parts of the changed lines, so this is optional.