| Message ID | 20210106132622.1122033-6-omosnace@redhat.com (mailing list archive) |
|---|---|
| State | Accepted |
| Delegated to: | Paul Moore |
| Headers | show |
| Series | Clean up SELinux global variables | expand |
On Wed, Jan 6, 2021 at 8:26 AM Ondrej Mosnacek <omosnace@redhat.com> wrote: > > This is motivated by a perfomance regression of selinux_xfrm_enabled() > that happened on a RHEL kernel due to false sharing between > selinux_xfrm_refcount and (the late) selinux_ss.policy_rwlock (i.e. the > .bss section memory layout changed such that they happened to share the > same cacheline). Since the policy rwlock's memory region was modified > upon each read-side critical section, the readers of > selinux_xfrm_refcount had frequent cache misses, eventually leading to a > significant performance degradation under a TCP SYN flood on a system > with many cores (32 in this case, but it's detectable on less cores as > well). > > While upstream has since switched to RCU locking, so the same can no > longer happen here, selinux_xfrm_refcount could still share a cacheline > with another frequently written region, thus marking it __read_mostly > still makes sense. __read_mostly helps, because it will put the symbol > in a separate section along with other read-mostly variables, so there > should never be a clash with frequently written data. > > Since selinux_xfrm_refcount is modified only in case of an explicit > action, it should be safe to do this (i.e. it shouldn't disrupt other > read-mostly variables too much). > > Signed-off-by: Ondrej Mosnacek <omosnace@redhat.com> > --- > security/selinux/xfrm.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) Even on a system that is using labeled IPsec, which is likely very rare, the refcount should really only be changing when the SPD or SAD changes which should be measured in minutes or seconds on a loaded system. If the __read_mostly tag proves to be a problem we can always look into other solutions. Merged into selinux/next, thanks.
diff --git a/security/selinux/xfrm.c b/security/selinux/xfrm.c index c367d36965d4f..634f3db24da67 100644 --- a/security/selinux/xfrm.c +++ b/security/selinux/xfrm.c @@ -47,7 +47,7 @@ #include "xfrm.h" /* Labeled XFRM instance counter */ -atomic_t selinux_xfrm_refcount = ATOMIC_INIT(0); +atomic_t selinux_xfrm_refcount __read_mostly = ATOMIC_INIT(0); /* * Returns true if the context is an LSM/SELinux context.
This is motivated by a perfomance regression of selinux_xfrm_enabled() that happened on a RHEL kernel due to false sharing between selinux_xfrm_refcount and (the late) selinux_ss.policy_rwlock (i.e. the .bss section memory layout changed such that they happened to share the same cacheline). Since the policy rwlock's memory region was modified upon each read-side critical section, the readers of selinux_xfrm_refcount had frequent cache misses, eventually leading to a significant performance degradation under a TCP SYN flood on a system with many cores (32 in this case, but it's detectable on less cores as well). While upstream has since switched to RCU locking, so the same can no longer happen here, selinux_xfrm_refcount could still share a cacheline with another frequently written region, thus marking it __read_mostly still makes sense. __read_mostly helps, because it will put the symbol in a separate section along with other read-mostly variables, so there should never be a clash with frequently written data. Since selinux_xfrm_refcount is modified only in case of an explicit action, it should be safe to do this (i.e. it shouldn't disrupt other read-mostly variables too much). Signed-off-by: Ondrej Mosnacek <omosnace@redhat.com> --- security/selinux/xfrm.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)