| Message ID | 20240806133607.869394-1-mjguzik@gmail.com (mailing list archive) |
|---|---|
| State | Handled Elsewhere |
| Headers | show |
| Series | evm: stop avoidably reading i_writecount in evm_file_release | expand |
On Tue, 2024-08-06 at 15:36 +0200, Mateusz Guzik wrote: > The EVM_NEW_FILE flag is unset if the file already existed at the time > of open and this can be checked without looking at i_writecount. Agreed. EVM_NEW_FILE is not going to be set during the open(), only before, in evm_post_path_mknod(). Looks good to me. Reviewed-by: Roberto Sassu <roberto.sassu@huawei.com> Thanks Roberto > Not accessing it reduces traffic on the cacheline during parallel open > of the same file and drop the evm_file_release routine from second place > to bottom of the profile. > > Signed-off-by: Mateusz Guzik <mjguzik@gmail.com> > --- > > The context is that I'm writing a patch which removes one lockref > get/put cycle on parallel open. An operational WIP reduces ping-pong in > that area and made do_dentry_open skyrocket along with evm_file_release, > due to i_writecount access. With the patch they go down again and > apparmor takes the rightful first place. > > The patch accounts for about 5% speed up at 20 cores running open3 from > will-it-scale on top of the above wip. (the apparmor + lockref thing > really don't scale, that's next) > > I would provide better measurements, but the wip is not ready (as the > description suggests) and I need evm out of the way for the actual > patch. > > security/integrity/evm/evm_main.c | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > > diff --git a/security/integrity/evm/evm_main.c b/security/integrity/evm/evm_main.c > index 62fe66dd53ce..309630f319e2 100644 > --- a/security/integrity/evm/evm_main.c > +++ b/security/integrity/evm/evm_main.c > @@ -1084,7 +1084,8 @@ static void evm_file_release(struct file *file) > if (!S_ISREG(inode->i_mode) || !(mode & FMODE_WRITE)) > return; > > - if (iint && atomic_read(&inode->i_writecount) == 1) > + if (iint && iint->flags & EVM_NEW_FILE && > + atomic_read(&inode->i_writecount) == 1) > iint->flags &= ~EVM_NEW_FILE; > } >
diff --git a/security/integrity/evm/evm_main.c b/security/integrity/evm/evm_main.c index 62fe66dd53ce..309630f319e2 100644 --- a/security/integrity/evm/evm_main.c +++ b/security/integrity/evm/evm_main.c @@ -1084,7 +1084,8 @@ static void evm_file_release(struct file *file) if (!S_ISREG(inode->i_mode) || !(mode & FMODE_WRITE)) return; - if (iint && atomic_read(&inode->i_writecount) == 1) + if (iint && iint->flags & EVM_NEW_FILE && + atomic_read(&inode->i_writecount) == 1) iint->flags &= ~EVM_NEW_FILE; }
The EVM_NEW_FILE flag is unset if the file already existed at the time of open and this can be checked without looking at i_writecount. Not accessing it reduces traffic on the cacheline during parallel open of the same file and drop the evm_file_release routine from second place to bottom of the profile. Signed-off-by: Mateusz Guzik <mjguzik@gmail.com> --- The context is that I'm writing a patch which removes one lockref get/put cycle on parallel open. An operational WIP reduces ping-pong in that area and made do_dentry_open skyrocket along with evm_file_release, due to i_writecount access. With the patch they go down again and apparmor takes the rightful first place. The patch accounts for about 5% speed up at 20 cores running open3 from will-it-scale on top of the above wip. (the apparmor + lockref thing really don't scale, that's next) I would provide better measurements, but the wip is not ready (as the description suggests) and I need evm out of the way for the actual patch. security/integrity/evm/evm_main.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)