Re: [apparmor] use per-cpu refcounts for apparmor labels?

[John Johansen]

On 9/25/23 23:38, Mateusz Guzik wrote:
> On Mon, Sep 25, 2023 at 11:21:26PM -0700, John Johansen wrote:
> > On 9/25/23 16:49, Vinicius Costa Gomes wrote:
> > > Hi Mateusz,
> > >
> > > Mateusz Guzik <mjguzik@xxxxxxxxx> writes:
> > >
> > > > 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.

> > > 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.