[PATCH v2 0/2] iommu: Allow passing custom allocators to pgtable drivers

[Robin Murphy]

On 2023-11-20 3:45 pm, Jason Gunthorpe wrote:
> On Mon, Nov 20, 2023 at 04:14:18PM +0100, Boris Brezillon wrote:
>> On Mon, 20 Nov 2023 10:46:04 -0400
>> Jason Gunthorpe <jgg at ziepe.ca> wrote:
>>
>>> On Mon, Nov 20, 2023 at 03:38:38PM +0100, Boris Brezillon wrote:
>>>
>>>>> At LPC there was quite a lot if interest in improving the io page
>>>>> table stuff to work better. Based on that I'm even more against adding
>>>>> an external allocator at this time. :\
>>>>
>>>> I'm sure this has been discussed with the IOMMU maintainers, but I'd
>>>> really like to hear it from them if you don't mind. Especially since,
>>>> as I mentioned, the custom allocator idea comes from Robin, not me...
>>>
>>> It is a community effort - this is my opinion. We now understand this
>>> io page table area needs a lot of work to meet all the new needs. I'm
>>> against hacking it up like this and making that work harder.
>>
>> Consider it a hack if you like, but the thing is pretty self-contained,
>> and I doubt it will get to a point where things become unmaintainable
>> before you get the generic caching system working. Not to mention
>> Gaurav's use case, which can't really be solved with your caching
>> solution.
> 
> Gaurav doesn't need a custom allocator, he needs a way to manage
> sharing page table memory with the hypervisor. A different set of ops
> that don't replace the entire allocator would be fine for them.
> 
> The issue with taking away the alloc_page is that it complicates the
> ability for the common code to use the struct page fields and more. We
> already know we are going to need those to do some of the things that
> are being asked for, eg RCU freeing and refcounting.

Oh come on, for all the things you claim are simple, you don't think 
it's trivial to make the fancy stuff conditional? (Hint: we already have 
control flow all over which is conditional on the cfg and/or calling 
context, and that future stuff may well end up wanting to be conditional 
in its own right anyway since not all io-pgtable-arm users will need or 
want it).

Having already 90% prototyped deferred freeing (oh, to be able to get 
back to fun things like that...), I see no significant impediment to 
fitting that in around this. Certainly no worse than fitting it around 
the !coherent_walk DMA API bits - in fact if anything, punting all that 
lot into custom allocator hooks starts to look like an attractive idea 
once we have the means (it really wants to be using dma_alloc_pages() 
these days).

>>> I would not expect a resolution in a few weeks.
>>
>> OOC, what's the time frame?
> 
> The larger need has been discussed is considerable work, I would be
> surprised if something could be merged in less than 6 months,
> especially with the upcoming holiday season
> 
>> I've maintained a subsystem for a few years, so I know what it means
>> to contribute upstream and ask or being asked to extend the core to do
>> something the right way. I'm generally patient and try to follow
>> maintainers' recommendations, but there's some point where waiting is
>> no more an option, and you have to be more pragmatic, because there's
>> always a better way of doing things, and perfection is often the enemy
>> of good...
> 
> Here I am not talking about perfection, I am concerned about messing
> up work that I now see as important for other iommu drivers related
> things by making a hack for a DRM driver (which as I've said has,
> unravelling these hacks has been traumatic for many of us
> historically)

In your own words, this is a community effort. You do not own this code, 
and you do not get to prevent the community from solving multiple 
problems they have now, in a way which is reasonable now, just because 
*you* don't like it, and *you* believe it might possibly make it harder 
to do something at some indeterminate point in future. Your objection is 
noted, but as above, I for one do not see a convincing basis for it anyway.

You know what *the community* sees as important? Not having to maintain 
multiple copies of near-identical pagetable code, because maintenance is 
an established and very real concern. This is why we have the io-pgtable 
library in the first place, and why we adapted io-pgtable-arm to support 
panfrost when that came along. If supporting panfrost and panthor as 
io-pgtable users really does ever become a practical problem at some 
point in the future, we will work to address that problem at that point. 
In the meantime, we can get on with the common goal of making Linux do 
the things people want it to do, in the best way that the maintainers 
are happy to maintain. This is not "a hack for a DRM driver", it's a 
simple and convenient generalisation of part of io-pgtable, in keeping 
with the design of io-pgtable (user-provided callbacks are nothing new), 
and straightforwardly serving at least two completely different 
use-cases already.

Thanks,
Robin.