2022-09-27 23:11:09

by Ankur Arora

[permalink] [raw]
Subject: [PATCH 3/3] audit: unify audit_filter_{uring(),inode_name(),syscall()}

audit_filter_uring(), audit_filter_inode_name() are substantially similar
to audit_filter_syscall(). Move the core logic to __audit_filter() which
can be parametrized for all three.

On a Skylakex system, getpid() latency (all results aggregated
across 12 boot cycles):

Min Mean Median Max pstdev
(ns) (ns) (ns) (ns)

- 173.11 182.51 179.65 202.09 (+- 4.34%)
+ 162.11 175.26 173.71 190.56 (+- 4.33%)

Performance counter stats for 'bin/getpid' (3 runs) go from:
cycles 706.13 ( +- 4.13% )
instructions 1654.70 ( +- .06% )
IPC 2.35 ( +- 4.25% )
branches 430.99 ( +- .06% )
branch-misses 0.50 ( +- 2.00% )
L1-dcache-loads 440.02 ( +- .07% )
L1-dcache-load-misses 5.22 ( +- 82.75% )

to:
cycles 678.79 ( +- 4.22% )
instructions 1657.79 ( +- .05% )
IPC 2.45 ( +- 4.08% )
branches 432.00 ( +- .05% )
branch-misses 0.38 ( +- 23.68% )
L1-dcache-loads 444.96 ( +- .03% )
L1-dcache-load-misses 5.13 ( +- 73.09% )

(Both aggregated over 12 boot cycles.)

Unclear if the improvement is just run-to-run variation or because of
a slightly denser loop (the list parameter in the list_for_each_entry_rcu()
exit check now comes from a register rather than a constant as before.)

Signed-off-by: Ankur Arora <[email protected]>
---
kernel/auditsc.c | 86 +++++++++++++++++++++++++-----------------------
1 file changed, 44 insertions(+), 42 deletions(-)

diff --git a/kernel/auditsc.c b/kernel/auditsc.c
index bf26f47b5226..dd89a52988b0 100644
--- a/kernel/auditsc.c
+++ b/kernel/auditsc.c
@@ -805,6 +805,41 @@ static bool audit_in_mask(const struct audit_krule *rule, unsigned long val)
return rule->mask[word] & bit;
}

+/**
+ * __audit_filter - common filter
+ *
+ * @tsk: associated task
+ * @ctx: audit context
+ * @list: audit filter list
+ * @op: current syscall/uring_op
+ * @name: name to be filtered (used by audit_filter_inode_name)
+ *
+ * return: 1 if we hit a filter, 0 if we don't
+ */
+static int __audit_filter(struct task_struct *tsk,
+ struct audit_context *ctx,
+ struct list_head *list,
+ unsigned long op,
+ struct audit_names *name)
+{
+ struct audit_entry *e;
+ enum audit_state state;
+
+ rcu_read_lock();
+ list_for_each_entry_rcu(e, list, list) {
+ if (unlikely(audit_in_mask(&e->rule, op))) {
+ if (audit_filter_rules(tsk, &e->rule, ctx, name,
+ &state, false)) {
+ rcu_read_unlock();
+ ctx->current_state = state;
+ return 1;
+ }
+ }
+ }
+ rcu_read_unlock();
+ return 0;
+}
+
/**
* audit_filter_uring - apply filters to an io_uring operation
* @tsk: associated task
@@ -813,24 +848,11 @@ static bool audit_in_mask(const struct audit_krule *rule, unsigned long val)
static void audit_filter_uring(struct task_struct *tsk,
struct audit_context *ctx)
{
- struct audit_entry *e;
- enum audit_state state;
-
if (auditd_test_task(tsk))
return;

- rcu_read_lock();
- list_for_each_entry_rcu(e, &audit_filter_list[AUDIT_FILTER_URING_EXIT],
- list) {
- if (audit_in_mask(&e->rule, ctx->uring_op) &&
- audit_filter_rules(tsk, &e->rule, ctx, NULL, &state,
- false)) {
- rcu_read_unlock();
- ctx->current_state = state;
- return;
- }
- }
- rcu_read_unlock();
+ __audit_filter(tsk, ctx, &audit_filter_list[AUDIT_FILTER_URING_EXIT],
+ ctx->uring_op, NULL);
}

/* At syscall exit time, this filter is called if the audit_state is
@@ -841,26 +863,11 @@ static void audit_filter_uring(struct task_struct *tsk,
static void audit_filter_syscall(struct task_struct *tsk,
struct audit_context *ctx)
{
- struct audit_entry *e;
- enum audit_state state;
- unsigned long major = ctx->major;
-
if (auditd_test_task(tsk))
return;

- rcu_read_lock();
- list_for_each_entry_rcu(e, &audit_filter_list[AUDIT_FILTER_EXIT], list) {
- if (unlikely(audit_in_mask(&e->rule, major))) {
- if (audit_filter_rules(tsk, &e->rule, ctx, NULL,
- &state, false)) {
- rcu_read_unlock();
- ctx->current_state = state;
- return;
- }
- }
- }
- rcu_read_unlock();
- return;
+ __audit_filter(tsk, ctx, &audit_filter_list[AUDIT_FILTER_EXIT],
+ ctx->major, NULL);
}

/*
@@ -872,17 +879,12 @@ static int audit_filter_inode_name(struct task_struct *tsk,
struct audit_context *ctx) {
int h = audit_hash_ino((u32)n->ino);
struct list_head *list = &audit_inode_hash[h];
- struct audit_entry *e;
- enum audit_state state;

- list_for_each_entry_rcu(e, list, list) {
- if (audit_in_mask(&e->rule, ctx->major) &&
- audit_filter_rules(tsk, &e->rule, ctx, n, &state, false)) {
- ctx->current_state = state;
- return 1;
- }
- }
- return 0;
+ /*
+ * We are called holding an rcu read lock. __audit_filter() will take
+ * one as well.
+ */
+ return __audit_filter(tsk, ctx, list, ctx->major, n);
}

