Re: [PATCH] apparmor: try to avoid refing the label in apparmor_file_open

[John Johansen]

On 6/20/24 09:41, Mateusz Guzik wrote:
> On Thu, Jun 20, 2024 at 09:26:00AM -0700, John Johansen wrote:
> > On 6/20/24 06:15, Mateusz Guzik wrote:
> > > It can be done in the common case.
> > > > A 24-way open1_processes from will-it-scale (separate file open) shows:
> > >     29.37%  [kernel]           [k] apparmor_file_open
> > >     26.84%  [kernel]           [k] apparmor_file_alloc_security
> > >     26.62%  [kernel]           [k] apparmor_file_free_security
> > >      1.32%  [kernel]           [k] clear_bhb_loop
> > >
> > > apparmor_file_open is eliminated from the profile with the patch.
> > >
> > > Throughput (ops/s):
> > > before:	6092196
> > > after:	8309726 (+36%)
> > >
> > > Signed-off-by: Mateusz Guzik <mjguzik@xxxxxxxxx>
> > can you cleanup the commit message and I will pull this in
>
> First of all thanks for a timely review.
>
> I thought that's a decent commit message though. ;)
>
> Would something like this work:
> <cm>
> apparmor: try to avoid refing the label in apparmor_file_open
>
> In the common case it can be avoided, which in turn reduces the
> performance impact apparmor on parallel open() invocations.
>
> When benchmarking on 24-core vm using will-it-scale's open1_process
> ("Separate file open"), the results are (ops/s):
> before: 6092196
> after:  8309726 (+36%)
> </cm>
>
> If this is fine I'll send a v2.
it will do, largely, I was just looking for something that explains
a little more than. "It can be done in the common case"


> If you are looking for something fundamentally different I would say it
> will be the fastest if you write your own commit message while borrowing
> the numbers and denoting all the wording is yours. I'm trying to reduce
> back and forth over email here.
>
> > > Am I missing something which makes the approach below not work to begin
> > > with?
> > no this will work in the short term. Long term there is work that will
> > break this. Both replacing unconfined and the object delegation work
> > will cause a performance regression as I am not sure we will be able
> > to conditionally get the label but that is something for those patch
> > series to work out. My biggest concern being people objecting to necessary
> > changes that regress performance, if it can't be worked out, but
> > that really isn't a reason to stop this now.
>
> hrm. I was looking at going a step further, now I'm going to have to
> poke around.