On Thu, Jun 29, 2023 at 09:21:14AM +0200, Vlastimil Babka wrote: > On 6/28/23 18:44, Roman Gushchin wrote: > > On Tue, Jun 27, 2023 at 12:32:15PM -0700, David Rientjes wrote: > >> On Tue, 27 Jun 2023, Julian Pidancet wrote: > >> > >> > Make CONFIG_SLAB_MERGE_DEFAULT default to n unless CONFIG_SLUB_TINY is > >> > enabled. Benefits of slab merging is limited on systems that are not > >> > memory constrained: the overhead is negligible and evidence of its > >> > effect on cache hotness is hard to come by. > >> > > >> > >> I don't have an objection to this, I think it makes sense. > > > > +1 > > > > I believe the overhead was much larger when we had per-memcg slab caches, > > but now it should be fairly small on most systems. > > > > But I wonder if we need a new flag (SLAB_MERGE?) to explicitly force merging > > on per-slab cache basis. > > Damn, we just tried to add SLAB_NO_MERGE, that is if Linus pulls the PR, as > I've just found out that the last time he hated the idea [1] :) (but at the > same time I think the current attempt is very different in that it's not > coming via a random tree, and the comments make it clear that it's not for > everyone to enable in production configs just because they think they are > special). > > But SLAB_MERGE, I doubt it would get many users being opt-in. People would > have to consciously opt-in to not being special. > > As for changing the default, we definitely need to see the memory usage > results first, as was mentioned. It's not expected that disabling merging > would decrease performance, so no wonder the test didn't find such decrease, > but the expected downside is really increased memory overhead. Did this analysis happen? Apologies if I missed it... > But then again it's just a default and most people would use a distro config > anyway, and neither option seems to be an obvious winner to me? As for the > "security by default" argument, AFAIK we don't enable freelist > hardening/randomization by default, and I thought (not being the expert on > this) the heap spraying attacks concerned mainly generic kmalloc cache users > (see also [2]) and not some specific named caches being merged? > > [1] https://lore.kernel.org/all/CA+55aFyepmdpbg9U2Pvp+aHjKmmGCrTK2ywzqfmaOTMXQasYNw@xxxxxxxxxxxxxx/ > [2] https://lore.kernel.org/all/20230626031835.2279738-1-gongruiqi@xxxxxxxxxxxxxxx/ I'm a fan of turning on any of these "by default", as that's been the historical approach, which tends to span years: - security feature introduced, default off in the kernel - distros enable it by default - kernel makes it default on So perhaps we're better off making the other hardening features on by default since distros have been shipping with them for years now? -Kees -- Kees Cook
