Conversation
For the agent, we don't need to process close() events where an error occurred because they won't affect our fd tracking. Drop these events early so we don't take a perf hit from superfluous but valid close() calls.
This avoids locking the consumer(s) and a getnstimeofday() call
If the close fd is invalid or not open, drop the enter event because it will have no side effects on the agent.
bertocci
changed the title
| } | ||
| } | ||
|
|
||
| if (drop_flags & UF_NEVER_DROP) { |
There was a problem hiding this comment.
Is the added code placed before this check on purpose? This means even when drop_flags is UF_NEVER_DROP, some close events will be dropped.
driver/main.c
Outdated
| // to spike dramatically, so drop close events if the fd is not valid. | ||
| if (consumer->dropping_mode) { | ||
| if (event_type == PPME_SYSCALL_CLOSE_X) { | ||
| if (syscall_get_return_value(current, regs) < 0) { |
There was a problem hiding this comment.
Coding style - no braces for single statement blocks
driver/main.c
Outdated
| spin_unlock(&files->file_lock); | ||
| } | ||
|
|
||
| if (close_return) { |
There was a problem hiding this comment.
Coding style - no braces for single statement blocks
| // It's annoying but valid for a program to make a large number of | ||
| // close() calls on nonexistent fds. That can cause driver cpu usage | ||
| // to spike dramatically, so drop close events if the fd is not valid. | ||
| if (consumer->dropping_mode) { |
There was a problem hiding this comment.
Would it make sense to merge this "if" together with the next one if (consumer->dropping_mode)?
There was a problem hiding this comment.
This check needs to be before UF_NEVER_DROP. I updated the comment, let me know if it's clear now.
driver/main.c
Outdated
| files = current->files; | ||
| spin_lock(&files->file_lock); | ||
| fdt = files_fdtable(files); | ||
| if (close_fd >= fdt->max_fds) { |
There was a problem hiding this comment.
Should this be in or with the one below? close_fd >= fdt->max_fds || !fd_is_open(close_fd, fdt)? Probably it's a refuse since it was or before: 132c5f1#diff-736f6eba33b36b171c5153d740c23121R1355
| #else | ||
| !fd_is_open(close_fd, fdt) | ||
| #endif | ||
| ) { |
There was a problem hiding this comment.
@ret2libc This violates the braces standards but I left it in to make the versioned if more clear. It wouldn't be needed if we were an in-tree module.
| files = current->files; | ||
| spin_lock(&files->file_lock); | ||
| fdt = files_fdtable(files); | ||
| if (close_fd < 0 || close_fd >= fdt->max_fds || |
There was a problem hiding this comment.
How can close_fd be < 0 if it's unsigned?
There was a problem hiding this comment.
Good catch. It was working because the negative values were > fdt->max_fds when represented unsigned, but the latest fix does it correctly.
* Drop close() exit events with error returns For the agent, we don't need to process close() events where an error occurred because they won't affect our fd tracking. Drop these events early so we don't take a perf hit from superfluous but valid close() calls. * Drop close() events early This avoids locking the consumer(s) and a getnstimeofday() call * Drop close() enter events If the close fd is invalid or not open, drop the enter event because it will have no side effects on the agent. * code cleanup * Restrict close() drop logic to the agent + code cleanup * Add support for kernels older than 3.4.x * Address review comments * Don't process close() events with negative fd * Handle negative fd values correctly
4 participants
Check close() exit/enter events for a valid and open fd before doing any further processing. This prevents us from getting overwhelmed by applications who call close() on a large number of potentially invalid fds.