Re: Phyr Starter

On Tue, Jan 11, 2022 at 04:21:59PM -0400, Jason Gunthorpe wrote:

On Tue, Jan 11, 2022 at 06:33:57PM +0000, Matthew Wilcox wrote:

quoted

quoted

Then we are we using get_user_phyr() at all if we are just storing it
in a sg?

I did consider just implementing get_user_sg() (actually 4 years ago),
but that cements the use of sg as both an input and output data structure
for DMA mapping, which I am under the impression we're trying to get
away from.

I know every time I talked about a get_user_sg() Christoph is against
it and we need to stop using scatter list...

quoted

quoted

Also 16 entries is way to small, it should be at least a whole PMD
worth so we don't have to relock the PMD level each iteration.

I would like to see a flow more like:

  cpu_phyr_list = get_user_phyr(uptr, 1G);
  dma_phyr_list = dma_map_phyr(device, cpu_phyr_list);
  [..]
  dma_unmap_phyr(device, dma_phyr_list);
  unpin_drity_free(cpu_phy_list);

Where dma_map_phyr() can build a temporary SGL for old iommu drivers
compatability. iommu drivers would want to implement natively, of
course.

ie no loops in drivers.

Let me just rewrite that for you ...

	umem->phyrs = get_user_phyrs(addr, size, &umem->phyr_len);
	umem->sgt = dma_map_phyrs(device, umem->phyrs, umem->phyr_len,
			DMA_BIDIRECTIONAL, dma_attr);
	...
	dma_unmap_phyr(device, umem->phyrs, umem->phyr_len, umem->sgt->sgl,
			umem->sgt->nents, DMA_BIDIRECTIONAL, dma_attr);
	sg_free_table(umem->sgt);
	free_user_phyrs(umem->phyrs, umem->phyr_len);

Why? As above we want to get rid of the sgl, so you are telling me to
adopt phyrs I need to increase the memory consumption by a hefty
amount to store the phyrs and still keep the sgt now? Why?

I don't need the sgt at all. I just need another list of physical
addresses for DMA. I see no issue with a phsr_list storing either CPU
Physical Address or DMA Physical Addresses, same data structure.

There's a difference between a phys_addr_t and a dma_addr_t.  They
can even be different sizes; some architectures use a 32-bit dma_addr_t
and a 64-bit phys_addr_t or vice-versa.  phyr cannot store DMA addresses.

In the fairly important passthrough DMA case the CPU list and DMA list
are identical, so we don't even need to do anything.

In the typical iommu case my dma map's phyrs is only one entry.

That becomes a very simple sg table then.

As an example coding - Use the first 8 bytes to encode this:

 51:0 - Physical address / 4k (ie pfn)
 56:52 - Order (simple, your order encoding can do better)
 61:57 - Unused
 63:62 - Mode, one of:
         00 = natural order pfn (8 bytes)
         01 = order aligned with length (12 bytes)
         10 = arbitary (12 bytes)

Then the optional 4 bytes are used as:

Mode 01 (Up to 2^48 bytes of memory on a 4k alignment)
  31:0 - # of order pages

Mode 10 (Up to 2^25 bytes of memory on a 1 byte alignment)
  11:0 - starting byte offset in the 4k
  31:12 - 20 bits, plus the 5 bit order from the first 8 bytes:
          length in bytes

Honestly, this looks awful to operate on.  Mandatory 8-bytes per entry
with an optional 4 byte extension?

quoted

quoted

The last case is, perhaps, a possible route to completely replace
scatterlist. Few places need true byte granularity for interior pages,
so we can invent some coding to say 'this is 8 byte aligned, and n
bytes long' that only fits < 4k or something. Exceptional cases can
then still work. I'm not sure what block needs here - is it just 512?

Replacing scatterlist is not my goal.  That seems like a lot more work
for little gain.  

Well, I'm not comfortable with the idea above where RDMA would have to
take a memory penalty to use the new interface. To avoid that memory
penalty we need to get rid of scatterlist entirely.

If we do the 16 byte struct from the first email then a umem for MRs
will increase in memory consumption by 160% compared today's 24
bytes/page. I think the HPC workloads will veto this.

Huh?  We do 16 bytes per physically contiguous range.  Then, if your HPC
workloads use an IOMMU that can map a virtually contiguous range
into a single sg entry, it uses 24 bytes for the entire mapping.
It should shrink.

quoted

I just want to delete page_link, offset and length from struct
scatterlist.  Given the above sequence of calls, we're going to get
sg lists that aren't chained.  They may have to be vmalloced, but
they should be contiguous.

I don't understand that? Why would the SGL out of the iommu suddenly
not be chained?

Because it's being given a single set of ranges to map, instead of
being given 512 pages at a time.

quoted

From what I've heard I'm also not keen on a physr list using vmalloc

either, that is said to be quite slow?

It would only be slow for degenerate cases where the pinned memory
is fragmented and not contiguous.

quoted

quoted

I would imagine a few steps to this process:
 1) 'phyr_list' datastructure, with chaining, pre-allocation, etc
 2) Wrapper around existing gup to get a phyr_list for user VA
 3) Compat 'dma_map_phyr()' that coverts a phyr_list to a sgl and back
    (However, with full performance for iommu passthrough)
 4) Patches changing RDMA/VFIO/DRM to this API
 5) Patches optimizing get_user_phyr()
 6) Patches implementing dma_map_phyr in the AMD or Intel IOMMU driver

I was thinking ...

1. get_user_phyrs() & free_user_phyrs()
2. dma_map_phyrs() and dma_unmap_phyrs() wrappers that create a
   scatterlist from phyrs and call dma_map_sg() / dma_unmap_sg() to work
   with current IOMMU drivers

IMHO, the scatterlist has to go away. The interface should be physr
list in, physr list out.

That's reproducing the bad decision of the scatterlist, only with
a different encoding.  You end up with something like:

struct neoscat {
	dma_addr_t dma_addr;
	phys_addr_t phys_addr;
	size_t dma_len;
	size_t phys_len;
};

and the dma_addr and dma_len are unused by all-but-the-first entry when
you have a competent IOMMU.  We want a different data structure in and
out, and we may as well keep using the scatterlist for the dma-map-out.