Messages in this thread | | | Date | Fri, 17 Feb 2023 11:44:48 +0100 | From | Sebastian Andrzej Siewior <> | Subject | Re: [PATCH v3] apparmor: global buffers spin lock may get contended |
| |
On 2023-02-16 16:08:10 [-0800], John Johansen wrote: > --- a/security/apparmor/lsm.c > +++ b/security/apparmor/lsm.c > @@ -49,12 +49,19 @@ union aa_buffer { > char buffer[1]; > }; > +struct aa_local_cache { > + unsigned int contention; > + unsigned int hold; > + struct list_head head; > +};
if you stick a local_lock_t into that struct, then you could replace cache = get_cpu_ptr(&aa_local_buffers); with local_lock(&aa_local_buffers.lock); cache = this_cpu_ptr(&aa_local_buffers);
You would get the preempt_disable() based locking for the per-CPU variable (as with get_cpu_ptr()) and additionally some lockdep validation which would warn if it is used outside of task context (IRQ).
I didn't parse completely the hold/contention logic but it seems to work ;) You check "cache->count >= 2" twice but I don't see an inc/ dec of it nor is it part of aa_local_cache.
I can't parse how many items can end up on the local list if the global list is locked. My guess would be more than 2 due the ->hold parameter.
Do you have any numbers on the machine and performance it improved? It sure will be a good selling point.
Sebastian
| |