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

fix: ensure all entries from bpf map are copied #1477

Merged
merged 1 commit into from
May 30, 2024

Conversation

sthaha
Copy link
Collaborator

@sthaha sthaha commented May 29, 2024

Previously only a MapSize (lesser than size of processes) of entries used to be copied from processes bpf map. The right use of a batch size less than the size of map is as follows:

Ref: https://lwn.net/Articles/797808/

    p_key = NULL;
    p_next_key = &key;
    while (true) {
       err = bpf_map_lookup_batch(fd, p_key, &p_next_key, keys, values,
                                  &batch_size, elem_flags, flags);
       if (err) ...
       if (p_next_key) break; // done
       if (!p_key) p_key = p_next_key;
    }

This PR fixes the issue by creating a entries byte array with the same size as the processes map.

Additionally, this commit makes use of bytes.Reader instead of bytes.Buffer to avoid unnecessary copy of ephemeral byte array.

@sthaha sthaha requested review from dave-tucker and rootfs May 29, 2024 04:05
Copy link

github-actions bot commented May 29, 2024

🤖 SeineSailor

Here's a concise summary of the pull request changes:

Summary: This PR fixes an issue where only a portion of entries were copied from the BPF map by creating an entries byte array with the same size as the processes map and using bytes.Reader instead of bytes.Buffer. The changes affect four functions: CollectCPUFreq, getCPUCores, resizeArrayEntries, and libbpfCollectProcessBatchSingleHash. The exporter struct is updated, and the libbpfCollectProcessBatchSingleHash function now uses processes.MaxEntries() instead of the hardcoded MapSize.

Key Modifications:

  • Fixed incomplete entry copying from the BPF map
  • Optimized code by avoiding unnecessary byte array copies
  • Updated exporter struct and libbpfCollectProcessBatchSingleHash function

Impact: The changes do not alter the signatures of exported functions, global data structures, or variables, and do not affect the external interface or behavior of the code. The PR provides a targeted fix and optimization without introducing any breaking changes.

@@ -327,7 +326,7 @@ func (e *exporter) CollectCPUFreq() (cpuFreqData map[int32]uint64, err error) {
if err != nil {
return
}
//cpuFreqkeySize := int(unsafe.Sizeof(uint32Key))
// cpuFreqkeySize := int(unsafe.Sizeof(uint32Key))
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

go fmt artefact

Previously only a `MapSize` (lesser than size of processs) of entries
used to be copied from processes bpf map. The right use of a batch size
less than the size of map is as follows:

Ref: https://lwn.net/Articles/797808/
```
    p_key = NULL;
    p_next_key = &key;
    while (true) {
       err = bpf_map_lookup_batch(fd, p_key, &p_next_key, keys, values,
                                  &batch_size, elem_flags, flags);
       if (err) ...
       if (p_next_key) break; // done
       if (!p_key) p_key = p_next_key;
    }
```

This PR fixes the issue by creating a entries byte array with the same
size as the `processes` map.

Additionally, this commit makes use of `bytes.Reader` instead of
bytes.Buffer to avoid unnecessary copy of ephemeral byte array.

Signed-off-by: Sunil Thaha <sthaha@redhat.com>
@rootfs rootfs merged commit 747e7eb into sustainable-computing-io:main May 30, 2024
22 checks passed
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