On 9/25/23 23:38, Mateusz Guzik wrote:

On Mon, Sep 25, 2023 at 11:21:26PM -0700, John Johansen wrote:

quoted

On 9/25/23 16:49, Vinicius Costa Gomes wrote:

quoted

Hi Mateusz,

Mateusz Guzik [off-list ref] writes:

quoted

I'm sanity-checking perf in various microbenchmarks and I found
apparmor to be the main bottleneck in some of them.

For example: will-it-scale open1_processes -t 16, top of the profile:
    20.17%  [kernel]                   [k] apparmor_file_alloc_security
    20.08%  [kernel]                   [k] apparmor_file_open
    20.05%  [kernel]                   [k] apparmor_file_free_security
    18.39%  [kernel]                   [k] apparmor_current_getsecid_subj
[snip]

This serializes on refing/unrefing apparmor objs, sounds like a great
candidate for per-cpu refcounting instead (I'm assuming they are
expected to be long-lived).

I would hack it up myself, but I failed to find a clear spot to switch
back from per-cpu to centalized operation and don't want to put
serious effort into it.

Can you sort this out?

I will add looking into it on the todo list. Its going to have to come
after some other major cleanups land, and I am not sure we can make
the semantic work well for some of these. For other we might get away
with switching to a critical section like Vinicius's patch has done
for apparmor_current_getsecid_subj.

Is there an eta?

sorry no

I looked at dodging ref round trips myself, but then found that ref
manipulation in apparmor_file_alloc_security and the free counterpart
cannot be avoided. Thus per-cpu refs instead.

right for file_aloc/free, I don't see a way around keeping a ref count.

Perhaps making the label as stale would be a good enough switching
point? Is it *guaranteed* to get labelled as stale before it gets freed?

no. the stale flag only indicates the label has been replaced, and we
make no guarentees as to when it will get set/be in use beyond so
point after it happens.

btw, __aa_proxy_redirect open-codes setting the flag.

yes, I am aware.

quoted

quoted

I was looking at this same workload, and proposed a patch[1] some time
ago, see if it helps:

https://lists.ubuntu.com/archives/apparmor/2023-August/012914.html

But my idea was different, in many cases, we are looking at the label
associated with the current task, and there's no need to take the
refcount.

yes, and thanks for that.