Message-ID: <22246322-d150-96df-58c6-436a70dd21da@redhat.com>
Date:   Wed, 6 Sep 2023 16:20:53 +0200
From:   Danilo Krummrich <dakr@...hat.com>
To:     Thomas Hellström (Intel) 
        <thomas_os@...pmail.org>
Cc:     airlied@...il.com, daniel@...ll.ch, matthew.brost@...el.com,
        thomas.hellstrom@...ux.intel.com, sarah.walker@...tec.com,
        donald.robson@...tec.com, boris.brezillon@...labora.com,
        christian.koenig@....com, faith.ekstrand@...labora.com,
        bskeggs@...hat.com, Liam.Howlett@...cle.com,
        nouveau@...ts.freedesktop.org, linux-kernel@...r.kernel.org,
        dri-devel@...ts.freedesktop.org
Subject: Re: [PATCH drm-misc-next 2/3] drm/gpuva_mgr: generalize
 dma_resv/extobj handling and GEM validation

On 9/1/23 14:10, Danilo Krummrich wrote:
> On Fri, Sep 01, 2023 at 07:59:21AM +0200, Thomas Hellström (Intel) wrote:
>>
>> On 8/31/23 21:07, Danilo Krummrich wrote:
>>> On Thu, Aug 31, 2023 at 06:53:01PM +0200, Thomas Hellström (Intel) wrote:
>>>> Hi,
>>>>
>>>> On 8/31/23 13:18, Danilo Krummrich wrote:
>>>>> On Thu, Aug 31, 2023 at 11:04:06AM +0200, Thomas Hellström (Intel) wrote:
>>>>>> Hi!
>>>>>>
>>>>>> On 8/30/23 17:00, Danilo Krummrich wrote:
>>>>>>> On Wed, Aug 30, 2023 at 03:42:08PM +0200, Thomas Hellström (Intel) wrote:
>>>>>>>> On 8/30/23 14:49, Danilo Krummrich wrote:
>>>>>>>>> Hi Thomas,
>>>>>>>>>
>>>>>>>>> thanks for having a look!
>>>>>>>>>
>>>>>>>>> On Wed, Aug 30, 2023 at 09:27:45AM +0200, Thomas Hellström (Intel) wrote:
>>>>>>>>>> Hi, Danilo.
>>>>>>>>>>
>>>>>>>>>> Some quick comments since I'm doing some Xe work in this area. Will probably
>>>>>>>>>> get back with more.
>>>>>>>>>>
>>>>>>>>>> On 8/20/23 23:53, Danilo Krummrich wrote:
>>>>> <snip>
>>>>>
>>>>>>>>>>> diff --git a/include/drm/drm_gpuva_mgr.h b/include/drm/drm_gpuva_mgr.h
>>>>>>>>>>> index ed8d50200cc3..693e2da3f425 100644
>>>>>>>>>>> --- a/include/drm/drm_gpuva_mgr.h
>>>>>>>>>>> +++ b/include/drm/drm_gpuva_mgr.h
>>>>>>>>>>> @@ -26,12 +26,16 @@
>>>>>>>>>>>         */
>>>>>>>>>>>        #include <linux/list.h>
>>>>>>>>>>> +#include <linux/dma-resv.h>
>>>>>>>>>>> +#include <linux/maple_tree.h>
>>>>>>>>>>>        #include <linux/rbtree.h>
>>>>>>>>>>>        #include <linux/types.h>
>>>>>>>>>>>        #include <drm/drm_gem.h>
>>>>>>>>>>> +#include <drm/drm_exec.h>
>>>>>>>>>>>        struct drm_gpuva_manager;
>>>>>>>>>>> +struct drm_gpuva_gem;
>>>>>>>>>>>        struct drm_gpuva_fn_ops;
>>>>>>>>>>>        /**
>>>>>>>>>>> @@ -140,7 +144,7 @@ struct drm_gpuva {
>>>>>>>>>>>        int drm_gpuva_insert(struct drm_gpuva_manager *mgr, struct drm_gpuva *va);
>>>>>>>>>>>        void drm_gpuva_remove(struct drm_gpuva *va);
>>>>>>>>>>> -void drm_gpuva_link(struct drm_gpuva *va);
>>>>>>>>>>> +void drm_gpuva_link(struct drm_gpuva *va, struct drm_gpuva_gem *vm_bo);
>>>>>>>>>>>        void drm_gpuva_unlink(struct drm_gpuva *va);
>>>>>>>>>>>        struct drm_gpuva *drm_gpuva_find(struct drm_gpuva_manager *mgr,
>>>>>>>>>>> @@ -240,15 +244,137 @@ struct drm_gpuva_manager {
>>>>>>>>>>>        	 * @ops: &drm_gpuva_fn_ops providing the split/merge steps to drivers
>>>>>>>>>>>        	 */
>>>>>>>>>>>        	const struct drm_gpuva_fn_ops *ops;
>>>>>>>>>>> +
>>>>>>>>>>> +	/**
>>>>>>>>>>> +	 * @d_obj: Dummy GEM object; used internally to pass the GPU VMs
>>>>>>>>>>> +	 * dma-resv to &drm_exec.
>>>>>>>>>>> +	 */
>>>>>>>>>>> +	struct drm_gem_object d_obj;
>>>>>>>>>>> +
>>>>>>>>>>> +	/**
>>>>>>>>>>> +	 * @resv: the &dma_resv for &drm_gem_objects mapped in this GPU VA
>>>>>>>>>>> +	 * space
>>>>>>>>>>> +	 */
>>>>>>>>>>> +	struct dma_resv *resv;
>>>>>>>>>>> +
>>>>>>>>>>> +	/**
>>>>>>>>>>> +	 * @exec: the &drm_exec helper to lock external &drm_gem_objects
>>>>>>>>>>> +	 */
>>>>>>>>>>> +	struct drm_exec exec;
>>>>>>>>>>> +
>>>>>>>>>>> +	/**
>>>>>>>>>>> +	 * @mt_ext: &maple_tree storing external &drm_gem_objects
>>>>>>>>>>> +	 */
>>>>>>>>>>> +	struct maple_tree mt_ext;
>>>>>>>>>> Why are you using a maple tree here? Insertion and removal is O(log(n))
>>>>>>>>>> instead of O(1) for a list?
>>>>>>>>>>
>>>>>>>>> Having a list of drm_gem_objects directly wouldn't work, as multiple GPU-VMs
>>>>>>>>> could have mappings of the same extobj.
>>>>>>>>>
>>>>>>>>> I considered using the VM_BO abstraction (struct drm_gpuva_gem) as list entry
>>>>>>>>> instead, which also seems to be the obvious choice. However, there is a locking
>>>>>>>>> conflict.
>>>>>>>>>
>>>>>>>>> A drm_gem_object keeps a list of drm_gpuva_gems, while each drm_gpuva_gem keeps
>>>>>>>>> a list of drm_gpuvas. Both lists are either protected with the dma-resv lock of
>>>>>>>>> the corresponding drm_gem_object, or with an external lock provided by the
>>>>>>>>> driver (see drm_gem_gpuva_set_lock()). The latter is used by drivers performing
>>>>>>>>> changes on the GPUVA space directly from the fence signalling path.
>>>>>>>>>
>>>>>>>>> Now, similar to what drm_gpuva_link() and drm_gpuva_unlink() are doing already,
>>>>>>>>> we'd want to add a drm_gpuva_gem to the extobj list for the first mapping being
>>>>>>>>> linked and we'd want to remove it for the last one being unlinked.
>>>>>>>>>
>>>>>>>>> (Actually we'd want to add the drm_gpuva_gem object to the extobj list even
>>>>>>>>> before, because otherwise we'd not acquire it's dma-resv lock of this GEM object
>>>>>>>>> through drm_gpuva_manager_lock(). But that's trival, we could do that when we
>>>>>>>>> create the drm_gpuva_gem, which we need to do anyways.)
>>>>>>>>>
>>>>>>>>> Anyway, we'd probably want to keep removing the drm_gpuva_gem from the extobj
>>>>>>>>> list from drm_gpuva_unlink() when the last mapping of this BO is unlinked. In
>>>>>>>>> order to do so, we'd (as discussed above) either need to hold the outer GPU-VM
>>>>>>>>> lock or the GPU-VMs dma-resv lock. Both would be illegal in the case
>>>>>>>>> drm_gpuva_unlink() is called from within the fence signalling path. For drivers
>>>>>>>>> like XE or Nouveau, we'd at least need to make sure to not mess up the locking
>>>>>>>>> hierarchy of GPU-VM lock and dma-resv lock of the corresponding BO.
>>>>>>>>>
>>>>>>>>> Considering all that, I thought it's probably better to track extobjs separate
>>>>>>>>> from the drm_gpuva_gem, hence the maple tree choice.
>>>>>>>> Hm. OK, in Xe we're having a list of the xe_vmas (drm_gpuvas) that point to
>>>>>>>> external objects, or in the case of multiple mappings to the same gem
>>>>>>>> object, only one of the drm_gpuvas is in the list. These are protected by
>>>>>>>> the GPU-VM lock. I don't see a problem with removing those from the fence
>>>>>>>> signalling path, though?
>>>>>>> I intentionally tried to avoid keeping a list of drm_gpuvas to track extobjs,
>>>>>>> since this is generic code I don't know how much mappings of an external object
>>>>>>> the corresponding driver potentially creates. This could become a pretty large
>>>>>>> list to iterate. Another reason was, that I want to keep the drm_gpuva structure
>>>>>>> as small as possible, hence avoiding another list_head.
>>>>>> Yes, the list might be pretty large, but OTOH you never iterate to access a
>>>>>> single list element. When you need to iterate the whole list you need to do
>>>>>> that regardless of the data structure used. As for the list head, it might
>>>>>> perhaps be aliased (union) with an upcoming userptr list head?
>>>>>>
>>>>> Oh, I did not mean that I'm concerned about the size of a list of extobjs in
>>>>> general, that would indeed be the same for every data structure chosen. But I
>>>>> would be concerned about keeping a list of *all* mappings being backed by an
>>>>> extobj.
>>>>>
>>>>>>> Now, it sounds like in XE you're doing some kind of optimization just keeping a
>>>>>>> single mapping of an extobj in the list? How do you know when to remove it? What
>>>>>>> if the mapping from the extobj list gets unmapped, but there is still another
>>>>>>> one left in the GPU-VM being backed by the same BO?
>>>>>> When removing from the lists, we iterate through the object's list of vmas,
>>>>>> and if there is one matching the same vm, we replace the old one with the
>>>>>> new one. A similar iteration is done when adding to avoid adding one that is
>>>>>> already on the list.
>>>>> I see, but wouldn't this be O(n) on insertion and O(m) on removal of an extobj,
>>>>> while using the maple tree is O(log(n))?
>>>> No, insertion and removal is O(m) where m is the number of vms the object is
>>>> currently bound to. Typically a very small number.
>>> Ok, my guess was that on insertion you'd actually walk the extobj list and see
>>> if there's a vma backed by the same BO already, while on removal you said you're
>>> walking the BO's vma list. So I guess on insertion you're also walking the BO's
>>> vma list and see if there's already a mapping for this VM?
>>>
>>> In your case that might make sense if you expect the extobj list to be larger
>>> than the BO's vma list typically. In general I don't think this is true.
>>
>> I think we're then optimizing for different scenarios. Our compute driver
>> will use mostly external objects only, and if shared, I don't forsee them
>> bound to many VMs. What saves us currently here is that in compute mode we
>> only really traverse the extobj list after a preempt fence wait, or when a
>> vm is using a new context for the first time. So vm's extobj list is pretty
>> large. Each bo's vma list will typically be pretty small.
> 
> Admittedly, I did not had in mind VMs where every GEM is an extobj. However,
> especially for iterating a lot of extobjs a maple tree should perform better
> than a list.
> 
>>
>> Another reason for us to use the list is that one possible, but not yet
>> implemented, workaround for this is the "vm fence", which when attached to
>> external bos pulls them off the extobj list and on "enable_signalling()"
>> splices its sublist of external bos back, and then snapshots the vm's
>> dma_resv and waits for all its fences. (The idea is that it should very
>> seldom be waited for in practice, and largely eliminate the extobj
>> handling). Here a list is an ideal data structure for list removal and
>> splicing. TBH we really want to avoid this optimization but we need to see
>> how bad extobj handling ends up in practice for the compute drivers.
> 
> If you end up doing this I highly doubt it'd make sense to use the GPUVA
> manager for that, even if it would implement extobjs as a list of drm_gpuva_gems
> (VM_BOs). It'd probably be a mess. When you remove extobjs from the GPUVA
> manager, not because they're actually gone, but because you want to keep them
> separate, you'd need to make sure to keep the drm_gpuva_gem structure alive,
> which means you would need to increase the GPUVA managers refcount for extobjs
> manually. You could probably also just "steal" them silently, but that'd be
> quite nasty as well.
> 
>>
>>
>>>
>>>>>>> Although assuming that's a no-go for GPUVA wouldn't an XArray be a better
>>>>>>> choice, keeping O(1)?
>>>>>>> When tracking extobjs, the address of the drm_gem_object is the key while the
>>>>>>> reference count is the value. I was thinking of an XArray as well, but I was
>>>>>>> worried that the corresponding indices could be too much distributed for an
>>>>>>> XArray to still be efficient. Now that I think about it, it's probably not that
>>>>>>> bad.
>>>>>>>
>>>>>>> Btw., while I agree trying to make things as efficient as possible, what is the
>>>>>>> magnitue for extobjs to be tracked, do we need to worry about the O(log(n))?
>>>>>> Not sure yet, TBH, but I think one of our UMDs can only use external object,
>>>>>> because they don't know at creation time which ones need exporting. However
>>>>>> if this turns out to be too bad, there are various flavours of "clever but
>>>>>> complicated" optimizations that we could think of to reduce the list size.
>>>>>> Still in our case, we opted for the vma list head for now.
>>>>> Considering the above, I would guess that if your current approach is good
>>>>> enough, a maple tree will work as well.
>>>> Hmm, Yeah it's probably a bikeshed since each drm_exec builds a realloced
>>>> array of all external objects on each exec.
>>> I did a quick sketchy benchmark, which is probably good enough. In a maple tree
>>> with 0xFFFF - 1 existing entries insertion of a random (non-existant) entry
>>> took on average ~530ns over 1k iterations.
>>>
>>> The average insertion time for each entry to build up a tree with 0xFFFF - 1
>>> entries in the first place was ~1.3us. That's expected since it should hit
>>> memory allocations more often than the previous one. The maximum peak was ~10us.
>>> Inserting already existing entries took ~300ns.
>>>
>>> That's probably good enough.
>>
>> That's hard to tell because we have nothing to compare with. For drm_exec,
>> Christian chose a realloced array because of linked list cache locality
>> issues, and Xarray locking requirements causing measurable performance
>> issues. Wouldn't a maple tree suffer from both of these?
> 
> Maple tree was designed for cache efficient traversal and to replace rbtree and
> linked lists in MM because of their lack of cache efficiency. (That's also why
> it is really unfortunate that we couldn't use maple tree for VMA tracking in the
> GPUVA manager.)
> 
> In terms of locking, I can only imagine an issue because Xarray always seems to
> use RCU and hence you can't get rid of some grace period latency? Otherwise it
> should just be a spinlock.
> 
> @Christian: Or was there a different issue?
> 
> Maple tree can disable RCU entirely [1] AFAIK, hence likely we can avoid such an
> issue.
> 
> [1] https://elixir.bootlin.com/linux/latest/source/include/linux/maple_tree.h#L612
> 
>>
>> In any case if you go for the maple tree would it be possible to hide the
>> implementation in a way as to make it not too hard to replace if real-world
>> workloads prove it necessary?
> 
> Of course, I would want to do that anyway.

