Re: [PATCH v2 2/2] mm, sl[aou]b: guarantee natural alignment for kmalloc(power-of-two)

From: Vlastimil Babka <vbabka@suse.cz>
To: Christopher Lameter <cl@linux.com>,
	Andrew Morton <akpm@linux-foundation.org>
Cc: David Sterba <dsterba@suse.cz>,
	linux-mm@kvack.org, linux-kernel@vger.kernel.org,
	Pekka Enberg <penberg@kernel.org>,
	David Rientjes <rientjes@google.com>,
	Ming Lei <ming.lei@redhat.com>,
	Dave Chinner <david@fromorbit.com>,
	Matthew Wilcox <willy@infradead.org>,
	"Darrick J . Wong" <darrick.wong@oracle.com>,
	Christoph Hellwig <hch@lst.de>,
	linux-xfs@vger.kernel.org, linux-fsdevel@vger.kernel.org,
	linux-block@vger.kernel.org,
	James Bottomley <James.Bottomley@HansenPartnership.com>,
	linux-btrfs@vger.kernel.org, Roman Gushchin <guro@fb.com>,
	Johannes Weiner <hannes@cmpxchg.org>
Subject: Re: [PATCH v2 2/2] mm, sl[aou]b: guarantee natural alignment for kmalloc(power-of-two)
Date: Thu, 26 Sep 2019 09:41:20 +0200	[thread overview]
Message-ID: <6a28a096-0e65-c7ea-9ca9-f72d68948e10@suse.cz> (raw)
In-Reply-To: <alpine.DEB.2.21.1909260005060.1508@www.lameter.com>

On 9/26/19 2:14 AM, Christopher Lameter wrote:
> On Tue, 24 Sep 2019, Andrew Morton wrote:
> 
>> I agree it's a bit regrettable to do this but it does appear that the
>> change will make the kernel overall a better place given the reality of
>> kernel development.
> 
> No it wont.
> 
> - It will only work for special cases like the kmalloc array
> without extras like metadata at the end of objects.

I don't understand what you mean here? The kmalloc caches are special
because they don't have metadata at the end of objects? Others do?

> - It will be an inconsistency in the alignments provided by the allocator.

I don't see a scenario where this will cause a kmalloc user problems.
Can you describe a scenario where a kmalloc users would have some
assumptions about alignment, but due to this change, those assumptions
will be incorrect, and how exactly would it break their code?

> - It will cause us in the future to constantly consider these exceptional
> alignments in the maintenance of the allocators.

Caches can be already created with explicit alignment. This patch just
means there are more of them.

> - These alignments are only needed in exceptional cases but with the patch
> we will provide the alignment by default even if the allocating subsystem
> does not need it.

True. This is where we have to make the decision whether to make things
simpler for those that don't realize they need the alignment, and
whether that's worth the cost. We have evidence of those cases, and the
cost is currently zero in the common cases (SLAB, SLUB without debug
runtime-enabled).

> - We have mechanisms to detect alignment problems using debug kernels and
> debug options that have been available for years. These were not used for
> testing in these cases it seems before the patches hit mainline. Once in
> mainly someone ran a debug kernel and found the issue.

Debugging options are useful if you know there's a bug and you want to
find it. AFAIK the various bots/CIs that do e.g. randconfig, or enable
debug options explicitly, run those kernels in a VM, so I guess that's
why potential breakage due to alignment can lurk in a hw-specific driver.

>> Given this, have you reviewed the patch for overall implementation
>> correctness?
> 
> Yes, the patch is fine.
> 
>> I'm wondering if we can avoid at least some of the patch's overhead if
>> slab debugging is disabled - the allocators are already returning
>> suitably aligned memory, so why add the new code in that case?
> 
> As far as I know this patch is not needed given that we have had the
> standards for alignments for a long time now.
> 
> Why would the allocators provide specially aligned memory just based on
> the size of an object? This is weird and unexpected behavior.

For some, it's expected.