[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 at gmail.com>
>> 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.