5:59 p.m.
This series attempts to reduce syscall-exit latency in the audit path,
especially for cases where there are a lot of audit exit rules.
From profiling, audit_filter_syscall() takes the largest chunk of
time,
specifically in this list-walk while processing the AUDIT_FILTER_EXIT
list:
list = &audit_filter_list[AUDIT_FILTER_EXIT];
list_for_each_entry_rcu(e, list, list) {
if (audit_in_mask(&e->rule, ctx->major) &&
audit_filter_rules(tsk, &e->rule, ctx, NULL,
&state, false, x)) {
rcu_read_unlock();
ctx->current_state = state;
return state;
}
}
(Note that almost identical logic exists in audit_filter_uring(),
audit_filter_inode_name().)
Comparing baseline performance with audit=0/audit=1 with a user-space
getpid() loop (executes 10^7 times.) For audit=1, this uses an
audit-rule set where the audit_filter_syscall() loop iterates over
42 AUDIT_FILTER_EXIT rules which, for getpid(), calls audit_filter_rules()
for 5 of them (we use this set of rules in production.)
Test system:
Server: ORACLE SERVER X8-2L
CPU: Intel Skylakex (6:85:6)
Microcode: 0x400320a
# v6.0.0-rc5.baseline, audit=0
Performance counter stats for 'bin/getpid' (3 runs):
734.10 msec task-clock # 0.999 CPUs utilized
( +- 0.03% )
1 context-switches # 1.361 /sec
( +- 66.67% )
0 cpu-migrations # 0.000 /sec
53 page-faults # 72.152 /sec
( +- 0.63% )
2,838,869,156 cycles # 3.865 GHz
( +- 0.01% ) (62.40%)
4,174,224,305 instructions # 1.47 insn per cycle
( +- 0.01% ) (74.93%)
890,798,133 branches # 1.213 G/sec
( +- 0.01% ) (74.93%)
5,015,118 branch-misses # 0.56% of all branches
( +- 0.05% ) (74.93%)
1,231,150,558 L1-dcache-loads # 1.676 G/sec
( +- 0.01% ) (74.94%)
418,297 L1-dcache-load-misses # 0.03% of all L1-dcache
accesses ( +- 0.68% ) (75.07%)
3,937 LLC-loads # 5.360 K/sec
( +- 3.76% ) (50.13%)
510 LLC-load-misses # 13.39% of all LL-cache
accesses ( +- 79.89% ) (50.00%)
0.735018 +- 0.000275 seconds time elapsed ( +- 0.04% )
# v6.0.0-rc5.baseline, audit=1
Performance counter stats for 'bin/getpid' (3 runs):
2,158.81 msec task-clock # 0.998 CPUs utilized
( +- 0.13% )
2 context-switches # 0.925 /sec
( +- 28.87% )
0 cpu-migrations # 0.000 /sec
52 page-faults # 24.056 /sec
( +- 0.64% )
8,364,119,898 cycles # 3.869 GHz
( +- 0.11% ) (62.48%)
19,996,521,678 instructions # 2.39 insn per cycle
( +- 0.01% ) (74.98%)
4,302,068,252 branches # 1.990 G/sec
( +- 0.00% ) (74.98%)
15,135,694 branch-misses # 0.35% of all branches
( +- 0.16% ) (74.99%)
4,714,694,841 L1-dcache-loads # 2.181 G/sec
( +- 0.01% ) (74.99%)
66,407,857 L1-dcache-load-misses # 1.41% of all L1-dcache
accesses ( +- 1.50% ) (75.01%)
6,785 LLC-loads # 3.139 K/sec
( +- 12.49% ) (50.03%)
3,235 LLC-load-misses # 41.29% of all LL-cache
accesses ( +- 6.08% ) (50.01%)
2.16213 +- 0.00288 seconds time elapsed ( +- 0.13% )
perf stat numbers for each getpid() iteration:
baseline baseline
audit=0 audit=1
cycles 283 836
instructions 417 1999
IPC 1.47 2.39
branches 89 430
branch-misses 0.50 1.51
L1-loads 123 471
L1-load-misses ~0 ~6.6*
* the L1-load-misses are largely stable for runs across a single
boot, but vary across boots due to vagaries of the SLAB allocator.
baseline audit=1 spends a significant amount of time executing in audit
code and incurs a new branch-miss and a few new L1-load-misses. Also
note that computed audit-only IPC is 2.86 so the baseline is not
ill-performant code.
Patches
==
Patch 1 "audit: cache ctx->major in audit_filter_syscall()", caches
ctx->major in a local variable. This gets rid of a persistent entry
from L1-dcache (audit_context::major) that had similar alignment
constraints as a potentially busy cache-set (audit_entry::list) and
allows some of the error checking in audit_in_mask() to be hoisted out
of the loop.
Patch 2: "audit: annotate branch direction for audit_in_mask()", so
the compiler can pessimize the call to audit_filter_rules().
Patch 3, "audit: unify audit_filter_{uring(),inode_name(),syscall()}"
centralizes this logic in a common function.
with these changes:
Performance counter stats for 'bin/getpid' (3 runs):
1,750.21 msec task-clock # 0.994 CPUs utilized
( +- 0.45% )
3 context-switches # 1.705 /sec
( +- 11.11% )
0 cpu-migrations # 0.000 /sec
52 page-faults # 29.548 /sec
( +- 0.64% )
6,770,976,590 cycles # 3.848 GHz
( +- 0.40% ) (27.74%)
16,588,372,024 instructions # 2.44 insn per cycle
( +- 0.03% ) (33.34%)
4,322,555,829 branches # 2.456 G/sec
( +- 0.02% ) (33.40%)
2,803,286 branch-misses # 0.06% of all branches
( +- 26.45% ) (33.46%)
4,449,426,336 L1-dcache-loads # 2.528 G/sec
( +- 0.01% ) (27.71%)
63,612,108 L1-dcache-load-misses # 1.43% of all L1-dcache
accesses ( +- 0.50% ) (27.71%)
6,123 LLC-loads # 3.479 K/sec
( +- 8.68% ) (27.71%)
1,890 LLC-load-misses # 26.69% of all LL-cache
accesses ( +- 46.99% ) (27.71%)
1.76033 +- 0.00791 seconds time elapsed ( +- 0.45% )
And, overall getpid() latency numbers (aggregated over 12 boots for each):
Min Mean Median Max pstdev
(ns) (ns) (ns) (ns)
baseline 201.30 216.14 216.22 228.46 (+- 1.45%)
patch1 196.63 207.86 206.60 230.98 (+- 3.92%)
patch1-2 173.11 182.51 179.65 202.09 (+- 4.34%)
patch1-3 162.11 175.26 173.71 190.56 (+- 4.33%)
This gives a reasonable speedup. My testing was on Intel Skylake, but I
suspect this should translate to other archs as well (especially on less
wide architectures.)
Please review.
Thanks
Ankur
Ankur Arora (3):
audit: cache ctx->major in audit_filter_syscall()
audit: annotate branch direction for audit_in_mask()
audit: unify audit_filter_{uring(),inode_name(),syscall()}
kernel/auditsc.c | 86 +++++++++++++++++++++++++-----------------------
1 file changed, 45 insertions(+), 41 deletions(-)
--
2.31.1
5:59 p.m.
New subject: [PATCH 1/3] audit: cache ctx->major in audit_filter_syscall()
ctx->major contains the current syscall number. This is, of course, a
constant for the duration of the syscall. Unfortunately, GCC's alias
analysis cannot prove that it is not modified via a pointer in the
audit_filter_syscall() loop, and so always loads it from memory.
In and of itself the load isn't very expensive (ops dependent on the
ctx->major load are only used to determine the direction of control flow
and have short dependence chains and, in any case the related branches
get predicted perfectly in the fastpath) but still cache ctx->major
in a local for two reasons:
* ctx->major is in the first cacheline of struct audit_context and has
similar alignment as audit_entry::list audit_entry. For cases
with a lot of audit rules, doing this reduces one source of contention
from a potentially busy cache-set.
* audit_in_mask() (called in the hot loop in audit_filter_syscall())
does cast manipulation and error checking on ctx->major:
audit_in_mask(const struct audit_krule *rule, unsigned long val):
if (val > 0xffffffff)
return false;
word = AUDIT_WORD(val);
if (word >= AUDIT_BITMASK_SIZE)
return false;
bit = AUDIT_BIT(val);
return rule->mask[word] & bit;
The clauses related to the rule need to be evaluated in the loop, but
the rest is unnecessarily re-evaluated for every loop iteration.
(Note, however, that most of these are cheap ALU ops and the branches
are perfectly predicted. However, see discussion on cycles
improvement below for more on why it is still worth hoisting.)
On a Skylakex system change in getpid() latency (aggregated over
12 boot cycles):
Min Mean Median Max pstdev
(ns) (ns) (ns) (ns)
- 201.30 216.14 216.22 228.46 (+- 1.45%)
+ 196.63 207.86 206.60 230.98 (+- 3.92%)
Performance counter stats for 'bin/getpid' (3 runs) go from:
cycles 836.89 ( +- .80% )
instructions 2000.19 ( +- .03% )
IPC 2.39 ( +- .83% )
branches 430.14 ( +- .03% )
branch-misses 1.48 ( +- 3.37% )
L1-dcache-loads 471.11 ( +- .05% )
L1-dcache-load-misses 7.62 ( +- 46.98% )
to:
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% )
(Both aggregated over 12 boot cycles.)
instructions: we reduce around 8 instructions/iteration because some of
the computation is now hoisted out of the loop (branch count does not
change because GCC, for reasons unclear, only hoists the computations
while keeping the basic-blocks.)
cycles: improve by about 5% (in aggregate and looking at individual run
numbers.) This is likely because we now waste fewer pipeline resources
on unnecessary instructions which allows the control flow to
speculatively execute further ahead shortening the execution of the loop
a little. The final gating factor on the performance of this loop
remains the long dependence chain due to the linked-list load.
Signed-off-by: Ankur Arora <ankur.a.arora(a)oracle.com>
---
kernel/auditsc.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/kernel/auditsc.c b/kernel/auditsc.c
index 79a5da1bc5bb..533b087c3c02 100644
--- a/kernel/auditsc.c
+++ b/kernel/auditsc.c
@@ -843,13 +843,14 @@ static void audit_filter_syscall(struct task_struct *tsk,
{
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, ctx->major) &&
+ if (audit_in_mask(&e->rule, major) &&
audit_filter_rules(tsk, &e->rule, ctx, NULL,
&state, false)) {
rcu_read_unlock();
--
2.31.1
5:03 p.m.
New subject: [PATCH 1/3] audit: cache ctx->major in audit_filter_syscall()
On Tue, Sep 27, 2022 at 6:59 PM Ankur Arora <ankur.a.arora(a)oracle.com> wrote:
ctx->major contains the current syscall number. This is, of course, a
constant for the duration of the syscall. Unfortunately, GCC's alias
analysis cannot prove that it is not modified via a pointer in the
audit_filter_syscall() loop, and so always loads it from memory.
...
Signed-off-by: Ankur Arora <ankur.a.arora(a)oracle.com>
---
kernel/auditsc.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
This looks pretty trivial to me, but it's too late in the current -rc
cycle for this to be merged, I'll queue it up for after the upcoming
merge window closes. Thanks.
diff --git a/kernel/auditsc.c b/kernel/auditsc.c
index 79a5da1bc5bb..533b087c3c02 100644
--- a/kernel/auditsc.c
+++ b/kernel/auditsc.c
@@ -843,13 +843,14 @@ static void audit_filter_syscall(struct task_struct *tsk,
{
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, ctx->major) &&
+ if (audit_in_mask(&e->rule, major) &&
audit_filter_rules(tsk, &e->rule, ctx, NULL,
&state, false)) {
rcu_read_unlock();
--
2.31.1
--
paul-moore.com
3:20 p.m.
New subject: [PATCH 1/3] audit: cache ctx->major in audit_filter_syscall()
Paul Moore <paul(a)paul-moore.com> writes:
On Tue, Sep 27, 2022 at 6:59 PM Ankur Arora
<ankur.a.arora(a)oracle.com> wrote:
>
> ctx->major contains the current syscall number. This is, of course, a
> constant for the duration of the syscall. Unfortunately, GCC's alias
> analysis cannot prove that it is not modified via a pointer in the
> audit_filter_syscall() loop, and so always loads it from memory.
>
> ...
>
> Signed-off-by: Ankur Arora <ankur.a.arora(a)oracle.com>
> ---
> kernel/auditsc.c | 3 ++-
> 1 file changed, 2 insertions(+), 1 deletion(-)
This looks pretty trivial to me, but it's too late in the current -rc
cycle for this to be merged, I'll queue it up for after the upcoming
merge window closes. Thanks.
Thanks.
Ankur
> diff --git a/kernel/auditsc.c b/kernel/auditsc.c
> index 79a5da1bc5bb..533b087c3c02 100644
> --- a/kernel/auditsc.c
> +++ b/kernel/auditsc.c
> @@ -843,13 +843,14 @@ static void audit_filter_syscall(struct task_struct *tsk,
> {
> 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, ctx->major) &&
> + if (audit_in_mask(&e->rule, major) &&
> audit_filter_rules(tsk, &e->rule, ctx, NULL,
> &state, false)) {
> rcu_read_unlock();
> --
> 2.31.1
--
ankur
1:23 p.m.
New subject: [PATCH 1/3] audit: cache ctx->major in audit_filter_syscall()
On Wed, Sep 28, 2022 at 6:03 PM Paul Moore <paul(a)paul-moore.com> wrote:
On Tue, Sep 27, 2022 at 6:59 PM Ankur Arora
<ankur.a.arora(a)oracle.com> wrote:
>
> ctx->major contains the current syscall number. This is, of course, a
> constant for the duration of the syscall. Unfortunately, GCC's alias
> analysis cannot prove that it is not modified via a pointer in the
> audit_filter_syscall() loop, and so always loads it from memory.
>
> ...
>
> Signed-off-by: Ankur Arora <ankur.a.arora(a)oracle.com>
> ---
> kernel/auditsc.c | 3 ++-
> 1 file changed, 2 insertions(+), 1 deletion(-)
This looks pretty trivial to me, but it's too late in the current -rc
cycle for this to be merged, I'll queue it up for after the upcoming
merge window closes. Thanks.
I just merged this into audit/next, thanks again!
--
paul-moore.com
12:45 p.m.
New subject: [PATCH 1/3] audit: cache ctx->major in audit_filter_syscall()
Hello,
Thanks for the detailed notes on this investigation. It really is a lot of
good information backing this up. However, there will come a day when someone
sees this "major = ctx->major" and they will send a patch to "fix"
this
unnecessary assignment. If you are sending a V2 of this set, I would suggest
adding some comment in the code that this is for a performance improvement
and to see the commit message for additional info.
Thanks,
-Steve
On Tuesday, September 27, 2022 6:59:42 PM EDT Ankur Arora wrote:
ctx->major contains the current syscall number. This is, of
course, a
constant for the duration of the syscall. Unfortunately, GCC's alias
analysis cannot prove that it is not modified via a pointer in the
audit_filter_syscall() loop, and so always loads it from memory.
In and of itself the load isn't very expensive (ops dependent on the
ctx->major load are only used to determine the direction of control flow
and have short dependence chains and, in any case the related branches
get predicted perfectly in the fastpath) but still cache ctx->major
in a local for two reasons:
* ctx->major is in the first cacheline of struct audit_context and has
similar alignment as audit_entry::list audit_entry. For cases
with a lot of audit rules, doing this reduces one source of contention
from a potentially busy cache-set.
* audit_in_mask() (called in the hot loop in audit_filter_syscall())
does cast manipulation and error checking on ctx->major:
audit_in_mask(const struct audit_krule *rule, unsigned long val):
if (val > 0xffffffff)
return false;
word = AUDIT_WORD(val);
if (word >= AUDIT_BITMASK_SIZE)
return false;
bit = AUDIT_BIT(val);
return rule->mask[word] & bit;
The clauses related to the rule need to be evaluated in the loop, but
the rest is unnecessarily re-evaluated for every loop iteration.
(Note, however, that most of these are cheap ALU ops and the branches
are perfectly predicted. However, see discussion on cycles
improvement below for more on why it is still worth hoisting.)
On a Skylakex system change in getpid() latency (aggregated over
12 boot cycles):
Min Mean Median Max pstdev
(ns) (ns) (ns) (ns)
- 201.30 216.14 216.22 228.46 (+- 1.45%)
+ 196.63 207.86 206.60 230.98 (+- 3.92%)
Performance counter stats for 'bin/getpid' (3 runs) go from:
cycles 836.89 ( +- .80% )
instructions 2000.19 ( +- .03% )
IPC 2.39 ( +- .83% )
branches 430.14 ( +- .03% )
branch-misses 1.48 ( +- 3.37% )
L1-dcache-loads 471.11 ( +- .05% )
L1-dcache-load-misses 7.62 ( +- 46.98% )
to:
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% )
(Both aggregated over 12 boot cycles.)
instructions: we reduce around 8 instructions/iteration because some of
the computation is now hoisted out of the loop (branch count does not
change because GCC, for reasons unclear, only hoists the computations
while keeping the basic-blocks.)
cycles: improve by about 5% (in aggregate and looking at individual run
numbers.) This is likely because we now waste fewer pipeline resources
on unnecessary instructions which allows the control flow to
speculatively execute further ahead shortening the execution of the loop
a little. The final gating factor on the performance of this loop
remains the long dependence chain due to the linked-list load.
Signed-off-by: Ankur Arora <ankur.a.arora(a)oracle.com>
---
kernel/auditsc.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/kernel/auditsc.c b/kernel/auditsc.c
index 79a5da1bc5bb..533b087c3c02 100644
--- a/kernel/auditsc.c
+++ b/kernel/auditsc.c
@@ -843,13 +843,14 @@ static void audit_filter_syscall(struct task_struct
*tsk, {
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, ctx->major) &&
+ if (audit_in_mask(&e->rule, major) &&
audit_filter_rules(tsk, &e->rule, ctx, NULL,
&state, false)) {
rcu_read_unlock();
1:22 p.m.
New subject: [PATCH 1/3] audit: cache ctx->major in audit_filter_syscall()
On Fri, Sep 30, 2022 at 1:45 PM Steve Grubb <sgrubb(a)redhat.com> wrote:
Hello,
Thanks for the detailed notes on this investigation. It really is a lot of
good information backing this up. However, there will come a day when someone
sees this "major = ctx->major" and they will send a patch to "fix"
this
unnecessary assignment. If you are sending a V2 of this set, I would suggest
adding some comment in the code that this is for a performance improvement
and to see the commit message for additional info.
Please do not send a v2 with the changes Steve is suggesting, I think
this patch is fine as-is.
--
paul-moore.com
7:55 p.m.
New subject: [PATCH 1/3] audit: cache ctx->major in audit_filter_syscall()
Steve Grubb <sgrubb(a)redhat.com> writes:
Hello,
Thanks for the detailed notes on this investigation. It really is a lot of
good information backing this up. However, there will come a day when someone
sees this "major = ctx->major" and they will send a patch to "fix"
this
unnecessary assignment. If you are sending a V2 of this set, I would suggest
adding some comment in the code that this is for a performance improvement
and to see the commit message for additional info.
Thanks for the comment. Just sent out v2 of the last patch which
addresses this tangentially -- it adds a separate function parameter
for ctx->major/uring_op, so this shouldn't be an issue anymore.
Thanks
Ankur
Thanks,
-Steve
On Tuesday, September 27, 2022 6:59:42 PM EDT Ankur Arora wrote:
> ctx->major contains the current syscall number. This is, of course, a
> constant for the duration of the syscall. Unfortunately, GCC's alias
> analysis cannot prove that it is not modified via a pointer in the
> audit_filter_syscall() loop, and so always loads it from memory.
>
> In and of itself the load isn't very expensive (ops dependent on the
> ctx->major load are only used to determine the direction of control flow
> and have short dependence chains and, in any case the related branches
> get predicted perfectly in the fastpath) but still cache ctx->major
> in a local for two reasons:
>
> * ctx->major is in the first cacheline of struct audit_context and has
> similar alignment as audit_entry::list audit_entry. For cases
> with a lot of audit rules, doing this reduces one source of contention
> from a potentially busy cache-set.
>
> * audit_in_mask() (called in the hot loop in audit_filter_syscall())
> does cast manipulation and error checking on ctx->major:
>
> audit_in_mask(const struct audit_krule *rule, unsigned long val):
> if (val > 0xffffffff)
> return false;
>
> word = AUDIT_WORD(val);
> if (word >= AUDIT_BITMASK_SIZE)
> return false;
>
> bit = AUDIT_BIT(val);
>
> return rule->mask[word] & bit;
>
> The clauses related to the rule need to be evaluated in the loop, but
> the rest is unnecessarily re-evaluated for every loop iteration.
> (Note, however, that most of these are cheap ALU ops and the branches
> are perfectly predicted. However, see discussion on cycles
> improvement below for more on why it is still worth hoisting.)
>
> On a Skylakex system change in getpid() latency (aggregated over
> 12 boot cycles):
>
> Min Mean Median Max pstdev
> (ns) (ns) (ns) (ns)
>
> - 201.30 216.14 216.22 228.46 (+- 1.45%)
> + 196.63 207.86 206.60 230.98 (+- 3.92%)
>
> Performance counter stats for 'bin/getpid' (3 runs) go from:
> cycles 836.89 ( +- .80% )
> instructions 2000.19 ( +- .03% )
> IPC 2.39 ( +- .83% )
> branches 430.14 ( +- .03% )
> branch-misses 1.48 ( +- 3.37% )
> L1-dcache-loads 471.11 ( +- .05% )
> L1-dcache-load-misses 7.62 ( +- 46.98% )
>
> to:
> 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% )
>
> (Both aggregated over 12 boot cycles.)
>
> instructions: we reduce around 8 instructions/iteration because some of
> the computation is now hoisted out of the loop (branch count does not
> change because GCC, for reasons unclear, only hoists the computations
> while keeping the basic-blocks.)
>
> cycles: improve by about 5% (in aggregate and looking at individual run
> numbers.) This is likely because we now waste fewer pipeline resources
> on unnecessary instructions which allows the control flow to
> speculatively execute further ahead shortening the execution of the loop
> a little. The final gating factor on the performance of this loop
> remains the long dependence chain due to the linked-list load.
>
> Signed-off-by: Ankur Arora <ankur.a.arora(a)oracle.com>
> ---
> kernel/auditsc.c | 3 ++-
> 1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/kernel/auditsc.c b/kernel/auditsc.c
> index 79a5da1bc5bb..533b087c3c02 100644
> --- a/kernel/auditsc.c
> +++ b/kernel/auditsc.c
> @@ -843,13 +843,14 @@ static void audit_filter_syscall(struct task_struct
> *tsk, {
> 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, ctx->major) &&
> + if (audit_in_mask(&e->rule, major) &&
> audit_filter_rules(tsk, &e->rule, ctx, NULL,
> &state, false)) {
> rcu_read_unlock();
--
ankur
5:59 p.m.
New subject: [PATCH 2/3] audit: annotate branch direction for audit_in_mask()
With sane audit rules, audit logging would only be triggered
infrequently. Keeping this in mind, annotate audit_in_mask() as
unlikely() to allow the compiler to pessimize the call to
audit_filter_rules().
This allows GCC to invert the branch direction for the audit_filter_rules()
basic block in this loop:
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)) {
...
such that it executes the common case in a straight line fashion.
On a Skylakex system change in 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%)
+ 173.11 182.51 179.65 202.09 (+- 4.34%)
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 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% )
(Both aggregated over 12 boot cycles.)
cycles: performance improves on average by ~100 cycles/call. IPC
improves commensurately. Two reasons for this improvement:
* one fewer branch mispred: no obvious reason for this
branch-miss reduction. There is no significant change in
basic-block structure (apart from the branch inversion.)
* the direction of the branch for the call is now inverted, so it
chooses the not-taken direction more often. The issue-latency
for not-taken branches is often cheaper.
Signed-off-by: Ankur Arora <ankur.a.arora(a)oracle.com>
---
kernel/auditsc.c | 15 ++++++++-------
1 file changed, 8 insertions(+), 7 deletions(-)
diff --git a/kernel/auditsc.c b/kernel/auditsc.c
index 533b087c3c02..bf26f47b5226 100644
--- a/kernel/auditsc.c
+++ b/kernel/auditsc.c
@@ -789,7 +789,7 @@ static enum audit_state audit_filter_task(struct task_struct *tsk,
char **key)
return AUDIT_STATE_BUILD;
}
-static int audit_in_mask(const struct audit_krule *rule, unsigned long val)
+static bool audit_in_mask(const struct audit_krule *rule, unsigned long val)
{
int word, bit;
@@ -850,12 +850,13 @@ static void audit_filter_syscall(struct task_struct *tsk,
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;
+ 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();
--
2.31.1
5:26 p.m.
New subject: [PATCH 2/3] audit: annotate branch direction for audit_in_mask()
On Tue, Sep 27, 2022 at 7:00 PM Ankur Arora <ankur.a.arora(a)oracle.com> wrote:
With sane audit rules, audit logging would only be triggered
infrequently. Keeping this in mind, annotate audit_in_mask() as
unlikely() to allow the compiler to pessimize the call to
audit_filter_rules().
This allows GCC to invert the branch direction for the audit_filter_rules()
basic block in this loop:
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)) {
...
such that it executes the common case in a straight line fashion.
On a Skylakex system change in 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%)
+ 173.11 182.51 179.65 202.09 (+- 4.34%)
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 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% )
(Both aggregated over 12 boot cycles.)
cycles: performance improves on average by ~100 cycles/call. IPC
improves commensurately. Two reasons for this improvement:
* one fewer branch mispred: no obvious reason for this
branch-miss reduction. There is no significant change in
basic-block structure (apart from the branch inversion.)
* the direction of the branch for the call is now inverted, so it
chooses the not-taken direction more often. The issue-latency
for not-taken branches is often cheaper.
Signed-off-by: Ankur Arora <ankur.a.arora(a)oracle.com>
---
kernel/auditsc.c | 15 ++++++++-------
1 file changed, 8 insertions(+), 7 deletions(-)
I generally dislike merging likely()/unlikely() additions to code
paths that can have varying levels of performance depending on runtime
configuration. While I appreciate the work you are doing to improve
audit performance, I don't think this is something I want to merge,
I'm sorry.
diff --git a/kernel/auditsc.c b/kernel/auditsc.c
index 533b087c3c02..bf26f47b5226 100644
--- a/kernel/auditsc.c
+++ b/kernel/auditsc.c
@@ -789,7 +789,7 @@ static enum audit_state audit_filter_task(struct task_struct *tsk,
char **key)
return AUDIT_STATE_BUILD;
}
-static int audit_in_mask(const struct audit_krule *rule, unsigned long val)
+static bool audit_in_mask(const struct audit_krule *rule, unsigned long val)
{
int word, bit;
@@ -850,12 +850,13 @@ static void audit_filter_syscall(struct task_struct *tsk,
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;
+ 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();
--
2.31.1
--
paul-moore.com
3:19 p.m.
New subject: [PATCH 2/3] audit: annotate branch direction for audit_in_mask()
Paul Moore <paul(a)paul-moore.com> writes:
On Tue, Sep 27, 2022 at 7:00 PM Ankur Arora
<ankur.a.arora(a)oracle.com> wrote:
>
> With sane audit rules, audit logging would only be triggered
> infrequently. Keeping this in mind, annotate audit_in_mask() as
> unlikely() to allow the compiler to pessimize the call to
> audit_filter_rules().
>
> This allows GCC to invert the branch direction for the audit_filter_rules()
> basic block in this loop:
>
> 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)) {
> ...
>
> such that it executes the common case in a straight line fashion.
>
> On a Skylakex system change in 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%)
> + 173.11 182.51 179.65 202.09 (+- 4.34%)
>
> 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 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% )
>
> (Both aggregated over 12 boot cycles.)
>
> cycles: performance improves on average by ~100 cycles/call. IPC
> improves commensurately. Two reasons for this improvement:
>
> * one fewer branch mispred: no obvious reason for this
> branch-miss reduction. There is no significant change in
> basic-block structure (apart from the branch inversion.)
>
> * the direction of the branch for the call is now inverted, so it
> chooses the not-taken direction more often. The issue-latency
> for not-taken branches is often cheaper.
>
> Signed-off-by: Ankur Arora <ankur.a.arora(a)oracle.com>
> ---
> kernel/auditsc.c | 15 ++++++++-------
> 1 file changed, 8 insertions(+), 7 deletions(-)
I generally dislike merging likely()/unlikely() additions to code
paths that can have varying levels of performance depending on runtime
configuration.
I think that's fair, and in this particular case the benchmark is quite
contrived.
But, just to elaborate a bit more on why that unlikely() clause made
sense to me: it seems to me that audit typically would be triggered for
control syscalls and the ratio between control and non-control ones
would be fairly lopsided.
Let me see if I can rewrite the conditional in a different way to get a
similar effect but I suspect that might be even more compiler dependent.
Also, let me run the audit-testsuite this time. Is there a good test
there that you would recommend that might serve as a more representative
workload?
Thanks
Ankur
While I appreciate the work you are doing to improve
audit performance, I don't think this is something I want to merge,
I'm sorry.
> diff --git a/kernel/auditsc.c b/kernel/auditsc.c
> index 533b087c3c02..bf26f47b5226 100644
> --- a/kernel/auditsc.c
> +++ b/kernel/auditsc.c
> @@ -789,7 +789,7 @@ static enum audit_state audit_filter_task(struct task_struct
*tsk, char **key)
> return AUDIT_STATE_BUILD;
> }
>
> -static int audit_in_mask(const struct audit_krule *rule, unsigned long val)
> +static bool audit_in_mask(const struct audit_krule *rule, unsigned long val)
> {
> int word, bit;
>
> @@ -850,12 +850,13 @@ static void audit_filter_syscall(struct task_struct *tsk,
>
> 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;
> + 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();
> --
> 2.31.1
--
ankur
1:48 p.m.
New subject: [PATCH 2/3] audit: annotate branch direction for audit_in_mask()
On Thu, Sep 29, 2022 at 4:20 PM Ankur Arora <ankur.a.arora(a)oracle.com> wrote:
Paul Moore <paul(a)paul-moore.com> writes:
> I generally dislike merging likely()/unlikely() additions to code
> paths that can have varying levels of performance depending on runtime
> configuration.
I think that's fair, and in this particular case the benchmark is quite
contrived.
But, just to elaborate a bit more on why that unlikely() clause made
sense to me: it seems to me that audit typically would be triggered for
control syscalls and the ratio between control and non-control ones
would be fairly lopsided.
I understand, and there is definitely some precedence in the audit
code for using likely()/unlikely() in a manner similar as you
described, but I'll refer to my previous comments - it's not something
I like. As a general rule, aside from the unlikely() calls in the
audit wrappers present in include/linux/audit.h I would suggest not
adding any new likely()/unlikely() calls.
Let me see if I can rewrite the conditional in a different way to get
a
similar effect but I suspect that might be even more compiler dependent.
I am okay with ordering conditionals to make the common case the
short-circuit case.
Also, let me run the audit-testsuite this time. Is there a good test
there that you would recommend that might serve as a more representative
workload?
Probably not. The audit-testsuite is intended to be a quick, easy to
run regression test that can be used by developers to help reduce
audit breakage. It is not representative of any particular workload,
and is definitely not comprehensive (it is woefully lacking in several
areas unfortunately).
--
paul-moore.com
7:57 p.m.
New subject: [PATCH 2/3] audit: annotate branch direction for audit_in_mask()
Paul Moore <paul(a)paul-moore.com> writes:
On Thu, Sep 29, 2022 at 4:20 PM Ankur Arora
<ankur.a.arora(a)oracle.com> wrote:
> Paul Moore <paul(a)paul-moore.com> writes:
> > I generally dislike merging likely()/unlikely() additions to code
> > paths that can have varying levels of performance depending on runtime
> > configuration.
>
> I think that's fair, and in this particular case the benchmark is quite
> contrived.
>
> But, just to elaborate a bit more on why that unlikely() clause made
> sense to me: it seems to me that audit typically would be triggered for
> control syscalls and the ratio between control and non-control ones
> would be fairly lopsided.
I understand, and there is definitely some precedence in the audit
code for using likely()/unlikely() in a manner similar as you
described, but I'll refer to my previous comments - it's not something
I like. As a general rule, aside from the unlikely() calls in the
audit wrappers present in include/linux/audit.h I would suggest not
adding any new likely()/unlikely() calls.
> Let me see if I can rewrite the conditional in a different way to get a
> similar effect but I suspect that might be even more compiler dependent.
I am okay with ordering conditionals to make the common case the
short-circuit case.
So I played around with a bunch of different combinations of the
conditionals but nothing really improved the code all that much.
Just sent out v2 dropping the unlikely() clause.
Thanks
Ankur
> Also, let me run the audit-testsuite this time. Is there a good test
> there that you would recommend that might serve as a more representative
> workload?
Probably not. The audit-testsuite is intended to be a quick, easy to
run regression test that can be used by developers to help reduce
audit breakage. It is not representative of any particular workload,
and is definitely not comprehensive (it is woefully lacking in several
areas unfortunately).
--
ankur
5:59 p.m.
New 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 <ankur.a.arora(a)oracle.com>
---
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
5:54 p.m.
New subject: [PATCH 3/3] audit: unify audit_filter_{uring(), inode_name(), syscall()}
On Tue, Sep 27, 2022 at 7:00 PM Ankur Arora <ankur.a.arora(a)oracle.com> 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 <ankur.a.arora(a)oracle.com>
---
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
3:23 p.m.
New subject: [PATCH 3/3] audit: unify audit_filter_{uring(),inode_name(),syscall()}
Paul Moore <paul(a)paul-moore.com> writes:
On Tue, Sep 27, 2022 at 7:00 PM Ankur Arora
<ankur.a.arora(a)oracle.com> 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 <ankur.a.arora(a)oracle.com>
> ---
> 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;
> +}
> +
7:49 p.m.
New 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 <ankur.a.arora(a)oracle.com>
---
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
6:11 p.m.
New subject: [PATCH v2] audit: unify audit_filter_{uring(), inode_name(), syscall()}
On Thu, Oct 6, 2022 at 8:49 PM Ankur Arora <ankur.a.arora(a)oracle.com> 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 <ankur.a.arora(a)oracle.com>
---
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
11:53 a.m.
New subject: [PATCH v2] audit: unify audit_filter_{uring(),inode_name(),syscall()}
Paul Moore <paul(a)paul-moore.com> writes:
On Thu, Oct 6, 2022 at 8:49 PM Ankur Arora
<ankur.a.arora(a)oracle.com> 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 <ankur.a.arora(a)oracle.com>
> ---
> 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
1:26 p.m.
New subject: [PATCH v2] audit: unify audit_filter_{uring(), inode_name(), syscall()}
On Thu, Oct 13, 2022 at 7:11 PM Paul Moore <paul(a)paul-moore.com> wrote:
On Thu, Oct 6, 2022 at 8:49 PM Ankur Arora
<ankur.a.arora(a)oracle.com> 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 <ankur.a.arora(a)oracle.com>
> ---
> 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
796
days inactive
816
days old
linux-audit@lists.linux-audit.osci.io
19 comments
3 participants
participants (3)
-
Ankur Arora -
Paul Moore -
Steve Grubb