* [PATCH v1] audit: merge loops in __audit_inode_child() @ 2025-09-04 16:59 Ricardo Robaina 2025-10-03 15:49 ` Ricardo Robaina 2025-10-13 19:17 ` Paul Moore 0 siblings, 2 replies; 6+ messages in thread From: Ricardo Robaina @ 2025-09-04 16:59 UTC (permalink / raw) To: audit, linux-kernel; +Cc: paul, eparis, Ricardo Robaina Whenever there's audit context, __audit_inode_child() gets called numerous times, which can lead to high latency in scenarios that create too many sysfs/debugfs entries at once, for instance, upon device_add_disk() invocation. # uname -r 6.17.0-rc3+ # auditctl -a always,exit -F path=/tmp -k foo # time insmod loop max_loop=1000 real 0m42.753s user 0m0.000s sys 0m42.494s # perf record -a insmod loop max_loop=1000 # perf report --stdio |grep __audit_inode_child 37.95% insmod [kernel.kallsyms] [k] __audit_inode_child __audit_inode_child() searches for both the parent and the child in two different loops that iterate over the same list. This process can be optimized by merging these into a single loop, without changing the function behavior or affecting the code's readability. This patch merges the two loops that walk through the list context->names_list into a single loop. This optimization resulted in around 54% performance enhancement for the benchmark. # uname -r 6.17.0-rc3+-enhanced # auditctl -a always,exit -F path=/tmp -k foo # time insmod loop max_loop=1000 real 0m19.388s user 0m0.000s sys 0m19.149s Signed-off-by: Ricardo Robaina <rrobaina@redhat.com> --- kernel/auditsc.c | 40 ++++++++++++++++++---------------------- 1 file changed, 18 insertions(+), 22 deletions(-) diff --git a/kernel/auditsc.c b/kernel/auditsc.c index eb98cd6fe91f..7abfb68687fb 100644 --- a/kernel/auditsc.c +++ b/kernel/auditsc.c @@ -2437,44 +2437,40 @@ void __audit_inode_child(struct inode *parent, if (inode) handle_one(inode); - /* look for a parent entry first */ list_for_each_entry(n, &context->names_list, list) { - if (!n->name || - (n->type != AUDIT_TYPE_PARENT && - n->type != AUDIT_TYPE_UNKNOWN)) + /* can only match entries that have a name */ + if (!n->name) continue; - if (n->ino == parent->i_ino && n->dev == parent->i_sb->s_dev && - !audit_compare_dname_path(dname, - n->name->name, n->name_len)) { + /* look for a parent entry first */ + if (!found_parent && + (n->type == AUDIT_TYPE_PARENT || n->type == AUDIT_TYPE_UNKNOWN) && + (n->ino == parent->i_ino && n->dev == parent->i_sb->s_dev && + !audit_compare_dname_path(dname, n->name->name, n->name_len))) { if (n->type == AUDIT_TYPE_UNKNOWN) n->type = AUDIT_TYPE_PARENT; found_parent = n; - break; } - } - cond_resched(); - - /* is there a matching child entry? */ - list_for_each_entry(n, &context->names_list, list) { - /* can only match entries that have a name */ - if (!n->name || - (n->type != type && n->type != AUDIT_TYPE_UNKNOWN)) - continue; - - if (!strcmp(dname->name, n->name->name) || - !audit_compare_dname_path(dname, n->name->name, + /* is there a matching child entry? */ + if (!found_child && + (n->type == type || n->type == AUDIT_TYPE_UNKNOWN) && + (!strcmp(dname->name, n->name->name) || + !audit_compare_dname_path(dname, n->name->name, found_parent ? found_parent->name_len : - AUDIT_NAME_FULL)) { + AUDIT_NAME_FULL))) { if (n->type == AUDIT_TYPE_UNKNOWN) n->type = type; found_child = n; - break; } + + if (found_parent && found_child) + break; } + cond_resched(); + if (!found_parent) { /* create a new, "anonymous" parent record */ n = audit_alloc_name(context, AUDIT_TYPE_PARENT); -- 2.51.0 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH v1] audit: merge loops in __audit_inode_child() 2025-09-04 16:59 [PATCH v1] audit: merge loops in __audit_inode_child() Ricardo Robaina @ 2025-10-03 15:49 ` Ricardo Robaina 2025-10-06 3:21 ` Paul Moore 2025-10-13 19:17 ` Paul Moore 1 sibling, 1 reply; 6+ messages in thread From: Ricardo Robaina @ 2025-10-03 15:49 UTC (permalink / raw) To: audit, linux-kernel, paul; +Cc: eparis On Thu, Sep 4, 2025 at 1:59 PM Ricardo Robaina <rrobaina@redhat.com> wrote: > > Whenever there's audit context, __audit_inode_child() gets called > numerous times, which can lead to high latency in scenarios that > create too many sysfs/debugfs entries at once, for instance, upon > device_add_disk() invocation. > > # uname -r > 6.17.0-rc3+ > > # auditctl -a always,exit -F path=/tmp -k foo > # time insmod loop max_loop=1000 > real 0m42.753s > user 0m0.000s > sys 0m42.494s > > # perf record -a insmod loop max_loop=1000 > # perf report --stdio |grep __audit_inode_child > 37.95% insmod [kernel.kallsyms] [k] __audit_inode_child > > __audit_inode_child() searches for both the parent and the child > in two different loops that iterate over the same list. This > process can be optimized by merging these into a single loop, > without changing the function behavior or affecting the code's > readability. > > This patch merges the two loops that walk through the list > context->names_list into a single loop. This optimization resulted > in around 54% performance enhancement for the benchmark. > > # uname -r > 6.17.0-rc3+-enhanced > > # auditctl -a always,exit -F path=/tmp -k foo > # time insmod loop max_loop=1000 > real 0m19.388s > user 0m0.000s > sys 0m19.149s > > Signed-off-by: Ricardo Robaina <rrobaina@redhat.com> > --- > kernel/auditsc.c | 40 ++++++++++++++++++---------------------- > 1 file changed, 18 insertions(+), 22 deletions(-) > > diff --git a/kernel/auditsc.c b/kernel/auditsc.c > index eb98cd6fe91f..7abfb68687fb 100644 > --- a/kernel/auditsc.c > +++ b/kernel/auditsc.c > @@ -2437,44 +2437,40 @@ void __audit_inode_child(struct inode *parent, > if (inode) > handle_one(inode); > > - /* look for a parent entry first */ > list_for_each_entry(n, &context->names_list, list) { > - if (!n->name || > - (n->type != AUDIT_TYPE_PARENT && > - n->type != AUDIT_TYPE_UNKNOWN)) > + /* can only match entries that have a name */ > + if (!n->name) > continue; > > - if (n->ino == parent->i_ino && n->dev == parent->i_sb->s_dev && > - !audit_compare_dname_path(dname, > - n->name->name, n->name_len)) { > + /* look for a parent entry first */ > + if (!found_parent && > + (n->type == AUDIT_TYPE_PARENT || n->type == AUDIT_TYPE_UNKNOWN) && > + (n->ino == parent->i_ino && n->dev == parent->i_sb->s_dev && > + !audit_compare_dname_path(dname, n->name->name, n->name_len))) { > if (n->type == AUDIT_TYPE_UNKNOWN) > n->type = AUDIT_TYPE_PARENT; > found_parent = n; > - break; > } > - } > > - cond_resched(); > - > - /* is there a matching child entry? */ > - list_for_each_entry(n, &context->names_list, list) { > - /* can only match entries that have a name */ > - if (!n->name || > - (n->type != type && n->type != AUDIT_TYPE_UNKNOWN)) > - continue; > - > - if (!strcmp(dname->name, n->name->name) || > - !audit_compare_dname_path(dname, n->name->name, > + /* is there a matching child entry? */ > + if (!found_child && > + (n->type == type || n->type == AUDIT_TYPE_UNKNOWN) && > + (!strcmp(dname->name, n->name->name) || > + !audit_compare_dname_path(dname, n->name->name, > found_parent ? > found_parent->name_len : > - AUDIT_NAME_FULL)) { > + AUDIT_NAME_FULL))) { > if (n->type == AUDIT_TYPE_UNKNOWN) > n->type = type; > found_child = n; > - break; > } > + > + if (found_parent && found_child) > + break; > } > > + cond_resched(); > + > if (!found_parent) { > /* create a new, "anonymous" parent record */ > n = audit_alloc_name(context, AUDIT_TYPE_PARENT); > -- > 2.51.0 > Hi Paul, I’m curious if you have any thoughts on this one. Please disregard this email if it’s already in your to-do list. It’s not my intention to rush you in any way. ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v1] audit: merge loops in __audit_inode_child() 2025-10-03 15:49 ` Ricardo Robaina @ 2025-10-06 3:21 ` Paul Moore 2025-10-06 9:51 ` Ricardo Robaina 0 siblings, 1 reply; 6+ messages in thread From: Paul Moore @ 2025-10-06 3:21 UTC (permalink / raw) To: Ricardo Robaina; +Cc: audit, linux-kernel, eparis On Fri, Oct 3, 2025 at 11:50 AM Ricardo Robaina <rrobaina@redhat.com> wrote: > > Hi Paul, > > I’m curious if you have any thoughts on this one. > Please disregard this email if it’s already in your to-do list. It’s > not my intention to rush you in any way. Hi Ricardo, Your patch was posted right before v6.17-rc5, and with only approximately a week before the general feature cutoff for the v6.18 merge window, I didn't have a chance to properly review it before the cutoff. Rest assured, it is in the review queue, and with a little bit of luck I should be able to start working through that this week. As a FYI, the doc below describes some of the basic process for the audit tree which may be helpful. If you lose the link, it is linked off the audit section in MAINTAINERS. https://github.com/linux-audit/audit-kernel/blob/main/README.md -- paul-moore.com ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v1] audit: merge loops in __audit_inode_child() 2025-10-06 3:21 ` Paul Moore @ 2025-10-06 9:51 ` Ricardo Robaina 0 siblings, 0 replies; 6+ messages in thread From: Ricardo Robaina @ 2025-10-06 9:51 UTC (permalink / raw) To: Paul Moore; +Cc: audit, linux-kernel, eparis Thanks, Paul! On Mon, Oct 6, 2025 at 12:21 AM Paul Moore <paul@paul-moore.com> wrote: > > On Fri, Oct 3, 2025 at 11:50 AM Ricardo Robaina <rrobaina@redhat.com> wrote: > > > > Hi Paul, > > > > I’m curious if you have any thoughts on this one. > > Please disregard this email if it’s already in your to-do list. It’s > > not my intention to rush you in any way. > > Hi Ricardo, > > Your patch was posted right before v6.17-rc5, and with only > approximately a week before the general feature cutoff for the v6.18 > merge window, I didn't have a chance to properly review it before the > cutoff. Rest assured, it is in the review queue, and with a little > bit of luck I should be able to start working through that this week. > > As a FYI, the doc below describes some of the basic process for the > audit tree which may be helpful. If you lose the link, it is linked > off the audit section in MAINTAINERS. > > https://github.com/linux-audit/audit-kernel/blob/main/README.md > > -- > paul-moore.com > ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v1] audit: merge loops in __audit_inode_child() 2025-09-04 16:59 [PATCH v1] audit: merge loops in __audit_inode_child() Ricardo Robaina 2025-10-03 15:49 ` Ricardo Robaina @ 2025-10-13 19:17 ` Paul Moore 2025-10-14 12:42 ` Ricardo Robaina 1 sibling, 1 reply; 6+ messages in thread From: Paul Moore @ 2025-10-13 19:17 UTC (permalink / raw) To: Ricardo Robaina, audit, linux-kernel; +Cc: eparis, Ricardo Robaina On Sep 4, 2025 Ricardo Robaina <rrobaina@redhat.com> wrote: > > Whenever there's audit context, __audit_inode_child() gets called > numerous times, which can lead to high latency in scenarios that > create too many sysfs/debugfs entries at once, for instance, upon > device_add_disk() invocation. > > # uname -r > 6.17.0-rc3+ > > # auditctl -a always,exit -F path=/tmp -k foo > # time insmod loop max_loop=1000 > real 0m42.753s > user 0m0.000s > sys 0m42.494s > > # perf record -a insmod loop max_loop=1000 > # perf report --stdio |grep __audit_inode_child > 37.95% insmod [kernel.kallsyms] [k] __audit_inode_child > > __audit_inode_child() searches for both the parent and the child > in two different loops that iterate over the same list. This > process can be optimized by merging these into a single loop, > without changing the function behavior or affecting the code's > readability. > > This patch merges the two loops that walk through the list > context->names_list into a single loop. This optimization resulted > in around 54% performance enhancement for the benchmark. > > # uname -r > 6.17.0-rc3+-enhanced > > # auditctl -a always,exit -F path=/tmp -k foo > # time insmod loop max_loop=1000 > real 0m19.388s > user 0m0.000s > sys 0m19.149s > > Signed-off-by: Ricardo Robaina <rrobaina@redhat.com> > --- > kernel/auditsc.c | 40 ++++++++++++++++++---------------------- > 1 file changed, 18 insertions(+), 22 deletions(-) Thanks Ricardo, that's a nice improvement! I saw a few additional things that could help simplify the code and possibly speed things up a bit more, see my comments below. > diff --git a/kernel/auditsc.c b/kernel/auditsc.c > index eb98cd6fe91f..7abfb68687fb 100644 > --- a/kernel/auditsc.c > +++ b/kernel/auditsc.c > @@ -2437,44 +2437,40 @@ void __audit_inode_child(struct inode *parent, > if (inode) > handle_one(inode); > > - /* look for a parent entry first */ > list_for_each_entry(n, &context->names_list, list) { > - if (!n->name || > - (n->type != AUDIT_TYPE_PARENT && > - n->type != AUDIT_TYPE_UNKNOWN)) > + /* can only match entries that have a name */ > + if (!n->name) > continue; > > - if (n->ino == parent->i_ino && n->dev == parent->i_sb->s_dev && > - !audit_compare_dname_path(dname, > - n->name->name, n->name_len)) { > + /* look for a parent entry first */ > + if (!found_parent && > + (n->type == AUDIT_TYPE_PARENT || n->type == AUDIT_TYPE_UNKNOWN) && > + (n->ino == parent->i_ino && n->dev == parent->i_sb->s_dev && > + !audit_compare_dname_path(dname, n->name->name, n->name_len))) { > if (n->type == AUDIT_TYPE_UNKNOWN) > n->type = AUDIT_TYPE_PARENT; We probably don't need to check 'n->type' first, as we want it to always be set to AUDIT_TYPE_PARENT regardless of it's current value. > found_parent = n; We can probably 'continue' here since a match can't be both a parent and a child at the same time. Similarly, if we add move the 'if (found_parent && found_child)' check up to here we don't need to run it on every pass through the loop, just when we find a match. Taking the two comment above into account, I would imagine something like this would good: found_parent = n; if (found_child) break; continue; > - break; > } > - } > > - cond_resched(); > - > - /* is there a matching child entry? */ > - list_for_each_entry(n, &context->names_list, list) { > - /* can only match entries that have a name */ > - if (!n->name || > - (n->type != type && n->type != AUDIT_TYPE_UNKNOWN)) > - continue; > - > - if (!strcmp(dname->name, n->name->name) || > - !audit_compare_dname_path(dname, n->name->name, > + /* is there a matching child entry? */ > + if (!found_child && > + (n->type == type || n->type == AUDIT_TYPE_UNKNOWN) && > + (!strcmp(dname->name, n->name->name) || > + !audit_compare_dname_path(dname, n->name->name, > found_parent ? > found_parent->name_len : > - AUDIT_NAME_FULL)) { > + AUDIT_NAME_FULL))) { > if (n->type == AUDIT_TYPE_UNKNOWN) > n->type = type; > found_child = n; Similar to the parent case above, let's check to see if both a parent and a child have been found. We can probably skip the 'continue' here are we are at the end of the loop. found_child = n; if (found_parent) break; > - break; > } > + > + if (found_parent && found_child) > + break; > } > > + cond_resched(); The 'cond_resched()' call was located between the two loops to help avoid a soft lockup caused by running through both loops; since we are now condensing that into one loop we can probably drop the 'cond_resched()' call ... which is definitely a good thing as it was a bit of a hack, a necessary hack, but still a hack :) > if (!found_parent) { > /* create a new, "anonymous" parent record */ > n = audit_alloc_name(context, AUDIT_TYPE_PARENT); > -- > 2.51.0 -- paul-moore.com ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v1] audit: merge loops in __audit_inode_child() 2025-10-13 19:17 ` Paul Moore @ 2025-10-14 12:42 ` Ricardo Robaina 0 siblings, 0 replies; 6+ messages in thread From: Ricardo Robaina @ 2025-10-14 12:42 UTC (permalink / raw) To: Paul Moore; +Cc: audit, linux-kernel, eparis On Mon, Oct 13, 2025 at 4:17 PM Paul Moore <paul@paul-moore.com> wrote: > > On Sep 4, 2025 Ricardo Robaina <rrobaina@redhat.com> wrote: > > > > Whenever there's audit context, __audit_inode_child() gets called > > numerous times, which can lead to high latency in scenarios that > > create too many sysfs/debugfs entries at once, for instance, upon > > device_add_disk() invocation. > > > > # uname -r > > 6.17.0-rc3+ > > > > # auditctl -a always,exit -F path=/tmp -k foo > > # time insmod loop max_loop=1000 > > real 0m42.753s > > user 0m0.000s > > sys 0m42.494s > > > > # perf record -a insmod loop max_loop=1000 > > # perf report --stdio |grep __audit_inode_child > > 37.95% insmod [kernel.kallsyms] [k] __audit_inode_child > > > > __audit_inode_child() searches for both the parent and the child > > in two different loops that iterate over the same list. This > > process can be optimized by merging these into a single loop, > > without changing the function behavior or affecting the code's > > readability. > > > > This patch merges the two loops that walk through the list > > context->names_list into a single loop. This optimization resulted > > in around 54% performance enhancement for the benchmark. > > > > # uname -r > > 6.17.0-rc3+-enhanced > > > > # auditctl -a always,exit -F path=/tmp -k foo > > # time insmod loop max_loop=1000 > > real 0m19.388s > > user 0m0.000s > > sys 0m19.149s > > > > Signed-off-by: Ricardo Robaina <rrobaina@redhat.com> > > --- > > kernel/auditsc.c | 40 ++++++++++++++++++---------------------- > > 1 file changed, 18 insertions(+), 22 deletions(-) > > Thanks Ricardo, that's a nice improvement! I saw a few additional things > that could help simplify the code and possibly speed things up a bit > more, see my comments below. > > > diff --git a/kernel/auditsc.c b/kernel/auditsc.c > > index eb98cd6fe91f..7abfb68687fb 100644 > > --- a/kernel/auditsc.c > > +++ b/kernel/auditsc.c > > @@ -2437,44 +2437,40 @@ void __audit_inode_child(struct inode *parent, > > if (inode) > > handle_one(inode); > > > > - /* look for a parent entry first */ > > list_for_each_entry(n, &context->names_list, list) { > > - if (!n->name || > > - (n->type != AUDIT_TYPE_PARENT && > > - n->type != AUDIT_TYPE_UNKNOWN)) > > + /* can only match entries that have a name */ > > + if (!n->name) > > continue; > > > > - if (n->ino == parent->i_ino && n->dev == parent->i_sb->s_dev && > > - !audit_compare_dname_path(dname, > > - n->name->name, n->name_len)) { > > + /* look for a parent entry first */ > > + if (!found_parent && > > + (n->type == AUDIT_TYPE_PARENT || n->type == AUDIT_TYPE_UNKNOWN) && > > + (n->ino == parent->i_ino && n->dev == parent->i_sb->s_dev && > > + !audit_compare_dname_path(dname, n->name->name, n->name_len))) { > > if (n->type == AUDIT_TYPE_UNKNOWN) > > n->type = AUDIT_TYPE_PARENT; > > We probably don't need to check 'n->type' first, as we want it to always > be set to AUDIT_TYPE_PARENT regardless of it's current value. > > > found_parent = n; > > We can probably 'continue' here since a match can't be both a parent and > a child at the same time. > > Similarly, if we add move the 'if (found_parent && found_child)' check > up to here we don't need to run it on every pass through the loop, just > when we find a match. > > Taking the two comment above into account, I would imagine something like > this would good: > > found_parent = n; > if (found_child) > break; > continue; > > > - break; > > } > > - } > > > > - cond_resched(); > > - > > - /* is there a matching child entry? */ > > - list_for_each_entry(n, &context->names_list, list) { > > - /* can only match entries that have a name */ > > - if (!n->name || > > - (n->type != type && n->type != AUDIT_TYPE_UNKNOWN)) > > - continue; > > - > > - if (!strcmp(dname->name, n->name->name) || > > - !audit_compare_dname_path(dname, n->name->name, > > + /* is there a matching child entry? */ > > + if (!found_child && > > + (n->type == type || n->type == AUDIT_TYPE_UNKNOWN) && > > + (!strcmp(dname->name, n->name->name) || > > + !audit_compare_dname_path(dname, n->name->name, > > found_parent ? > > found_parent->name_len : > > - AUDIT_NAME_FULL)) { > > + AUDIT_NAME_FULL))) { > > if (n->type == AUDIT_TYPE_UNKNOWN) > > n->type = type; > > found_child = n; > > Similar to the parent case above, let's check to see if both a parent and > a child have been found. We can probably skip the 'continue' here are we > are at the end of the loop. > > found_child = n; > if (found_parent) > break; > > > - break; > > } > > + > > + if (found_parent && found_child) > > + break; > > } > > > > + cond_resched(); > > The 'cond_resched()' call was located between the two loops to help avoid > a soft lockup caused by running through both loops; since we are now > condensing that into one loop we can probably drop the 'cond_resched()' > call ... which is definitely a good thing as it was a bit of a hack, a > necessary hack, but still a hack :) > > > if (!found_parent) { > > /* create a new, "anonymous" parent record */ > > n = audit_alloc_name(context, AUDIT_TYPE_PARENT); > > -- > > 2.51.0 > > -- > paul-moore.com > Thank you for reviewing this one, Paul! And thanks a lot for your suggestions, I'll work on a v2 addressing those shortly. ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2025-10-14 12:42 UTC | newest] Thread overview: 6+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2025-09-04 16:59 [PATCH v1] audit: merge loops in __audit_inode_child() Ricardo Robaina 2025-10-03 15:49 ` Ricardo Robaina 2025-10-06 3:21 ` Paul Moore 2025-10-06 9:51 ` Ricardo Robaina 2025-10-13 19:17 ` Paul Moore 2025-10-14 12:42 ` Ricardo Robaina
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox