On Thu, Mar 21, 2024 at 11:39:10PM +0100, Christoph Hellwig wrote:
> On Tue, Mar 19, 2024 at 12:36:20PM -0300, Jason Gunthorpe wrote:
> > I kind of understand your thinking on the DMA side, but I don't see
> > how this is good for users of the API beyond BIO.
> >
> > How will this make RDMA better? We have one MR, the MR has pages, the
> > HW doesn't care about the SW distinction of p2p, swiotlb, direct,
> > encrypted, iommu, etc. It needs to create one HW page list for
> > whatever user VA range was given.
>
> Well, the hardware (as in the PCIe card) never cares. But the setup
> path for the IOMMU does, and something in the OS needs to know about
> it. So unless we want to stash away a 'is this P2P' flag in every
> page / SG entry / bvec, or a do a lookup to find that out for each
> of them we need to manage chunks at these boundaries. And that's
> what I'm proposing.
Okay, if we look at the struct-page-less world (which we want for
DMABUF) then we need to keep track for sure. What I had drafted was to
keep track in the new "per-SG entry" because that seemed easiest to
migrate existing code into.
Though the datastructure could also be written to be a list of uniform
memory types and then a list of SG entries. (more like how bio is
organized)
No idea right now which is better, and I'm happy make it go either
way.
But Leon's series is not quite getting to this, it it still struct
page based and struct page itself has all the metadata - though as you
say it is a bit expensive to access.
> > Or worse, whatever thing is inside a DMABUF from a DRM
> > driver. DMABUF's can have a (dynamic!) mixture of P2P and regular
> > AFAIK based on the GPU's migration behavior.
>
> And that's fine. We just need to track it efficiently.
Right, DMABUF/etc will return a something that has a list of physical
addresses and some meta-data to indicate the "p2p memory provider" for
the P2P part.
Perhaps it could be as simple as 1 bit in the physical address/length
and a global "P2P memory provider" pointer for the entire DMA
BUF. Unclear to me right now, but sure.
> > Or triple worse, ODP can dynamically change on a page by page basis
> > the type depending on what hmm_range_fault() returns.
>
> Same. If this changes all the time you need to track it. And we
> should find a way to shared the code if we have multiple users for it.
ODP (for at least the forseeable furture) is simpler because it is
always struct page based so we don't need more metadata if we pay the
cost to reach into the struct page. I suspect that is the right trade
off for hmm_range_fault users.
> But most DMA API consumers will never see P2P, and when they see it
> it will be static. So don't build the DMA API to automically do
> the (not exactly super cheap) checks and add complexity for it.
Okay, I think I get what you'd like to see.
If we are going to make caller provided uniformity a requirement, lets
imagine a formal memory type idea to help keep this a little
abstracted?
DMA_MEMORY_TYPE_NORMAL
DMA_MEMORY_TYPE_P2P_NOT_ACS
DMA_MEMORY_TYPE_ENCRYPTED
DMA_MEMORY_TYPE_BOUNCE_BUFFER // ??
Then maybe the driver flow looks like:
if (transaction.memory_type == DMA_MEMORY_TYPE_NORMAL && dma_api_has_iommu(dev)) {
struct dma_api_iommu_state state;
dma_api_iommu_start(&state, transaction.num_pages);
for_each_range(transaction, range)
dma_api_iommu_map_range(&state, range.start_page, range.length);
num_hwsgls = 1;
hwsgl.addr = state.iova;
hwsgl.length = transaction.length
dma_api_iommu_batch_done(&state);
} else if (transaction.memory_type == DMA_MEMORY_TYPE_P2P_NOT_ACS) {
num_hwsgls = transcation.num_sgls;
for_each_range(transaction, range) {
hwsgl[i].addr = dma_api_p2p_not_acs_map(range.start_physical, range.length, p2p_memory_provider);
hwsgl[i].len = range.size;
}
} else {
/* Must be DMA_MEMORY_TYPE_NORMAL, DMA_MEMORY_TYPE_ENCRYPTED, DMA_MEMORY_TYPE_BOUNCE_BUFFER? */
num_hwsgls = transcation.num_sgls;
for_each_range(transaction, range) {
hwsgl[i].addr = dma_api_map_cpu_page(range.start_page, range.length);
hwsgl[i].len = range.size;
}
}
And the hmm_range_fault case is sort of like:
struct dma_api_iommu_state state;
dma_api_iommu_start(&state, mr.num_pages);
[..]
hmm_range_fault(...)
if (present)
dma_link_page(&state, faulting_address_offset, page);
else
dma_unlink_page(&state, faulting_address_offset, page);
Is this looking closer?
> > So I take it as a requirement that RDMA MUST make single MR's out of a
> > hodgepodge of page types. RDMA MRs cannot be split. Multiple MR's are
> > not a functional replacement for a single MR.
>
> But MRs consolidate multiple dma addresses anyway.
I'm not sure I understand this?
> > Go back to the start of what are we trying to do here:
> > 1) Make a DMA API that can support hmm_range_fault() users in a
> > sensible and performant way
> > 2) Make a DMA API that can support RDMA MR's backed by DMABUF's, and
> > user VA's without restriction
> > 3) Allow to remove scatterlist from BIO paths
> > 4) Provide a DMABUF API that is not scatterlist that can feed into
> > the new DMA API - again supporting DMABUF's hodgepodge of types.
> >
> > I'd like to do all of these things. I know 3 is your highest priority,
> > but it is my lowest :)
>
> Well, 3 an 4. And 3 is not just limited to bio, but all the other
> pointless scatterlist uses.
Well, I didn't write a '5) remove all the other pointless scatterlist
case' :)
Anyhow, I think we all agree on the high level objective, we just need
to get to an API that fuses all of these goals together.
To go back to my main thesis - I would like a high performance low
level DMA API that is capable enough that it could implement
scatterlist dma_map_sg() and thus also implement any future
scatterlist_v2, bio, hmm_range_fault or any other thing we come up
with on top of it. This is broadly what I thought we agreed to at LSF
last year.
Jason
