Re: [PATCH v14 02/11] rust: add dma coherent allocator abstraction.

[Boqun Feng <boqun.feng@gmail.com>]

On Fri, Mar 21, 2025 at 03:25:39PM -0300, Jason Gunthorpe wrote:
> On Tue, Mar 11, 2025 at 07:47:58PM +0200, Abdiel Janulgue wrote:
> > +pub struct CoherentAllocation<T: AsBytes + FromBytes> {
> > +    dev: ARef<Device>,
> > +    dma_handle: bindings::dma_addr_t,
> > +    count: usize,
> > +    cpu_addr: *mut T,
> > +    dma_attrs: Attrs,
> > +}
> 
> I'd like to point out how memory wasteful this is from what real
> drivers are doing today when they use the coherent API. Let's compare
> against SMMUv3's use for the CD table..
> 
> This would be the code in arm_smmu_alloc_cd_ptr()
> 
> It is making a 2 level radix tree.
> 
> The cpu_addr is stored in a linear array of pointers:
> 
> 			struct arm_smmu_cdtab_l2 **l2ptrs;
> 
> The dma_addr is encoded into the HW data structure itself:
> 
> 		arm_smmu_write_cd_l1_desc(&cd_table->l2.l1tab[idx],
> 						  l2ptr_dma);
> 
> The size of the allocation is fixed size:
> 			*l2ptr = dma_alloc_coherent(smmu->dev, sizeof(**l2ptr),
>                                                                 ^^^^^^^^^^^^
> 						    &l2ptr_dma, GFP_KERNEL);
> 
> It doesn't need a struct device pointer or reference because this uses
> the usual kernel 'fence' reasoning for destruction.
> 
> It doesn't even use dma_attrs. (why is this in a long term struct?)
> 
> So, smmu manages to do this with a single array of 8 bytes/entry to shadow
> the CPU pointer, and recovers the dma_addr from the HW data structure:
> 
> 			dma_free_coherent(smmu->dev,
> 					  sizeof(*cd_table->l2.l2ptrs[i]),
> 					  cd_table->l2.l2ptrs[i],
> 					  arm_smmu_cd_l1_get_desc(&cd_table->l2.l1tab[i]));
> 
> Basically, it was designed to be very memory efficient.
> 
> If we imagine driving the same HW in rust the array storing the CPU
> pointer would have to expand to 40 bytes/entry to hold every
> CoherentAllocation. This means rust would need a new high order memory
> allocation to hold the CoherentAllocation memory array!
> 

Thanks for the example, it seems to me that your case needs a

    pub struct CoherentAllocationVec<T: AsBytes + FromBytes> {
        dev: ARef<Device>,
        cpu_addrs: KVec<(*mut T, bindings::dma_addr_t)>,
        dma_attrs: Attrs,
    }

of course, we can get rid of `bindings::dma_addr_t` if there is a
method:

    impl<T: ...> CoherentAllocationVec<T> {
        pub fn get_dma_handle(&self, idx: usize) -> bindings::dma_addr_t { 
	    ...
	    // probably only availabe for a particular T or Vec.
	}
    }

    // and drop of `CoherentAllocationVec` will be:

    impl<T: ...> Drop for CoherentAllocationVec<T> {
        fn drop(&mut self) {
            for (i, cpu_addr) in self.cpu_addrs.as_slice().iter().enumerate() {
	    	dma_free_coherent_attr(self.dev.as_raw(),
		                       core::mem::size_of::<T>(),
				       cpu_addr,
				       self.get_dma_handle(i),
				       self.attrs);
	    }
	    ...
	}
    }

Then we have:

    pub struct CoherentAllocationVec<T: AsBytes + FromBytes> {
        dev: ARef<Device>,
        cpu_addrs: KVec<*mut T>,
        dma_attrs: Attrs,
    }

And we can make `dma_attrs` a const of the type:

    pub struct CoherentAllocationVec<T: AsBytes + FromBytes, const ATTRS: Attrs = Attrs(0)> {
        dev: ARef<Device>,
        cpu_addr: KVec<*mut T>,
    }

As for getting rid of the `dev` pointer, the struct arm_smmu_device has
a pointer to struct device as well, so it's all about how to organize
the fields, at very least, you could do:

    pub struct ArmSmmuDevice {
        // avoid using an ARef<Device> here since we already has it in
	// cdtable.

        cdtable: CoherentAllocationVec<arm_smmu_cdtab_l2>,
	...,
    }

and whenever you need to get a pointer/reference to the device, you can
get it from:

    .cdtable.dev

it may not be the best organization of the fields, but we will see the
real Rust use for a better design.

Regards,
Boqun

> Jason