SLOB breaks Crypto
in [Kernel]
|
Prev: [RELEASE] LTTng 0.214 for kernel 2.6.33.4
Next: vlynq: make whole Kconfig-menu dependant on architecture
From: Paul Mundt on 18 May 2010 18:40 On Tue, May 18, 2010 at 02:20:21PM -0700, David Miller wrote: > From: Pekka Enberg <penberg(a)cs.helsinki.fi> > Date: Wed, 19 May 2010 00:15:46 +0300 > > > On Tue, May 18, 2010 at 11:59 PM, David Miller <davem(a)davemloft.net> wrote: > >> From: Matt Mackall <mpm(a)selenic.com> > >> Date: Tue, 18 May 2010 14:33:55 -0500 > >> > >>> SLOB honors ARCH_KMALLOC_MINALIGN. If your arch has alignment > >>> requirements, I recommend you set it. > >> > >> I recommend that the alignment provided by the allocator is not > >> determined by which allocator I happen to have enabled. > >> > >> The values and ifdef'ery should be identical in all of our > >> allocators. > > > > Why? It doesn't make much sense for SLOB, which tries to be as space > > efficient as possible, as a default. If things break on sparc, it > > really needs to set ARCH_KMALLOC_MINALIGN as slab default alignment is > > not something you really want to depend on. > > I think it does make sense to expect that, whatever my architecture > defines or does not define, I can expect the allocators to provide the > same minimum alignment guarentee. Otherwise it is no guarantee at all. > SLAB/SLUB/SLOB all used to have the same BYTES_PER_WORD alignment guarantee, with SLAB and SLUB having moved away from this to unsigned long long in b46b8f19 and 47bfdc0d respectively. This was due to mixing 64-bit integers in data structures, which in the SLAB case resulted in misaligned structures and also broke redzoning (architecture overrides also disabled it completely). The SLUB change was made a couple of days earlier for the same structure misalignment reasons (64-bit integers on 32-bit platforms). The default changes in SLAB/SLUB at least assume that 32-bit architectures can only address 64-bit values on a 64-bit boundary. While this is true for most cases, these have always been handled through the bumping of the architecture minalign values in the past. Indeed, this was the rationale I had for adding the architecture-specific slab minalign override in the first place. The kmalloc one on the other hand is largely just overriden for platforms with DMA requirements -- usually a cacheline boundary. > It's already obvious from these reports that such dependencies do > exist. > These dependencies were then introduced after SLAB/SLUB changed the rules, suggesting that not enough testing was done. > So one of two things should happen: > > 1) SLOB conforms to SLAB/SLUB in it's test > > 2) SLAB/SLUB conforms to SLOB in it's test > > And yes this is an either-or, you can't say they are both valid. I don't see any reason to punish SLOB for the assumptions that SLAB/SLUB arbitrarily took up, presumably on an architecture that should have specified its own alignment requirements and simply couldn't be bothered. Making SLAB redzoning work with arbitrary alignment is another matter entirely, and something that should probably be revisited. Anything that assumes more than BYTES_PER_WORD is simply broken and should be reverted. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo(a)vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
From: Herbert Xu on 18 May 2010 18:50 On Tue, May 18, 2010 at 09:32:54PM +0200, Adrian-Ken Rueegsegger wrote: > > When doing the revert it is necessary to either have > ARCH_KMALLOC_MINALIGN defined or explicitly define CRYPTO_MINALIGN in > the case where it is not. Otherwise shash compilation fails because it > needs CRYPTO_MINALIGN. Thanks, I'll make sure that it gets replaced with crypto_tfm_ctx_alignment(). CRYPTO_MINALIGN shouldn't be used in running code. Cheers, -- Visit Openswan at http://www.openswan.org/ Email: Herbert Xu ~{PmV>HI~} <herbert(a)gondor.apana.org.au> Home Page: http://gondor.apana.org.au/~herbert/ PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo(a)vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
From: David Miller on 18 May 2010 18:50 From: Paul Mundt <lethal(a)linux-sh.org> Date: Wed, 19 May 2010 07:35:10 +0900 > On Tue, May 18, 2010 at 02:20:21PM -0700, David Miller wrote: >> So one of two things should happen: >> >> 1) SLOB conforms to SLAB/SLUB in it's test >> >> 2) SLAB/SLUB conforms to SLOB in it's test >> >> And yes this is an either-or, you can't say they are both valid. > > I don't see any reason to punish SLOB for the assumptions that SLAB/SLUB > arbitrarily took up, presumably on an architecture that should have > specified its own alignment requirements and simply couldn't be bothered. > Making SLAB redzoning work with arbitrary alignment is another matter > entirely, and something that should probably be revisited. > > Anything that assumes more than BYTES_PER_WORD is simply broken and > should be reverted. You can't make the default different in each allocator, PERIOD. If you can't know what the default is, how in the world can you know if you need to override it? You can't. It's a guess, and you can't say otherwise. All of the CPP tests like the one used by linux/crypto.h are ludicrious. It should absolutely be not necessary for any code to duplicate this kind of calculation. Instead, this sequence should be in linux/slab.h, and be used universally by slab, slub, slob and anything that wants to know the allocators alignment guarentees. I don't even know of a 32-bit chip outside of x86 that doesn't potentially emit alignment requiring 64-bit memory operations for 64-bit objects. So what SLOB is doing with a different default is even more strange. And I bet you that even without the requirement, x86 runs faster with 64-bit alignment of 64-bit objects. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo(a)vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
From: Paul Mundt on 18 May 2010 19:20 On Tue, May 18, 2010 at 03:40:59PM -0700, David Miller wrote: > From: Paul Mundt <lethal(a)linux-sh.org> > Date: Wed, 19 May 2010 07:35:10 +0900 > > > On Tue, May 18, 2010 at 02:20:21PM -0700, David Miller wrote: > >> So one of two things should happen: > >> > >> 1) SLOB conforms to SLAB/SLUB in it's test > >> > >> 2) SLAB/SLUB conforms to SLOB in it's test > >> > >> And yes this is an either-or, you can't say they are both valid. > > > > I don't see any reason to punish SLOB for the assumptions that SLAB/SLUB > > arbitrarily took up, presumably on an architecture that should have > > specified its own alignment requirements and simply couldn't be bothered. > > Making SLAB redzoning work with arbitrary alignment is another matter > > entirely, and something that should probably be revisited. > > > > Anything that assumes more than BYTES_PER_WORD is simply broken and > > should be reverted. > > You can't make the default different in each allocator, PERIOD. > I don't disagree that having different defaults is a bit unorthodox, but that has not been a problem for any code that didn't attempt to make its own assumptions about the alignment. > If you can't know what the default is, how in the world can you know > if you need to override it? You can't. It's a guess, and you can't > say otherwise. > If an architecture requires 64-bit alignment for addressing 64-bit values on 32-bit, then that's a hard architecture requirement irrespective of whatever the slab allocator defaults to. The whole idea behind architecture minimum alignment requirements is that they are minimum requirements. > All of the CPP tests like the one used by linux/crypto.h are > ludicrious. It should absolutely be not necessary for any code to > duplicate this kind of calculation. > Agreed. > Instead, this sequence should be in linux/slab.h, and be used > universally by slab, slub, slob and anything that wants to know the > allocators alignment guarentees. > Having a slab_alignment() routine or something sounds fine, but we already have headers split out for all of the different allocators, so there's no specific reason why slob_def.h can't deviate here, too. Again, it's the crypto code that is broken for making bogus alignment assumptions in the first place. > I don't even know of a 32-bit chip outside of x86 that doesn't > potentially emit alignment requiring 64-bit memory operations for > 64-bit objects. So what SLOB is doing with a different default is > even more strange. And I bet you that even without the requirement, > x86 runs faster with 64-bit alignment of 64-bit objects. I assumed ppc32 too, but if they're really in a minority then we could also consider just inverting the logic (both x86 and ppc set HAVE_EFFICIENT_UNALIGNED_ACCESS for example). -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo(a)vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
From: Matt Mackall on 18 May 2010 19:20
On Tue, 2010-05-18 at 15:40 -0700, David Miller wrote: > All of the CPP tests like the one used by linux/crypto.h are > ludicrious. It should absolutely be not necessary for any code to > duplicate this kind of calculation. > > Instead, this sequence should be in linux/slab.h, and be used > universally by slab, slub, slob and anything that wants to know the > allocators alignment guarentees. Agreed. However, every arch should -also- set ARCH_KMALLOC_MINALIGN appropriately so that we have documentation of the hardware requirements on each platform. > I don't even know of a 32-bit chip outside of x86 that doesn't > potentially emit alignment requiring 64-bit memory operations for > 64-bit objects. So what SLOB is doing with a different default is > even more strange. And I bet you that even without the requirement, > x86 runs faster with 64-bit alignment of 64-bit objects. No doubt but SLOB explicitly trades faster for smaller. -- Mathematics is the supreme nostalgia of our time. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo(a)vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/ |