On Tue, Mar 03, 2020 at 11:42:22AM +0900, David Stevens wrote:
> > cmd_p->hdr.ctx_id =
> >
> > Before this completion of this hypercall, this resource can be
> > considered context local, while afterward it can be considered
> > "exported".
>
> Maybe I'm misunderstanding render contexts, but exporting a resource
> doesn't seem related to render contexts.
It isn't indeed. Binding resources to contexts might need dma-buf
imports/exports on the host side, but that is another story and not
related to dma-buf exports inside the guest.
cheers,
Gerd
Hi,
> + if (vgdev->has_resource_assign_uuid) {
> + spin_lock(&vgdev->resource_export_lock);
> + if (bo->uuid_state == UUID_NOT_INITIALIZED) {
> + bo->uuid_state = UUID_INITIALIZING;
> + needs_init = true;
> + }
> + spin_unlock(&vgdev->resource_export_lock);
> +
> + if (needs_init) {
> + ret = virtio_gpu_cmd_resource_assign_uuid(vgdev, bo);
You can submit a fenced command, then wait on the fence here. Removes
the need for UUID_INITIALIZING.
Also note that this function will be called only once, on the first
export. When exporting the same object again drm will simply reuse
the existing dmabuf. You can drop UUID_NOT_INITIALIZED and needs_init.
So you are left with only two uuid_state states. You could turn uuid
into a pointer, so it gets only allocated when needed. Also uuid ==
NULL can be used for "uuid not available" then.
cheers,
Gerd
On Mon, Mar 02, 2020 at 09:15:21PM +0900, David Stevens wrote:
> This change adds a new dma-buf operation that allows dma-bufs to be used
> by virtio drivers to share exported objects. The new operation allows
> the importing driver to query the exporting driver for the UUID which
> identifies the underlying exported object.
>
> Signed-off-by: David Stevens <stevensd(a)chromium.org>
> ---
> drivers/dma-buf/dma-buf.c | 14 ++++++++++++++
> include/linux/dma-buf.h | 22 ++++++++++++++++++++++
> 2 files changed, 36 insertions(+)
>
> diff --git a/drivers/dma-buf/dma-buf.c b/drivers/dma-buf/dma-buf.c
> index d4097856c86b..a04632284ec2 100644
> --- a/drivers/dma-buf/dma-buf.c
> +++ b/drivers/dma-buf/dma-buf.c
> @@ -1158,6 +1158,20 @@ void dma_buf_vunmap(struct dma_buf *dmabuf, void *vaddr)
> }
> EXPORT_SYMBOL_GPL(dma_buf_vunmap);
>
> +#ifdef CONFIG_VIRTIO
> +int dma_buf_get_uuid(struct dma_buf *dmabuf, uuid_t *uuid)
Hmm, I think I would drop the #ifdef
cheers,
Gerd
On Thu, 27 Feb 2020 13:38:03 -0800 Cong Wang <xiyou.wangcong(a)gmail.com> wrote:
> On Tue, Feb 25, 2020 at 5:54 PM Andrew Morton <akpm(a)linux-foundation.org> wrote:
> >
> > On Tue, 25 Feb 2020 12:44:46 -0800 Cong Wang <xiyou.wangcong(a)gmail.com> wrote:
> >
> > > dma-buff name can be set via DMA_BUF_SET_NAME ioctl, but once set
> > > it never gets freed.
> > >
> > > Free it in dma_buf_release().
> > >
> > > ...
> > >
> > > --- a/drivers/dma-buf/dma-buf.c
> > > +++ b/drivers/dma-buf/dma-buf.c
> > > @@ -108,6 +108,7 @@ static int dma_buf_release(struct inode *inode, struct file *file)
> > > dma_resv_fini(dmabuf->resv);
> > >
> > > module_put(dmabuf->owner);
> > > + kfree(dmabuf->name);
> > > kfree(dmabuf);
> > > return 0;
> > > }
> >
> > ow. Is that ioctl privileged?
>
> It looks unprivileged to me, as I don't see capable() called along
> the path.
>
OK, thanks. I added cc:stable to my copy.
Am 23.02.20 um 17:54 schrieb Thomas Hellström (VMware):
> On 2/23/20 4:45 PM, Christian König wrote:
>> Am 21.02.20 um 18:12 schrieb Daniel Vetter:
>>> [SNIP]
>>> Yeah the Great Plan (tm) is to fully rely on ww_mutex slowly
>>> degenerating
>>> into essentially a global lock. But only when there's actual contention
>>> and thrashing.
>>
>> Yes exactly. A really big problem in TTM is currently that we drop
>> the lock after evicting BOs because they tend to move in again
>> directly after that.
>>
>> From practice I can also confirm that there is exactly zero benefit
>> from dropping locks early and reacquire them for example for the VM
>> page tables. That's just makes it more likely that somebody needs to
>> roll back and this is what we need to avoid in the first place.
>
> If you have a benchmarking setup available it would be very
> interesting for future reference to see how changing from WD to WW
> mutexes affects the roll back frequency. WW is known to cause
> rollbacks much less frequently but there is more work associated with
> each rollback.
Not of hand. To be honest I still have a hard time to get a grip on the
difference between WD and WW from the algorithm point of view. So I
can't judge that difference at all.
>> Contention on BO locks during command submission is perfectly fine as
>> long as this is as lightweight as possible while we don't have
>> trashing. When we have trashing multi submission performance is best
>> archived to just favor a single process to finish its business and
>> block everybody else.
>
> Hmm. Sounds like we need a per-manager ww_rwsem protecting manager
> allocation, taken in write-mode then there's thrashing. In read-mode
> otherwise. That would limit the amount of "unnecessary" locks we'd
> have to keep and reduce unwanted side-effects, (see below):
Well per-manager (you mean per domain here don't you?) doesn't sound
like that useful because we rarely use only one domain, but I'm actually
questioning for quite a while if the per BO lock scheme was the right
approach.
See from the performance aspect the closest to ideal solution I can
think of would be a ww_rwsem per user of a resource.
In other words we don't lock BOs, but instead a list of all their users
and when you want to evict a BO you need to walk that list and inform
all users that the BO will be moving.
During command submission you then have the fast path which rather just
grabs the read side of the user lock and check if all BOs are still in
the expected place.
If some BOs were evicted you back off and start the slow path, e.g.
maybe even copy additional data from userspace then grab the write side
of the lock etc.. etc...
That approach is similar to what we use in amdgpu with the per-VM BOs,
but goes a step further. Problem is that we are so used to per BO locks
in the kernel that this is probably not doable any more.
>> Because of this I would actually vote for forbidding to release
>> individual ww_mutex() locks in a context.
>
> Yes, I see the problem.
>
> But my first reaction is that this might have undersirable
> side-effects. Let's say somebody wanted to swap the evicted BOs out?
Please explain further, I off hand don't see the problem here.
In general I actually wanted to re-work TTM in a way that BOs in the
SYSTEM/SWAPABLE domain are always backed by a shmem file instead of the
struct page array we currently have.
> Or cpu-writes to them causing faults, that might also block the
> mm_sem, which in turn blocks hugepaged?
Mhm, I also only have a higher level view how hugepaged works so why
does it grabs the mm_sem on the write side?
Thanks,
Christian.
>
> Still it's a fairly simple solution to a problem that seems otherwise
> hard to solve efficiently.
>
> Thanks,
> Thomas
>
>
>>
>> Regards,
>> Christian.
>>
>>> -Daniel
>
>
On Wed, Feb 26, 2020 at 12:56:58PM +0900, David Stevens wrote:
> On Tue, Feb 25, 2020 at 3:10 PM Gerd Hoffmann <kraxel(a)redhat.com> wrote:
> >
> > How about dma_buf_{get,set}_uuid, simliar to dma_buf_set_name?
>
> While I'm not opposed to such an API, I'm also hesitant to make
> changes to the dma-buf API for a single use case.
See virtio-wayland discussion. I expect we will see more cases show up.
Maybe this should even go one level up, to struct file.
cheers,
Gerd
On Tue, 25 Feb 2020 12:44:46 -0800 Cong Wang <xiyou.wangcong(a)gmail.com> wrote:
> dma-buff name can be set via DMA_BUF_SET_NAME ioctl, but once set
> it never gets freed.
>
> Free it in dma_buf_release().
>
> ...
>
> --- a/drivers/dma-buf/dma-buf.c
> +++ b/drivers/dma-buf/dma-buf.c
> @@ -108,6 +108,7 @@ static int dma_buf_release(struct inode *inode, struct file *file)
> dma_resv_fini(dmabuf->resv);
>
> module_put(dmabuf->owner);
> + kfree(dmabuf->name);
> kfree(dmabuf);
> return 0;
> }
ow. Is that ioctl privileged?
Am 23.02.20 um 12:56 schrieb Pan, Xinhui:
> If shared fence list is not empty, even we want to test all fences, excl fence is ignored.
> That is abviously wrong, so fix it.
Yeah that is a known issue and I completely agree with you, but other
disagree.
See the shared fences are meant to depend on the exclusive fence. So all
shared fences must finish only after the exclusive one has finished as well.
The problem now is that for error handling this isn't necessary true. In
other words when a shared fence completes with an error it is perfectly
possible that he does this before the exclusive fence is finished.
I'm trying to convince Daniel that this is a problem for years :)
Regards,
Christian.
>
> Signed-off-by: xinhui pan <xinhui.pan(a)amd.com>
> ---
> drivers/dma-buf/dma-resv.c | 9 +++++----
> 1 file changed, 5 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/dma-buf/dma-resv.c b/drivers/dma-buf/dma-resv.c
> index 4264e64788c4..44dc64c547c6 100644
> --- a/drivers/dma-buf/dma-resv.c
> +++ b/drivers/dma-buf/dma-resv.c
> @@ -632,14 +632,14 @@ static inline int dma_resv_test_signaled_single(struct dma_fence *passed_fence)
> */
> bool dma_resv_test_signaled_rcu(struct dma_resv *obj, bool test_all)
> {
> - unsigned seq, shared_count;
> + unsigned int seq, shared_count, left;
> int ret;
>
> rcu_read_lock();
> retry:
> ret = true;
> shared_count = 0;
> - seq = read_seqcount_begin(&obj->seq);
> + left = seq = read_seqcount_begin(&obj->seq);
>
> if (test_all) {
> unsigned i;
> @@ -647,7 +647,7 @@ bool dma_resv_test_signaled_rcu(struct dma_resv *obj, bool test_all)
> struct dma_resv_list *fobj = rcu_dereference(obj->fence);
>
> if (fobj)
> - shared_count = fobj->shared_count;
> + left = shared_count = fobj->shared_count;
>
> for (i = 0; i < shared_count; ++i) {
> struct dma_fence *fence = rcu_dereference(fobj->shared[i]);
> @@ -657,13 +657,14 @@ bool dma_resv_test_signaled_rcu(struct dma_resv *obj, bool test_all)
> goto retry;
> else if (!ret)
> break;
> + left--;
> }
>
> if (read_seqcount_retry(&obj->seq, seq))
> goto retry;
> }
>
> - if (!shared_count) {
> + if (!left) {
> struct dma_fence *fence_excl = rcu_dereference(obj->fence_excl);
>
> if (fence_excl) {