[PATCH] dma-fence: allow dma fence to have their own lock

[PATCH] dma-fence: allow dma fence to have their own lock

[Sergey Senozhatsky]

	RFC

	I don't have a good name for this yet and I did not spend
	any time on documentataion (for that reason)

We create fences (out fences) as part of operations execution, which
are short-lived objects, we want to release all memory after operation
execution is completed or when operation gets cancelled/deleted via
ioctl().

This creates a bit of a problem. DMA fences are refcounted objects and
exporter never knows when importer imports a fence or puts its refcount,
so exporter never knows when fence will be destoyed, which should not
be a problem for refcounted objects, but here comes the twist...

	operation A - creates and exports out fence X
	... user-space imports fence X
	operation A - finishes execution, signals fence X
	kfree operation A, put dma_fence

DMA fences are designed to borrow spinlock that DMA fences use to
protect struct dma_fence members:

	struct dma_fence {
	        spinlock_t *lock;

	        const struct dma_fence_ops *ops;
		.....
	};

	void dma_fence_init(struct dma_fence *fence,
			const struct dma_fence_ops *ops,
			spinlock_t *lock,
			u64 context,
			u64 seqno);

So the `lock` should have at least same lifespan as the DMA fence
that borrows it, which is impossible to guarantee in our case. When
we kfree operation A struct we also kfree ->lock that operation
lends to DMA fence, which outlives operation A (depending on what
fence importers do and when they drop imported fence refcount).

This patch adds a new memnber to struct dma_fence: __lock_inplace.
Which is a lock that DMA fence will use to protect its own data when
it cannot reliably borrow a lock from the outside object.

I also had a patch that puts inplace and borrowed locks to an unnamed
uninon and adds one more dma_fence_flag_bits to distinguish between
fences with borrowed and inplace locks

	struct dma_fence {
		uninon {
			spinlock_t *lock;
			spinlock_t __lock_inplace;
		};
		...
	};

And then instead of locking/unlocking ->lock directly we would use
dma_fence_lock_irqsave()/dma_fence_unlock_irqrestore() macros which
would check fence flags and either use borrowed lock or inplace lock.
But after seeing how owten drivers directly access fence ->lock I
decided to scratch that approach and just add extra spinlock member.

Not-Yet-Signed-off-by: Sergey Senozhatsky <senozhatsky@chromium.org>
---
 drivers/dma-buf/dma-fence.c | 10 ++++++++++
 include/linux/dma-fence.h   |  6 ++++++
 2 files changed, 16 insertions(+)

diff --git a/drivers/dma-buf/dma-fence.c b/drivers/dma-buf/dma-fence.c
index 066400ed8841..7ae40b8adb73 100644
--- a/drivers/dma-buf/dma-fence.c
+++ b/drivers/dma-buf/dma-fence.c
@@ -958,3 +958,13 @@ dma_fence_init(struct dma_fence *fence, const struct dma_fence_ops *ops,
 	trace_dma_fence_init(fence);
 }
 EXPORT_SYMBOL(dma_fence_init);
