1:49 a.m.
Profiling showed up a couple more bottlenecks Steve spotted a task
refcounting leak too. I'm _fairly_ sure the removal of
audit_zero_context() is OK. We'll be filling it in again before we use
it anyway.
* Fri Jul 29 2005 David Woodhouse <dwmw2(a)redhat.com> audit.82
- Speed up audit_filter_syscall() for the fast path
- Fix task refcounting in audit_free() and audit_syscall_exit()
- Remove audit_zero_context() in audit_syscall_exit()
--- linux-2.6.9/kernel/auditsc.c.p20064 2005-07-29 15:46:05.000000000 +0100
+++ linux-2.6.9/kernel/auditsc.c 2005-07-29 15:46:05.000000000 +0100
@@ -523,20 +523,23 @@ static enum audit_state audit_filter_sys
struct list_head *list)
{
struct audit_entry *e;
- enum audit_state state;
- int word = AUDIT_WORD(ctx->major);
- int bit = AUDIT_BIT(ctx->major);
+ enum audit_state state;
if (audit_pid && tsk->tgid == audit_pid)
return AUDIT_DISABLED;
rcu_read_lock();
- list_for_each_entry_rcu(e, list, list) {
- if ((e->rule.mask[word] & bit) == bit
- && audit_filter_rules(tsk, &e->rule, ctx, &state)) {
- rcu_read_unlock();
- return state;
- }
+ if (!list_empty(list)) {
+ int word = AUDIT_WORD(ctx->major);
+ int bit = AUDIT_BIT(ctx->major);
+
+ list_for_each_entry_rcu(e, list, list) {
+ if ((e->rule.mask[word] & bit) == bit
+ && audit_filter_rules(tsk, &e->rule, ctx, &state)) {
+ rcu_read_unlock();
+ return state;
+ }
+ }
}
rcu_read_unlock();
return AUDIT_BUILD_CONTEXT;
@@ -944,7 +947,7 @@ void audit_free(struct task_struct *tsk)
task_unlock(tsk);
if (likely(!context))
- return;
+ goto out;
/* Check for system calls that do not go through the exit
* function (e.g., exit_group), then free context block.
@@ -953,6 +956,7 @@ void audit_free(struct task_struct *tsk)
if (context->in_syscall && context->auditable)
audit_log_exit(context, GFP_ATOMIC);
+ out:
audit_free_context(context);
}
@@ -1053,7 +1057,7 @@ void audit_syscall_exit(struct task_stru
/* Not having a context here is ok, since the parent may have
* called __put_task_struct. */
if (likely(!context))
- return;
+ goto out;
if (context->in_syscall && context->auditable)
audit_log_exit(context, GFP_KERNEL);
@@ -1069,9 +1073,9 @@ void audit_syscall_exit(struct task_stru
} else {
audit_free_names(context);
audit_free_aux(context);
- audit_zero_context(context, context->state);
tsk->audit_context = context;
}
+ out:
put_task_struct(tsk);
}
--
dwmw2
7:54 a.m.
On Saturday 30 July 2005 02:49, David Woodhouse wrote:
Profiling showed up a couple more bottlenecks
I just ran the same test and we have made a lot of progress in speeding things
up when there are no rules loaded. The test checks disk access with audit
enabled and disabled.
This is the current numbers -
DISABLED:
real 0m32.591s
user 0m2.336s
sys 0m30.239s
ENABLED:
real 0m38.829s
user 0m2.036s
sys 0m36.792s
Yesterday, enabled real time was 46s. So, this is a big improvement. Here's
the current bottlenecks:
36103 total 0.0170
2552 audit_data_get 4.2604
1568 avc_lookup 8.9600
1482 inode_has_perm 17.6429
1239 audit_syscall_exit 1.0098
1207 __might_sleep 8.8102
1016 path_lookup 3.2880
858 dput 0.8102
818 audit_notify_watch 9.6235
The patch didn't do anything for audit_data_get and that is now showing up as
our current problem. Is there any chance of using a different lock scheme
that is lightweight for readers and heavier for writers? I feel that if we
can do something for this function, we've got the performance issues solved
for people that aren't using the audit system.
I'm _fairly_ sure the removal of audit_zero_context() is OK.
Kris, this new kernel needs thorough testing so we can spot any regressions.
David, my patch differed a little from the one sent to the mail list:
@@ -1052,24 +1055,27 @@ void audit_syscall_exit(struct task_stru
<snip>
- context->in_syscall = 0;
- context->auditable = 0;
-
if (context->previous) {
struct audit_context *new_context = context->previous;
context->previous = NULL;
audit_free_context(context);
tsk->audit_context = new_context;
} else {
+ context->in_syscall = 0;
+ context->auditable = 0;
audit_free_names(context);
audit_free_aux(context);
This moves the unconditional zeroing into the path that keeps the context.
-Steve
10:07 a.m.
On Sat, 2005-07-30 at 08:54 -0400, Steve Grubb wrote:
The patch didn't do anything for audit_data_get and that is now
showing up as
our current problem. Is there any chance of using a different lock scheme
that is lightweight for readers and heavier for writers? I feel that if we
can do something for this function, we've got the performance issues solved
for people that aren't using the audit system.
We could almost certainly get away with checking the common case
(!allocate && !(flags & I_AUDIT)) without actually taking the lock.
That ought to suffice, but I didn't want to make that change in the same
release as the other changes.
David, my patch differed a little from the one sent to the mail list:
<...>
This moves the unconditional zeroing into the path that keeps the
context.
That's the common case by far anyway. It'll make zero difference in the
profile, AFAICT. Let's not just move stuff around for the sake of it.
--
dwmw2
9:50 p.m.
On Saturday 30 July 2005 11:07, David Woodhouse wrote:
That ought to suffice, but I didn't want to make that change in
the same
release as the other changes.
I'm game to do the performance testing. Do you want to make a .83 kernel or
just post a patch? audit_data_get is the only stand-out problem that we have
to address performance-wise regarding all users not loading any rules. .82
made major improvements for the average user that has no rules loaded.
It'll make zero difference in the profile, AFAICT. Let's not
just move stuff
around for the sake of it.
Well, in this profile - you're right. But regarding nested function calls, it
does save some time. This function is important, audit_syscall_exit.
Everything does it. Lets make it 100% optimal.
-Steve
5:11 a.m.
On Sat, 2005-07-30 at 22:50 -0400, Steve Grubb wrote:
I'm game to do the performance testing. Do you want to make a .83
kernel or
just post a patch?
I'm building audit.83 with this...
--- linux-2.6.9/kernel/auditfs.c~ 2005-07-20 15:42:46.000000000 +0100
+++ linux-2.6.9/kernel/auditfs.c 2005-07-31 10:46:06.000000000 +0100
@@ -116,10 +116,17 @@ static struct audit_inode_data *audit_da
struct audit_inode_data *ret = NULL;
int h;
+ /* Short-circuit _without_ getting the lock. Even if i_state is being
+ modified, it won't affect the I_AUDIT bit, unless the I_AUDIT
+ bit itself is actually being changed -- which is fine. Either
+ we tested before or after the change; either is fine. */
+ if (!allocate && !(inode->i_state & I_AUDIT))
+ return NULL;
+
spin_lock(&auditfs_hash_lock);
- /* I_AUDIT bit can only be changed under auditfs_hash_lock, so no need
- to lock inode_lock (on all known hardware) */
+ /* If we think there are audit data attached, double-check that
+ now we have the lock */
if (!allocate && !(inode->i_state & I_AUDIT))
goto out;
>It'll make zero difference in the profile, AFAICT. Let's
not just move stuff
>around for the sake of it.
Well, in this profile - you're right. But regarding nested function calls, it
does save some time. This function is important, audit_syscall_exit.
Everything does it. Lets make it 100% optimal.
Why do you speak of 'nested function calls'? We're talking about moving
two simple assignments which are going to be executed in 99.999% of
cases anyway.
You'd do better to look at access patterns and re-order the members of
the audit context for optimal cache performance. At the moment, when we
zero 'in_syscall' and 'auditable' that's actually likely to be
dirtying
two separate cache lines.
But I don't really want to be doing even that at this stage. We can do
it upstream but now is not the time to be changing anything more than we
really need to in the kernels which are supposed to be in certification
_already_. If the performance testing had started a few months ago, then
perhaps we could have done it before the certification -- but I really
don't think we should be going too far with it right now.
--
dwmw2
12:08 p.m.
On Sunday 31 July 2005 06:11, David Woodhouse wrote:
I'm building audit.83 with this...
Here's the new numbers -
DISABLED:
real 0m35.197s
user 0m2.005s
sys 0m33.192s
ENABLED:
real 0m41.511s
user 0m2.722s
sys 0m38.789s
performance for this test seemed better under kernel .82. This change must
have done something with caching. The current stats:
43376 total 0.0204
5558 link_path_walk 1.5971
4592 __d_lookup 9.2768
2907 inode_has_perm 34.6071
1877 avc_lookup 10.7257
1749 selinux_inode_permission 10.7963
1723 atomic_dec_and_lock 9.5722
1500 path_lookup 4.8544
1463 dput 1.3815
1417 direct_strncpy_from_user 15.2366
1406 audit_notify_watch 16.5412
1292 audit_syscall_exit 1.0530
1282 audit_inode 7.6766
1275 system_call 28.9773
Audit functions are way down the list. SE Linux has more to do with
performance now than audit.
Why do you speak of 'nested function calls'?
That's what the comments say the ->previous pointer is used for.
-Steve
7083
days inactive
7084
days old
linux-audit@lists.linux-audit.osci.io
5 comments
2 participants
participants (2)
-
David Woodhouse -
Steve Grubb