| Message ID | 20240806133607.869394-1-mjguzik@gmail.com (mailing list archive) |
|---|---|
| State | New |
| 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; > } >
On Fri, Aug 16, 2024 at 1:53 PM Roberto Sassu <roberto.sassu@huaweicloud.com> wrote: > > 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 thanks for the review are there plans to pick this up for this merge window? > > 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; > > } > > >
On Mon, 2024-09-23 at 07:26 +0200, Mateusz Guzik wrote: > On Fri, Aug 16, 2024 at 1:53 PM Roberto Sassu > <roberto.sassu@huaweicloud.com> wrote: > > > > 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 > > thanks for the review > > are there plans to pick this up for this merge window? Welcome! Sorry, Mimi wants to do few more checks. It is more likely that we will pick this for the next version. Roberto > > > > 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; > > > } > > > > > > >
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. > > 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> Sorry for the delay. It's now queued in next-integrity. thanks, Mimi
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(-)