/* At syscall exit time, this filter is called if any audit_names have been
--
2.31.1


2022-09-28 23:34:26

by Paul Moore

[permalink] [raw]
Subject: Re: [PATCH 3/3] audit: unify audit_filter_{uring(),inode_name(),syscall()}

On Tue, Sep 27, 2022 at 7:00 PM Ankur Arora <[email protected]> wrote:
>
> audit_filter_uring(), audit_filter_inode_name() are substantially similar
> to audit_filter_syscall(). Move the core logic to __audit_filter() which
> can be parametrized for all three.
>
> On a Skylakex system, getpid() latency (all results aggregated
> across 12 boot cycles):
>
> Min Mean Median Max pstdev
> (ns) (ns) (ns) (ns)
>
> - 173.11 182.51 179.65 202.09 (+- 4.34%)
> + 162.11 175.26 173.71 190.56 (+- 4.33%)
>
> Performance counter stats for 'bin/getpid' (3 runs) go from:
> cycles 706.13 ( +- 4.13% )
> instructions 1654.70 ( +- .06% )
> IPC 2.35 ( +- 4.25% )
> branches 430.99 ( +- .06% )
> branch-misses 0.50 ( +- 2.00% )
> L1-dcache-loads 440.02 ( +- .07% )
> L1-dcache-load-misses 5.22 ( +- 82.75% )
>
> to:
> cycles 678.79 ( +- 4.22% )
> instructions 1657.79 ( +- .05% )
> IPC 2.45 ( +- 4.08% )
> branches 432.00 ( +- .05% )
> branch-misses 0.38 ( +- 23.68% )
> L1-dcache-loads 444.96 ( +- .03% )
> L1-dcache-load-misses 5.13 ( +- 73.09% )
>
> (Both aggregated over 12 boot cycles.)
>
> Unclear if the improvement is just run-to-run variation or because of
> a slightly denser loop (the list parameter in the list_for_each_entry_rcu()
> exit check now comes from a register rather than a constant as before.)
>
> Signed-off-by: Ankur Arora <[email protected]>
> ---
> kernel/auditsc.c | 86 +++++++++++++++++++++++++-----------------------
> 1 file changed, 44 insertions(+), 42 deletions(-)
>
> diff --git a/kernel/auditsc.c b/kernel/auditsc.c
> index bf26f47b5226..dd89a52988b0 100644
> --- a/kernel/auditsc.c
> +++ b/kernel/auditsc.c
> @@ -805,6 +805,41 @@ static bool audit_in_mask(const struct audit_krule *rule, unsigned long val)
> return rule->mask[word] & bit;
> }
>
> +/**
> + * __audit_filter - common filter
> + *

Please remove the vertical whitespace between the function description
line and the parameter descriptions.

I'd also suggest renaming this function to "__audit_filter_op(...)"
since we have a few audit filtering functions and a generic
"__audit_filter()" function with no qualification in the name seems
just a bit too generic to not be misleading ... at least I think so.

I also might try to be just a bit more descriptive in the text above,
for example:

"__audit_filter_op - filter helper function for operations (syscall/uring/etc.)"

> + * @tsk: associated task
> + * @ctx: audit context
> + * @list: audit filter list
> + * @op: current syscall/uring_op
> + * @name: name to be filtered (used by audit_filter_inode_name)

I would change this to "@name: audit_name to use in filtering (can be
NULL)" and leave it at that.

> + *
> + * return: 1 if we hit a filter, 0 if we don't

The function header block comment description should be in regular
English sentences. Perhaps something like this:

"Run the audit filters specified in @list against @tsk using @ctx,
@op, and @name as necessary; the caller is responsible for ensuring
that the call is made while the RCU read lock is held. The @name
parameter can be NULL, but all others must be specified. Returns
1/true if the filter finds a match, 0/false if none are found."

> + */
> +static int __audit_filter(struct task_struct *tsk,
> + struct audit_context *ctx,
> + struct list_head *list,
> + unsigned long op,
> + struct audit_names *name)
> +{
> + struct audit_entry *e;
> + enum audit_state state;
> +
> + rcu_read_lock();

I would move the RCU locking to the callers since not every caller requires it.

> + list_for_each_entry_rcu(e, list, list) {
> + if (unlikely(audit_in_mask(&e->rule, op))) {

As discussed in patch 2/3, please remove the unlikely() call.

> + if (audit_filter_rules(tsk, &e->rule, ctx, name,
> + &state, false)) {
> + rcu_read_unlock();
> + ctx->current_state = state;
> + return 1;
> + }
> + }
> + }
> + rcu_read_unlock();
> + return 0;
> +}
> +

--
paul-moore.com

2022-09-29 21:09:28

by Ankur Arora

[permalink] [raw]
Subject: Re: [PATCH 3/3] audit: unify audit_filter_{uring(),inode_name(),syscall()}


Paul Moore <[email protected]> writes:

> On Tue, Sep 27, 2022 at 7:00 PM Ankur Arora <[email protected]> wrote:
>>
>> audit_filter_uring(), audit_filter_inode_name() are substantially similar
>> to audit_filter_syscall(). Move the core logic to __audit_filter() which
>> can be parametrized for all three.
>>
>> On a Skylakex system, getpid() latency (all results aggregated
>> across 12 boot cycles):
>>
>> Min Mean Median Max pstdev
>> (ns) (ns) (ns) (ns)
>>
>> - 173.11 182.51 179.65 202.09 (+- 4.34%)
>> + 162.11 175.26 173.71 190.56 (+- 4.33%)
>>
>> Performance counter stats for 'bin/getpid' (3 runs) go from:
>> cycles 706.13 ( +- 4.13% )
>> instructions 1654.70 ( +- .06% )
>> IPC 2.35 ( +- 4.25% )
>> branches 430.99 ( +- .06% )
>> branch-misses 0.50 ( +- 2.00% )
>> L1-dcache-loads 440.02 ( +- .07% )
>> L1-dcache-load-misses 5.22 ( +- 82.75% )
>>
>> to:
>> cycles 678.79 ( +- 4.22% )
>> instructions 1657.79 ( +- .05% )
>> IPC 2.45 ( +- 4.08% )
>> branches 432.00 ( +- .05% )
>> branch-misses 0.38 ( +- 23.68% )
>> L1-dcache-loads 444.96 ( +- .03% )
>> L1-dcache-load-misses 5.13 ( +- 73.09% )
>>
>> (Both aggregated over 12 boot cycles.)
>>
>> Unclear if the improvement is just run-to-run variation or because of
>> a slightly denser loop (the list parameter in the list_for_each_entry_rcu()
>> exit check now comes from a register rather than a constant as before.)
>>
>> Signed-off-by: Ankur Arora <[email protected]>
>> ---
>> kernel/auditsc.c | 86 +++++++++++++++++++++++++-----------------------
>> 1 file changed, 44 insertions(+), 42 deletions(-)
>>
>> diff --git a/kernel/auditsc.c b/kernel/auditsc.c
>> index bf26f47b5226..dd89a52988b0 100644
>> --- a/kernel/auditsc.c
>> +++ b/kernel/auditsc.c
>> @@ -805,6 +805,41 @@ static bool audit_in_mask(const struct audit_krule *rule, unsigned long val)
>> return rule->mask[word] & bit;
>> }
>>
>> +/**
>> + * __audit_filter - common filter
>> + *
>
> Please remove the vertical whitespace between the function description
> line and the parameter descriptions.
>
> I'd also suggest renaming this function to "__audit_filter_op(...)"
> since we have a few audit filtering functions and a generic
> "__audit_filter()" function with no qualification in the name seems
> just a bit too generic to not be misleading ... at least I think so.
>
> I also might try to be just a bit more descriptive in the text above,
> for example:
>
> "__audit_filter_op - filter helper function for operations (syscall/uring/etc.)"
>
>> + * @tsk: associated task
>> + * @ctx: audit context
>> + * @list: audit filter list
>> + * @op: current syscall/uring_op
>> + * @name: name to be filtered (used by audit_filter_inode_name)
>
> I would change this to "@name: audit_name to use in filtering (can be
> NULL)" and leave it at that.
>
>> + *
>> + * return: 1 if we hit a filter, 0 if we don't
>
> The function header block comment description should be in regular
> English sentences. Perhaps something like this:
>
> "Run the audit filters specified in @list against @tsk using @ctx,
> @op, and @name as necessary; the caller is responsible for ensuring
> that the call is made while the RCU read lock is held. The @name
> parameter can be NULL, but all others must be specified. Returns
> 1/true if the filter finds a match, 0/false if none are found."
>
>> + */
>> +static int __audit_filter(struct task_struct *tsk,
>> + struct audit_context *ctx,
>> + struct list_head *list,
>> + unsigned long op,
>> + struct audit_names *name)
>> +{
>> + struct audit_entry *e;
>> + enum audit_state state;
>> +
>> + rcu_read_lock();
>
> I would move the RCU locking to the callers since not every caller requires it.
>
>> + list_for_each_entry_rcu(e, list, list) {
>> + if (unlikely(audit_in_mask(&e->rule, op))) {
>
> As discussed in patch 2/3, please remove the unlikely() call.

I'll pull it out of this patch.

And thanks for the comments. Will address and send out a v2.


Ankur

>
>> + if (audit_filter_rules(tsk, &e->rule, ctx, name,
>> + &state, false)) {
>> + rcu_read_unlock();
>> + ctx->current_state = state;
>> + return 1;
>> + }
>> + }
>> + }
>> + rcu_read_unlock();
>> + return 0;
>> +}
>> +

2022-10-07 01:15:05

by Ankur Arora

[permalink] [raw]
Subject: [PATCH v2] audit: unify audit_filter_{uring(),inode_name(),syscall()}

audit_filter_uring(), audit_filter_inode_name() are substantially
similar to audit_filter_syscall(). Move the core logic to
__audit_filter_op() which can be parametrized for all three.

On a Skylakex system, getpid() latency (all results aggregated
across 12 boot cycles):

Min Mean Median Max pstdev
(ns) (ns) (ns) (ns)

- 196.63 207.86 206.60 230.98 (+- 3.92%)
+ 183.73 196.95 192.31 232.49 (+- 6.04%)

Performance counter stats for 'bin/getpid' (3 runs) go from:
cycles 805.58 ( +- 4.11% )
instructions 1654.11 ( +- .05% )
IPC 2.06 ( +- 3.39% )
branches 430.02 ( +- .05% )
branch-misses 1.55 ( +- 7.09% )
L1-dcache-loads 440.01 ( +- .09% )
L1-dcache-load-misses 9.05 ( +- 74.03% )
to:
cycles 765.37 ( +- 6.66% )
instructions 1677.07 ( +- 0.04% )
IPC 2.20 ( +- 5.90% )
branches 431.10 ( +- 0.04% )
branch-misses 1.60 ( +- 11.25% )
L1-dcache-loads 521.04 ( +- 0.05% )
L1-dcache-load-misses 6.92 ( +- 77.60% )

