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

DONTMERGE: NETOBSERV-983: avoid preallocating huge chunk of memory by default #119

Closed
wants to merge 1 commit into from

Conversation

msherif1234
Copy link
Contributor

@msherif1234 msherif1234 commented May 10, 2023

using BPF_F_NO_PREALLOC to allocate memory as needed and avoid default preallocation to help with better scale

for reference https://www.kernel.org/doc/html/next/bpf/map_hash.html

using BPF_F_NO_PREALLOC to allocate memory as needed and avoid
default preallocation to help with better scale

Signed-off-by: msherif1234 <mmahmoud@redhat.com>
@openshift-ci-robot
Copy link
Collaborator

openshift-ci-robot commented May 10, 2023

@msherif1234: This pull request references NETOBSERV-975 which is a valid jira issue.

In response to this:

using BPF_F_NO_PREALLOC to allocate memory as needed and avoid default preallocation to help with better scale

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@openshift-ci
Copy link

openshift-ci bot commented May 10, 2023

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please ask for approval from msherif1234. For more information see the Kubernetes Code Review Process.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@openshift-ci-robot
Copy link
Collaborator

openshift-ci-robot commented May 10, 2023

@msherif1234: This pull request references NETOBSERV-975 which is a valid jira issue.

In response to this:

using BPF_F_NO_PREALLOC to allocate memory as needed and avoid default preallocation to help with better scale

for reference https://www.kernel.org/doc/html/next/bpf/map_hash.html

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@codecov
Copy link

codecov bot commented May 10, 2023

Codecov Report

Merging #119 (7e64030) into main (3f192b3) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##             main     #119   +/-   ##
=======================================
  Coverage   41.80%   41.80%           
=======================================
  Files          30       30           
  Lines        2050     2050           
=======================================
  Hits          857      857           
  Misses       1155     1155           
  Partials       38       38           
Flag Coverage Δ
unittests 41.80% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

@msherif1234
Copy link
Contributor Author

/ok-to-test

@openshift-ci openshift-ci bot added the ok-to-test To set manually when a PR is safe to test. Triggers image build on PR. label May 10, 2023
@github-actions
Copy link

New image: ["quay.io/netobserv/netobserv-ebpf-agent:d8ab1c2"]. It will expire after two weeks.

@msherif1234 msherif1234 changed the title NETOBSERV-975: avoid preallocating huge chunk of memory by default NETOBSERV-983: avoid preallocating huge chunk of memory by default May 10, 2023
@openshift-ci-robot
Copy link
Collaborator

openshift-ci-robot commented May 10, 2023

@msherif1234: This pull request references NETOBSERV-983 which is a valid jira issue.

In response to this:

using BPF_F_NO_PREALLOC to allocate memory as needed and avoid default preallocation to help with better scale

for reference https://www.kernel.org/doc/html/next/bpf/map_hash.html

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@msherif1234
Copy link
Contributor Author

/ok-to-test

@jotak
Copy link
Member

jotak commented May 25, 2023

@dushyantbehl and/or @praveingk that's just a small config change ; does it seems good to you?

@jotak jotak added no-doc This PR doesn't require documentation change on the NetObserv operator no-qe This PR doesn't necessitate QE approval and removed no-qe This PR doesn't necessitate QE approval labels May 25, 2023
@msherif1234
Copy link
Contributor Author

if we are migrating to global hashmap #118 then there is no need for this PR I am already taking care of the same there
/hold

Copy link
Contributor

@dushyantbehl dushyantbehl 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 to me.

@dushyantbehl
Copy link
Contributor

if we are migrating to global hashmap #118 then there is no need for this PR I am already taking care of the same there /hold

@msherif1234 Even with a global hashmap I believe the spirit of your approach works which is just lazy allocation of the Hashmap memory so I feel this can be added even if we have global or local hashmap.

@msherif1234 msherif1234 changed the title NETOBSERV-983: avoid preallocating huge chunk of memory by default DONTMERGE: NETOBSERV-983: avoid preallocating huge chunk of memory by default May 25, 2023
@msherif1234
Copy link
Contributor Author

if we are migrating to global hashmap #118 then there is no need for this PR I am already taking care of the same there /hold

@msherif1234 Even with a global hashmap I believe the spirit of your approach works which is just lazy allocation of the Hashmap memory so I feel this can be added even if we have global or local hashmap.

right the global hashmap PR has the same change as well hence we won't need this PR anymore once the #118 merge

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
do-not-merge/hold jira/valid-reference lgtm no-doc This PR doesn't require documentation change on the NetObserv operator ok-to-test To set manually when a PR is safe to test. Triggers image build on PR.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants