Re: [PATCH 07/10] crypto: Use ARCH_DMA_MINALIGN instead of ARCH_KMALLOC_MINALIGN

[Greg Kroah-Hartman]

On Thu, Apr 14, 2022 at 04:36:46PM +0200, Ard Biesheuvel wrote:
> On Thu, 14 Apr 2022 at 16:27, Greg Kroah-Hartman
> <gregkh@xxxxxxxxxxxxxxxxxxx> wrote:
> >
> > On Thu, Apr 14, 2022 at 03:52:53PM +0200, Ard Biesheuvel wrote:
> > > On Thu, 14 Apr 2022 at 07:38, Greg Kroah-Hartman
> > > <gregkh@xxxxxxxxxxxxxxxxxxx> wrote:
> > > >
> > > > On Wed, Apr 13, 2022 at 09:53:24AM -1000, Linus Torvalds wrote:
> ...
> > > > > Honestly, I think it would probably be worth discussing the "kmalloc
> > > > > DMA alignment" issues.
> > > > >
> > > > > 99.9% of kmalloc users don't want to do DMA.
> > > > >
> > > > > And there's actually a fair amount of small kmalloc for random stuff.
> > > > > Right now on my laptop, I have
> > > > >
> > > > >     kmalloc-8          16907  18432      8  512    1 : ...
> > > > >
> > > > > according to slabinfo, so almost 17 _thousand_ allocations of 8 bytes.
> > > > >
> > > > > It's all kinds of sad if those allocations need to be 64 bytes in size
> > > > > just because of some silly DMA alignment issue, when none of them want
> > > > > it.
> > > > >
> > >
> > > Actually, the alignment for non-cache coherent DMA is 128 bytes on
> > > arm64, not 64 bytes.
> > >
> > > > > Yeah, yeah, wasting a megabyte of memory is "just a megabyte" these
> > > > > days. Which is crazy. It's literally memory that could have been used
> > > > > for something much more useful than just pure and utter waste.
> > > > >
> > > > > I think we could and should just say "people who actually require DMA
> > > > > accesses should say so at kmalloc time". We literally have that
> > > > > GFP_DMA and ZOME_DMA for various historical reasons, so we've been
> > > > > able to do that before.
> > > > >
> > > > > No, that historical GFP_DMA isn't what arm64 wants - it's the old
> > > > > crazy "legacy 16MB DMA" thing that ISA DMA used to have.
> > > > >
> > > > > But the basic issue was true then, and is true now - DMA allocations
> > > > > are fairly special, and should not be that hard to just mark as such.
> > > >
> > > > "fairly special" == "all USB transactions", so it will take a lot of
> > > > auditing here.  I think also many SPI controllers require this and maybe
> > > > I2C?  Perhaps other bus types do as well.
> > > >
> > > > So please don't make this change without some way of figuring out just
> > > > what drivers need to be fixed up, as it's going to be a lot...
> > > >
> > >
> > > Yeah, the current de facto contract of being able to DMA map anything
> > > that was allocated via the linear map makes it quite hard to enforce
> > > the use of dma_kmalloc() for this.
> > >
> > > What we might do, given the fact that only inbound non-cache coherent
> > > DMA is problematic, is dropping the kmalloc alignment to 8 like on
> > > x86, and falling back to bounce buffering when a misaligned, non-cache
> > > coherent inbound DMA mapping is created, using the SWIOTLB bounce
> > > buffering code that we already have, and is already in use on most
> > > affected systems for other reasons (i.e., DMA addressing limits)
> >
> > Ick, that's a mess.
> >
> > > This will cause some performance regressions, but in a way that seems
> > > fixable to me: taking network drivers as an example, the RX buffers
> > > that are filled using inbound DMA are typically owned by the driver
> > > itself, which could be updated to round up its allocations and DMA
> > > mappings. Block devices typically operate on quantities that are
> > > aligned sufficiently already. In other cases, we will likely notice
> > > if/when this fallback is taken on a hot path, but if we don't, at
> > > least we know a bounce buffer is being used whenever we cannot perform
> > > the DMA safely in-place.
> >
> > We can move to having an "allocator-per-bus" for memory like this to
> > allow the bus to know if this is a DMA requirement or not.
> >
> > So for all USB drivers, we would have:
> >         usb_kmalloc(size, flags);
> > and then it might even be easier to verify with static tools that the
> > USB drivers are sending only properly allocated data.  Same for SPI and
> > other busses.
> >
> 
> As I pointed out earlier in the thread, alignment/padding requirements
> for non-coherent DMA are a property of the CPU's cache hierarchy, not
> of the device. So I'm not sure I follow how a per-subsystem
> distinction would help here. In the case of USB especially, would that
> mean that block, media and networking subsystems would need to be
> aware of the USB-ness of the underlying transport?

That's what we have required today, yes.  That's only because we knew
that for some USB controllers, that was a requirement and we had no way
of passing that information back up the stack so we just made it a
requirement.

But I do agree this is messy.  It's even messier for things like USB
where it's not the USB device itself that matters, it's the USB
controller that the USB device is attached to.  And that can be _way_ up
the device hierarchy.  Attach something like a NFS mount over a PPP
network connection on a USB to serial device and ugh, where do you
begin?  :)

And is this always just an issue of the CPU cache hierarchy?  And not the
specific bridge that a device is connected to that CPU on?  Or am I
saying the same thing here?

I mean take a USB controller for example.  We could have a system where
one USB controller is on a PCI bus, while another is on a "platform"
bus.  Both of those are connected to the CPU in different ways and so
could have different DMA rules.  Do we downgrade everything in the
system for the worst connection possible?

Again, consider a USB driver allocating memory to transfer stuff, should
it somehow know the cache hierarchy that it is connected to?  Right now
we punt and do not do that at the expense of a bit of potentially
wasted memory for small allocations.


> > https://lore.kernel.org/r/230a9486fc68ea0182df46255e42a51099403642.1648032613.git.christophe.leroy@xxxxxxxxxx
> > is an example of a SPI driver that has been there "for forever" yet
> > always got it wrong.  If we could have had some way to know "only memory
> > allocated with this function is allowed on the bus" that would have
> > fixed the issue a long time ago.
> >
> > Anyway, just an idea, it's up to you all if this is worth it or not.
> > Adding performance regressions at the expense of memory size feels like
> > a rough trade-off to go through until things are fixed up.
> >
> 
> Yeah, I hear you. But we already have distinct classes of memory,
> i.e., vmalloc vs kmalloc, where only the latter is permitted for DMA
> (which is why VMAP stack broke that SPI driver), and I'm not sure
> adding more types is going to make this tractable going forward.

Sorry, it would not be a "real" type of memory at all, just something
with a static tag on it at the compiler level (like we do for endian
stuff with sparse today).  The real memory would be allocated with the
normal kmalloc() and friends it's just that static tools can be run to
verify "yes, all of the calls to usb_submit_urb() were done with memory
allocated with usb_kmalloc(), and not from the stack"

No runtime changes needed for different types, that would be a mess.
Let's lean on static tools when we can.

thanks,

greg k-h