On Fri, 2024-02-23 at 08:58 -0400, Jason Gunthorpe wrote: > On Fri, Feb 23, 2024 at 12:38:18PM +0100, Niklas Schnelle wrote: > > > Although I doubt that generating long TLP from byte writes is > > > really necessary. > > > > I might have gotten confused but I think these are not byte writes. > > Remember that the count is in terms of the number of bits sized > > quantities to copy so "count == 1" is 4/8 bytes here. > > Right. > > There seem to be two callers of this API in the kernel, one is calling > with a constant size and wants a large TLP > > Another seems to want memcpy_to_io with a guarenteed 32/64 bit store. I don't really understand how that works together with the order not being guaranteed. Do they use normal ioremap() and then require 32/64 bit TLPs and don't care about the order? But then the generic and ARM variants do things in order so who knows if they actually rely on that. > > > > IIRC you were merging at most 4 writes. > > > So better to do a single 32bit write instead. > > > (Unless you have misaligned source data - unlikely.) > > > > > > While write-combining to generate long TLP is probably mostly > > > safe for PCIe targets, there are some that will only handle > > > TLP for single 32bit data items. > > > Which might be why the code is explicitly requesting 4 byte copies. > > > So it may be entirely wrong to write-combine anything except > > > the generic memcpy_toio(). > > > > On anything other than s390x this should only do write-combine if the > > memory mapping allows it, no? Meaning a driver that can't handle larger > > TLPs really shouldn't use ioremap_wc() then. > > Right. > > > On s390x one could argue that our version of __iowriteXX_copy() is > > strictly speaking not correct in that zpci_memcpy_toio() doesn't really > > use XX bit writes which is why for us memcpy_toio() was actually a > > better fit indeed. On the other hand doing 32 bit PCI stores (an s390x > > thing) can't combine multiple stores into a single TLP which these > > functions are used for and which has much more use cases than forcing a > > copy loop with 32/64 bit sized writes which would also be a lot slower > > on s390x than an aligned zpci_memcpy_toio(). > > mlx5 will definitely not work right if __iowrite64_copy() results in > anything smaller than 32/64 bit PCIe TLPs. > > Jason Yes and we do actually have mlx5 on s390x so this is my priority.
