Message-ID: <44bc28c7-05f4-4419-5183-453c4951aac0@linux.intel.com>
Date:   Thu, 9 Nov 2023 16:50:59 +0100
From:   Thomas Hellström 
        <thomas.hellstrom@...ux.intel.com>
To:     Danilo Krummrich <dakr@...hat.com>,
        Christian König <christian.koenig@....com>
Cc:     airlied@...il.com, daniel@...ll.ch, matthew.brost@...el.com,
        sarah.walker@...tec.com, donald.robson@...tec.com,
        boris.brezillon@...labora.com, faith@...strand.net,
        dri-devel@...ts.freedesktop.org, nouveau@...ts.freedesktop.org,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH drm-misc-next v8 09/12] drm/gpuvm: reference count
 drm_gpuvm structures

Danilo, Christian

On 11/6/23 17:42, Danilo Krummrich wrote:
> On Mon, Nov 06, 2023 at 04:10:50PM +0100, Christian König wrote:
>> Am 06.11.23 um 15:11 schrieb Danilo Krummrich:
>>> On Mon, Nov 06, 2023 at 02:05:13PM +0100, Christian König wrote:
>>>> Am 06.11.23 um 13:16 schrieb Danilo Krummrich:
>>>>> [SNIP]
>>>>> This reference count just prevents that the VM is freed as long as other
>>>>> ressources are attached to it that carry a VM pointer, such as mappings and
>>>>> VM_BOs. The motivation for that are VM_BOs. For mappings it's indeed a bit
>>>>> paranoid, but it doesn't hurt either and keeps it consistant.
>>>> Ah! Yeah, we have similar semantics in amdgpu as well.
>>>>
>>>> But we keep the reference to the root GEM object and not the VM.
>>>>
>>>> Ok, that makes much more sense then keeping one reference for each mapping.
>>>>
>>>>>> Because of this the mapping should *never* have a reference to the VM, but
>>>>>> rather the VM destroys all mapping when it is destroyed itself.
>>>>>>
>>>>>>> Hence, If the VM is still alive at a point where you don't expect it to
>>>>>>> be, then it's
>>>>>>> simply a driver bug.
>>>>>> Driver bugs is just what I try to prevent here. When individual mappings
>>>>>> keep the VM structure alive then drivers are responsible to clean them up,
>>>>>> if the VM cleans up after itself then we don't need to worry about it in the
>>>>>> driver.
>>>>> Drivers are *always* responsible for that. This has nothing to do with whether
>>>>> the VM is reference counted or not. GPUVM can't clean up mappings after itself.
>>>> Why not?
>>> I feel like we're talking past each other here, at least to some extend.
>>> However, I can't yet see where exactly the misunderstanding resides.
>> +1
>>
>>>> At least in amdgpu we have it exactly like that. E.g. the higher level can
>>>> cleanup the BO_VM structure at any time possible, even when there are
>>>> mappings.
>>> What do you mean with "cleanup the VM_BO structue" exactly?
>>>
>>> The VM_BO structure keeps track of all the mappings mapped in the VM_BO's VM
>>> being backed by the VM_BO's GEM object. And the GEM objects keeps a list of
>>> the corresponding VM_BOs.
>>>
>>> Hence, as long as there are mappings that this VM_BO keeps track of, this VM_BO
>>> should stay alive.
>> No, exactly the other way around. When the VM_BO structure is destroyed the
>> mappings are destroyed with them.
> This seems to be the same misunderstanding as with the VM reference count.
>
> It seems to me that you want to say that for amdgpu it seems to be a use-case
> to get rid of all mappings backed by a given BO and mapped in a given VM, hence
> a VM_BO. You can do that. Thers's even a helper for that in GPUVM.
>
> But also in this case you first need to get rid of all mappings before you
> *free* the VM_BO - GPUVM ensures that.
>
>> Otherwise you would need to destroy each individual mapping separately
>> before teardown which is quite inefficient.
> Not sure what you mean, but I don't see a difference between walking all VM_BOs
> and removing their mappings and walking the VM's tree of mappings and removing
> each of them. Comes down to the same effort in the end. But surely can go both
> ways if you know all the existing VM_BOs.
>
>>>> The VM then keeps track which areas still need to be invalidated
>>>> in the physical representation of the page tables.
>>> And the VM does that through its tree of mappings (struct drm_gpuva). Hence, if
>>> the VM would just remove those structures on cleanup by itself, you'd loose the
>>> ability of cleaning up the page tables. Unless, you track this separately, which
>>> would make the whole tracking of GPUVM itself kinda pointless.
>> But how do you then keep track of areas which are freed and needs to be
>> updated so that nobody can access the underlying memory any more?
> "areas which are freed", what do refer to? What do yo mean with that?
>
> Do you mean areas of the VA space not containing mappings? Why would I need to
> track them explicitly? When the mapping is removed the corresponding page tables
> / page table entries are gone as well, hence no subsequent access to the
> underlaying memory would be possible.
>
>>>> I would expect that the generalized GPU VM handling would need something
>>>> similar. If we leave that to the driver then each driver would have to
>>>> implement that stuff on it's own again.
>>> Similar to what? What exactly do you think can be generalized here?
>> Similar to how amdgpu works.
> I don't think it's quite fair to just throw the "look at what amdgpu does"
> argument at me. What am I supposed to do? Read and understand *every* detail of
> *every* driver?
>
> Did you read through the GPUVM code? That's a honest question and I'm asking it
> because I feel like you're picking up some details from commit messages and
> start questioning them (and that's perfectly fine and absolutely welcome).
>
> But if the answers don't satisfy you or do not lead to a better understanding it
> just seems you ask others to check out amdgpu rather than taking the time to go
> though the proposed code yourself making suggestions to improve it or explicitly
> point out the changes you require.
>
>>  From what I can see you are basically re-inventing everything we already
>> have in there and asking the same questions we stumbled over years ago.
> I did not ask any questions in the first place. I came up with something that
> Nouveau, Xe, Panthor, PowerVR, etc. required and that works for them.
>
> They also all provided a lot of ideas and contributed code through the review
> process.
>
> Of course, I want this to work for amdgpu as well. So, if you think we're
> missing something fundamential or if you see something that simply doesn't work
> for other drivers, like amdgpu, please educate us. I'm surely willing to learn
> and, if required, change the code.
>
> But please don't just tell me I would re-invent amdgpu and assume that I know
> all the details of this driver. None of that is reasonable.
>
>>>>> If the driver left mappings, GPUVM would just leak them without reference count.
>>>>> It doesn't know about the drivers surrounding structures, nor does it know about
>>>>> attached ressources such as PT(E)s.
>>>> What are we talking with the word "mapping"? The BO_VM structure? Or each
>>>> individual mapping?
>>> An individual mapping represented by struct drm_gpuva.
>> Yeah than that certainly doesn't work. See below.
>>
>>>> E.g. what we need to prevent is that VM structure (or the root GEM object)
>>>> is released while VM_BOs are still around. That's what I totally agree on.
>>>>
>>>> But each individual mapping is a different story. Userspace can create so
>>>> many of them that we probably could even overrun a 32bit counter quite
>>>> easily.
>>> REFCOUNT_MAX is specified as 0x7fff_ffff. I agree there can be a lot of
>>> mappings, but (including the VM_BO references) more than 2.147.483.647 per VM?
>> IIRC on amdgpu we can create something like 100k mappings per second and
>> each takes ~64 bytes.
>>
>> So you just need 128GiB of memory and approx 20 seconds to let the kernel
>> run into a refcount overrun.
> 100.000 * 20 = 2.000.000
>
> That's pretty far from REFCOUNT_MAX with 2.147.483.647. So, it's more like
> 20.000s if we can keep the pace and have enough memory. Also, it's not only the
> mapping structures itself, it's also page tables, userspace structures, etc.
>
> Again, is the number of ~2.15 Billion mappings something we really need to worry
> about?
>
> I'm still not convinced about that. But I think we can also just cap GPUVM at,
> let's say, 1 Billion mappings?
>
>> The worst I've seen in a real world game was around 19k mappings, but that
>> doesn't mean that this here can't be exploited.
>>
>> What can be done is to keep one reference per VM_BO structure, but I think
>> per mapping is rather unrealistic.
>>
>> Regards,
>> Christian.
>>
>>
Did we get any resolution on this?

FWIW, my take on this is that it would be possible to get GPUVM to work 
both with and without internal refcounting; If with, the driver needs a 
vm close to resolve cyclic references, if without that's not necessary. 
If GPUVM is allowed to refcount in mappings and vm_bos, that comes with 
a slight performance drop but as Danilo pointed out, the VM lifetime 
problem iterating over a vm_bo's mapping becomes much easier and the 
code thus becomes easier to maintain moving forward. That convinced me 
it's a good thing.

Another issue Christian brought up is that something intended to be 
embeddable (a base class) shouldn't really have its own refcount. I 
think that's a valid point. If you at some point need to derive from 
multiple such structs each having its own refcount, things will start to 
get weird. One way to resolve that would be to have the driver's 
subclass provide get() and put() ops, and export a destructor for the 
base-class, rather than to have the base-class provide the refcount and 
a destructor  ops.

That would also make it possible for the driver to decide the context 
for the put() call: If the driver needs to be able to call put() from 
irq / atomic context but the base-class'es destructor doesn't allow 
atomic context, the driver can push freeing out to a work item if needed.

Finally, the refcount overflow Christian pointed out. Limiting the 
number of mapping sounds like a reasonable remedy to me.

But I think all of this is fixable as follow-ups if needed, unless I'm 
missing something crucial.

Just my 2 cents.

/Thomas

Powered by blists - more mailing lists