Just a heads-up. It looks like (with the help of Boris) I can come up with a solution
everyone should be happy with. I think we can move extobjs to a list and API and locking
wise just do (almost) everything as we would as if there wouldn't be the use-case of
updating the VA space with direct callback from the fence signaling path.

Drivers doing that can simply schedule work to call drm_gpuva_unlink() from the fence
signaling path for explicit unmaps to avoid locking issues.

Drivers relying on VA space updates in the IOCTL already aren't affected at all.

I'll probably send out a v2 today or tomorrow.

- Danilo

> 
>>
>>>
>>>>> Otherwise, if you want, I could do some experiments with Xarray and see how
>>>>> that works out compared to using a maple tree.
>>>>>
>>>>> Btw. another nice thing about using Xarray or maple tree for that is that
>>>>> drivers updating the VA space from the fence signalling path don't need to
>>>>> hold a GPU-VM lock to update the extobj list. Actually, they might not need
>>>>> a GPU-VM lock at all.
>>>> I still don't follow why drivers would want to do that. Isn't the VA space /
>>>> fence object list always updated sync from the IOCTL?
>>> For the extobj list I don't see any advantage not doing that in the IOCTL right
>>> away. For the VA space there are a few advantages doing it in the fence
>>> signalling path.
>>>
>>> (1) No need to allocate drm_gpuva_ops at all. For a given map / unmap request
>>>       the driver can receive the callbacks for map / remap / unmap directly.
>>> (2) No need to unwind VA space updates on failure, also no need for any other
>>>       unwind tricks.
>>> (3) Synchronous bind jobs can be injected at any point of time and don't need to
>>>       be queued up in the scheduler to preserve ordering.
>>> (4) Potentially less error prone ressource management. Although, I admit partly
>>>       this is just the consequence of (1) and (2).
>>>
>>> Actually, once I get the page table management prepared for that I'd like to
>>> move Nouveau over this approach.
>>
>> OK. I guess I need to look at the resulting implementation to fully digest
>> this.
>>
>> Thanks,
>>
>> Thomas
>>
>>
>>>
>>>> /Thomas
>>>>
>>>>
>>>>>> /Thomas
>>>>>>
>>>>>>
>>>>>>>>>>> +
>>>>>>>>>>> +	/**
>>>>>>>>>>> +	 * @evict: structure holding the evict list and evict list lock
>>>>>>>>>>> +	 */
>>>>>>>>>>> +	struct {
>>>>>>>>>>> +		/**
>>>>>>>>>>> +		 * @list: &list_head storing &drm_gem_objects currently being
>>>>>>>>>>> +		 * evicted
>>>>>>>>>>> +		 */
>>>>>>>>>>> +		struct list_head list;
>>>>>>>>>>> +
>>>>>>>>>>> +		/**
>>>>>>>>>>> +		 * @lock: spinlock to protect the evict list against concurrent
>>>>>>>>>>> +		 * insertion / removal of different &drm_gpuva_gems
>>>>>>>>>>> +		 */
>>>>>>>>>>> +		spinlock_t lock;
>>>>>>>>>>> +	} evict;
>>>>>>>>>>>        };
>>>>>>>>>>>        void drm_gpuva_manager_init(struct drm_gpuva_manager *mgr,
>>>>>>>>>>> +			    struct drm_device *drm,
>>>>>>>>>>>        			    const char *name,
>>>>>>>>>>>        			    u64 start_offset, u64 range,
>>>>>>>>>>>        			    u64 reserve_offset, u64 reserve_range,
>>>>>>>>>>>        			    const struct drm_gpuva_fn_ops *ops);
>>>>>>>>>>>        void drm_gpuva_manager_destroy(struct drm_gpuva_manager *mgr);
>>>>>>>>>>> +/**
>>>>>>>>>>> + * DRM_GPUVA_EXEC - returns the &drm_gpuva_managers &drm_exec instance
>>>>>>>>>>> + * @mgr: the &drm_gpuva_managers to return the &drm_exec instance for
>>>>>>>>>>> + */
>>>>>>>>>>> +#define DRM_GPUVA_EXEC(mgr)	&(mgr)->exec
>>>>>>>>>> A struct ww_acquire_ctx and thus a drm_exec is fundamentally per task and
>>>>>>>>>> should typically be allocated on the stack. Otherwise you'd need to protect
>>>>>>>>>> the mgr->exec member with an exclusive lock throughout the locking process,
>>>>>>>>>> and that's not what we want.
>>>>>>>>> Oh, good point. I think it works in Nouveau, because there it's implicitly
>>>>>>>>> protected with the job submission lock.
>>>>>>>>>
>>>>>>>>>> Did you consider subclassing a drm_exec for drm_gpuva purposes and add
>>>>>>>>>> needed ops to it: Like so:
>>>>>>>>> That's a good idea, will take this into V2.
>>>>>>>> Actually, I'm not fully sure that was a good idea: I've now have a working
>>>>>>>> version of Xe ported over to drm_exec, having these helpers in mind and with
>>>>>>>> the intention to start using them as they mature. What I found, though is
>>>>>>>> that open-coding the drm_exec loop is not all that bad, but that building
>>>>>>>> blocks that can be called from within the loop are useful:
>>>>>>>>
>>>>>>>> Like the drm_gpuva_prepare_objects() and an imaginary
>>>>>>>> drm_gpuva_prepare_gpuva() that locks the vm resv and the resv of the object
>>>>>>>> (if different and the gpuva points to the object. And
>>>>>>>> drm_gpuva_prepare_array() although we don't use it within Xe. That means you
>>>>>>>> can use these building blocks like helpers and avoid the fn() callback by
>>>>>>>> instead open-coding.
>>>>>>>>
>>>>>>>> But I guess YMMV.
>>>>>>> That's exactly why those building blocks are exported, I already had in mind
>>>>>>> that there might be drivers which still want to open-code the drm_exec loop,
>>>>>>> while others might just want a simple interface to lock everything.
>>>>>>>
>>>>>>> I still think it is a good idea, but I'd keep that as simple as possible. And
>>>>>>> for everything else just let the driver open-code it and use the "building
>>>>>>> blocks" - will also expand the bulding blocks to what you mentioned above.
>>>>>>>
>>>>>>>>>> struct drm_gpuva_exec_ops {
>>>>>>>>>>          int (*fn) (struct drm_gpuva_exec *exec, int num_fences);
>>>>>>>>> Is this the fn argument from drm_gpuva_manager_lock_extra()?
>>>>>>>>>
>>>>>>>>>>          int (*bo_validate) (struct drm_gpuva_exec *exec, struct drm_gem_object
>>>>>>>>>> *obj);
>>>>>>>>> I guess we could also keep that within the drm_gpuva_fn_ops? This should always
>>>>>>>>> be the same callback, right?
>>>>>>>>>
>>>>>>>>>> };
>>>>>>>>>>
>>>>>>>>>> struct drm_gpuva_exec {
>>>>>>>>>>          const struct drm_gpuva_exec_ops *ops;
>>>>>>>>>>          struct drm_exec exec;
>>>>>>>>>>          struct drm_gpuva_manager *mgr;
>>>>>>>>>> };
>>>>>>>>>>
>>>>>>>>>> Although I'd actually expect bo_validate to be part of fn in the typical
>>>>>>>>>> case. The drm_gpuva_exec would then be allocated by the caller on the stack.
>>>>>>>>> This doesn't sound like my assumption about fn() above is correct.
>>>>>>>> Well one important thing in our conversion is that ttm_bo_validate () needs
>>>>>>>> to be in the until_all_locked() loop. We want to be able soon to use
>>>>>>>> sleeping locks for eviction, so a xe_bo_validate() would, at least
>>>>>>>> temporarily, add locked objects to the drm_exec list of locked objects. That
>>>>>>>> means everything that may end up calling validate deep within the call chain
>>>>>>>> needs to be part of the until_all_locked() loop, so our
>>>>>>>> drm_gpuva_manager_lock_extra() fn callback would include those validates and
>>>>>>>> look different all the time. Hence that's why open-coding isn't all that
>>>>>>>> bad...
>>>>>>> Oh, I see. You indeed want to call validate() from within until_all_locked().
>>>>>>>
>>>>>>>> /Thomas
>>>>>>>>
>>>>>>>>
>>>>> <snip>
>>

Powered by blists - more mailing lists