+
+void dma_fence_inplace_lock_init(struct dma_fence *fence,
+				 const struct dma_fence_ops *ops,
+				 u64 context, u64 seqno)
+{
+	spin_lock_init(&fence->__lock_inplace);
+
+	dma_fence_init(fence, ops, &fence->__lock_inplace, context, seqno);
+}
+EXPORT_SYMBOL(dma_fence_inplace_lock_init);
diff --git a/include/linux/dma-fence.h b/include/linux/dma-fence.h
index 1ea691753bd3..6b15a0d2eccf 100644
--- a/include/linux/dma-fence.h
+++ b/include/linux/dma-fence.h
@@ -64,6 +64,8 @@ struct dma_fence_cb;
  */
 struct dma_fence {
 	spinlock_t *lock;
+	spinlock_t __lock_inplace;
+
 	const struct dma_fence_ops *ops;
 	/*
 	 * We clear the callback list on kref_put so that by the time we
@@ -262,6 +264,10 @@ struct dma_fence_ops {
 void dma_fence_init(struct dma_fence *fence, const struct dma_fence_ops *ops,
 		    spinlock_t *lock, u64 context, u64 seqno);
 
+void dma_fence_inplace_lock_init(struct dma_fence *fence,
+				 const struct dma_fence_ops *ops,
+				 u64 context, u64 seqno);
+
 void dma_fence_release(struct kref *kref);
 void dma_fence_free(struct dma_fence *fence);
 void dma_fence_describe(struct dma_fence *fence, struct seq_file *seq);
-- 
2.36.1.124.g0e6072fb45-goog

[PATCH] dma-fence: allow dma fence to have their own lock

[Sergey Senozhatsky]

	RFC

	I don't have a good name for this yet and I did not spend
	any time on documentataion (for that reason)

We create fences (out fences) as part of operations execution, which
are short-lived objects, we want to release all memory after operation
execution is completed or when operation gets cancelled/deleted via
ioctl().

This creates a bit of a problem. DMA fences are refcounted objects and
exporter never knows when importer imports a fence or puts its refcount,
so exporter never knows when fence will be destoyed, which should not
be a problem for refcounted objects, but here comes the twist...

	operation A - creates and exports out fence X
	... user-space imports fence X
	operation A - finishes execution, signals fence X
	kfree operation A, put dma_fence

DMA fences are designed to borrow spinlock that DMA fences use to
protect struct dma_fence members:

	struct dma_fence {
	        spinlock_t *lock;

	        const struct dma_fence_ops *ops;
		.....
	};

	void dma_fence_init(struct dma_fence *fence,
			const struct dma_fence_ops *ops,
			spinlock_t *lock,
			u64 context,
			u64 seqno);

So the `lock` should have at least same lifespan as the DMA fence
that borrows it, which is impossible to guarantee in our case. When
we kfree operation A struct we also kfree ->lock that operation
lends to DMA fence, which outlives operation A (depending on what
fence importers do and when they drop imported fence refcount).

This patch adds a new memnber to struct dma_fence: __lock_inplace.
Which is a lock that DMA fence will use to protect its own data when
it cannot reliably borrow a lock from the outside object.

I also had a patch that puts inplace and borrowed locks to an unnamed
uninon and adds one more dma_fence_flag_bits to distinguish between
fences with borrowed and inplace locks

	struct dma_fence {
		uninon {
			spinlock_t *lock;
			spinlock_t __lock_inplace;
		};
		...
	};

And then instead of locking/unlocking ->lock directly we would use
dma_fence_lock_irqsave()/dma_fence_unlock_irqrestore() macros which
would check fence flags and either use borrowed lock or inplace lock.
But after seeing how owten drivers directly access fence ->lock I
decided to scratch that approach and just add extra spinlock member.

Not-Yet-Signed-off-by: Sergey Senozhatsky <senozhatsky@chromium.org>
---
 drivers/dma-buf/dma-fence.c | 10 ++++++++++
 include/linux/dma-fence.h   |  6 ++++++
 2 files changed, 16 insertions(+)

diff --git a/drivers/dma-buf/dma-fence.c b/drivers/dma-buf/dma-fence.c
index 066400ed8841..7ae40b8adb73 100644
--- a/drivers/dma-buf/dma-fence.c
+++ b/drivers/dma-buf/dma-fence.c
@@ -958,3 +958,13 @@ dma_fence_init(struct dma_fence *fence, const struct dma_fence_ops *ops,
 	trace_dma_fence_init(fence);
 }
 EXPORT_SYMBOL(dma_fence_init);
+
+void dma_fence_inplace_lock_init(struct dma_fence *fence,
+				 const struct dma_fence_ops *ops,
+				 u64 context, u64 seqno)
+{
+	spin_lock_init(&fence->__lock_inplace);
+
+	dma_fence_init(fence, ops, &fence->__lock_inplace, context, seqno);
+}
+EXPORT_SYMBOL(dma_fence_inplace_lock_init);
diff --git a/include/linux/dma-fence.h b/include/linux/dma-fence.h
index 1ea691753bd3..6b15a0d2eccf 100644
--- a/include/linux/dma-fence.h
+++ b/include/linux/dma-fence.h
@@ -64,6 +64,8 @@ struct dma_fence_cb;
  */
 struct dma_fence {
 	spinlock_t *lock;
+	spinlock_t __lock_inplace;
+
 	const struct dma_fence_ops *ops;
 	/*
 	 * We clear the callback list on kref_put so that by the time we
@@ -262,6 +264,10 @@ struct dma_fence_ops {
 void dma_fence_init(struct dma_fence *fence, const struct dma_fence_ops *ops,
 		    spinlock_t *lock, u64 context, u64 seqno);
 
+void dma_fence_inplace_lock_init(struct dma_fence *fence,
+				 const struct dma_fence_ops *ops,
+				 u64 context, u64 seqno);
+
 void dma_fence_release(struct kref *kref);
 void dma_fence_free(struct dma_fence *fence);
 void dma_fence_describe(struct dma_fence *fence, struct seq_file *seq);
-- 
2.36.1.124.g0e6072fb45-goog

Re: [PATCH] dma-fence: allow dma fence to have their own lock

[Christian König]

Hi Sergey,

I'm removing most of the mail because you have a very fundamental 
misunderstanding about what this dma_fence lock is all about.

Am 30.05.22 um 16:22 schrieb Sergey Senozhatsky:
> [SNIP]
> So the `lock` should have at least same lifespan as the DMA fence
> that borrows it, which is impossible to guarantee in our case.

Nope, that's not correct. The lock should have at least same lifespan as 
the context of the DMA fence.

The idea here is that DMA fence signaling and callback calling 
serializes. E.g. when you have fence a,b,c,d... they must signal in the 
order a,b,c,d... and that's what this lock is good for.

If you just want to create a single dma_fence which is also only bound 
to a single context you can embed the lock into the fence without much 
problem.

See how the dma_fence_array does that for example: 
https://elixir.bootlin.com/linux/latest/source/include/linux/dma-fence-array.h#L37

Regards,
Christian.

Re: [PATCH] dma-fence: allow dma fence to have their own lock

[Christian König]

Hi Sergey,

I'm removing most of the mail because you have a very fundamental 
misunderstanding about what this dma_fence lock is all about.

Am 30.05.22 um 16:22 schrieb Sergey Senozhatsky:
> [SNIP]
> So the `lock` should have at least same lifespan as the DMA fence
> that borrows it, which is impossible to guarantee in our case.

Nope, that's not correct. The lock should have at least same lifespan as 
the context of the DMA fence.

The idea here is that DMA fence signaling and callback calling 
serializes. E.g. when you have fence a,b,c,d... they must signal in the 
order a,b,c,d... and that's what this lock is good for.

If you just want to create a single dma_fence which is also only bound 
to a single context you can embed the lock into the fence without much 
problem.

See how the dma_fence_array does that for example: 
https://elixir.bootlin.com/linux/latest/source/include/linux/dma-fence-array.h#L37

Regards,
Christian.

Re: [PATCH] dma-fence: allow dma fence to have their own lock

[Sergey Senozhatsky]

Hi Christian,

On (22/05/30 16:55), Christian König wrote:
> Hi Sergey,
> 
> I'm removing most of the mail because you have a very fundamental
> misunderstanding about what this dma_fence lock is all about.

Happy to learn.

> Am 30.05.22 um 16:22 schrieb Sergey Senozhatsky:
> > [SNIP]
> > So the `lock` should have at least same lifespan as the DMA fence
> > that borrows it, which is impossible to guarantee in our case.
>
> Nope, that's not correct. The lock should have at least same lifespan as the
> context of the DMA fence.

In our case we have one context and it lives as long as the module is loaded.
Does this mean that all DMA fences within that context should be serialized
by a single spinlock? We can have a number of "active" fences so the lock
can become a bit congested. But each operation creates, exports and signals
just once fence.

> The idea here is that DMA fence signaling and callback calling serializes.
> E.g. when you have fence a,b,c,d... they must signal in the order a,b,c,d...
> and that's what this lock is good for.

Hmm, OK. So that borrowed ->lock is in fact something like
context_lock_irqsave() and context_unlock_irqrestore().

> If you just want to create a single dma_fence which is also only bound to a
> single context you can embed the lock into the fence without much problem.

Aha, I guess this is what we need then. I'll take a look. Thanks.

Re: [PATCH] dma-fence: allow dma fence to have their own lock

[Sergey Senozhatsky]

Hi Christian,

On (22/05/30 16:55), Christian König wrote:
> Hi Sergey,
> 
> I'm removing most of the mail because you have a very fundamental
> misunderstanding about what this dma_fence lock is all about.

Happy to learn.

> Am 30.05.22 um 16:22 schrieb Sergey Senozhatsky:
> > [SNIP]
> > So the `lock` should have at least same lifespan as the DMA fence
> > that borrows it, which is impossible to guarantee in our case.
>
> Nope, that's not correct. The lock should have at least same lifespan as the
> context of the DMA fence.

In our case we have one context and it lives as long as the module is loaded.
Does this mean that all DMA fences within that context should be serialized
by a single spinlock? We can have a number of "active" fences so the lock
can become a bit congested. But each operation creates, exports and signals
just once fence.

> The idea here is that DMA fence signaling and callback calling serializes.
> E.g. when you have fence a,b,c,d... they must signal in the order a,b,c,d...
> and that's what this lock is good for.

Hmm, OK. So that borrowed ->lock is in fact something like
context_lock_irqsave() and context_unlock_irqrestore().

> If you just want to create a single dma_fence which is also only bound to a
> single context you can embed the lock into the fence without much problem.

Aha, I guess this is what we need then. I'll take a look. Thanks.

Re: [PATCH] dma-fence: allow dma fence to have their own lock

[Sergey Senozhatsky]

On (22/05/30 16:55), Christian König wrote:
> 
> If you just want to create a single dma_fence which is also only bound to a
> single context you can embed the lock into the fence without much problem.
> 
> See how the dma_fence_array does that for example: https://elixir.bootlin.com/linux/latest/source/include/linux/dma-fence-array.h#L37

Christian, I'm not sure I'm following you on the "embed the lock into the
fence without much problem" part. If I understand it correctly this should
be something like:

	fences = kmalloc_array(1, sizeof(*fences), GFP_KERNEL);
	for_each_fence(...) {

		// what spinlock should I use here?

		dma_fence_init(&fences[i], .. &lock ..);
		dma_fence_get(&fences[i]);
	}
	fence_array = dma_fence_array_create(1, fences, ....);
	sync_file_create(&fence_array->base);

Re: [PATCH] dma-fence: allow dma fence to have their own lock

[Sergey Senozhatsky]

On (22/05/30 16:55), Christian König wrote:
> 
> If you just want to create a single dma_fence which is also only bound to a
> single context you can embed the lock into the fence without much problem.
> 
> See how the dma_fence_array does that for example: https://elixir.bootlin.com/linux/latest/source/include/linux/dma-fence-array.h#L37

Christian, I'm not sure I'm following you on the "embed the lock into the
fence without much problem" part. If I understand it correctly this should
be something like:

	fences = kmalloc_array(1, sizeof(*fences), GFP_KERNEL);
	for_each_fence(...) {

		// what spinlock should I use here?

		dma_fence_init(&fences[i], .. &lock ..);
		dma_fence_get(&fences[i]);
	}
	fence_array = dma_fence_array_create(1, fences, ....);
	sync_file_create(&fence_array->base);

Re: [PATCH] dma-fence: allow dma fence to have their own lock

[Sergey Senozhatsky]

On (22/05/30 16:55), Christian König wrote:
> Am 30.05.22 um 16:22 schrieb Sergey Senozhatsky:
> > [SNIP]
> > So the `lock` should have at least same lifespan as the DMA fence
> > that borrows it, which is impossible to guarantee in our case.
> 
> Nope, that's not correct. The lock should have at least same lifespan as the
> context of the DMA fence.

How does one know when it's safe to release the context? DMA fence
objects are still transparently refcount-ed and "live their own lives",
how does one synchronize lifespans?

Re: [PATCH] dma-fence: allow dma fence to have their own lock

[Sergey Senozhatsky]

On (22/05/30 16:55), Christian König wrote:
> Am 30.05.22 um 16:22 schrieb Sergey Senozhatsky:
> > [SNIP]
> > So the `lock` should have at least same lifespan as the DMA fence
> > that borrows it, which is impossible to guarantee in our case.
> 
> Nope, that's not correct. The lock should have at least same lifespan as the
> context of the DMA fence.

How does one know when it's safe to release the context? DMA fence
objects are still transparently refcount-ed and "live their own lives",
how does one synchronize lifespans?

Re: [Linaro-mm-sig] Re: [PATCH] dma-fence: allow dma fence to have their own lock

[Christian König]

Am 31.05.22 um 04:51 schrieb Sergey Senozhatsky:
> On (22/05/30 16:55), Christian König wrote:
>> Am 30.05.22 um 16:22 schrieb Sergey Senozhatsky:
>>> [SNIP]
>>> So the `lock` should have at least same lifespan as the DMA fence
>>> that borrows it, which is impossible to guarantee in our case.
>> Nope, that's not correct. The lock should have at least same lifespan as the
>> context of the DMA fence.
> How does one know when it's safe to release the context? DMA fence
> objects are still transparently refcount-ed and "live their own lives",
> how does one synchronize lifespans?

Well, you don't.

If you have a dynamic context structure you need to reference count that 
as well. In other words every time you create a fence in your context 
you need to increment the reference count and every time a fence is 
release you decrement it.

If you have a static context structure like most drivers have then you 
must make sure that all fences at least signal before you unload your 
driver. We still somewhat have a race when you try to unload a driver 
and the fence_ops structure suddenly disappear, but we currently live 
with that.

Apart from that you are right, fences can live forever and we need to 
deal with that.

Regards,
Christian.

> _______________________________________________
> Linaro-mm-sig mailing list -- linaro-mm-sig@lists.linaro.org
> To unsubscribe send an email to linaro-mm-sig-leave@lists.linaro.org

Re: [Linaro-mm-sig] Re: [PATCH] dma-fence: allow dma fence to have their own lock

[Christian König]

Am 31.05.22 um 04:51 schrieb Sergey Senozhatsky:
> On (22/05/30 16:55), Christian König wrote:
>> Am 30.05.22 um 16:22 schrieb Sergey Senozhatsky:
>>> [SNIP]
>>> So the `lock` should have at least same lifespan as the DMA fence
>>> that borrows it, which is impossible to guarantee in our case.
>> Nope, that's not correct. The lock should have at least same lifespan as the
>> context of the DMA fence.
> How does one know when it's safe to release the context? DMA fence
> objects are still transparently refcount-ed and "live their own lives",
> how does one synchronize lifespans?

Well, you don't.

If you have a dynamic context structure you need to reference count that 
as well. In other words every time you create a fence in your context 
you need to increment the reference count and every time a fence is 
release you decrement it.

If you have a static context structure like most drivers have then you 
must make sure that all fences at least signal before you unload your 
driver. We still somewhat have a race when you try to unload a driver 
and the fence_ops structure suddenly disappear, but we currently live 
with that.

Apart from that you are right, fences can live forever and we need to 
deal with that.

Regards,
Christian.

> _______________________________________________
> Linaro-mm-sig mailing list -- linaro-mm-sig@lists.linaro.org
> To unsubscribe send an email to linaro-mm-sig-leave@lists.linaro.org

Re: [Linaro-mm-sig] Re: [PATCH] dma-fence: allow dma fence to have their own lock

[Daniel Vetter]

On Wed, Jun 01, 2022 at 02:45:42PM +0200, Christian König wrote:
> Am 31.05.22 um 04:51 schrieb Sergey Senozhatsky:
> > On (22/05/30 16:55), Christian König wrote:
> > > Am 30.05.22 um 16:22 schrieb Sergey Senozhatsky:
> > > > [SNIP]
> > > > So the `lock` should have at least same lifespan as the DMA fence
> > > > that borrows it, which is impossible to guarantee in our case.
> > > Nope, that's not correct. The lock should have at least same lifespan as the
> > > context of the DMA fence.
> > How does one know when it's safe to release the context? DMA fence
> > objects are still transparently refcount-ed and "live their own lives",
> > how does one synchronize lifespans?
> 
> Well, you don't.
> 
> If you have a dynamic context structure you need to reference count that as
> well. In other words every time you create a fence in your context you need
> to increment the reference count and every time a fence is release you
> decrement it.
> 
> If you have a static context structure like most drivers have then you must
> make sure that all fences at least signal before you unload your driver. We
> still somewhat have a race when you try to unload a driver and the fence_ops
> structure suddenly disappear, but we currently live with that.
> 
> Apart from that you are right, fences can live forever and we need to deal
> with that.

Yeah this entire thing is a bit an "oops we might have screwed up" moment.
I think the cleanest way is to essentially do what the drm/sched codes
does, which is split the gpu job into the public dma_fence (which can live
forever) and the internal job fence (which has to deal with all the
resource refcounting issues). And then make sure that only ever the public
fence escapes to places where the fence can live forever (dma_resv,
drm_syncobj, sync_file as our uapi container objects are the prominent
cases really).

It sucks a bit.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

Re: [Linaro-mm-sig] Re: [PATCH] dma-fence: allow dma fence to have their own lock

[Daniel Vetter]

On Wed, Jun 01, 2022 at 02:45:42PM +0200, Christian König wrote:
> Am 31.05.22 um 04:51 schrieb Sergey Senozhatsky:
> > On (22/05/30 16:55), Christian König wrote:
> > > Am 30.05.22 um 16:22 schrieb Sergey Senozhatsky:
> > > > [SNIP]
> > > > So the `lock` should have at least same lifespan as the DMA fence
> > > > that borrows it, which is impossible to guarantee in our case.
> > > Nope, that's not correct. The lock should have at least same lifespan as the
> > > context of the DMA fence.
> > How does one know when it's safe to release the context? DMA fence
> > objects are still transparently refcount-ed and "live their own lives",
> > how does one synchronize lifespans?
> 
> Well, you don't.
> 
> If you have a dynamic context structure you need to reference count that as
> well. In other words every time you create a fence in your context you need
> to increment the reference count and every time a fence is release you
> decrement it.
> 
> If you have a static context structure like most drivers have then you must
> make sure that all fences at least signal before you unload your driver. We
> still somewhat have a race when you try to unload a driver and the fence_ops
> structure suddenly disappear, but we currently live with that.
> 
> Apart from that you are right, fences can live forever and we need to deal
> with that.

Yeah this entire thing is a bit an "oops we might have screwed up" moment.
I think the cleanest way is to essentially do what the drm/sched codes
does, which is split the gpu job into the public dma_fence (which can live
forever) and the internal job fence (which has to deal with all the
resource refcounting issues). And then make sure that only ever the public
fence escapes to places where the fence can live forever (dma_resv,
drm_syncobj, sync_file as our uapi container objects are the prominent
cases really).

It sucks a bit.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

Re: [Linaro-mm-sig] Re: [PATCH] dma-fence: allow dma fence to have their own lock

[Christian König]

Am 30.05.22 um 17:45 schrieb Sergey Senozhatsky:
> On (22/05/30 16:55), Christian König wrote:
>> If you just want to create a single dma_fence which is also only bound to a
>> single context you can embed the lock into the fence without much problem.
>>
>> See how the dma_fence_array does that for example: https://elixir.bootlin.com/linux/latest/source/include/linux/dma-fence-array.h#L37
> Christian, I'm not sure I'm following you on the "embed the lock into the
> fence without much problem" part. If I understand it correctly this should
> be something like:
>
> 	fences = kmalloc_array(1, sizeof(*fences), GFP_KERNEL);
> 	for_each_fence(...) {
>
> 		// what spinlock should I use here?
>
> 		dma_fence_init(&fences[i], .. &lock ..);
> 		dma_fence_get(&fences[i]);
> 	}
> 	fence_array = dma_fence_array_create(1, fences, ....);
> 	sync_file_create(&fence_array->base);

Well no, that's the high level usage of the dma_fence_array.

What I meant was this here:

struct dma_fence_array {
     struct dma_fence base;

     spinlock_t lock;
...
};

Regards,
Christian.

> _______________________________________________
> Linaro-mm-sig mailing list -- linaro-mm-sig@lists.linaro.org
> To unsubscribe send an email to linaro-mm-sig-leave@lists.linaro.org

Re: [Linaro-mm-sig] Re: [PATCH] dma-fence: allow dma fence to have their own lock

[Christian König]

Am 30.05.22 um 17:45 schrieb Sergey Senozhatsky:
> On (22/05/30 16:55), Christian König wrote:
>> If you just want to create a single dma_fence which is also only bound to a
>> single context you can embed the lock into the fence without much problem.
>>
>> See how the dma_fence_array does that for example: https://elixir.bootlin.com/linux/latest/source/include/linux/dma-fence-array.h#L37
> Christian, I'm not sure I'm following you on the "embed the lock into the
> fence without much problem" part. If I understand it correctly this should
> be something like:
>
> 	fences = kmalloc_array(1, sizeof(*fences), GFP_KERNEL);
> 	for_each_fence(...) {
>
> 		// what spinlock should I use here?
>
> 		dma_fence_init(&fences[i], .. &lock ..);
> 		dma_fence_get(&fences[i]);
> 	}
> 	fence_array = dma_fence_array_create(1, fences, ....);
> 	sync_file_create(&fence_array->base);

Well no, that's the high level usage of the dma_fence_array.

What I meant was this here:

struct dma_fence_array {
     struct dma_fence base;

     spinlock_t lock;
...
};

Regards,
Christian.

> _______________________________________________
> Linaro-mm-sig mailing list -- linaro-mm-sig@lists.linaro.org
> To unsubscribe send an email to linaro-mm-sig-leave@lists.linaro.org

Re: [Linaro-mm-sig] Re: [PATCH] dma-fence: allow dma fence to have their own lock

[Christian König]

Am 01.06.22 um 15:22 schrieb Daniel Vetter:
> On Wed, Jun 01, 2022 at 02:45:42PM +0200, Christian König wrote:
>> Am 31.05.22 um 04:51 schrieb Sergey Senozhatsky:
>>> On (22/05/30 16:55), Christian König wrote:
>>>> Am 30.05.22 um 16:22 schrieb Sergey Senozhatsky:
>>>>> [SNIP]
>>>>> So the `lock` should have at least same lifespan as the DMA fence
>>>>> that borrows it, which is impossible to guarantee in our case.
>>>> Nope, that's not correct. The lock should have at least same lifespan as the
>>>> context of the DMA fence.
>>> How does one know when it's safe to release the context? DMA fence
>>> objects are still transparently refcount-ed and "live their own lives",
>>> how does one synchronize lifespans?
>> Well, you don't.
>>
>> If you have a dynamic context structure you need to reference count that as
>> well. In other words every time you create a fence in your context you need
>> to increment the reference count and every time a fence is release you
>> decrement it.
>>
>> If you have a static context structure like most drivers have then you must
>> make sure that all fences at least signal before you unload your driver. We
>> still somewhat have a race when you try to unload a driver and the fence_ops
>> structure suddenly disappear, but we currently live with that.
>>
>> Apart from that you are right, fences can live forever and we need to deal
>> with that.
> Yeah this entire thing is a bit an "oops we might have screwed up" moment.
> I think the cleanest way is to essentially do what the drm/sched codes
> does, which is split the gpu job into the public dma_fence (which can live
> forever) and the internal job fence (which has to deal with all the
> resource refcounting issues). And then make sure that only ever the public
> fence escapes to places where the fence can live forever (dma_resv,
> drm_syncobj, sync_file as our uapi container objects are the prominent
> cases really).
>
> It sucks a bit.

It's actually not that bad.

See after signaling the dma_fence_ops is mostly used for debugging I 
think, e.g. timeline name etc...

Christian.

> -Daniel

Re: [Linaro-mm-sig] Re: [PATCH] dma-fence: allow dma fence to have their own lock

[Sergey Senozhatsky]

On (22/06/01 14:45), Christian König wrote:
> Am 31.05.22 um 04:51 schrieb Sergey Senozhatsky:
> > On (22/05/30 16:55), Christian König wrote:
> > > Am 30.05.22 um 16:22 schrieb Sergey Senozhatsky:
> > > > [SNIP]
> > > > So the `lock` should have at least same lifespan as the DMA fence
> > > > that borrows it, which is impossible to guarantee in our case.
> > > Nope, that's not correct. The lock should have at least same lifespan as the
> > > context of the DMA fence.
> > How does one know when it's safe to release the context? DMA fence
> > objects are still transparently refcount-ed and "live their own lives",
> > how does one synchronize lifespans?
> 
> Well, you don't.
> 
> If you have a dynamic context structure you need to reference count that as
> well. In other words every time you create a fence in your context you need
> to increment the reference count and every time a fence is release you
> decrement it.

OK then fence release should be able to point back to its "context"
structure. Either a "private" data in dma fence or we need to "embed"
fence into another object (refcounted) that owns the lock and provide
dma fence ops->release callback, which can container_of() to the object
that dma fence is embedded into.

I think you are suggesting the latter. Thanks for clarifications.

The limiting factor of this approach is that now our ops->release() is
under the same "pressure" as dma_fence_put()->dma_fence_release() are.
dma_fence_put() and dma_fence_release() can be called from any context,
as far as I understand, e.g. IRQ, however our normal object ->release
can schedule, we do things like synchronize_rcu() and so on. Nothing is
impossible, just saying that even this approach is not 100% perfect and
may need additional workarounds.

> If you have a static context structure like most drivers have then you must
> make sure that all fences at least signal before you unload your driver. We
> still somewhat have a race when you try to unload a driver and the fence_ops
> structure suddenly disappear, but we currently live with that.

Hmm, indeed... I didn't consider fence_ops case.

> Apart from that you are right, fences can live forever and we need to deal
> with that.

OK. I see.

Re: [Linaro-mm-sig] Re: [PATCH] dma-fence: allow dma fence to have their own lock

[Sergey Senozhatsky]

On (22/06/01 14:45), Christian König wrote:
> Am 31.05.22 um 04:51 schrieb Sergey Senozhatsky:
> > On (22/05/30 16:55), Christian König wrote:
> > > Am 30.05.22 um 16:22 schrieb Sergey Senozhatsky:
> > > > [SNIP]
> > > > So the `lock` should have at least same lifespan as the DMA fence
> > > > that borrows it, which is impossible to guarantee in our case.
> > > Nope, that's not correct. The lock should have at least same lifespan as the
> > > context of the DMA fence.
> > How does one know when it's safe to release the context? DMA fence
> > objects are still transparently refcount-ed and "live their own lives",
> > how does one synchronize lifespans?
> 
> Well, you don't.
> 
> If you have a dynamic context structure you need to reference count that as
> well. In other words every time you create a fence in your context you need
> to increment the reference count and every time a fence is release you
> decrement it.

OK then fence release should be able to point back to its "context"
structure. Either a "private" data in dma fence or we need to "embed"
fence into another object (refcounted) that owns the lock and provide
dma fence ops->release callback, which can container_of() to the object
that dma fence is embedded into.

I think you are suggesting the latter. Thanks for clarifications.

The limiting factor of this approach is that now our ops->release() is
under the same "pressure" as dma_fence_put()->dma_fence_release() are.
dma_fence_put() and dma_fence_release() can be called from any context,
as far as I understand, e.g. IRQ, however our normal object ->release
can schedule, we do things like synchronize_rcu() and so on. Nothing is
impossible, just saying that even this approach is not 100% perfect and
may need additional workarounds.

> If you have a static context structure like most drivers have then you must
> make sure that all fences at least signal before you unload your driver. We
> still somewhat have a race when you try to unload a driver and the fence_ops
> structure suddenly disappear, but we currently live with that.

Hmm, indeed... I didn't consider fence_ops case.

> Apart from that you are right, fences can live forever and we need to deal
> with that.

OK. I see.

Re: [Linaro-mm-sig] Re: [PATCH] dma-fence: allow dma fence to have their own lock

[Christian König]

Am 01.06.22 um 16:27 schrieb Sergey Senozhatsky:
> On (22/06/01 14:45), Christian König wrote:
>> Am 31.05.22 um 04:51 schrieb Sergey Senozhatsky:
>>> On (22/05/30 16:55), Christian König wrote:
>>>> Am 30.05.22 um 16:22 schrieb Sergey Senozhatsky:
>>>>> [SNIP]
>>>>> So the `lock` should have at least same lifespan as the DMA fence
>>>>> that borrows it, which is impossible to guarantee in our case.
>>>> Nope, that's not correct. The lock should have at least same lifespan as the
>>>> context of the DMA fence.
>>> How does one know when it's safe to release the context? DMA fence
>>> objects are still transparently refcount-ed and "live their own lives",
>>> how does one synchronize lifespans?
>> Well, you don't.
>>
>> If you have a dynamic context structure you need to reference count that as
>> well. In other words every time you create a fence in your context you need
>> to increment the reference count and every time a fence is release you
>> decrement it.
> OK then fence release should be able to point back to its "context"
> structure. Either a "private" data in dma fence or we need to "embed"
> fence into another object (refcounted) that owns the lock and provide
> dma fence ops->release callback, which can container_of() to the object
> that dma fence is embedded into.
>
> I think you are suggesting the latter. Thanks for clarifications.

Daniel might hurt me for this, but if you really only need a pointer to 
your context then we could say that using a pointer value for the 
context field is ok as well.

That should be fine as well as long as you can guarantee that it will be 
unique during the lifetime of all it's fences.

We would just have to adjust the documentation a bit.

> The limiting factor of this approach is that now our ops->release() is
> under the same "pressure" as dma_fence_put()->dma_fence_release() are.
> dma_fence_put() and dma_fence_release() can be called from any context,
> as far as I understand, e.g. IRQ, however our normal object ->release
> can schedule, we do things like synchronize_rcu() and so on. Nothing is
> impossible, just saying that even this approach is not 100% perfect and
> may need additional workarounds.

Well just use a work item for release.

Regards,
Christian.

>> If you have a static context structure like most drivers have then you must
>> make sure that all fences at least signal before you unload your driver. We
>> still somewhat have a race when you try to unload a driver and the fence_ops
>> structure suddenly disappear, but we currently live with that.
> Hmm, indeed... I didn't consider fence_ops case.
>
>> Apart from that you are right, fences can live forever and we need to deal
>> with that.
> OK. I see.

Re: [Linaro-mm-sig] Re: [PATCH] dma-fence: allow dma fence to have their own lock

[Christian König]

Am 01.06.22 um 16:27 schrieb Sergey Senozhatsky:
> On (22/06/01 14:45), Christian König wrote:
>> Am 31.05.22 um 04:51 schrieb Sergey Senozhatsky:
>>> On (22/05/30 16:55), Christian König wrote:
>>>> Am 30.05.22 um 16:22 schrieb Sergey Senozhatsky:
>>>>> [SNIP]
>>>>> So the `lock` should have at least same lifespan as the DMA fence
>>>>> that borrows it, which is impossible to guarantee in our case.
>>>> Nope, that's not correct. The lock should have at least same lifespan as the
>>>> context of the DMA fence.
>>> How does one know when it's safe to release the context? DMA fence
>>> objects are still transparently refcount-ed and "live their own lives",
>>> how does one synchronize lifespans?
>> Well, you don't.
>>
>> If you have a dynamic context structure you need to reference count that as
>> well. In other words every time you create a fence in your context you need
>> to increment the reference count and every time a fence is release you
>> decrement it.
> OK then fence release should be able to point back to its "context"
> structure. Either a "private" data in dma fence or we need to "embed"
> fence into another object (refcounted) that owns the lock and provide
> dma fence ops->release callback, which can container_of() to the object
> that dma fence is embedded into.
>
> I think you are suggesting the latter. Thanks for clarifications.

Daniel might hurt me for this, but if you really only need a pointer to 
your context then we could say that using a pointer value for the 
context field is ok as well.

That should be fine as well as long as you can guarantee that it will be 
unique during the lifetime of all it's fences.

We would just have to adjust the documentation a bit.

> The limiting factor of this approach is that now our ops->release() is
> under the same "pressure" as dma_fence_put()->dma_fence_release() are.
> dma_fence_put() and dma_fence_release() can be called from any context,
> as far as I understand, e.g. IRQ, however our normal object ->release
> can schedule, we do things like synchronize_rcu() and so on. Nothing is
> impossible, just saying that even this approach is not 100% perfect and
> may need additional workarounds.

Well just use a work item for release.

Regards,
Christian.

>> If you have a static context structure like most drivers have then you must
>> make sure that all fences at least signal before you unload your driver. We
>> still somewhat have a race when you try to unload a driver and the fence_ops
>> structure suddenly disappear, but we currently live with that.
> Hmm, indeed... I didn't consider fence_ops case.
>
>> Apart from that you are right, fences can live forever and we need to deal
>> with that.
> OK. I see.

Re: [Linaro-mm-sig] Re: [PATCH] dma-fence: allow dma fence to have their own lock

[Sergey Senozhatsky]

On (22/06/01 16:38), Christian König wrote:
> > > Well, you don't.
> > > 
> > > If you have a dynamic context structure you need to reference count that as
> > > well. In other words every time you create a fence in your context you need
> > > to increment the reference count and every time a fence is release you
> > > decrement it.
> > OK then fence release should be able to point back to its "context"
> > structure. Either a "private" data in dma fence or we need to "embed"
> > fence into another object (refcounted) that owns the lock and provide
> > dma fence ops->release callback, which can container_of() to the object
> > that dma fence is embedded into.
> > 
> > I think you are suggesting the latter. Thanks for clarifications.
> 
> Daniel might hurt me for this, but if you really only need a pointer to your
> context then we could say that using a pointer value for the context field
> is ok as well.
> 
> That should be fine as well as long as you can guarantee that it will be
> unique during the lifetime of all it's fences.

I think we can guarantee that. Object that creates fence is kmalloc-ed and
it sticks around until dma_fence_release() calls ops->release() and kfree-s
it. We *probably* can even do something like it now, by re-purposing dma_fence
context member:

        dma_fence_init(obj->fence,
                       &fence_ops,
                       &obj->fence_lock,
                       (u64)obj,                             <<   :/
                       atomic64_inc_return(&obj->seqno));

I'd certainly refrain from being creative here and doing things that
are not documented/common. DMA fence embedding should work for us.

> > The limiting factor of this approach is that now our ops->release() is
> > under the same "pressure" as dma_fence_put()->dma_fence_release() are.
> > dma_fence_put() and dma_fence_release() can be called from any context,
> > as far as I understand, e.g. IRQ, however our normal object ->release
> > can schedule, we do things like synchronize_rcu() and so on. Nothing is
> > impossible, just saying that even this approach is not 100% perfect and
> > may need additional workarounds.
> 
> Well just use a work item for release.

Yup, that's the plan.

Re: [Linaro-mm-sig] Re: [PATCH] dma-fence: allow dma fence to have their own lock

[Sergey Senozhatsky]

On (22/06/01 16:38), Christian König wrote:
> > > Well, you don't.
> > > 
> > > If you have a dynamic context structure you need to reference count that as
> > > well. In other words every time you create a fence in your context you need
> > > to increment the reference count and every time a fence is release you
> > > decrement it.
> > OK then fence release should be able to point back to its "context"
> > structure. Either a "private" data in dma fence or we need to "embed"
> > fence into another object (refcounted) that owns the lock and provide
> > dma fence ops->release callback, which can container_of() to the object
> > that dma fence is embedded into.
> > 
> > I think you are suggesting the latter. Thanks for clarifications.
> 
> Daniel might hurt me for this, but if you really only need a pointer to your
> context then we could say that using a pointer value for the context field
> is ok as well.
> 
> That should be fine as well as long as you can guarantee that it will be
> unique during the lifetime of all it's fences.

I think we can guarantee that. Object that creates fence is kmalloc-ed and
it sticks around until dma_fence_release() calls ops->release() and kfree-s
it. We *probably* can even do something like it now, by re-purposing dma_fence
context member:

        dma_fence_init(obj->fence,
                       &fence_ops,
                       &obj->fence_lock,
                       (u64)obj,                             <<   :/
                       atomic64_inc_return(&obj->seqno));

I'd certainly refrain from being creative here and doing things that
are not documented/common. DMA fence embedding should work for us.

> > The limiting factor of this approach is that now our ops->release() is
> > under the same "pressure" as dma_fence_put()->dma_fence_release() are.
> > dma_fence_put() and dma_fence_release() can be called from any context,
> > as far as I understand, e.g. IRQ, however our normal object ->release
> > can schedule, we do things like synchronize_rcu() and so on. Nothing is
> > impossible, just saying that even this approach is not 100% perfect and
> > may need additional workarounds.
> 
> Well just use a work item for release.

Yup, that's the plan.

Re: [Linaro-mm-sig] Re: [PATCH] dma-fence: allow dma fence to have their own lock

[Christian König]

Am 01.06.22 um 16:52 schrieb Sergey Senozhatsky:
> On (22/06/01 16:38), Christian König wrote:
>>>> Well, you don't.
>>>>
>>>> If you have a dynamic context structure you need to reference count that as
>>>> well. In other words every time you create a fence in your context you need
>>>> to increment the reference count and every time a fence is release you
>>>> decrement it.
>>> OK then fence release should be able to point back to its "context"
>>> structure. Either a "private" data in dma fence or we need to "embed"
>>> fence into another object (refcounted) that owns the lock and provide
>>> dma fence ops->release callback, which can container_of() to the object
>>> that dma fence is embedded into.
>>>
>>> I think you are suggesting the latter. Thanks for clarifications.
>> Daniel might hurt me for this, but if you really only need a pointer to your
>> context then we could say that using a pointer value for the context field
>> is ok as well.
>>
>> That should be fine as well as long as you can guarantee that it will be
>> unique during the lifetime of all it's fences.
> I think we can guarantee that. Object that creates fence is kmalloc-ed and
> it sticks around until dma_fence_release() calls ops->release() and kfree-s
> it. We *probably* can even do something like it now, by re-purposing dma_fence
> context member:
>
>          dma_fence_init(obj->fence,
>                         &fence_ops,
>                         &obj->fence_lock,
>                         (u64)obj,                             <<   :/
>                         atomic64_inc_return(&obj->seqno));
>
> I'd certainly refrain from being creative here and doing things that
> are not documented/common. DMA fence embedding should work for us.

Yeah, exactly that's the idea. But if you are fine to create a subclass 
of the dma_fence than that would indeed be cleaner.

Christian.

>
>>> The limiting factor of this approach is that now our ops->release() is
>>> under the same "pressure" as dma_fence_put()->dma_fence_release() are.
>>> dma_fence_put() and dma_fence_release() can be called from any context,
>>> as far as I understand, e.g. IRQ, however our normal object ->release
>>> can schedule, we do things like synchronize_rcu() and so on. Nothing is
>>> impossible, just saying that even this approach is not 100% perfect and
>>> may need additional workarounds.
>> Well just use a work item for release.
> Yup, that's the plan.

Re: [Linaro-mm-sig] Re: [PATCH] dma-fence: allow dma fence to have their own lock

[Christian König]

Am 01.06.22 um 16:52 schrieb Sergey Senozhatsky:
> On (22/06/01 16:38), Christian König wrote:
>>>> Well, you don't.
>>>>
>>>> If you have a dynamic context structure you need to reference count that as
>>>> well. In other words every time you create a fence in your context you need
>>>> to increment the reference count and every time a fence is release you
>>>> decrement it.
>>> OK then fence release should be able to point back to its "context"
>>> structure. Either a "private" data in dma fence or we need to "embed"
>>> fence into another object (refcounted) that owns the lock and provide
>>> dma fence ops->release callback, which can container_of() to the object
>>> that dma fence is embedded into.
>>>
>>> I think you are suggesting the latter. Thanks for clarifications.
>> Daniel might hurt me for this, but if you really only need a pointer to your
>> context then we could say that using a pointer value for the context field
>> is ok as well.
>>
>> That should be fine as well as long as you can guarantee that it will be
>> unique during the lifetime of all it's fences.
> I think we can guarantee that. Object that creates fence is kmalloc-ed and
> it sticks around until dma_fence_release() calls ops->release() and kfree-s
> it. We *probably* can even do something like it now, by re-purposing dma_fence
> context member:
>
>          dma_fence_init(obj->fence,
>                         &fence_ops,
>                         &obj->fence_lock,
>                         (u64)obj,                             <<   :/
>                         atomic64_inc_return(&obj->seqno));
>
> I'd certainly refrain from being creative here and doing things that
> are not documented/common. DMA fence embedding should work for us.

Yeah, exactly that's the idea. But if you are fine to create a subclass 
of the dma_fence than that would indeed be cleaner.

Christian.

>
>>> The limiting factor of this approach is that now our ops->release() is
>>> under the same "pressure" as dma_fence_put()->dma_fence_release() are.
>>> dma_fence_put() and dma_fence_release() can be called from any context,
>>> as far as I understand, e.g. IRQ, however our normal object ->release
>>> can schedule, we do things like synchronize_rcu() and so on. Nothing is
>>> impossible, just saying that even this approach is not 100% perfect and
>>> may need additional workarounds.
>> Well just use a work item for release.
> Yup, that's the plan.