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

chore(eBPF) set the threshold for a blocking call individually per pid #110

Merged
merged 4 commits into from
Dec 3, 2024

Conversation

der-whity
Copy link
Contributor

do this by inserting in the PIDS_TO_TRACK map with key: pid and value: duration_in_microseconds.
This PR also fixes am small bug that could occur if you remove a pid from the hashmap.

@der-whity der-whity requested a review from fhilgers November 26, 2024 08:19
@der-whity
Copy link
Contributor Author

There is no urge to merge this until the demo but it also won't have any negative impact on existing code build upon sendmsg

fhilgers
fhilgers previously approved these changes Nov 26, 2024
Copy link
Collaborator

@fhilgers fhilgers 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, but please lets merge this after the other branches, because we have to change something in the daemon and config code as well.

Another question? Why do we convert things to microseconds? Why do we not just keep it as nanos? Sure we most likely do not need the precision, but also, nanoseconds are the default and we would not have to do any conversion. We use u64 anyway, and we also do not need larger durations than u64 in nanoseconds would offer.

@der-whity
Copy link
Contributor Author

As you said, there is no need for this kind of precision and regarding the minimal impact our product should have, I figured I would minimize just a little bit.

@fhilgers
Copy link
Collaborator

I mean the overhead is exactly the same, as its a u64, we are just losing precision and the apis differ, e.g timestamp is in nanos and duration is in micros, or am I misunderstanding something?

@der-whity
Copy link
Contributor Author

no, the duration, that has to be transmitted from the frontend to the backend map, only needs to be a u32. And the precision-loss really is irrelevant.

@fhilgers
Copy link
Collaborator

I still think we should use u64, as the overhead is negligible and I see no need to lose precision. More precision might actually be useful for things like blocking calls. I think the lower bits of the duration is also way more important than the higher bits, as our system is most likely completely unresponsive if we get into minute durations for a single call. If we decide that we want to reduce overhead by 32 bits (which is not worth the effort in my opinion), it would be better to truncate the time to 32 bits than to loose the last 3 digits of precision.

@der-whity
Copy link
Contributor Author

Ok then I will change that

@der-whity der-whity marked this pull request as draft November 27, 2024 14:08
der-whity added a commit that referenced this pull request Nov 28, 2024
@der-whity der-whity marked this pull request as ready for review November 28, 2024 13:44
@der-whity der-whity requested a review from fhilgers November 28, 2024 13:44
Copy link
Contributor Author

@der-whity der-whity left a comment

Choose a reason for hiding this comment

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

The fd in the syscall is a C int and the rust doc states that it is an i32
And what happens with negative fd's now, when we only have an unsigned int?

@fhilgers
Copy link
Collaborator

fhilgers commented Nov 30, 2024

Yeah, but the tracepoint defines it with size: 8 and signed: 0:

name: sys_enter_sendmsg
ID: 1324
format:
	field:unsigned short common_type;	offset:0;	size:2;	signed:0;
	field:unsigned char common_flags;	offset:2;	size:1;	signed:0;
	field:unsigned char common_preempt_count;	offset:3;	size:1;	signed:0;
	field:int common_pid;	offset:4;	size:4;	signed:1;

	field:int __syscall_nr;	offset:8;	size:4;	signed:1;
	field:int fd;	offset:16;	size:8;	signed:0;
	field:struct user_msghdr * msg;	offset:24;	size:8;	signed:0;
	field:unsigned int flags;	offset:32;	size:8;	signed:0;

print fmt: "fd: 0x%08lx, msg: 0x%08lx, flags: 0x%08lx", ((unsigned long)(REC->fd)), ((unsigned long)(REC->msg)), ((unsigned long)(REC->flags))

It lead to a crash on @ffranzgitHub machine as well. The format description also casts it to unsigned long.

@der-whity der-whity requested a review from fhilgers December 2, 2024 15:23
fhilgers pushed a commit that referenced this pull request Dec 3, 2024
@fhilgers fhilgers force-pushed the 80-sendmsgConfigure branch from 8aa5e98 to e043509 Compare December 3, 2024 00:36
@fhilgers fhilgers force-pushed the 80-sendmsgConfigure branch from e043509 to 4e28a75 Compare December 3, 2024 00:37
@fhilgers fhilgers force-pushed the 80-sendmsgConfigure branch from ed803dc to 8cd794c Compare December 3, 2024 00:47
@fhilgers fhilgers requested a review from luca-dot-sh December 3, 2024 00:50
@fhilgers fhilgers merged commit ea47b0f into dev Dec 3, 2024
7 checks passed
@BenediktZinn BenediktZinn deleted the 80-sendmsgConfigure branch December 10, 2024 13:16
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.

3 participants