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

libbpf-tools/profile: Batch lookup support for hash map #4990

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

ekyooo
Copy link
Contributor

@ekyooo ekyooo commented May 8, 2024

Use bpf_map_lookup_batch() instead of bpf_map_get_next_key() to read the hash map, if available.

} else {
return err;
}
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

In map_helpers.c, we have dump_hash(). Could you try to use that function?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, there is duplicate code in this patch.
read_counts_map() in profile.c returns data as an array of type "struct key_ext_t", which is a combination of the keys and values ​​from the hash map. This is the difference from the design of dump_hash() in map_helpers.c.

In the current patch, the combination is only needed for lookup_batch. However, using map_helpers' dump_hash() requires combination (data copying) for both the batch and iteration cases. Perhaps that's why syscount.c doesn't use map_helpers either. Could you tell me what you think about it?

There is also a way to utilize only dump_hash_batch() of map_helpers.c after removing static.
Thank you.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think dump_hash() should work. It will cover both batch and non-batch case. I suggest to replace non-batch case with dump_hash(). Could you give a try?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Of course, I'll update it.

Use bpf_map_lookup_batch() instead of bpf_map_get_next_key() to read
the hash map, if available.

lookup_key = &items[i].k;
i++;
for (i = 0; i < n; i++) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

In previous patches, maps were read directly into the "items" variable, so this copy would not work unless it was in a batch. Now we use a little more memory and CPU. However, this change is good for eliminating duplicate code.

@ekyooo
Copy link
Contributor Author

ekyooo commented May 22, 2024

Could you let me know if I've misunderstood any of your comments?
Thank you.

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