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

Feat xdp map pinning #283

Merged

Conversation

maryamtahhan
Copy link
Contributor

@maryamtahhan maryamtahhan commented Mar 24, 2023

Enable bpf map pinning for CNDP

tested with intel/afxdp-plugins-for-kubernetes#47

Tested with the AF_XDP device plugin and standalone.

examples/cndpfwd/main.h Outdated Show resolved Hide resolved
Copy link
Contributor Author

@maryamtahhan maryamtahhan left a comment

Choose a reason for hiding this comment

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

rebase and address the comments

containerization/docker/ubuntu/Dockerfile Outdated Show resolved Hide resolved
tools/jsonc_gen.sh Outdated Show resolved Hide resolved
Signed-off-by: Maryam Tahhan <mtahhan@redhat.com>
Signed-off-by: Maryam Tahhan <mtahhan@redhat.com>
Signed-off-by: Maryam Tahhan <mtahhan@redhat.com>
@maryamtahhan maryamtahhan marked this pull request as ready for review March 27, 2023 15:40
tools/jsonc_gen.sh Outdated Show resolved Hide resolved
Signed-off-by: Maryam Tahhan <mtahhan@redhat.com>
tools/jsonc_gen.sh Outdated Show resolved Hide resolved
tools/jsonc_gen.sh Outdated Show resolved Hide resolved
Signed-off-by: Maryam Tahhan <mtahhan@redhat.com>
Signed-off-by: Maryam Tahhan <mtahhan@redhat.com>
Copy link
Contributor

@KeithWiles KeithWiles left a comment

Choose a reason for hiding this comment

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

LGTM, I only had one comment. Sorry it took me so long to review.

@maryamtahhan
Copy link
Contributor Author

LGTM, I only had one comment. Sorry it took me so long to review.

TY for reviewing. no worries at all. I still have to rework based on our discussion. I will remove the unnecessary flags and repost

@maryamtahhan
Copy link
Contributor Author

WIP - hoping to push some patches today - but blocked atm with the failing fedora docker build for RUST...

@maryamtahhan maryamtahhan marked this pull request as draft April 26, 2023 13:51
containerization/docker/ubuntu/fwd.jsonc Outdated Show resolved Hide resolved
examples/cndpfwd/fwd.jsonc Outdated Show resolved Hide resolved
examples/cnet-graph/cnetfwd-graph.jsonc Outdated Show resolved Hide resolved
lib/usr/app/jcfg/jcfg_lport.c Outdated Show resolved Hide resolved
Signed-off-by: Maryam Tahhan <mtahhan@redhat.com>
Pinned xsk_maps could differ on a per lport basis. Change
the configuration from being an app option to an lport option.
Also automatically configure the lport unpriviliged flag when
the `xsk_pin_path` option is configured.

Signed-off-by: Maryam Tahhan <mtahhan@redhat.com>
Signed-off-by: Maryam Tahhan <mtahhan@redhat.com>
Signed-off-by: Maryam Tahhan <mtahhan@redhat.com>
Signed-off-by: Maryam Tahhan <mtahhan@redhat.com>
Signed-off-by: Maryam Tahhan <mtahhan@redhat.com>
Signed-off-by: Maryam Tahhan <mtahhan@redhat.com>
@maryamtahhan maryamtahhan marked this pull request as ready for review April 27, 2023 15:19
@maryamtahhan
Copy link
Contributor Author

maryamtahhan commented Apr 27, 2023

Ok - I think this is finally ready for review. I've gotten rid of the privileged key for lports. it's automatically configured if the uds or xsk pinned map is used.

Tested with the AF_XDP device plugin and standalone

Last outstanding things is to clean up the rust implementation...

Do we want to support xsk pinned map for the lport groups?

@KeithWiles
Copy link
Contributor

KeithWiles commented Apr 27, 2023

Not sure if we want to support xsk pinned map in lport group. I guess I do not fully understand the reason to add it, sorry.

Being I do not fully understand, I think it is up to you and if it provides any extra feature or support. If the feature is to be used by everyone then I would say yes, but it only is some very small number of use cases then we add it as an enhancement for later.

If we do want this feature I would suggest we add it in a new PR.

@maryamtahhan
Copy link
Contributor Author

Not sure if we want to support xsk pinned map in lport group. I guess I do not fully understand the reason to add it, sorry.

It would be needed if we want to use this config script in K8s Pods... But as this isn't supported today (group script for configuring a large number of ports in the pod) then it's not needed.

I will add an issue for the rust support.

Also - can I just say I hate the superlinter :) it has dinged so many scripts that I just deleted a line from... but of course I will fix the complaints :)

@KeithWiles
Copy link
Contributor

Yes the super linter is not very good as it does not support Go code very well and has some strange configurations. :-(

Copy link
Contributor

@KeithWiles KeithWiles left a comment

Choose a reason for hiding this comment

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

LGTM

Signed-off-by: Maryam Tahhan <mtahhan@redhat.com>
@maryamtahhan
Copy link
Contributor Author

Added an issue for rust support here

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants