Re: [PATCH RFC 06/12] dma: direct: Provide accessor to dmem region

[Robin Murphy]

On 2025-03-10 4:28 pm, Maxime Ripard wrote:
> On Mon, Mar 10, 2025 at 02:56:37PM +0000, Robin Murphy wrote:
> > On 2025-03-10 12:06 pm, Maxime Ripard wrote:
> > > Consumers of the direct DMA API will have to know which region their
> > > device allocate from in order for them to charge the memory allocation
> > > in the right one.
> >
> > This doesn't seem to make much sense - dma-direct is not an allocator
> > itself, it just provides the high-level dma_alloc_attrs/dma_alloc_pages/etc.
> > interfaces wherein the underlying allocations _could_ come from CMA, but
> > also a per-device coherent/restricted pool, or a global coherent/atomic
> > pool, or the regular page allocator, or in one weird corner case the SWIOTLB
> > buffer, or...
>
> I guess it wasn't super clear, but what I meant is that it's an
> allocator to the consumer: it gets called, and returns a buffer. How it
> does so is transparent to the device, and on the other side of the
> abstraction.
>
> I do agree that the logic is complicated to follow, and that's what I
> was getting at in the cover letter.

Right, but ultimately my point is that when we later end up with:

struct dmem_cgroup_region *
dma_get_dmem_cgroup_region(struct device *dev)
{
	if (dma_alloc_direct(dev, get_dma_ops(dev)))
		return dma_direct_get_dmem_cgroup_region(dev);

		= dma_contiguous_get_dmem_cgroup_region(dev);

it's objectively wrong given what dma_alloc_direct() means in context:

void *dma_alloc_attrs(...)
{
	if (dma_alloc_direct(dev, ops))
		cpu_addr = dma_direct_alloc(...);

where dma_direct_alloc() may then use at least 5 different allocation 
methods, only one of which is CMA. Accounting things which are not CMA 
to CMA seems to thoroughly defeat the purpose of having such 
fine-grained accounting at all.

This is why the very notion of "consumers of dma-direct" should 
fundamentally not be a thing IMO. Drivers consume the DMA API 
interfaces, and the DMA API ultimately consumes various memory 
allocators, but what happens in between is nobody else's business; 
dma-direct happens to represent *some* paths between the two, but there 
are plenty more paths to the same (and different) allocators through 
other DMA API implementations as well. Which route a particular call 
takes to end up at a particular allocator is not meaningful unless you 
are the DMA ops dispatch code.

Or to put it another way, to even go for the "dumbest possible correct 
solution", the plumbing of dma_get_dmem_cgroup_region() would need to be 
about as complex and widespread as the plumbing of dma_alloc_attrs() 
itself ;)

I think I see why a simple DMA attribute couldn't be made to work, as 
dmem_cgroup_uncharge() can't simply look up the pool the same way 
dmem_cgroup_try_charge() found it, since we still need a cg for that and 
get_current_dmemcs() can't be assumed to be stable over time, right?
At the point I'm probably starting to lean towards a whole new DMA op 
with a properly encapsulated return type (and maybe a long-term goal of 
consolidating the 3 or 4 different allocation type we already have), or 
just have a single dmem region for "DMA API memory" and don't care where 
it came from (although I do see the issues with that too - you probably 
wouldn't want to ration a device-private pool the same way as global 
system memory, for example)

Thanks,
Robin.