(Both aggregated over 12 boot cycles.)

The increased L1-dcache-loads are due to some intermediate values now
coming from the stack.

The improvement in cycles is due to a slightly denser loop (the list
parameter in the list_for_each_entry_rcu() exit check now comes from
a register rather than a constant as before.)

Signed-off-by: Ankur Arora <[email protected]>
---
kernel/auditsc.c | 76 +++++++++++++++++++++++++-----------------------
1 file changed, 39 insertions(+), 37 deletions(-)

diff --git a/kernel/auditsc.c b/kernel/auditsc.c
index 2206cdf1ba2c..4991348e300a 100644
--- a/kernel/auditsc.c
+++ b/kernel/auditsc.c
@@ -807,6 +807,40 @@ static int audit_in_mask(const struct audit_krule *rule, unsigned long val)
return rule->mask[word] & bit;
}

+/**
+ * __audit_filter_op - common filter helper for operations (syscall/uring/etc)
+ * @tsk: associated task
+ * @ctx: audit context
+ * @list: audit filter list
+ * @name: audit_name (can be NULL)
+ * @op: current syscall/uring_op
+ *
+ * Run the udit filters specified in @list against @tsk using @ctx,
+ * @name, and @op, as necessary; the caller is responsible for ensuring
+ * that the call is made while the RCU read lock is held. The @name
+ * parameter can be NULL, but all others must be specified.
+ * Returns 1/true if the filter finds a match, 0/false if none are found.
+ */
+static int __audit_filter_op(struct task_struct *tsk,
+ struct audit_context *ctx,
+ struct list_head *list,
+ struct audit_names *name,
+ unsigned long op)
+{
+ struct audit_entry *e;
+ enum audit_state state;
+
+ list_for_each_entry_rcu(e, list, list) {
+ if (audit_in_mask(&e->rule, op) &&
+ audit_filter_rules(tsk, &e->rule, ctx, name,
+ &state, false)) {
+ ctx->current_state = state;
+ return 1;
+ }
+ }
+ return 0;
+}
+
/**
* audit_filter_uring - apply filters to an io_uring operation
* @tsk: associated task
@@ -815,23 +849,12 @@ static int audit_in_mask(const struct audit_krule *rule, unsigned long val)
static void audit_filter_uring(struct task_struct *tsk,
struct audit_context *ctx)
{
- struct audit_entry *e;
- enum audit_state state;
-
if (auditd_test_task(tsk))
return;

rcu_read_lock();
- list_for_each_entry_rcu(e, &audit_filter_list[AUDIT_FILTER_URING_EXIT],
- list) {
- if (audit_in_mask(&e->rule, ctx->uring_op) &&
- audit_filter_rules(tsk, &e->rule, ctx, NULL, &state,
- false)) {
- rcu_read_unlock();
- ctx->current_state = state;
- return;
- }
- }
+ __audit_filter_op(tsk, ctx, &audit_filter_list[AUDIT_FILTER_URING_EXIT],
+ NULL, ctx->uring_op);
rcu_read_unlock();
}

@@ -843,25 +866,13 @@ static void audit_filter_uring(struct task_struct *tsk,
static void audit_filter_syscall(struct task_struct *tsk,
struct audit_context *ctx)
{
- struct audit_entry *e;
- enum audit_state state;
- unsigned long major = ctx->major;
-
if (auditd_test_task(tsk))
return;

rcu_read_lock();
- list_for_each_entry_rcu(e, &audit_filter_list[AUDIT_FILTER_EXIT], list) {
- if (audit_in_mask(&e->rule, major) &&
- audit_filter_rules(tsk, &e->rule, ctx, NULL,
- &state, false)) {
- rcu_read_unlock();
- ctx->current_state = state;
- return;
- }
- }
+ __audit_filter_op(tsk, ctx, &audit_filter_list[AUDIT_FILTER_EXIT],
+ NULL, ctx->major);
rcu_read_unlock();
- return;
}

/*
@@ -873,17 +884,8 @@ static int audit_filter_inode_name(struct task_struct *tsk,
struct audit_context *ctx) {
int h = audit_hash_ino((u32)n->ino);
struct list_head *list = &audit_inode_hash[h];
- struct audit_entry *e;
- enum audit_state state;

- list_for_each_entry_rcu(e, list, list) {
- if (audit_in_mask(&e->rule, ctx->major) &&
- audit_filter_rules(tsk, &e->rule, ctx, n, &state, false)) {
- ctx->current_state = state;
- return 1;
- }
- }
- return 0;
+ return __audit_filter_op(tsk, ctx, list, n, ctx->major);
}

/* At syscall exit time, this filter is called if any audit_names have been
--
2.31.1

2022-10-14 00:37:16

by Paul Moore

[permalink] [raw]
Subject: Re: [PATCH v2] audit: unify audit_filter_{uring(),inode_name(),syscall()}

On Thu, Oct 6, 2022 at 8:49 PM Ankur Arora <[email protected]> wrote:
>
> audit_filter_uring(), audit_filter_inode_name() are substantially
> similar to audit_filter_syscall(). Move the core logic to
> __audit_filter_op() which can be parametrized for all three.
>
> On a Skylakex system, getpid() latency (all results aggregated
> across 12 boot cycles):
>
> Min Mean Median Max pstdev
> (ns) (ns) (ns) (ns)
>
> - 196.63 207.86 206.60 230.98 (+- 3.92%)
> + 183.73 196.95 192.31 232.49 (+- 6.04%)
>
> Performance counter stats for 'bin/getpid' (3 runs) go from:
> cycles 805.58 ( +- 4.11% )
> instructions 1654.11 ( +- .05% )
> IPC 2.06 ( +- 3.39% )
> branches 430.02 ( +- .05% )
> branch-misses 1.55 ( +- 7.09% )
> L1-dcache-loads 440.01 ( +- .09% )
> L1-dcache-load-misses 9.05 ( +- 74.03% )
> to:
> cycles 765.37 ( +- 6.66% )
> instructions 1677.07 ( +- 0.04% )
> IPC 2.20 ( +- 5.90% )
> branches 431.10 ( +- 0.04% )
> branch-misses 1.60 ( +- 11.25% )
> L1-dcache-loads 521.04 ( +- 0.05% )
> L1-dcache-load-misses 6.92 ( +- 77.60% )
>
> (Both aggregated over 12 boot cycles.)
>
> The increased L1-dcache-loads are due to some intermediate values now
> coming from the stack.
>
> The improvement in cycles is due to a slightly denser loop (the list
> parameter in the list_for_each_entry_rcu() exit check now comes from
> a register rather than a constant as before.)
>
> Signed-off-by: Ankur Arora <[email protected]>
> ---
> kernel/auditsc.c | 76 +++++++++++++++++++++++++-----------------------
> 1 file changed, 39 insertions(+), 37 deletions(-)

Thanks, this looks good to me. I'll queue this up for when the merge
window closes.

--
paul-moore.com

2022-10-14 17:22:22

by Ankur Arora

[permalink] [raw]
Subject: Re: [PATCH v2] audit: unify audit_filter_{uring(),inode_name(),syscall()}


Paul Moore <[email protected]> writes:

> On Thu, Oct 6, 2022 at 8:49 PM Ankur Arora <[email protected]> wrote:
>>
>> audit_filter_uring(), audit_filter_inode_name() are substantially
>> similar to audit_filter_syscall(). Move the core logic to
>> __audit_filter_op() which can be parametrized for all three.
>>
>> On a Skylakex system, getpid() latency (all results aggregated
>> across 12 boot cycles):
>>
>> Min Mean Median Max pstdev
>> (ns) (ns) (ns) (ns)
>>
>> - 196.63 207.86 206.60 230.98 (+- 3.92%)
>> + 183.73 196.95 192.31 232.49 (+- 6.04%)
>>
>> Performance counter stats for 'bin/getpid' (3 runs) go from:
>> cycles 805.58 ( +- 4.11% )
>> instructions 1654.11 ( +- .05% )
>> IPC 2.06 ( +- 3.39% )
>> branches 430.02 ( +- .05% )
>> branch-misses 1.55 ( +- 7.09% )
>> L1-dcache-loads 440.01 ( +- .09% )
>> L1-dcache-load-misses 9.05 ( +- 74.03% )
>> to:
>> cycles 765.37 ( +- 6.66% )
>> instructions 1677.07 ( +- 0.04% )
>> IPC 2.20 ( +- 5.90% )
>> branches 431.10 ( +- 0.04% )
>> branch-misses 1.60 ( +- 11.25% )
>> L1-dcache-loads 521.04 ( +- 0.05% )
>> L1-dcache-load-misses 6.92 ( +- 77.60% )
>>
>> (Both aggregated over 12 boot cycles.)
>>
>> The increased L1-dcache-loads are due to some intermediate values now
>> coming from the stack.
>>
>> The improvement in cycles is due to a slightly denser loop (the list
>> parameter in the list_for_each_entry_rcu() exit check now comes from
>> a register rather than a constant as before.)
>>
>> Signed-off-by: Ankur Arora <[email protected]>
>> ---
>> kernel/auditsc.c | 76 +++++++++++++++++++++++++-----------------------
>> 1 file changed, 39 insertions(+), 37 deletions(-)
>
> Thanks, this looks good to me. I'll queue this up for when the merge
> window closes.

Great. Thanks Paul.

--
ankur

2022-10-17 18:45:49

by Paul Moore

[permalink] [raw]
Subject: Re: [PATCH v2] audit: unify audit_filter_{uring(),inode_name(),syscall()}

On Thu, Oct 13, 2022 at 7:11 PM Paul Moore <[email protected]> wrote:
> On Thu, Oct 6, 2022 at 8:49 PM Ankur Arora <[email protected]> wrote:
> >
> > audit_filter_uring(), audit_filter_inode_name() are substantially
> > similar to audit_filter_syscall(). Move the core logic to
> > __audit_filter_op() which can be parametrized for all three.
> >
> > On a Skylakex system, getpid() latency (all results aggregated
> > across 12 boot cycles):
> >
> > Min Mean Median Max pstdev
> > (ns) (ns) (ns) (ns)
> >
> > - 196.63 207.86 206.60 230.98 (+- 3.92%)
> > + 183.73 196.95 192.31 232.49 (+- 6.04%)
> >
> > Performance counter stats for 'bin/getpid' (3 runs) go from:
> > cycles 805.58 ( +- 4.11% )
> > instructions 1654.11 ( +- .05% )
> > IPC 2.06 ( +- 3.39% )
> > branches 430.02 ( +- .05% )
> > branch-misses 1.55 ( +- 7.09% )
> > L1-dcache-loads 440.01 ( +- .09% )
> > L1-dcache-load-misses 9.05 ( +- 74.03% )
> > to:
> > cycles 765.37 ( +- 6.66% )
> > instructions 1677.07 ( +- 0.04% )
> > IPC 2.20 ( +- 5.90% )
> > branches 431.10 ( +- 0.04% )
> > branch-misses 1.60 ( +- 11.25% )
> > L1-dcache-loads 521.04 ( +- 0.05% )
> > L1-dcache-load-misses 6.92 ( +- 77.60% )
> >
> > (Both aggregated over 12 boot cycles.)
> >
> > The increased L1-dcache-loads are due to some intermediate values now
> > coming from the stack.
> >
> > The improvement in cycles is due to a slightly denser loop (the list
> > parameter in the list_for_each_entry_rcu() exit check now comes from
> > a register rather than a constant as before.)
> >
> > Signed-off-by: Ankur Arora <[email protected]>
> > ---
> > kernel/auditsc.c | 76 +++++++++++++++++++++++++-----------------------
> > 1 file changed, 39 insertions(+), 37 deletions(-)
>
> Thanks, this looks good to me. I'll queue this up for when the merge
> window closes.

This is also merged into audit/next, thanks!

--
paul-moore.com