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

filter icmp id with sock_filter, to optimize rawsocket preformance #136

Merged
merged 2 commits into from
Jan 2, 2025

Conversation

nvksie
Copy link
Contributor

@nvksie nvksie commented Dec 4, 2024

rawsocket will receive all packets matching the target protocol, including replies to another ping on this host.
with sock_filter we can filter icmp reply packets, only accepting icmp replies that match the id in the sent packet.

Copy link
Contributor

@SuperQ SuperQ left a comment

Choose a reason for hiding this comment

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

This is an interesting optimization, I like it.

One thing I was thinking about recently is what happens to performance when users are running a lot of Pingers.

Currently we create a new socket for ever Pinger. I was wondering if this was causing us to do a lot of memory copies of all the incoming packets, most of which aren't matched by the listener. I haven't tested this, but if this is correct I was thinking about adding an option to create a single listener socket and pass that to each newly created pinger.

This may slightly conflict with this PR, as it also creates a filter for the packet ID.

But, this could easily be refactored later.

What do you think?

@nvksie
Copy link
Contributor Author

nvksie commented Dec 31, 2024

Thanks!
Sharing a single socket among all Pingers in a single-process scenario is a great idea, as it reduces redundant memory copies and IRQ caused by multiple raw sockets. However, I believe sock_filter has more advantages. It rejects unexpected packets in kernel space, resulting in even fewer IRQs or memory copies to user space compared to a single raw socket. Additionally, it offers better performance in multi-process scenarios.

@nvksie
Copy link
Contributor Author

nvksie commented Dec 31, 2024

Oh, I see! You mean using a single raw socket together with sock_filter? Maintaining a map to match ICMP IDs might make the BPF more complex, but it sounds more powerful!

@SuperQ
Copy link
Contributor

SuperQ commented Dec 31, 2024

My thought is that we can just remove the ID filter from the BPF. Just allow all ICMP packets to be read by the library.

Copy link
Contributor

@SuperQ SuperQ left a comment

Choose a reason for hiding this comment

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

Needs an error handler.

@nvksie
Copy link
Contributor Author

nvksie commented Jan 1, 2025

Needs an error handler.

Done

@SuperQ SuperQ merged commit e89c957 into prometheus-community:main Jan 2, 2025
5 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