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

dlt-system: Avoid expensive getpid() syscalls #717

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

Conversation

aleixpol
Copy link

@aleixpol aleixpol commented Dec 4, 2024

Since glibc 2.25, getpid() no longer caches the PID, instead performing a full syscall every time. This means we're performing a full syscall roundtrip for every single log message on the system.

This patch caches the result of getpid() when appropriate, avoiding unnessicary syscalls.

@minminlittleshrimp
Copy link
Collaborator

Thanks for the finding, do you have any references for us to refer @aleixpol
Thanks a lots.

@aleixpol
Copy link
Author

aleixpol commented Dec 6, 2024

I'm sorry, what references are you referring to?

@minminlittleshrimp
Copy link
Collaborator

I found doc of getpid enter and exit syscall on internet, thanks.
I am doing some metrics measuring, if it is possible from your side can you show how much the improvement would be? Maybe with 100-1000 user registered -> 1000 pids?

@@ -271,6 +271,10 @@ typedef struct
#ifdef DLT_TRACE_LOAD_CTRL_ENABLE
pthread_rwlock_t trace_load_limit_lock;
#endif
// Since glibc 2.25, getpid() no longer caches the PID, instead performing a
// full syscall every time. This means we're performing a full syscall
// roundtrip for *every* *single* *log* *message* on the system.
Copy link
Contributor

Choose a reason for hiding this comment

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

"Kindly please shorten these lines for variable definitions and align your comment with the standard used in the DltUser struct ?
Ex: /* Local DltUser process identifier */

Copy link
Collaborator

Choose a reason for hiding this comment

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

Do we still wait for author to refactor here? if yes then @aleixpol can you push latest patch?

Copy link
Author

Choose a reason for hiding this comment

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

Addressed.

@@ -4011,7 +4014,11 @@ DltReturnValue dlt_user_log_send_log(DltContextData *log, const int mtype, int *
/* send session id */
if (dlt_user.with_session_id) {
msg.standardheader->htyp |= DLT_HTYP_WSID;
msg.headerextra.seid = (uint32_t) getpid();
// 99.99999% of the time, we'll already have a PID set.
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you remove this comment? You're right, but it is better to remove this comment.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yeah we actually dont need it, unless any dev really want to know the func call then gcc doc already mentioned in detail

Copy link
Author

Choose a reason for hiding this comment

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

Removed.

Copy link
Contributor

@Bichdao021195 Bichdao021195 left a comment

Choose a reason for hiding this comment

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

Thanks for your contribution , can you check my comments?

@minminlittleshrimp
Copy link
Collaborator

Hello @aleixpol
Could you kindly as least show some metrics before and after applying this change?
I agree with the logic, but to get it merged, more information/proof should be provided.
Thanks

@minminlittleshrimp
Copy link
Collaborator

GCC reference:
https://gcc.gnu.org/onlinedocs/gcc/Other-Builtins.html

__builtin_expect()

@michael-methner
Copy link
Collaborator

Hello @aleixpol , hello @minminlittleshrimp , hello @Bichdao021195 , hello @lti9hc ,

i have verified the change with the test code at: dlt-daemon/src/tests/dlt-test-fork-handler.c
and it is working fine and providing correct pids even in this use case. I am fine with merging the change.

@minminlittleshrimp
Copy link
Collaborator

minminlittleshrimp commented Jan 8, 2025

Hello @aleixpol , hello @minminlittleshrimp , hello @Bichdao021195 , hello @lti9hc ,

i have verified the change with the test code at: dlt-daemon/src/tests/dlt-test-fork-handler.c

and it is working fine and providing correct pids even in this use case. I am fine with merging the change.

That's cool, will approve and merge after latest patchset uploaded

Since glibc 2.25, getpid() no longer caches the PID, instead performing
a full syscall every time. This means we're performing a full syscall
roundtrip for *every* *single* *log* *message* on the system.

This patch caches the result of getpid() when appropriate, avoiding
unnessicary syscalls.

Signed-off-by: Aleix Pol Gonzalez <[email protected]>
@aleixpol aleixpol force-pushed the work/apolgon/cache-getpid branch from 7167c39 to 34d8272 Compare January 8, 2025 18:38
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.

4 participants